James Cook (Gerrit)

unread,

Apr 1, 2026, 5:54:49 PM (21 hours ago) Apr 1

to Michael Wojcicka, chromium...@chromium.org, Achuith Bhandarkar, Chromium LUCI CQ, AyeAye, chromium-a...@chromium.org, extension...@chromium.org

Attention needed from Michael Wojcicka

James Cook voted and added 1 comment

Votes added by James Cook

Code-Review

+1

1 comment

Patchset-level comments

File-level comment, Patchset 4 (Latest):

James Cook . resolved

LGTM. Nice.

Open in Gerrit

Related details

Attention is currently required from:

Michael Wojcicka

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

open

diffy

Achuith Bhandarkar (Gerrit)

unread,

Apr 1, 2026, 6:03:40 PM (21 hours ago) Apr 1

to Michael Wojcicka, James Cook, chromium...@chromium.org, Chromium LUCI CQ, AyeAye, chromium-a...@chromium.org, extension...@chromium.org

Attention needed from Michael Wojcicka

Achuith Bhandarkar added 2 comments

File chrome/browser/extensions/extension_user_script_loader_unittest.cc

Line 409, Patchset 4 (Latest): return std::unique_ptr<KeyedService>(new ExtensionPrefValueMap());

Achuith Bhandarkar . unresolved

Nit: Use `std::make_unique` instead of `new`:
```cpp
      base::BindRepeating([](content::BrowserContext* context)
                              -> std::unique_ptr<KeyedService> {
        return std::make_unique<ExtensionPrefValueMap>();
      }));
```

Line 430, Patchset 4 (Latest): ExtensionsBrowserClient::Set(nullptr);

Achuith Bhandarkar . unresolved

Wdyt about restoring the previous `ExtensionsBrowserClient` rather than resetting it to `nullptr`? Other tests in the same process or the overarching test suite might have set one up and rely on it being available.

Consider capturing it at the start of the test:
```cpp
  ExtensionsBrowserClient* old_client = ExtensionsBrowserClient::Get();
  ExtensionsBrowserClient::Set(&client);
  // ...
  ExtensionsBrowserClient::Set(old_client);
```

Open in Gerrit

Related details

Attention is currently required from:

Michael Wojcicka

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

Michael Wojcicka (Gerrit)

unread,

1:29 PM (2 hours ago) 1:29 PM

to James Cook, chromium...@chromium.org, Achuith Bhandarkar, Chromium LUCI CQ, AyeAye, chromium-a...@chromium.org, extension...@chromium.org

Attention needed from Achuith Bhandarkar

Michael Wojcicka added 2 comments

File chrome/browser/extensions/extension_user_script_loader_unittest.cc

Line 409, Patchset 4: return std::unique_ptr<KeyedService>(new ExtensionPrefValueMap());

Achuith Bhandarkar . resolved

Nit: Use `std::make_unique` instead of `new`:
```cpp
      base::BindRepeating([](content::BrowserContext* context)
                              -> std::unique_ptr<KeyedService> {
        return std::make_unique<ExtensionPrefValueMap>();
      }));
```

Michael Wojcicka

Done

Line 430, Patchset 4: ExtensionsBrowserClient::Set(nullptr);

Achuith Bhandarkar . resolved

Wdyt about restoring the previous `ExtensionsBrowserClient` rather than resetting it to `nullptr`? Other tests in the same process or the overarching test suite might have set one up and rely on it being available.
Consider capturing it at the start of the test:
```cpp
  ExtensionsBrowserClient* old_client = ExtensionsBrowserClient::Get();
  ExtensionsBrowserClient::Set(&client);
  // ...
  ExtensionsBrowserClient::Set(old_client);
```

Michael Wojcicka

Done.

Open in Gerrit

Related details

Attention is currently required from:

Achuith Bhandarkar

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

open

diffy

Michael Wojcicka (Gerrit)

unread,

2:09 PM (1 hour ago) 2:09 PM

to James Cook, chromium...@chromium.org, Achuith Bhandarkar, Chromium LUCI CQ, AyeAye, chromium-a...@chromium.org, extension...@chromium.org

Attention needed from Achuith Bhandarkar

Michael Wojcicka voted Commit-Queue+2

Commit-Queue

+2

satisfied_requirement

open

diffy

Chromium LUCI CQ (Gerrit)

unread,

2:13 PM (1 hour ago) 2:13 PM

to Michael Wojcicka, James Cook, chromium...@chromium.org, Achuith Bhandarkar, AyeAye, chromium-a...@chromium.org, extension...@chromium.org

