Mike Wasserman (Gerrit)

unread,

Oct 27, 2025, 7:06:12 PM (2 days ago) Oct 27

to Michał Kaczmarczyk, Colin Blundell, Enterprise Policy Reviews, AyeAye, Chromium LUCI CQ, devtools...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org

Attention needed from Colin Blundell and Michał Kaczmarczyk

Mike Wasserman voted and added 1 comment

Votes added by Mike Wasserman

Code-Review

+1

1 comment

Patchset-level comments

File-level comment, Patchset 14 (Latest):

Mike Wasserman . resolved

browser_command_controller.cc and chrome/test/BUILD.gn lgtm

Open in Gerrit

Related details

Attention is currently required from:

Colin Blundell
Michał Kaczmarczyk

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Colin Blundell (Gerrit)

unread,

Oct 28, 2025, 4:45:36 AM (yesterday) Oct 28

to Michał Kaczmarczyk, Yang Guo, Chrome Enterprise Premium Reviews, Mike Wasserman, Colin Blundell, Enterprise Policy Reviews, AyeAye, Chromium LUCI CQ, devtools...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org

Attention needed from Chrome Enterprise Premium Reviews, Michał Kaczmarczyk and Yang Guo

Colin Blundell added 1 comment

Patchset-level comments

File-level comment, Patchset 14 (Latest):

Colin Blundell . resolved

Thanks! Adding closer OWNERS, as the CL is nontrivial:

Yang for devtools/
Enterprise for *policy*

Will stamp whatever's left for OWNERS afther those LGTMs come in

Open in Gerrit

Related details

Attention is currently required from:

Chrome Enterprise Premium Reviews
Michał Kaczmarczyk
Yang Guo

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

gwsq (Gerrit)

unread,

Oct 28, 2025, 4:49:55 AM (yesterday) Oct 28

to Michał Kaczmarczyk, Chrome Enterprise Premium Reviews, Nasser Al-shawwa, Yang Guo, Mike Wasserman, Colin Blundell, Enterprise Policy Reviews, AyeAye, Chromium LUCI CQ, devtools...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org

Attention needed from Michał Kaczmarczyk, Nasser Al-shawwa and Yang Guo

Message from gwsq

Reviewer source(s):
alsh...@chromium.org is from context(chrome/enterprise/gwsq/cep-review.gwsq)

Open in Gerrit

Related details

Attention is currently required from:

Michał Kaczmarczyk
Nasser Al-shawwa
Yang Guo

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Danil Somsikov (Gerrit)

unread,

Oct 28, 2025, 5:22:31 AM (yesterday) Oct 28

to Michał Kaczmarczyk, Yang Guo, Chrome Enterprise Premium Reviews, Nasser Al-shawwa, Mike Wasserman, Colin Blundell, Enterprise Policy Reviews, AyeAye, Chromium LUCI CQ, devtools...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org

Attention needed from Michał Kaczmarczyk and Nasser Al-shawwa

Danil Somsikov added 2 comments

File chrome/browser/devtools/devtools_availability_checker_unittest.cc

Line 99, Patchset 14 (Latest):}

Danil Somsikov . unresolved

Please also cover the following cases:

URL is neither allowlisted nor blocklisted
extension-specific overload
nullptr as the second parameter

File chrome/browser/devtools/devtools_window.cc

Line 1238, Patchset 14 (Latest): // profile_->GetProfilePolicyConnector()->policy_service()->AddObserver(

Danil Somsikov . unresolved

Please remove

Open in Gerrit

Related details

Attention is currently required from:

Michał Kaczmarczyk
Nasser Al-shawwa

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review

No-Unresolved-Comments
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Sylvain Defresne (Gerrit)

unread,

Oct 28, 2025, 12:09:39 PM (22 hours ago) Oct 28

to Michał Kaczmarczyk, Colin Blundell, Vasilii Sukhanov, Danil Somsikov, Chrome Enterprise Premium Reviews, Nasser Al-shawwa, Mike Wasserman, Enterprise Policy Reviews, AyeAye, Chromium LUCI CQ, devtools...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org

Attention needed from Michał Kaczmarczyk, Nasser Al-shawwa and Vasilii Sukhanov

Sylvain Defresne added 1 comment

Patchset-level comments

File-level comment, Patchset 15 (Latest):

Sylvain Defresne . resolved

I don't think I'm a proper reviewer for this code.

It looks like @blun...@chromium.org is also asked to review this change, and his OWNERS overlaps with mine.

