Implement BrowserCollectionObserver and ProfileBrowserCollection. [chromium/src : main]

0 views
Skip to first unread message

Tom Lukaszewicz (Gerrit)

unread,
Nov 9, 2025, 5:45:47 PMNov 9
to Glenn Hartmann, Chromium LUCI CQ, chromium...@chromium.org, dewitt...@chromium.org, asvitki...@chromium.org, chrome-gr...@chromium.org, chromium-a...@chromium.org, chromiumme...@microsoft.com, dmurph+watchin...@chromium.org, dmurph+wat...@chromium.org, grt+...@chromium.org, loyso...@chromium.org, mac-r...@chromium.org, mgiuca...@chromium.org, webap...@microsoft.com, jatapiaro+wat...@google.com, lwinston+watc...@google.com, rginda...@chromium.org, trewin...@google.com
Attention needed from Glenn Hartmann

Tom Lukaszewicz voted and added 7 comments

Votes added by Tom Lukaszewicz

Code-Review+1

7 comments

Patchset-level comments
File-level comment, Patchset 17:
Glenn Hartmann . resolved

Also did a few renames:

  • looks like we're agreed on the new class names (ProfileBrowserCollection, GlobalBrowserCollection, BrowserCollectionObserver) so I moved ProfileBrowsers -> ProfileBrowserCollection
  • also renamed some methods as Dana suggested: OnBrowserWasCreated -> OnBrowserCreated, OnBrowserWasClosed -> OnBrowserClosed, OnBrowserDidBecomeActive -> OnBrowserActivated, and OnBrowserDidBecomeInactive -> OnBrowserDeactivated
Tom Lukaszewicz

Good stuff - new names sgtm!

File-level comment, Patchset 18 (Latest):
Tom Lukaszewicz . resolved

still lgtm (just a few nits)

File chrome/browser/ui/browser_manager_service.h
Line 61, Patchset 18 (Latest): std::vector<std::unique_ptr<BrowserAndSubscriptions>>
browsers_and_subscriptions_;
Tom Lukaszewicz . unresolved

nit: We should be able to make this
```
std::vector<BrowserAndSubscriptions> browsers_and_subscriptions_;
```

