Sky Malice (Gerrit)

unread,

Sep 17, 2025, 2:56:45 PM9/17/25

to Fiaz Muhammad, chromium...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org

Attention needed from Fiaz Muhammad

Sky Malice voted and added 12 comments

Votes added by Sky Malice

Code-Review

+1

12 comments

File chrome/browser/android/extensions/extensions_url_override_registry_manager.cc

Line 37, Patchset 1 (Latest):void ExtensionsUrlOverrideRegistryManager::OnUrlOverrideDisabled(

Sky Malice . unresolved

Why doesn't this have incognito_enabled? Does disable always affect both if present?

File chrome/browser/android/extensions/extensions_url_override_state_tracker_impl.h

Line 80, Patchset 1 (Latest): std::unique_ptr<Observer> observer_;

Sky Malice . unresolved

Does this need to be a unique ptr? Isn't this observer's lifetime exactly identical to the tracker? Can't it just be a member (not a ptr)?

Line 75, Patchset 1 (Latest): bool GetAndSetIncognitoStatus(const Extension& extension);

Sky Malice . unresolved

Nit: I'd call this cache.

Line 66, Patchset 1 (Latest): // Updates the override counts for the given `overrides` by `overrides_delta`.

Sky Malice . unresolved

Nit: Newline for consistency?

File chrome/browser/android/extensions/extensions_url_override_state_tracker_impl.cc

File-level comment, Patchset 1 (Latest):

Sky Malice . unresolved

Tests?

Line 51, Patchset 1 (Latest): registrar_->AddObserver(&*observer_);

Sky Malice . unresolved

Why not `observer_.get()`?

Line 60, Patchset 1 (Latest): const URLOverrides::URLOverrideMap& overrides) {

Sky Malice . unresolved

Do we only get each override registered once? Do we not have to be idempotent?

Line 79, Patchset 1 (Latest): std::string chrome_url_path = std::string(page_override_pair.first);

Sky Malice . unresolved

Same as elsewhere, we shouldn't need to make a new string here. Maybe `chrome_url_path` should be a `const std::string&`?

Line 117, Patchset 1 (Latest): auto it = override_map_.try_emplace(chrome_url_path).first;

Sky Malice . unresolved

I don't understand why we don't either std::move or just directly pass page_override_pair.first here.

Line 117, Patchset 1 (Latest): auto it = override_map_.try_emplace(chrome_url_path).first;

Sky Malice . resolved

All this usage of try_emplace feels surprising to me. You could just check the try_emplace(...).second, but I guess it doesn't really simplify. Nevermind.

Line 127, Patchset 1 (Latest): IncognitoInfo::IsSplitMode(&extension) &&

Sky Malice . unresolved

What exactly does this mean? If this is true, it means the extension gets to run in both regular and incog browsing modes?

Line 128, Patchset 1 (Latest): util::IsIncognitoEnabled(extension.id(), profile_);

Sky Malice . unresolved

Erm what? How's this different from IsSplitMode? Does this only return true when the profile is incognito?

Open in Gerrit

Related details

Attention is currently required from:

Fiaz Muhammad

Submit Requirements:

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

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

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Fiaz Muhammad (Gerrit)

unread,

Sep 18, 2025, 10:14:37 AM9/18/25

to Sky Malice, chromium...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org

Fiaz Muhammad added 11 comments

File chrome/browser/android/extensions/extensions_url_override_registry_manager.cc

Line 37, Patchset 1:void ExtensionsUrlOverrideRegistryManager::OnUrlOverrideDisabled(

Sky Malice . resolved

Why doesn't this have incognito_enabled? Does disable always affect both if present?

Fiaz Muhammad

Yes, that's exactly it! If the incognito status changes, we will call OnUrlOverrideEnabled with the applicable incognito status. I'll update the comments to further elaborate on this point

File chrome/browser/android/extensions/extensions_url_override_state_tracker_impl.h

Line 80, Patchset 1: std::unique_ptr<Observer> observer_;

Sky Malice . resolved

Does this need to be a unique ptr? Isn't this observer's lifetime exactly identical to the tracker? Can't it just be a member (not a ptr)?

Fiaz Muhammad

I got pointer brain and forgot this was an option. Removing the ptr!

Line 75, Patchset 1: bool GetAndSetIncognitoStatus(const Extension& extension);

Sky Malice . resolved

Nit: I'd call this cache.

Fiaz Muhammad

Done

Line 66, Patchset 1: // Updates the override counts for the given `overrides` by `overrides_delta`.

Sky Malice . resolved

Nit: Newline for consistency?

Fiaz Muhammad

Done

File chrome/browser/android/extensions/extensions_url_override_state_tracker_impl.cc

File-level comment, Patchset 1:

Sky Malice . resolved

Tests?

Fiaz Muhammad

I'll need to figure out how to set up the BUILD.gn for this so I'll do this in a follow-up!

Line 51, Patchset 1: registrar_->AddObserver(&*observer_);

Sky Malice . resolved

Why not `observer_.get()`?

Fiaz Muhammad

I somehow forgot this was an option... thanks for reminding me!

Line 60, Patchset 1: const URLOverrides::URLOverrideMap& overrides) {

Sky Malice . resolved

Do we only get each override registered once? Do we not have to be idempotent?

Fiaz Muhammad

The observer guarantees that each override registration/unregistration will be called once for each URL override status change, so we don't need to make this idempotent.

When the incognito status changes, the observer methods OnExtensionOverrideRemoved and then OnExtensionOverrideAdded will be called sequentially!

Line 79, Patchset 1: std::string chrome_url_path = std::string(page_override_pair.first);

Sky Malice . resolved

Same as elsewhere, we shouldn't need to make a new string here. Maybe `chrome_url_path` should be a `const std::string&`?

Fiaz Muhammad

Done

Line 117, Patchset 1: auto it = override_map_.try_emplace(chrome_url_path).first;

Sky Malice . resolved

I don't understand why we don't either std::move or just directly pass page_override_pair.first here.

Fiaz Muhammad

Done

Line 127, Patchset 1: IncognitoInfo::IsSplitMode(&extension) &&

Sky Malice . resolved

What exactly does this mean? If this is true, it means the extension gets to run in both regular and incog browsing modes?

Fiaz Muhammad

Split mode is an extensions manifest flag that has the property of allowing for URL Overrides even in incognito mode.

When split mode is enabled, we run extension in a separate instance in incognito, which is the only case URL overrides are acceptable.

See https://chromium-review.googlesource.com/c/chromium/src/+/6897267/comment/178fceb6_2b817ec7/

Line 128, Patchset 1: util::IsIncognitoEnabled(extension.id(), profile_);

Sky Malice . resolved

Erm what? How's this different from IsSplitMode? Does this only return true when the profile is incognito?

Fiaz Muhammad

No, it tells us whether the extension is enabled for use in incognito mode. So the extension may be in split mode, but the user may not have given permission for it to run in incognito.

IsIncognitoEnabled checks for the latter!

Open in Gerrit

Related details

Attention set is empty

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review

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

Gerrit

satisfied_requirement

open

diffy

Fiaz Muhammad (Gerrit)

unread,

Sep 18, 2025, 10:25:43 AM9/18/25

to Sky Malice, chromium...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org

Fiaz Muhammad voted Commit-Queue+2

Commit-Queue

+2

Open in Gerrit

Related details

Attention set is empty

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review

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

Gerrit

satisfied_requirement

open

diffy

Fiaz Muhammad (Gerrit)

unread,

Sep 18, 2025, 11:09:24 AM9/18/25

to Chromium LUCI CQ, Sky Malice, chromium...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org

Fiaz Muhammad voted Commit-Queue+2

Commit-Queue

+2

Open in Gerrit

Related details

Attention set is empty

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review

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

Gerrit

satisfied_requirement

open

diffy

Fiaz Muhammad (Gerrit)

unread,

Sep 18, 2025, 11:16:23 AM9/18/25

to Chromium LUCI CQ, Sky Malice, chromium...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org

Fiaz Muhammad voted Commit-Queue+2

Commit-Queue

+2

Open in Gerrit

Related details

Attention set is empty

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review

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

Gerrit

satisfied_requirement

open

diffy

Chromium LUCI CQ (Gerrit)

unread,

Sep 18, 2025, 11:49:56 AM9/18/25

to Fiaz Muhammad, Sky Malice, chromium...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org

Chromium LUCI CQ submitted the change with unreviewed changes

Unreviewed changes

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

```
The name of the file: chrome/browser/android/extensions/extensions_url_override_state_tracker_impl.h
Insertions: 3, Deletions: 2.

@@ -63,6 +63,7 @@
   // deactivated.
   void OnUrlOverrideDeactivated(const Extension& extension,
                                 const URLOverrides::URLOverrideMap& overrides);
+


   // Updates the override counts for the given `overrides` by `overrides_delta`.

   // `overrides_delta` is 1 for registration / activation and -1 for
   // deactivation. Notifies observers if the state of an override changes.
@@ -72,12 +73,12 @@
 
   void EnsureOverridesInitialized(
       const URLOverrides::URLOverrideMap& overrides);
-  bool GetAndSetIncognitoStatus(const Extension& extension);
+  bool GetAndCacheIncognitoStatus(const Extension& extension);
 
   base::flat_map<std::string, bool> extension_id_to_incognito_status_;
   base::flat_map<std::string, IncognitoStatusToOverrideCount> override_map_;
 
-  std::unique_ptr<Observer> observer_;
+  Observer observer_;
 
   raw_ptr<ExtensionWebUIOverrideRegistrar> registrar_;
   raw_ptr<StateListener> listener_;
```
```
The name of the file: chrome/browser/android/extensions/extensions_url_override_state_tracker.h
Insertions: 3, Deletions: 2.

@@ -16,8 +16,9 @@
   class StateListener {
    public:
     // To be called when overrides now exist for this chrome URL path.
-    // This will be called again when URL overrides are enabled in
-    // incognito mode.
+    // `incognito_enabled` refers to whether this URL Override is enabled for
+    // use in incognito. This can also be called again when the
+    // incognito_enabled status of this override changes.
     virtual void OnUrlOverrideEnabled(const std::string& chrome_url_path,
                                       bool incognito_enabled) = 0;
 
```
```
The name of the file: chrome/browser/android/extensions/extensions_url_override_state_tracker_impl.cc
Insertions: 12, Deletions: 11.

@@ -8,7 +8,6 @@
 
 #include "base/notreached.h"
 #include "chrome/browser/android/extensions/extensions_url_override_state_tracker.h"
-#include "chrome/common/url_constants.h"
 #include "extensions/browser/extension_util.h"
 #include "extensions/common/extension.h"
 #include "extensions/common/manifest_handlers/incognito_info.h"
@@ -36,29 +35,27 @@
     const Extension& extension) {
   const URLOverrides::URLOverrideMap& overrides =
       URLOverrides::GetChromeURLOverrides(&extension);
-  state_tracker_->OnUrlOverrideRegistered(extension, overrides);
+  state_tracker_->OnUrlOverrideDeactivated(extension, overrides);
 }
 
 // ExtensionUrlOverrideStateTrackerImpl Implementation.
 ExtensionUrlOverrideStateTrackerImpl::ExtensionUrlOverrideStateTrackerImpl(
     Profile* profile,
     StateListener* listener)
-    : listener_(listener), profile_(profile) {
-  observer_ =
-      std::make_unique<ExtensionUrlOverrideStateTrackerImpl::Observer>(this);
+    : observer_(this), listener_(listener), profile_(profile) {
   registrar_ =
       ExtensionWebUIOverrideRegistrar::GetFactoryInstance()->Get(profile_);
-  registrar_->AddObserver(&*observer_);
+  registrar_->AddObserver(&observer_);
 }
 
 ExtensionUrlOverrideStateTrackerImpl::~ExtensionUrlOverrideStateTrackerImpl() {
-  registrar_->RemoveObserver(&*observer_);
+  registrar_->RemoveObserver(&observer_);
 }
 
 void ExtensionUrlOverrideStateTrackerImpl::OnUrlOverrideRegistered(
     const Extension& extension,
     const URLOverrides::URLOverrideMap& overrides) {
-  GetAndSetIncognitoStatus(extension);
+  GetAndCacheIncognitoStatus(extension);
   UpdateOverrides(extension, overrides, 1);
 }
 void ExtensionUrlOverrideStateTrackerImpl::OnUrlOverrideDeactivated(
@@ -76,22 +73,26 @@
   bool incognito_override_allowed =
       extension_id_to_incognito_status_[extension.id()];
   for (const auto& page_override_pair : overrides) {
-    std::string chrome_url_path = std::string(page_override_pair.first);
+    const std::string& chrome_url_path = page_override_pair.first;
     auto override_it = override_map_.find(chrome_url_path);
     if (override_it == override_map_.end()) {
       NOTREACHED() << "Override map missing entry for initialized override";
     }
+
     IncognitoStatusToOverrideCount& counts = override_it->second;
     int prev_regular_overrides_count = counts[false];
     int prev_incognito_overrides_count = counts[true];
     counts[false] += overrides_delta;
+
     if (incognito_override_allowed) {
       counts[true] += overrides_delta;
     }
+
     int new_regular_overrides_count = counts[false];
     int new_incognito_overrides_count = counts[true];
     DCHECK_GE(new_regular_overrides_count, 0);
     DCHECK_GE(new_incognito_overrides_count, 0);
+
     bool was_enabled = prev_regular_overrides_count > 0;
     bool is_enabled = new_regular_overrides_count > 0;
     if (!was_enabled && is_enabled) {
@@ -113,7 +114,7 @@
 void ExtensionUrlOverrideStateTrackerImpl::EnsureOverridesInitialized(
     const URLOverrides::URLOverrideMap& overrides) {
   for (const auto& page_override_pair : overrides) {
-    std::string chrome_url_path = std::string(page_override_pair.first);
+    const std::string& chrome_url_path = page_override_pair.first;
     auto it = override_map_.try_emplace(chrome_url_path).first;
     IncognitoStatusToOverrideCount& counts = it->second;
     counts.try_emplace(true, 0);
@@ -121,7 +122,7 @@
   }
 }
 
-bool ExtensionUrlOverrideStateTrackerImpl::GetAndSetIncognitoStatus(
+bool ExtensionUrlOverrideStateTrackerImpl::GetAndCacheIncognitoStatus(
     const Extension& extension) {
   bool incognito_override_allowed =
       IncognitoInfo::IsSplitMode(&extension) &&
```
```
The name of the file: chrome/browser/android/extensions/BUILD.gn
Insertions: 14, Deletions: 2.

@@ -3,6 +3,7 @@
 # found in the LICENSE file.
 
 import("//build/config/android/rules.gni")
+import("//extensions/buildflags/buildflags.gni")
 import("//third_party/jni_zero/jni_zero.gni")
 
 source_set("android") {
@@ -10,8 +11,6 @@
     "extensions_url_override_registry_manager.cc",
     "extensions_url_override_registry_manager.h",
     "extensions_url_override_state_tracker.h",
-    "extensions_url_override_state_tracker_impl.cc",
-    "extensions_url_override_state_tracker_impl.h",
   ]
 
   deps = [
@@ -19,8 +18,21 @@
     "//base",
     "//chrome/browser/profiles:profile",
     "//content/public/browser",
+    "//extensions/buildflags",
     "//third_party/jni_zero",
   ]
+
+  if (enable_extensions) {
+    sources += [
+      "extensions_url_override_state_tracker_impl.cc",
+      "extensions_url_override_state_tracker_impl.h",
+    ]
+
+    deps += [
+      "//extensions/browser:browser_sources",
+      "//extensions/common",
+    ]
+  }
 }
 
 generate_jni("jni_headers") {
```

Change information

Commit message:

[URL Override] Extensions URL Overrides State Tracking

- Creates several classes to manage state tracking.
- Managers own a state tracker, which performs state tracking on their
	behalf. The state tracker observers the
	ExtensionsWebUiRegistrar.
- URL Override State tracking is done separately for incognito-enabled
	extensions.
- Only initialize state tracker if extensions are enabled.

Bug: 445719491

Change-Id: Ib338536ff42df268aa8319065865bc947aed322b

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

Reviewed-by: Sky Malice <sk...@chromium.org>

Commit-Queue: Fiaz Muhammad <mf...@google.com>

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

Files:

M chrome/browser/android/extensions/BUILD.gn
M chrome/browser/android/extensions/extensions_url_override_registry_manager.cc
M chrome/browser/android/extensions/extensions_url_override_registry_manager.h
A chrome/browser/android/extensions/extensions_url_override_state_tracker.h
A chrome/browser/android/extensions/extensions_url_override_state_tracker_impl.cc
A chrome/browser/android/extensions/extensions_url_override_state_tracker_impl.h
M chrome/browser/android/extensions/java/src/org/chromium/chrome/browser/extensions/ExtensionsUrlOverrideRegistryManager.java

Change size: L

Delta: 7 files changed, 331 insertions(+), 9 deletions(-)

Branch: refs/heads/main

Submit Requirements:

Code-Review: +1 by Sky Malice

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

[URL Override] Extensions URL Overrides State Tracking [chromium/src : main]

Sky Malice (Gerrit)

Sky Malice voted and added 12 comments

Votes added by Sky Malice

12 comments

Related details

Fiaz Muhammad (Gerrit)

Fiaz Muhammad added 11 comments

Related details

Fiaz Muhammad (Gerrit)

Fiaz Muhammad voted Commit-Queue+2

Related details

Fiaz Muhammad (Gerrit)

Fiaz Muhammad voted Commit-Queue+2

Related details

Fiaz Muhammad (Gerrit)

Fiaz Muhammad voted Commit-Queue+2

Related details

Chromium LUCI CQ (Gerrit)

Chromium LUCI CQ submitted the change with unreviewed changes

Unreviewed changes

Change information