Removing myself from the requested reviewers.

Open in Gerrit

Related details

Attention is currently required from:

Michał Kaczmarczyk
Nasser Al-shawwa
Vasilii Sukhanov

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Victor Vianna (Gerrit)

unread,

Oct 28, 2025, 2:30:04 PM (20 hours ago) Oct 28

to Michał Kaczmarczyk, Colin Blundell, Vasilii Sukhanov, Danil Somsikov, Chrome Enterprise Premium Reviews, Nasser Al-shawwa, Mike Wasserman, Enterprise Policy Reviews, AyeAye, Chromium LUCI CQ, devtools...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org

Attention needed from Michał Kaczmarczyk and Nasser Al-shawwa

Victor Vianna added 1 comment

Patchset-level comments

File-level comment, Patchset 15 (Latest):

Victor Vianna . resolved

When adding owners, specify what directory each person should look at, there are often overlaps in ownership
Gerrit's "suggest reviewers" is still quite poor. In this case I was probably suggested because I own //chrome, but you should prefer more specific reviewers (in this case a devtools reviewer and a policy reviewer)

Open in Gerrit

Related details

Attention is currently required from:

Michał Kaczmarczyk
Nasser Al-shawwa

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

gwsq (Gerrit)

unread,

Oct 28, 2025, 2:42:17 PM (19 hours ago) Oct 28

to Michał Kaczmarczyk, Enterprise Policy Reviews, Igor, Emilia Paz, Ryan Sultanem, Dominique Fauteux-Chapleau, Danil Somsikov, Chrome Enterprise Premium Reviews, Nasser Al-shawwa, Mike Wasserman, AyeAye, Chromium LUCI CQ, devtools...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org

Attention needed from Dominique Fauteux-Chapleau, Emilia Paz, Igor, Nasser Al-shawwa and Ryan Sultanem

Message from gwsq

Reviewer source(s):
igo...@chromium.org is from context(chrome/enterprise/gwsq/enterprise-policy-review.gwsq)

Open in Gerrit

Related details

Attention is currently required from:

Dominique Fauteux-Chapleau
Emilia Paz
Igor
Nasser Al-shawwa
Ryan Sultanem

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Dominique Fauteux-Chapleau (Gerrit)

unread,

Oct 28, 2025, 4:58:33 PM (17 hours ago) Oct 28

to Michał Kaczmarczyk, Enterprise Policy Reviews, Igor, Emilia Paz, Ryan Sultanem, Danil Somsikov, Chrome Enterprise Premium Reviews, Mike Wasserman, AyeAye, Chromium LUCI CQ, devtools...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org

Attention needed from Emilia Paz, Igor, Michał Kaczmarczyk and Ryan Sultanem

Dominique Fauteux-Chapleau added 4 comments

File chrome/browser/devtools/devtools_availability_checker.cc

Line 62, Patchset 15 (Latest): profile, static_cast<const extensions::Extension*>(nullptr));

Dominique Fauteux-Chapleau . unresolved

nit: You shouldn't need to cast nullptr, do you get a compilation error without this cast?

Line 91, Patchset 15 (Latest):

      if (extension) {
        if (extensions::Manifest::IsPolicyLocation(extension->location())) {
          return false;
        }
        if (extensions::Manifest::IsComponentLocation(extension->location()) &&
            profile->GetProfilePolicyConnector()->IsManaged()) {
          return false;
        }
      }

Dominique Fauteux-Chapleau . unresolved

nit: Merge these into one condition and one return statement.

Line 129, Patchset 15 (Latest):

    policy::DeveloperToolsPolicyChecker* checker =
        policy::DeveloperToolsPolicyCheckerFactory::GetForBrowserContext(
            profile);
    if (checker->IsUrlAllowedByPolicy(extension->url())) {
      return true;
    }
    if (checker->IsUrlBlockedByPolicy(extension->url())) {
      return false;
    }

Dominique Fauteux-Chapleau . unresolved

You use this snippet in 3 locations in this file, I think it should be in its own helper function, and also have an explanation that the Allow policy has precedence over the Blocklist one. You can make that helper in the Checker directly and have it return a URLBlocklistState value so you know if you have to return directly or check the enum policy.

File chrome/browser/policy/developer_tools_policy_checker_factory.cc

Line 55, Patchset 15 (Latest): // TODO(mickaczmarczyk) Add implementation for mobile.

Dominique Fauteux-Chapleau . unresolved

nit: Preferably, put bug IDs in TODOs and not your email.