Line 59, Patchset 18 (Latest): std::pair<base::CallbackListSubscription,
base::CallbackListSubscription>>;
Tom Lukaszewicz . unresolved
optional: We could make this a vector (since we don't care about the specific subscriptions and it simplifies the typedef slightly) - though playing around with construction it didn't seem easy with a `std::vector`, the simplest I could make `AddBrowser()` was
```
  std::vector<base::CallbackListSubscription> subscriptions;
  subscriptions.push_back(browser->RegisterDidBecomeActive(base::BindRepeating(
      &BrowserManagerService::OnBrowserActivated, base::Unretained(this))));
  subscriptions.push_back(browser->RegisterDidBecomeActive(base::BindRepeating(
      &BrowserManagerService::OnBrowserDeactivated, base::Unretained(this))));
  browsers_and_subscriptions_.push_back(
      BrowserAndSubscriptions(std::move(browser), std::move(subscriptions)));
```
so not sure if worth it (up to your judgement)
File chrome/browser/ui/browser_manager_service.cc
Line 22, Patchset 18 (Latest): std::make_unique<BrowserAndSubscriptions>(
Tom Lukaszewicz . unresolved

nit: If we eliminate the `unique_ptr` from the vector above this becomes `std::pair` I believe

Line 43, Patchset 18 (Latest): std::unique_ptr<BrowserAndSubscriptions> target_browser_and_subscriptions;
Tom Lukaszewicz . unresolved

nit: This can become a `std::optional` (assuming change to `browsers_and_subscriptions_` above)

File chrome/browser/ui/browser_window/public/browser_collection_observer.h
Line 13, Patchset 18 (Latest): public:
Tom Lukaszewicz . unresolved

nit: We should add a defaulted overridden destructor here also

Open in Gerrit

Related details

Attention is currently required from:
  • Glenn Hartmann
Submit Requirements:
  • requirement satisfiedCode-Coverage
  • requirement satisfiedCode-Owners
  • requirement satisfiedCode-Review
  • requirement is not satisfiedNo-Unresolved-Comments
  • requirement satisfiedReview-Enforcement
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I985ca0ceb68c72d97ea5721951fcc81d6a6a6964
Gerrit-Change-Number: 7088443
Gerrit-PatchSet: 18
Gerrit-Owner: Glenn Hartmann <hart...@chromium.org>
Gerrit-Reviewer: Glenn Hartmann <hart...@chromium.org>
Gerrit-Reviewer: Tom Lukaszewicz <tl...@chromium.org>
Gerrit-Attention: Glenn Hartmann <hart...@chromium.org>
Gerrit-Comment-Date: Sun, 09 Nov 2025 22:45:39 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Comment-In-Reply-To: Glenn Hartmann <hart...@chromium.org>
satisfied_requirement
unsatisfied_requirement
open
diffy

Glenn Hartmann (Gerrit)

unread,
Nov 10, 2025, 10:38:26 AMNov 10
to Tom Lukaszewicz, Chromium LUCI CQ, chromium...@chromium.org, dewitt...@chromium.org, asvitki...@chromium.org, chrome-gr...@chromium.org, chromium-a...@chromium.org, chromiumme...@microsoft.com, dmurph+watchin...@chromium.org, dmurph+wat...@chromium.org, grt+...@chromium.org, loyso...@chromium.org, mac-r...@chromium.org, mgiuca...@chromium.org, webap...@microsoft.com, jatapiaro+wat...@google.com, lwinston+watc...@google.com, rginda...@chromium.org, trewin...@google.com

Glenn Hartmann voted and added 5 comments

Votes added by Glenn Hartmann

Commit-Queue+2

5 comments

File chrome/browser/ui/browser_manager_service.h
Line 61, Patchset 18: std::vector<std::unique_ptr<BrowserAndSubscriptions>>
browsers_and_subscriptions_;
Tom Lukaszewicz . resolved

nit: We should be able to make this
```
std::vector<BrowserAndSubscriptions> browsers_and_subscriptions_;
```

Glenn Hartmann

Done

Line 59, Patchset 18: std::pair<base::CallbackListSubscription,
base::CallbackListSubscription>>;
Tom Lukaszewicz . resolved
optional: We could make this a vector (since we don't care about the specific subscriptions and it simplifies the typedef slightly) - though playing around with construction it didn't seem easy with a `std::vector`, the simplest I could make `AddBrowser()` was
```
  std::vector<base::CallbackListSubscription> subscriptions;
  subscriptions.push_back(browser->RegisterDidBecomeActive(base::BindRepeating(
      &BrowserManagerService::OnBrowserActivated, base::Unretained(this))));
  subscriptions.push_back(browser->RegisterDidBecomeActive(base::BindRepeating(
      &BrowserManagerService::OnBrowserDeactivated, base::Unretained(this))));
  browsers_and_subscriptions_.push_back(
      BrowserAndSubscriptions(std::move(browser), std::move(subscriptions)));
```
so not sure if worth it (up to your judgement)
Glenn Hartmann

I prefer it this way, personally (using pair over vector). The typedef is a bit more complex, yes, but it feels weird to me to use a dynamically-sized type when we know there will always be exactly 2 items. I think it would be (slightly) misleading for a reader to see a vector - they could assume there could be any number of subscriptions.

File chrome/browser/ui/browser_manager_service.cc
Line 22, Patchset 18: std::make_unique<BrowserAndSubscriptions>(
Tom Lukaszewicz . resolved

nit: If we eliminate the `unique_ptr` from the vector above this becomes `std::pair` I believe

Glenn Hartmann

Done

Line 43, Patchset 18: std::unique_ptr<BrowserAndSubscriptions> target_browser_and_subscriptions;
Tom Lukaszewicz . resolved

nit: This can become a `std::optional` (assuming change to `browsers_and_subscriptions_` above)

Glenn Hartmann

Done

File chrome/browser/ui/browser_window/public/browser_collection_observer.h
Line 13, Patchset 18: public:
Tom Lukaszewicz . resolved

nit: We should add a defaulted overridden destructor here also

Glenn Hartmann

Done

Open in Gerrit

Related details

Attention set is empty
Submit Requirements:
    • requirement satisfiedCode-Coverage
    • requirement satisfiedCode-Owners
    • requirement satisfiedCode-Review
    • requirement satisfiedReview-Enforcement
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: I985ca0ceb68c72d97ea5721951fcc81d6a6a6964
    Gerrit-Change-Number: 7088443
    Gerrit-PatchSet: 19
    Gerrit-Owner: Glenn Hartmann <hart...@chromium.org>
    Gerrit-Reviewer: Glenn Hartmann <hart...@chromium.org>
    Gerrit-Reviewer: Tom Lukaszewicz <tl...@chromium.org>
    Gerrit-Comment-Date: Mon, 10 Nov 2025 15:38:20 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: Yes
    Comment-In-Reply-To: Tom Lukaszewicz <tl...@chromium.org>
    satisfied_requirement
    open
    diffy

    Chromium LUCI CQ (Gerrit)

    unread,
    Nov 10, 2025, 11:23:48 AMNov 10
    to Glenn Hartmann, Tom Lukaszewicz, chromium...@chromium.org, dewitt...@chromium.org, asvitki...@chromium.org, chrome-gr...@chromium.org, chromium-a...@chromium.org, chromiumme...@microsoft.com, dmurph+watchin...@chromium.org, dmurph+wat...@chromium.org, grt+...@chromium.org, loyso...@chromium.org, mac-r...@chromium.org, mgiuca...@chromium.org, webap...@microsoft.com, jatapiaro+wat...@google.com, lwinston+watc...@google.com, rginda...@chromium.org, trewin...@google.com

    Chromium LUCI CQ submitted the change with unreviewed changes

    Unreviewed changes

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

    ```
    The name of the file: chrome/browser/ui/browser_manager_service.cc
    Insertions: 12, Deletions: 14.

    The diff is too large to show. Please review the diff.
    ```
    ```
    The name of the file: chrome/browser/ui/browser_manager_service.h
    Insertions: 1, Deletions: 2.

    The diff is too large to show. Please review the diff.
    ```
    ```
    The name of the file: chrome/browser/ui/browser_window/public/browser_collection_observer.h
    Insertions: 2, Deletions: 0.

    The diff is too large to show. Please review the diff.
    ```

    Change information

    Commit message:
    Implement BrowserCollectionObserver and ProfileBrowserCollection.

    `BrowserCollectionObserver` is intended to be a direct replacement for
    `BrowserListObserver` (but for `BrowserWindowInterface` instead of
    `Browser`).

    `ProfileBrowserCollection` represents an observable collection of all
    BrowserWindowInterface objects **in a given profile**. In a future CL,
    we'll add a `GlobalBrowserCollection` class as well, which will be a more direct
    equivalent of the current BrowserList but for BrowserWindowInterfaces.

    See
    https://docs.google.com/document/d/1aQRPDX9RjWHE48rAHntsV64VU1-4uZO_nkL6A7ohr2k/edit?tab=t.5u9lkywxkk2n
    for design details.
    Bug: 458021720
    Change-Id: I985ca0ceb68c72d97ea5721951fcc81d6a6a6964
    Commit-Queue: Glenn Hartmann <hart...@chromium.org>
    Reviewed-by: Tom Lukaszewicz <tl...@chromium.org>
    Cr-Commit-Position: refs/heads/main@{#1542570}
    Files:
    • M chrome/browser/ui/BUILD.gn
    • M chrome/browser/ui/browser_manager_service.cc
    • M chrome/browser/ui/browser_manager_service.h
    • A chrome/browser/ui/browser_manager_service_browsertest.cc
    • M chrome/browser/ui/browser_window/BUILD.gn
    • M chrome/browser/ui/browser_window/internal/BUILD.gn
    • A chrome/browser/ui/browser_window/internal/profile_browser_collection.cc
    • A chrome/browser/ui/browser_window/public/browser_collection_observer.h
    • A chrome/browser/ui/browser_window/public/profile_browser_collection.h
    • M chrome/test/BUILD.gn
    Change size: L
    Delta: 10 files changed, 304 insertions(+), 10 deletions(-)
    Branch: refs/heads/main
    Submit Requirements:
    • requirement satisfiedCode-Review: +1 by Tom Lukaszewicz
    Open in Gerrit
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: merged
    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: I985ca0ceb68c72d97ea5721951fcc81d6a6a6964
    Gerrit-Change-Number: 7088443
    Gerrit-PatchSet: 20
    Gerrit-Owner: Glenn Hartmann <hart...@chromium.org>
    Gerrit-Reviewer: Chromium LUCI CQ <chromiu...@luci-project-accounts.iam.gserviceaccount.com>
    Gerrit-Reviewer: Glenn Hartmann <hart...@chromium.org>
    Gerrit-Reviewer: Tom Lukaszewicz <tl...@chromium.org>
    open
    diffy
    satisfied_requirement
    Reply all
    Reply to author
    Forward
    0 new messages