Chromium LUCI CQ submitted the change with unreviewed changes

Unreviewed changes

4 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:

```
The name of the file: chrome/browser/extensions/extension_user_script_loader_unittest.cc
Insertions: 8, Deletions: 8.

@@ -343,7 +343,6 @@
 }
 
 TEST_F(ExtensionUserScriptLoaderTest, ComponentExtensionContentScriptIsLoaded) {
-  // Define mock manager
   class MockResourceManager : public ComponentExtensionResourceManager {
    public:
     bool IsComponentExtensionResource(const base::FilePath& extension_path,
@@ -362,10 +361,9 @@
     }
   };
 
-  // Define test client
-  class MyTestClient : public TestExtensionsBrowserClient {
+  class TestClient : public TestExtensionsBrowserClient {
    public:
-    MyTestClient() = default;
+    TestClient() = default;
     const ComponentExtensionResourceManager*
     GetComponentExtensionResourceManager() override {
       return &mock_manager_;
@@ -380,9 +378,10 @@
     MockResourceManager mock_manager_;
   };
 
-  MyTestClient client;
+  TestClient client;
   client.set_extension_system_factory(
       ChromeExtensionSystemFactory::GetInstance());
+  ExtensionsBrowserClient* old_client = ExtensionsBrowserClient::Get();
   ExtensionsBrowserClient::Set(&client);
 
   base::FilePath resources_dir;
@@ -405,8 +404,9 @@
       base::BindRepeating(&TestExtensionSystem::Build));
   profile_builder.AddTestingFactory(
       ExtensionPrefValueMapFactory::GetInstance(),
-      base::BindRepeating([](content::BrowserContext* context) {
-        return std::unique_ptr<KeyedService>(new ExtensionPrefValueMap());
+      base::BindRepeating([](content::BrowserContext* context)
+                              -> std::unique_ptr<KeyedService> {
+        return std::make_unique<ExtensionPrefValueMap>();
       }));
   std::unique_ptr<TestingProfile> profile = profile_builder.Build();
 
@@ -427,7 +427,7 @@
   histogram_tester.ExpectTotalCount(
       "Extensions.ContentScripts.DynamicContentScriptsLengthPerLoad", 1);
 
-  ExtensionsBrowserClient::Set(nullptr);
+  ExtensionsBrowserClient::Set(old_client);
 }
 
 TEST_F(ExtensionUserScriptLoaderTest, RecordScriptLengthUmas) {
```

Change information

Commit message:

extensions: Port ComponentExtensionContentScriptIsLoaded to Desktop Android

- This CL enables and fixes the ComponentExtensionContentScriptIsLoaded
  unit test for Desktop Android by addressing initialization crashes and
  missing resource dependencies.

- Fixed SIGSEGV crashes by using TestingProfile::Builder to inject test
  factories and overriding context resolution in the test client
  preventing null derefs during profile setup.

- Fixed assertion failures caused by missing PDF assets on Android by
  mocking ComponentExtensionResourceManager to return a standard,
  cross-platform resource ID (IDR_EXTENSION_DEFAULT_ICON).

Bug: crbug.com/469417243

Change-Id: I217b0de84133ad8b69fd217c7ed2e0fb4fa2bbb1

Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7722408

Reviewed-by: James Cook <jame...@chromium.org>

Commit-Queue: Michael Wojcicka <mw...@google.com>

Cr-Commit-Position: refs/heads/main@{#1609365}

Files:

M chrome/browser/extensions/extension_user_script_loader_unittest.cc

Change size: M

Delta: 1 file changed, 63 insertions(+), 7 deletions(-)

Branch: refs/heads/main

Submit Requirements:

Code-Review: +1 by James Cook

Open in Gerrit

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

Gerrit

open

diffy

satisfied_requirement

Reply all

Reply to author

Forward

extensions: Port ComponentExtensionContentScriptIsLoaded to Desktop Android [chromium/src : main]

James Cook (Gerrit)

James Cook voted and added 1 comment

Votes added by James Cook

1 comment

Related details

Achuith Bhandarkar (Gerrit)

Achuith Bhandarkar added 2 comments

Related details

Michael Wojcicka (Gerrit)

Michael Wojcicka added 2 comments

Related details

Michael Wojcicka (Gerrit)

Michael Wojcicka voted Commit-Queue+2

Chromium LUCI CQ (Gerrit)

Chromium LUCI CQ submitted the change with unreviewed changes

Unreviewed changes

Change information