Open in Gerrit

Related details

Attention is currently required from:

Emilia Paz
Igor
Michał Kaczmarczyk
Ryan Sultanem

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Emilia Paz (Gerrit)

unread,

Oct 28, 2025, 11:29:22 PM (11 hours ago) Oct 28

to Michał Kaczmarczyk, Enterprise Policy Reviews, Igor, Ryan Sultanem, Dominique Fauteux-Chapleau, Danil Somsikov, Chrome Enterprise Premium Reviews, Mike Wasserman, AyeAye, Chromium LUCI CQ, devtools...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org

Attention needed from Igor, Michał Kaczmarczyk and Ryan Sultanem

Emilia Paz voted and added 2 comments

Votes added by Emilia Paz

Code-Review

+1

2 comments

Patchset-level comments

File-level comment, Patchset 15 (Latest):

Emilia Paz . resolved

c/b/extensions lgtm, thanks!

File chrome/browser/extensions/extension_context_menu_model.cc

Line 128, Patchset 15 (Latest):

  if (checker) {
    if (checker->IsUrlAllowedByPolicy(extension.url())) {
      return true;
    }
    if (checker->IsUrlBlockedByPolicy(extension.url())) {
      return false;
    }
  }

Emilia Paz . unresolved

nit: lets move this before availability (line 125), since we don't need to retrieve it if we return early. Can also be a helper method as mentioned on the other comment

Wish we could have a test for this, but `INSPECT_POPUP` visibility is not tested because there is no popup delegate on the [test](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/extensions/extension_context_menu_model_browsertest.cc;l=704-707. Adding that is not straightforward, so it's ok from the extensions side as long as DeveloperToolsPolicyCheckerFactory is tested separately.

Open in Gerrit

Related details

Attention is currently required from:

Igor
Michał Kaczmarczyk
Ryan Sultanem

Submit Requirements:

Code-Coverage
Code-Owners

Code-Review

satisfied_requirement

unsatisfied_requirement

open

diffy

Ryan Sultanem (Gerrit)

unread,

6:11 AM (4 hours ago) 6:11 AM

to Michał Kaczmarczyk, Emilia Paz, Enterprise Policy Reviews, Igor, Dominique Fauteux-Chapleau, Danil Somsikov, Chrome Enterprise Premium Reviews, Mike Wasserman, AyeAye, Chromium LUCI CQ, devtools...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org

Attention needed from Igor and Michał Kaczmarczyk

Ryan Sultanem voted and added 2 comments

Votes added by Ryan Sultanem

Code-Review

+1

2 comments

Patchset-level comments

File-level comment, Patchset 15 (Latest):

Ryan Sultanem . resolved

LGTM for `chrome/browser/profiles/profile_keyed_service_browsertest.cc` with a minor nit.

Thanks!

File chrome/browser/profiles/profile_keyed_service_browsertest.cc

Line 519, Patchset 15 (Latest): "DeveloperToolsPolicyChecker",

Ryan Sultanem . unresolved

minor nit: Please try to keep this list ordered alphabetically. (aware that we are already not doing a good job :), but it would be great that new additions are as correct as possible!)

Maybe around line 414?

Open in Gerrit

Related details

Attention is currently required from:

Igor
Michał Kaczmarczyk

satisfied_requirement

unsatisfied_requirement

open

diffy

Reply all

Reply to author

Forward

Add UI support to DeveloperToolsPolicyAvailiabilityAllowlist and Blocklist. [chromium/src : main]

Mike Wasserman (Gerrit)

Mike Wasserman voted and added 1 comment

Votes added by Mike Wasserman

1 comment

Related details

Colin Blundell (Gerrit)

Colin Blundell added 1 comment

Related details

gwsq (Gerrit)

Message from gwsq

Related details

Danil Somsikov (Gerrit)

Danil Somsikov added 2 comments

Related details

Sylvain Defresne (Gerrit)

Sylvain Defresne added 1 comment

Related details

Victor Vianna (Gerrit)

Victor Vianna added 1 comment

Related details

gwsq (Gerrit)

Message from gwsq

Related details

Dominique Fauteux-Chapleau (Gerrit)

Dominique Fauteux-Chapleau added 4 comments

Related details

Emilia Paz (Gerrit)

Emilia Paz voted and added 2 comments

Votes added by Emilia Paz

2 comments

Related details

Ryan Sultanem (Gerrit)

Ryan Sultanem voted and added 2 comments

Votes added by Ryan Sultanem

2 comments

Related details