[bedrock] Migrate BrowserList begin/end Part 1. [chromium/src : main]

0 views

Skip to first unread message

Qikai Zhong (Gerrit)

unread,

Oct 7, 2025, 12:18:56 PM (7 days ago) Oct 7

to AyeAye, Tom Lukaszewicz, Chromium LUCI CQ, chromium...@chromium.org, weiluanw...@google.com, dmurph+watc...@chromium.org, mgiuca...@chromium.org, oshima...@chromium.org, dmurph+wat...@chromium.org, webap...@microsoft.com, menghua...@google.com, dtraino...@chromium.org, extension...@chromium.org, kuragin+web-ap...@chromium.org, dennyh...@google.com, zelin+watch-we...@chromium.org, mac-r...@chromium.org, permissio...@chromium.org, dibyapal+wa...@chromium.org, devtools...@chromium.org, dullweb...@chromium.org, chromium-a...@chromium.org, chromeos-kio...@google.com, msrame...@chromium.org, chungshe...@google.com, mfoltz+wa...@chromium.org, cbe-cep-eng...@google.com, loyso...@chromium.org, philli...@chromium.org, byronle...@chromium.org

Attention needed from Tom Lukaszewicz

Qikai Zhong added 2 comments

File chrome/browser/ui/webui/webui_util_desktop.cc

Line 131, Patchset 2: BrowserWindowInterface* const browser_window_interface =

Tom Lukaszewicz . resolved

nit: I think this can be `const BrowserWindowInterface*`

Qikai Zhong

Initially, because `GetBrowserForMigrationOnly` was not marked as const, can not to use `const BrowserWindowInterface*` here. However, since other scenarios also required a const version of `GetBrowserForMigrationOnly`, I added `const Browser* GetBrowserForMigrationOnly() const = 0;`. And updated here.

Line 134, Patchset 2:

  if (!browser_window_interface) {
    return nullptr;
  }

  Browser* const last_active_browser =
      browser_window_interface->GetBrowserForMigrationOnly();

  return last_active_browser->window()->GetThemeProvider();

Tom Lukaszewicz . resolved

optional: 
```
return browser_window_interface
 ? browser_window_interface->GetBrowserForMigrationOnly()
     ->window()->GetThemeProvider()
 : nullptr;
```

Qikai Zhong

Done

Open in Gerrit

Related details

Attention is currently required from:

Tom Lukaszewicz

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

Qikai Zhong (Gerrit)

unread,

Oct 9, 2025, 4:17:23 AM (5 days ago) Oct 9

to Alex Carutasu, AyeAye, Tom Lukaszewicz, Chromium LUCI CQ, chromium...@chromium.org, weiluanw...@google.com, dmurph+watc...@chromium.org, mgiuca...@chromium.org, oshima...@chromium.org, dmurph+wat...@chromium.org, webap...@microsoft.com, menghua...@google.com, dtraino...@chromium.org, extension...@chromium.org, kuragin+web-ap...@chromium.org, dennyh...@google.com, zelin+watch-we...@chromium.org, mac-r...@chromium.org, permissio...@chromium.org, dibyapal+wa...@chromium.org, devtools...@chromium.org, dullweb...@chromium.org, chromium-a...@chromium.org, chromeos-kio...@google.com, msrame...@chromium.org, chungshe...@google.com, mfoltz+wa...@chromium.org, cbe-cep-eng...@google.com, loyso...@chromium.org, philli...@chromium.org, byronle...@chromium.org

Attention needed from Alex Carutasu and Tom Lukaszewicz

Qikai Zhong added 1 comment

Patchset-level comments

File-level comment, Patchset 15 (Latest):

Qikai Zhong . resolved

CQ Dry Run passed, please help review, thanks~
And can I ignore Code Coverage Check since it is just a migrate CL?

Open in Gerrit

Related details

Attention is currently required from:

Alex Carutasu
Tom Lukaszewicz

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

Tom Lukaszewicz (Gerrit)

unread,

Oct 9, 2025, 6:14:15 PM (4 days ago) Oct 9

to Qikai Zhong, Alex Carutasu, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, weiluanw...@google.com, dmurph+watc...@chromium.org, mgiuca...@chromium.org, oshima...@chromium.org, dmurph+wat...@chromium.org, webap...@microsoft.com, menghua...@google.com, dtraino...@chromium.org, extension...@chromium.org, kuragin+web-ap...@chromium.org, dennyh...@google.com, zelin+watch-we...@chromium.org, mac-r...@chromium.org, permissio...@chromium.org, dibyapal+wa...@chromium.org, devtools...@chromium.org, dullweb...@chromium.org, chromium-a...@chromium.org, chromeos-kio...@google.com, msrame...@chromium.org, chungshe...@google.com, mfoltz+wa...@chromium.org, cbe-cep-eng...@google.com, loyso...@chromium.org, philli...@chromium.org, byronle...@chromium.org

Attention needed from Alex Carutasu and Qikai Zhong

Tom Lukaszewicz added 14 comments

Patchset-level comments

File-level comment, Patchset 15 (Latest):

Tom Lukaszewicz . resolved

Overall looks good - just some initial comments.

Also would we be able to split the chrome/browser/ash and chrome/browser/chromeos changes into a separate CL? (it will make reviews easier as DEPS files will require a specific ChromeOS owner)

File chrome/browser/app_controller_mac.mm

Line 969, Patchset 15 (Latest): if (!has_browser_windows &&

Tom Lukaszewicz . unresolved

We could also use `GetLastActiveBrowserWindowInterfaceWithAnyProfile()` here (which will return nullptr if there are no open browsers)

File chrome/browser/ash/child_accounts/time_limits/web_time_activity_provider.h

Line 90, Patchset 15 (Latest): std::set<BrowserWindowInterface*> active_browsers_;

Tom Lukaszewicz . unresolved

Are we able to preserve const for the set here? (i.e. `std::set<const BrowserWindowInterface*>`?

It looks like it may be possible (we'd need to preserve const in the returned pointers of the anonymous functions and const on some of the pointer variables in the method impls).

File chrome/browser/chromeos/app_mode/kiosk_browser_window_handler.cc

Line 391, Patchset 15 (Latest):

                               ? browser_window_interface
                                     ->GetActiveTabInterface()
                                     ->GetContents()
                                     ->GetVisibleURL()
                                     .spec()
                               : "unknown")

Tom Lukaszewicz . unresolved

nit: Could we update `GetUrlOfActiveTab()` to take a `BrowserWindowInterface`? It's a relatively simple function and should clean this up slightly.

File chrome/browser/chromeos/extensions/login_screen/login/cleanup/browser_cleanup_handler.cc

Line 88, Patchset 15 (Latest): if (any_open) {

Tom Lukaszewicz . unresolved

I think we should just be able to use `HasBrowsersForProfile(browser->GetProfile())` here

File chrome/browser/chromeos/extensions/telemetry/api/events/events_api_browsertest.cc

Line 720, Patchset 15 (Latest): is_diagnostic_app_open = true;

Tom Lukaszewicz . unresolved

nit: Should we also return false here?

File chrome/browser/chromeos/policy/dlp/dlp_content_manager.cc

Line 532, Patchset 15 (Latest):

  ForEachCurrentBrowserWindowInterfaceOrderedByActivation(
      [this](BrowserWindowInterface* browser_window_interface) {
        browser_window_interface->GetTabStripModel()->RemoveObserver(this);
        return true;
      });

Tom Lukaszewicz . unresolved

nit: We should be able to elide the `TabStripModel::RemoveObserver()` call completely (see destructor [here](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/tabs/tab_strip_model_observer.cc;l=342;drc=ff17d15d2d56c9a26f8f696d0fa05b845f5db386)).

File chrome/browser/devtools/devtools_browser_context_manager.cc

Line 131, Patchset 15 (Latest):

  std::vector<BrowserWindowInterface*> browsers_to_close;
  ForEachCurrentBrowserWindowInterfaceOrderedByActivation(
      [profile,
       &browsers_to_close](BrowserWindowInterface* browser_window_interface) {
        if (browser_window_interface->GetProfile() == profile) {
          browsers_to_close.push_back(browser_window_interface);
        }
        return true;
      });
  for (BrowserWindowInterface* browser_window_interface : browsers_to_close) {
    browser_window_interface->GetWindow()->Close();
  }

Tom Lukaszewicz . unresolved

nit: It looks like it should just work to close the browsers in the iteration loop (since it handles cases where Browsers are destroyed during iteration). i.e.
```
  ForEachCurrentBrowserWindowInterfaceOrderedByActivation(
      [profile,
       &browsers_to_close](BrowserWindowInterface* browser_window_interface) {
        if (browser_window_interface->GetProfile() == profile) {
          browser_window_interface->GetWindow()->Close();
        }
        return true;
      });
```

File chrome/browser/devtools/protocol/browser_handler.cc

Line 52, Patchset 15 (Latest): ui::BaseWindow* window) {

Tom Lukaszewicz . resolved

nice!

File chrome/browser/download/download_ui_controller.cc

Line 140, Patchset 15 (Latest):

        if (browser_window_interface &&
            browser_window_interface->GetBrowserForMigrationOnly()
                ->window()
                ->GetDownloadBubbleUIController()) {
          browser_window_interface->GetBrowserForMigrationOnly()
              ->window()
              ->GetDownloadBubbleUIController()
              ->HandleButtonPressed();
        }
        return true;

Tom Lukaszewicz . unresolved

nit: We can get this from `BrowserWindowFeatures` also.
```
  DownloadToolbarUIController* download_controller =
    browser_window_interface->GetFeatures().download_toolbar_ui_controller());
  DownloadBubbleUIController* bubble_ui_controller = download_controller
    ? download_controller->bubble_controller()
    : nullptr;
  if (bubble_ui_controller) {
    bubble_ui_controller->HandleButtonPressed();
  }
```

File chrome/browser/extensions/api/management/management_apitest.cc

Line 82, Patchset 15 (Latest):

BrowserWindowInterface* FindOtherBrowserWindowInterface(Browser* browser) {
  BrowserWindowInterface* found = nullptr;
  ForEachCurrentBrowserWindowInterfaceOrderedByActivation(
      [browser, &found](BrowserWindowInterface* browser_window_interface) {
        if (browser_window_interface == browser) {
          return true;
        }
        found = browser_window_interface;
        return false;
      });
  return found;
}

Tom Lukaszewicz . unresolved

We should remove this and update clients to use `ui_test_utils::GetBrowserNotInSet({browser})` instead.

File chrome/browser/extensions/api/tabs/tabs_test.cc

Line 1316, Patchset 15 (Latest):

    BrowserWindowInterface* iwa_browser = nullptr;
    ForEachCurrentBrowserWindowInterfaceOrderedByActivation(
        [&iwa_browser](BrowserWindowInterface* browser_window_interface) {
          iwa_browser = browser_window_interface;
          return false;
        });

Tom Lukaszewicz . unresolved

nit: We should be able to just use `GetLastActiveBrowserWindowInterfaceWithAnyProfile();`

File chrome/browser/extensions/window_open_apitest.cc

Line 87, Patchset 15 (Latest): ForEachCurrentBrowserWindowInterfaceOrderedByActivation(

Tom Lukaszewicz . unresolved

We could add the following utility to the fixture
```
  int CountBrowsersForType(BrowserWindowInterface::Type type) {
    return ui_test_utils::FindMatchingBrowsers(
        [type](BrowserWindowInterface* browser) {
          return browser->GetType() == type;
        });
  }
```

Then below becomes 
```
  EXPECT_EQ(num_popups,
    CountBrowsersForType(BrowserWindowInterface::Type::TYPE_POPUP);
  EXPECT_EQ(num_app_popups,
    CountBrowsersForType(BrowserWindowInterface::Type::TYPE_APP_POPUP);
```

Also the test below can be
```
const int expected_app_popups = BrowserList::GetInstance::size() - 1;
EXPECT_NE(browser()->GetType(), BrowserWindowInterface::Type::TYPE_APP_POPUP);
EXPECT_EQ(expected_app_popups,
  CountBrowsersForType(BrowserWindowInterface::Type::TYPE_APP_POPUP)
```

File chrome/browser/fast_shutdown_browsertest.cc

Line 52, Patchset 15 (Latest):


  ui_test_utils::NavigateToURLWithDisposition(
      browser(), url, WindowOpenDisposition::NEW_FOREGROUND_TAB,
      ui_test_utils::BROWSER_TEST_NO_WAIT);
  ui_test_utils::WaitForBrowserToOpen();

Tom Lukaszewicz . unresolved

nit: We should wait for the creation of the second Browser as follows
```
  ui_test_utils::BrowserCreatedObserver browser_created_observer;
  ui_test_utils::NavigateToURLWithDisposition(
      browser(), url, WindowOpenDisposition::NEW_FOREGROUND_TAB,
      ui_test_utils::BROWSER_TEST_NO_WAIT);
  BrowserWindowInterface* second_browser = browser_created_observer.Wait();
  EXECT_TRUE(second_browser);
  chrome::CloseWindow(second_browser);
```

Open in Gerrit

Related details

Attention is currently required from:

Alex Carutasu
Qikai Zhong

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

Alex Carutasu (Gerrit)

unread,

Oct 9, 2025, 10:13:30 PM (4 days ago) Oct 9

to Qikai Zhong, AyeAye, Tom Lukaszewicz, Chromium LUCI CQ, chromium...@chromium.org, weiluanw...@google.com, dmurph+watc...@chromium.org, mgiuca...@chromium.org, oshima...@chromium.org, dmurph+wat...@chromium.org, webap...@microsoft.com, menghua...@google.com, dtraino...@chromium.org, extension...@chromium.org, kuragin+web-ap...@chromium.org, dennyh...@google.com, zelin+watch-we...@chromium.org, mac-r...@chromium.org, permissio...@chromium.org, dibyapal+wa...@chromium.org, devtools...@chromium.org, dullweb...@chromium.org, chromium-a...@chromium.org, chromeos-kio...@google.com, msrame...@chromium.org, chungshe...@google.com, mfoltz+wa...@chromium.org, cbe-cep-eng...@google.com, loyso...@chromium.org, philli...@chromium.org, byronle...@chromium.org

Attention needed from Qikai Zhong

Alex Carutasu added 27 comments

Patchset-level comments

File-level comment, Patchset 15 (Latest):

Alex Carutasu . resolved

Thanks for the changes, left some comments.

File chrome/browser/apps/app_service/metrics/website_metrics.cc

Line 69, Patchset 15 (Latest):

          ui::BaseWindow* base_window = browser_window_interface->GetWindow();
          found_window = base_window ? base_window->GetNativeWindow() : nullptr;

Alex Carutasu . unresolved

nit:


```suggestion
          if (ui::BaseWindow* const base_window =
              browser_window_interface->GetWindow()) {
            found_window = base_window->GetNativeWindow();
          }
```

File chrome/browser/ash/app_restore/full_restore_app_launch_handler_browsertest.cc

Line 336, Patchset 15 (Latest): aura::Window* window =

Alex Carutasu . unresolved

nit:


```suggestion
          aura::Window* const window =
```

Line 2632, Patchset 15 (Latest): BrowserWindowInterface* restore_app_browser =

Alex Carutasu . unresolved

nit (also applicable to other BWI* decls below in this file):
```suggestion
  BrowserWindowInterface* const restore_app_browser =
```

Line 2975, Patchset 15 (Latest): aura::Window* native_window =

Alex Carutasu . unresolved

nit: 
```suggestion
        aura::Window* const native_window =
```

File chrome/browser/ash/child_accounts/time_limits/web_time_activity_provider.cc

Line 82, Patchset 15 (Latest): const auto* tab_strip_model =

Alex Carutasu . unresolved

nit:


```suggestion
        const auto* const tab_strip_model =
```

Line 135, Patchset 15 (Latest): BrowserWindowInterface* browser_window_interface =

Alex Carutasu . unresolved

nit (also applicable to other BWI* decls below in this file):
```suggestion
  BrowserWindowInterface* const browser_window_interface =
```

File chrome/browser/browser_process_platform_part_ash_browsertest.cc

Line 70, Patchset 15 (Latest): TabStripModel* tab_strip_model = browser_window_interface->GetTabStripModel();

Alex Carutasu . unresolved

nit:
```suggestion
  TabStripModel* const tab_strip_model = browser_window_interface->GetTabStripModel();
```

Line 126, Patchset 15 (Latest): auto* tab_strip_model = new_browser->GetTabStripModel();

Alex Carutasu . unresolved

nit:
```suggestion
  auto* const tab_strip_model = new_browser->GetTabStripModel();
```

File chrome/browser/browsing_data/navigation_entry_remover_browsertest.cc

Line 90, Patchset 15 (Latest): GetAllBrowserWindowInterfaces();

Alex Carutasu . unresolved

curious: why can't this use `ForEachCurrentBrowserWindowInterfaceOrderedByActivation`?

File chrome/browser/chromeos/app_mode/kiosk_browser_window_handler.cc

Line 400, Patchset 15 (Latest): CloseBrowserAndSetTimer(

Alex Carutasu . unresolved

I think this function and `closing_browsers_` could be trivially updated to take/use `BrowserWindowInterface` as well.

File chrome/browser/chromeos/extensions/telemetry/api/events/events_api_browsertest.cc

Line 715, Patchset 15 (Latest): content::WebContents* target_contents =

Alex Carutasu . unresolved

nit:


```suggestion
          content::WebContents* const target_contents =
```

File chrome/browser/chromeos/policy/dlp/dlp_content_manager.cc

Line 525, Patchset 15 (Latest):

  BrowserList* browser_list = BrowserList::GetInstance();
  browser_list->AddObserver(this);

Alex Carutasu . unresolved

nit:


```suggestion
  BrowserList::GetInstance()->AddObserver(this);
```

File chrome/browser/chromeos/tablet_mode/tablet_mode_page_behavior.cc

Line 89, Patchset 15 (Latest): TabStripModel* tab_strip_model =

Alex Carutasu . unresolved

nit:


```suggestion
          TabStripModel* const tab_strip_model =
```

File chrome/browser/devtools/protocol/target_handler.cc

Line 131, Patchset 15 (Latest): if (!browser->IsAttemptingToCloseBrowser()) {

Alex Carutasu . unresolved

this is technically accessible through `GetTabStripModel()->delegate()->IsAttemptingToCloseBrowser();`

Swapping to this would allow changing `target_browser` to BWI* and moving the GetBrowserForMigrationOnly() down only to where `target_browser` is passed into the navigation params creation below.

Not sure if this is "cleaner" or not but wanted to call out the option.
Thoughts @tl...@chromium.org?

File chrome/browser/download/download_status_updater_win.cc

Line 45, Patchset 15 (Latest): ui::BaseWindow* window = browser_window_interface->GetWindow();

Alex Carutasu . unresolved

nit: 
```suggestion
        ui::BaseWindow* const window = browser_window_interface->GetWindow();
```

File chrome/browser/extensions/api/management/management_apitest.cc

Line 582, Patchset 15 (Latest):

  BrowserWindowInterface* app_browser_window =
      FindOtherBrowserWindowInterface(browser());
  ASSERT_TRUE(app_browser_window->GetType() ==
              BrowserWindowInterface::TYPE_APP);

Alex Carutasu . unresolved

nit:
```suggestion
  ASSERT_TRUE(FindOtherBrowserWindowInterface(browser())->GetType() ==
              BrowserWindowInterface::TYPE_APP);
```

File chrome/browser/extensions/extension_tab_util.cc

Line 834, Patchset 15 (Latest): TabStripModel* tab_strip = browser_window_interface->GetTabStripModel();

Alex Carutasu . unresolved

nit:


```suggestion
        TabStripModel* const tab_strip = browser_window_interface->GetTabStripModel();
```

Line 1228, Patchset 15 (Latest):

          TabStripModel* target_tab_strip =
              browser_window_interface->GetTabStripModel();
          active_contents.push_back(target_tab_strip->GetActiveWebContents());

Alex Carutasu . unresolved

nit:


```suggestion
          active_contents.push_back(browser_window_interface->GetTabStripModel()
				        ->GetActiveWebContents());
```

File chrome/browser/glic/host/context/glic_pinned_tab_manager.cc

Line 416, Patchset 15 (Latest): TabStripModel* tab_strip_model =

Alex Carutasu . unresolved

nit:


```suggestion
        TabStripModel* const tab_strip_model =
```

Line 419, Patchset 15 (Latest): auto* tab = tab_strip_model->GetTabAtIndex(i);

Alex Carutasu . unresolved

nit:


```suggestion
          auto* const tab = tab_strip_model->GetTabAtIndex(i);
```

File chrome/browser/glic/test_support/glic_test_util.cc

Line 56, Patchset 15 (Latest): ForEachCurrentBrowserWindowInterfaceOrderedByActivation(

Alex Carutasu . unresolved

Could this use `GetLastActiveBrowserWindowInterfaceWithAnyProfile()` instead of the loop?

Line 78, Patchset 15 (Latest): if (auto* browser_view =

Alex Carutasu . unresolved

nit:
```suggestion
  if (auto* const browser_view =
```

File chrome/browser/glic/widget/application_hotkey_delegate.cc

Line 58, Patchset 15 (Latest): if (auto* browser_view = BrowserView::GetBrowserViewForBrowser(

Alex Carutasu . unresolved

nit:


```suggestion
          if (auto* const browser_view = BrowserView::GetBrowserViewForBrowser(
```

Line 75, Patchset 15 (Latest): if (auto* browser_view =

Alex Carutasu . unresolved

nit:


```suggestion
    if (auto* const browser_view =
```

File chrome/browser/ui/web_applications/web_app_ui_manager_impl.cc

Line 191, Patchset 15 (Latest): GetAllBrowserWindowInterfaces()) {

Alex Carutasu . unresolved

Could this use `ForEachCurrentAndNewBrowserWindowInterfaceOrderedByActivation()`?

Line 231, Patchset 15 (Latest):

        const AppBrowserController* app_controller =
            browser_window_interface->GetFeatures().app_browser_controller();

Alex Carutasu . unresolved

nit:


```suggestion
        const AppBrowserController* const app_controller =
            browser_window_interface->GetAppBrowserController();
```

Open in Gerrit

Related details

Attention is currently required from:

Qikai Zhong

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

Tom Lukaszewicz (Gerrit)

unread,

Oct 10, 2025, 1:47:58 AM (4 days ago) Oct 10

Attention needed from Qikai Zhong

Tom Lukaszewicz added 1 comment

File chrome/browser/devtools/protocol/target_handler.cc

Line 131, Patchset 15: if (!browser->IsAttemptingToCloseBrowser()) {

Alex Carutasu . unresolved

this is technically accessible through `GetTabStripModel()->delegate()->IsAttemptingToCloseBrowser();`
Swapping to this would allow changing `target_browser` to BWI* and moving the GetBrowserForMigrationOnly() down only to where `target_browser` is passed into the navigation params creation below.
Not sure if this is "cleaner" or not but wanted to call out the option.
Thoughts @tl...@chromium.org?

Tom Lukaszewicz

This is true and Alex is right - though I suspect you meant we should go through `BWI::capabilities()` (I didn't realize this was on capabilities and this is a good suggestion)

satisfied_requirement

unsatisfied_requirement

open

diffy

Qikai Zhong (Gerrit)

unread,

Oct 10, 2025, 9:53:13 AM (4 days ago) Oct 10

Attention needed from Alex Carutasu and Tom Lukaszewicz

Qikai Zhong added 41 comments

Patchset-level comments

File-level comment, Patchset 22 (Latest):

Qikai Zhong . resolved

Thank you very much for your patience in reviewing. All comments have been fixed.
I have reset the files in the ash and chromeos folders. I will submit them separately in the second CL.

File chrome/browser/app_controller_mac.mm

Line 969, Patchset 15: if (!has_browser_windows &&

Tom Lukaszewicz . resolved

We could also use `GetLastActiveBrowserWindowInterfaceWithAnyProfile()` here (which will return nullptr if there are no open browsers)

Qikai Zhong

Marked as resolved.

File chrome/browser/apps/app_service/metrics/website_metrics.cc

Line 69, Patchset 15: ui::BaseWindow* base_window = browser_window_interface->GetWindow();


          found_window = base_window ? base_window->GetNativeWindow() : nullptr;

Alex Carutasu . resolved

nit:


```suggestion
          if (ui::BaseWindow* const base_window =
              browser_window_interface->GetWindow()) {
            found_window = base_window->GetNativeWindow();
          }
```

Qikai Zhong

Marked as resolved.

File chrome/browser/ash/app_restore/full_restore_app_launch_handler_browsertest.cc

Line 336, Patchset 15: aura::Window* window =

Alex Carutasu . resolved

nit:


```suggestion
          aura::Window* const window =
```

Qikai Zhong

Marked as resolved.

Line 2632, Patchset 15: BrowserWindowInterface* restore_app_browser =

Alex Carutasu . resolved

nit (also applicable to other BWI* decls below in this file):
```suggestion
  BrowserWindowInterface* const restore_app_browser =
```

Qikai Zhong

restore_app_browser will be reassigned later.

Line 2975, Patchset 15: aura::Window* native_window =

Alex Carutasu . resolved

nit: 
```suggestion
        aura::Window* const native_window =
```

Qikai Zhong

Marked as resolved.

File chrome/browser/ash/child_accounts/time_limits/web_time_activity_provider.h

Line 90, Patchset 15: std::set<BrowserWindowInterface*> active_browsers_;

Tom Lukaszewicz . resolved

Are we able to preserve const for the set here? (i.e. `std::set<const BrowserWindowInterface*>`?
It looks like it may be possible (we'd need to preserve const in the returned pointers of the anonymous functions and const on some of the pointer variables in the method impls).

Qikai Zhong

Done

File chrome/browser/ash/child_accounts/time_limits/web_time_activity_provider.cc

Line 82, Patchset 15: const auto* tab_strip_model =

Alex Carutasu . resolved

nit:


```suggestion
        const auto* const tab_strip_model =
```

Qikai Zhong

Marked as resolved.

Line 135, Patchset 15: BrowserWindowInterface* browser_window_interface =

Alex Carutasu . resolved

nit (also applicable to other BWI* decls below in this file):
```suggestion
  BrowserWindowInterface* const browser_window_interface =
```

Qikai Zhong

Marked as resolved.

File chrome/browser/browser_process_platform_part_ash_browsertest.cc

Line 70, Patchset 15: TabStripModel* tab_strip_model = browser_window_interface->GetTabStripModel();

Alex Carutasu . resolved

nit:
```suggestion
  TabStripModel* const tab_strip_model = browser_window_interface->GetTabStripModel();
```

Qikai Zhong

Marked as resolved.

Line 126, Patchset 15: auto* tab_strip_model = new_browser->GetTabStripModel();

Alex Carutasu . resolved

nit:
```suggestion
  auto* const tab_strip_model = new_browser->GetTabStripModel();
```

Qikai Zhong

Marked as resolved.

File chrome/browser/browsing_data/navigation_entry_remover_browsertest.cc

Line 90, Patchset 15: GetAllBrowserWindowInterfaces();

Alex Carutasu . resolved

curious: why can't this use `ForEachCurrentBrowserWindowInterfaceOrderedByActivation`?

Qikai Zhong

This test depends on the browser creation order.

File chrome/browser/chromeos/app_mode/kiosk_browser_window_handler.cc

Line 391, Patchset 15: ? browser_window_interface


                                     ->GetActiveTabInterface()
                                     ->GetContents()
                                     ->GetVisibleURL()
                                     .spec()
                               : "unknown")

Tom Lukaszewicz . resolved

nit: Could we update `GetUrlOfActiveTab()` to take a `BrowserWindowInterface`? It's a relatively simple function and should clean this up slightly.

Qikai Zhong

Marked as resolved.

Line 400, Patchset 15: CloseBrowserAndSetTimer(

Alex Carutasu . resolved

I think this function and `closing_browsers_` could be trivially updated to take/use `BrowserWindowInterface` as well.

Qikai Zhong

Done

File chrome/browser/chromeos/extensions/login_screen/login/cleanup/browser_cleanup_handler.cc

Line 88, Patchset 15: if (any_open) {

Tom Lukaszewicz . resolved

I think we should just be able to use `HasBrowsersForProfile(browser->GetProfile())` here

Qikai Zhong

Nice!

Line 88, Patchset 15: if (any_open) {

Tom Lukaszewicz . resolved

I think we should just be able to use `HasBrowsersForProfile(browser->GetProfile())` here

Qikai Zhong

Nice!

File chrome/browser/chromeos/extensions/telemetry/api/events/events_api_browsertest.cc

Line 715, Patchset 15: content::WebContents* target_contents =

Alex Carutasu . resolved

nit:


```suggestion
          content::WebContents* const target_contents =
```

Qikai Zhong

Marked as resolved.

Line 720, Patchset 15: is_diagnostic_app_open = true;

Tom Lukaszewicz . resolved

nit: Should we also return false here?

Qikai Zhong

Marked as resolved.

File chrome/browser/chromeos/policy/dlp/dlp_content_manager.cc

Line 525, Patchset 15:

  BrowserList* browser_list = BrowserList::GetInstance();
  browser_list->AddObserver(this);

Alex Carutasu . resolved

nit:


```suggestion
  BrowserList::GetInstance()->AddObserver(this);
```

Qikai Zhong

Marked as resolved.

Line 532, Patchset 15: ForEachCurrentBrowserWindowInterfaceOrderedByActivation(


      [this](BrowserWindowInterface* browser_window_interface) {
        browser_window_interface->GetTabStripModel()->RemoveObserver(this);
        return true;
      });

Tom Lukaszewicz . resolved

nit: We should be able to elide the `TabStripModel::RemoveObserver()` call completely (see destructor [here](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/tabs/tab_strip_model_observer.cc;l=342;drc=ff17d15d2d56c9a26f8f696d0fa05b845f5db386)).

Qikai Zhong

Marked as resolved.

File chrome/browser/chromeos/tablet_mode/tablet_mode_page_behavior.cc

Line 89, Patchset 15: TabStripModel* tab_strip_model =

Alex Carutasu . resolved

nit:


```suggestion
          TabStripModel* const tab_strip_model =

```

Qikai Zhong

Marked as resolved.

File chrome/browser/devtools/devtools_browser_context_manager.cc

Line 131, Patchset 15: std::vector<BrowserWindowInterface*> browsers_to_close;


  ForEachCurrentBrowserWindowInterfaceOrderedByActivation(
      [profile,
       &browsers_to_close](BrowserWindowInterface* browser_window_interface) {
        if (browser_window_interface->GetProfile() == profile) {
          browsers_to_close.push_back(browser_window_interface);
        }
        return true;
      });
  for (BrowserWindowInterface* browser_window_interface : browsers_to_close) {
    browser_window_interface->GetWindow()->Close();
  }

Tom Lukaszewicz . resolved

nit: It looks like it should just work to close the browsers in the iteration loop (since it handles cases where Browsers are destroyed during iteration). i.e.
```
  ForEachCurrentBrowserWindowInterfaceOrderedByActivation(
      [profile,
       &browsers_to_close](BrowserWindowInterface* browser_window_interface) {
        if (browser_window_interface->GetProfile() == profile) {
          browser_window_interface->GetWindow()->Close();
        }
        return true;
      });
```

Qikai Zhong

Marked as resolved.

File chrome/browser/devtools/protocol/target_handler.cc

Line 131, Patchset 15: if (!browser->IsAttemptingToCloseBrowser()) {

Alex Carutasu . resolved

this is technically accessible through `GetTabStripModel()->delegate()->IsAttemptingToCloseBrowser();`
Swapping to this would allow changing `target_browser` to BWI* and moving the GetBrowserForMigrationOnly() down only to where `target_browser` is passed into the navigation params creation below.
Not sure if this is "cleaner" or not but wanted to call out the option.
Thoughts @tl...@chromium.org?

Qikai Zhong

Yes, update to `browser_window_interface->capabilities()->IsAttemptingToCloseBrowser()`

File chrome/browser/download/download_status_updater_win.cc

Line 45, Patchset 15: ui::BaseWindow* window = browser_window_interface->GetWindow();

Alex Carutasu . resolved

nit: 
```suggestion
        ui::BaseWindow* const window = browser_window_interface->GetWindow();
```

Qikai Zhong

Marked as resolved.

File chrome/browser/download/download_ui_controller.cc

Line 140, Patchset 15: if (browser_window_interface &&


            browser_window_interface->GetBrowserForMigrationOnly()
                ->window()
                ->GetDownloadBubbleUIController()) {
          browser_window_interface->GetBrowserForMigrationOnly()
              ->window()
              ->GetDownloadBubbleUIController()
              ->HandleButtonPressed();
        }
        return true;

Tom Lukaszewicz . resolved

nit: We can get this from `BrowserWindowFeatures` also.
```
  DownloadToolbarUIController* download_controller =
    browser_window_interface->GetFeatures().download_toolbar_ui_controller());
  DownloadBubbleUIController* bubble_ui_controller = download_controller
    ? download_controller->bubble_controller()
    : nullptr;
  if (bubble_ui_controller) {
    bubble_ui_controller->HandleButtonPressed();
  }
```

Qikai Zhong

Marked as resolved.

File chrome/browser/extensions/api/management/management_apitest.cc

Line 82, Patchset 15:BrowserWindowInterface* FindOtherBrowserWindowInterface(Browser* browser) {


  BrowserWindowInterface* found = nullptr;
  ForEachCurrentBrowserWindowInterfaceOrderedByActivation(
      [browser, &found](BrowserWindowInterface* browser_window_interface) {
        if (browser_window_interface == browser) {
          return true;
        }
        found = browser_window_interface;
        return false;
      });
  return found;
}

Tom Lukaszewicz . resolved

We should remove this and update clients to use `ui_test_utils::GetBrowserNotInSet({browser})` instead.

Qikai Zhong

Marked as resolved.

Line 582, Patchset 15: BrowserWindowInterface* app_browser_window =


      FindOtherBrowserWindowInterface(browser());
  ASSERT_TRUE(app_browser_window->GetType() ==
              BrowserWindowInterface::TYPE_APP);

Alex Carutasu . resolved

nit:
```suggestion
  ASSERT_TRUE(FindOtherBrowserWindowInterface(browser())->GetType() ==
              BrowserWindowInterface::TYPE_APP);
```

Qikai Zhong

app_browser_window is used in many places later.

File chrome/browser/extensions/api/tabs/tabs_test.cc

Line 1316, Patchset 15: BrowserWindowInterface* iwa_browser = nullptr;


    ForEachCurrentBrowserWindowInterfaceOrderedByActivation(
        [&iwa_browser](BrowserWindowInterface* browser_window_interface) {
          iwa_browser = browser_window_interface;
          return false;
        });

Tom Lukaszewicz . resolved

nit: We should be able to just use `GetLastActiveBrowserWindowInterfaceWithAnyProfile();`

Qikai Zhong

Marked as resolved.

File chrome/browser/extensions/extension_tab_util.cc

Line 834, Patchset 15: TabStripModel* tab_strip = browser_window_interface->GetTabStripModel();

Alex Carutasu . resolved

nit:


```suggestion
        TabStripModel* const tab_strip = browser_window_interface->GetTabStripModel();
```

Qikai Zhong

Marked as resolved.

Line 1228, Patchset 15:

          TabStripModel* target_tab_strip =
              browser_window_interface->GetTabStripModel();
          active_contents.push_back(target_tab_strip->GetActiveWebContents());

Alex Carutasu . resolved

nit:


```suggestion
          active_contents.push_back(browser_window_interface->GetTabStripModel()
				        ->GetActiveWebContents());
```

Qikai Zhong

Marked as resolved.

File chrome/browser/extensions/window_open_apitest.cc

Line 87, Patchset 15: ForEachCurrentBrowserWindowInterfaceOrderedByActivation(

Tom Lukaszewicz . unresolved

We could add the following utility to the fixture
```
  int CountBrowsersForType(BrowserWindowInterface::Type type) {
    return ui_test_utils::FindMatchingBrowsers(
        [type](BrowserWindowInterface* browser) {
          return browser->GetType() == type;
        });
  }
```

Then below becomes 
```
  EXPECT_EQ(num_popups,
    CountBrowsersForType(BrowserWindowInterface::Type::TYPE_POPUP);
  EXPECT_EQ(num_app_popups,
    CountBrowsersForType(BrowserWindowInterface::Type::TYPE_APP_POPUP);
```

Also the test below can be
```
const int expected_app_popups = BrowserList::GetInstance::size() - 1;
EXPECT_NE(browser()->GetType(), BrowserWindowInterface::Type::TYPE_APP_POPUP);
EXPECT_EQ(expected_app_popups,
  CountBrowsersForType(BrowserWindowInterface::Type::TYPE_APP_POPUP)
```

Qikai Zhong

Done. But don't we also have to migrate `BrowseList::GetInstance::size()` in these days?

Line 87, Patchset 15: ForEachCurrentBrowserWindowInterfaceOrderedByActivation(

Tom Lukaszewicz . unresolved

We could add the following utility to the fixture
```
  int CountBrowsersForType(BrowserWindowInterface::Type type) {
    return ui_test_utils::FindMatchingBrowsers(
        [type](BrowserWindowInterface* browser) {
          return browser->GetType() == type;
        });
  }
```

Then below becomes 
```
  EXPECT_EQ(num_popups,
    CountBrowsersForType(BrowserWindowInterface::Type::TYPE_POPUP);
  EXPECT_EQ(num_app_popups,
    CountBrowsersForType(BrowserWindowInterface::Type::TYPE_APP_POPUP);
```

Also the test below can be
```
const int expected_app_popups = BrowserList::GetInstance::size() - 1;
EXPECT_NE(browser()->GetType(), BrowserWindowInterface::Type::TYPE_APP_POPUP);
EXPECT_EQ(expected_app_popups,
  CountBrowsersForType(BrowserWindowInterface::Type::TYPE_APP_POPUP)
```

Qikai Zhong

Done, But is it reasonable to continue using `BrowserList::GetInstance::size()` here? Don’t we want to migrate it eventually?

File chrome/browser/fast_shutdown_browsertest.cc

Line 52, Patchset 15:


  ui_test_utils::NavigateToURLWithDisposition(
      browser(), url, WindowOpenDisposition::NEW_FOREGROUND_TAB,
      ui_test_utils::BROWSER_TEST_NO_WAIT);
  ui_test_utils::WaitForBrowserToOpen();

Tom Lukaszewicz . resolved

nit: We should wait for the creation of the second Browser as follows
```
  ui_test_utils::BrowserCreatedObserver browser_created_observer;
  ui_test_utils::NavigateToURLWithDisposition(
      browser(), url, WindowOpenDisposition::NEW_FOREGROUND_TAB,
      ui_test_utils::BROWSER_TEST_NO_WAIT);
  BrowserWindowInterface* second_browser = browser_created_observer.Wait();
  EXECT_TRUE(second_browser);
  chrome::CloseWindow(second_browser);
```

Qikai Zhong

Marked as resolved.

File chrome/browser/glic/host/context/glic_pinned_tab_manager.cc

Line 416, Patchset 15: TabStripModel* tab_strip_model =

Alex Carutasu . resolved

nit:


```suggestion
        TabStripModel* const tab_strip_model =

```

Qikai Zhong

Marked as resolved.

Line 419, Patchset 15: auto* tab = tab_strip_model->GetTabAtIndex(i);

Alex Carutasu . resolved

nit:


```suggestion
          auto* const tab = tab_strip_model->GetTabAtIndex(i);
```

Qikai Zhong

Marked as resolved.

File chrome/browser/glic/test_support/glic_test_util.cc

Line 56, Patchset 15: ForEachCurrentBrowserWindowInterfaceOrderedByActivation(

Alex Carutasu . resolved

Could this use `GetLastActiveBrowserWindowInterfaceWithAnyProfile()` instead of the loop?

Qikai Zhong

Marked as resolved.

Line 78, Patchset 15: if (auto* browser_view =

Alex Carutasu . resolved

nit:
```suggestion
  if (auto* const browser_view =
```

Qikai Zhong

Marked as resolved.

File chrome/browser/glic/widget/application_hotkey_delegate.cc

Line 58, Patchset 15: if (auto* browser_view = BrowserView::GetBrowserViewForBrowser(

Alex Carutasu . resolved

nit:


```suggestion
          if (auto* const browser_view = BrowserView::GetBrowserViewForBrowser(
```

Qikai Zhong

Marked as resolved.

Line 75, Patchset 15: if (auto* browser_view =

Alex Carutasu . resolved

nit:


```suggestion
    if (auto* const browser_view =
```

Qikai Zhong

Marked as resolved.

File chrome/browser/ui/web_applications/web_app_ui_manager_impl.cc

Line 191, Patchset 15: GetAllBrowserWindowInterfaces()) {

Alex Carutasu . resolved

Could this use `ForEachCurrentAndNewBrowserWindowInterfaceOrderedByActivation()`?

Qikai Zhong

Updated to `ForEachCurrentAndNewBrowserWindowInterfaceOrderedByActivation()`

Line 231, Patchset 15:

        const AppBrowserController* app_controller =
            browser_window_interface->GetFeatures().app_browser_controller();

Alex Carutasu . resolved

nit:


```suggestion
        const AppBrowserController* const app_controller =
            browser_window_interface->GetAppBrowserController();
```

Qikai Zhong

Marked as resolved.

Open in Gerrit

Related details

Attention is currently required from:

Alex Carutasu
Tom Lukaszewicz

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

Tom Lukaszewicz (Gerrit)

unread,

Oct 12, 2025, 3:38:42 PM (2 days ago) Oct 12

Attention needed from Alex Carutasu and Qikai Zhong

Tom Lukaszewicz added 7 comments

Patchset-level comments

File-level comment, Patchset 22 (Latest):

Tom Lukaszewicz . resolved

Thanks for splitting - overall looks good just some minor comments

File chrome/browser/app_controller_mac.mm

Line 307, Patchset 22 (Latest): [result](BrowserWindowInterface* browser_window_interface) {

Tom Lukaszewicz . resolved

aside: Not for this change but in case it helps going forward - we can simply use `browser` for the `BWI` variable name instead of `browser_window_interface` (very minor point but it can often reduce verbosity / improve line wrapping in a few places).

File chrome/browser/browsing_data/navigation_entry_remover_browsertest.cc

Line 90, Patchset 22 (Latest): GetAllBrowserWindowInterfaces();

Tom Lukaszewicz . unresolved

nit: We should unify on `ForEachCurrentBrowserWindowInterfaceOrderedByActivation` here (we may or may not want to remove `GetAllBrowserWindowInterfaces()` later on but that's a separate discussion).

File chrome/browser/devtools/protocol/target_handler.cc

Line 191, Patchset 22 (Latest):

  Browser* target_browser =
      create_new_window
          ? nullptr
          : target_browser_interface->GetBrowserForMigrationOnly();

  NavigateParams params = CreateNavigateParams(
      profile, gurl, ui::PAGE_TRANSITION_AUTO_TOPLEVEL, create_new_window,
      create_in_background, target_browser);

Tom Lukaszewicz . unresolved

nit: This is the only client for `CreateNavigateParams()` - we could probably just modify that to take the `target_browser_interface` + use `BWI::GetBrowserForMigrationOnly()` in `CreateNavigateParams()` to avoid needing to define the `target_browser` here.

File chrome/browser/extensions/window_open_apitest.cc

Line 87, Patchset 15: ForEachCurrentBrowserWindowInterfaceOrderedByActivation(

Tom Lukaszewicz . unresolved

We could add the following utility to the fixture
```
  int CountBrowsersForType(BrowserWindowInterface::Type type) {
    return ui_test_utils::FindMatchingBrowsers(
        [type](BrowserWindowInterface* browser) {
          return browser->GetType() == type;
        });
  }
```

Then below becomes 
```
  EXPECT_EQ(num_popups,
    CountBrowsersForType(BrowserWindowInterface::Type::TYPE_POPUP);
  EXPECT_EQ(num_app_popups,
    CountBrowsersForType(BrowserWindowInterface::Type::TYPE_APP_POPUP);
```

Also the test below can be
```
const int expected_app_popups = BrowserList::GetInstance::size() - 1;
EXPECT_NE(browser()->GetType(), BrowserWindowInterface::Type::TYPE_APP_POPUP);
EXPECT_EQ(expected_app_popups,
  CountBrowsersForType(BrowserWindowInterface::Type::TYPE_APP_POPUP)
```

Qikai Zhong

Done. But don't we also have to migrate `BrowseList::GetInstance::size()` in these days?

Tom Lukaszewicz

That's a good point. The tests states the expected number of popup browsers above so we can probably do something like
```
constexpr int kExpectedAppPopups = 2;
EXPECT_TRUE(WaitForTabsPopupsApps(browser(), 0, 0, kExpectedAppPopups));

EXPECT_NE(browser()->GetType(), BrowserWindowInterface::Type::TYPE_APP_POPUP);

EXPECT_EQ(kExpectedAppPopups,
  CountBrowsersForType(BrowserWindowInterface::Type::TYPE_APP_POPUP)
```

File chrome/browser/file_system_access/BUILD.gn

Line 64, Patchset 22 (Latest): if (!is_android || is_desktop_android) {

Tom Lukaszewicz . unresolved

QQ - Is `is_desktop_android` needed if the include and code that uses it is gated behind `!BUILDFLAG(IS_ANDROID)`?

File chrome/browser/file_system_access/chrome_file_system_access_permission_context.cc

Line 3599, Patchset 22 (Latest): browser_window_interface->GetBrowserForMigrationOnly()

Tom Lukaszewicz . unresolved

nit: Not necessary here since `GetActiveTabInterface()` is on `BWI`

Open in Gerrit

Related details

Attention is currently required from:

Alex Carutasu
Qikai Zhong

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

Qikai Zhong (Gerrit)

unread,

Oct 12, 2025, 10:51:50 PM (2 days ago) Oct 12

Attention needed from Alex Carutasu and Tom Lukaszewicz

Qikai Zhong added 6 comments

Patchset-level comments

File-level comment, Patchset 22:

Qikai Zhong . resolved

Done~

File chrome/browser/browsing_data/navigation_entry_remover_browsertest.cc

Line 90, Patchset 22: GetAllBrowserWindowInterfaces();

Tom Lukaszewicz . resolved

nit: We should unify on `ForEachCurrentBrowserWindowInterfaceOrderedByActivation` here (we may or may not want to remove `GetAllBrowserWindowInterfaces()` later on but that's a separate discussion).

Qikai Zhong

Some cases in `NavigationEntryRemoverTest` depend on the browser creation order. I plan to put all cases that depend on the creation order into a separate CL.

File chrome/browser/devtools/protocol/target_handler.cc

Line 191, Patchset 22: Browser* target_browser =


      create_new_window
          ? nullptr
          : target_browser_interface->GetBrowserForMigrationOnly();

  NavigateParams params = CreateNavigateParams(
      profile, gurl, ui::PAGE_TRANSITION_AUTO_TOPLEVEL, create_new_window,
      create_in_background, target_browser);

Tom Lukaszewicz . resolved

nit: This is the only client for `CreateNavigateParams()` - we could probably just modify that to take the `target_browser_interface` + use `BWI::GetBrowserForMigrationOnly()` in `CreateNavigateParams()` to avoid needing to define the `target_browser` here.

Qikai Zhong

Marked as resolved.

File chrome/browser/extensions/window_open_apitest.cc

Line 87, Patchset 15: ForEachCurrentBrowserWindowInterfaceOrderedByActivation(

Tom Lukaszewicz . resolved

Qikai Zhong

Marked as resolved.

File chrome/browser/file_system_access/BUILD.gn

Line 64, Patchset 22: if (!is_android || is_desktop_android) {

Tom Lukaszewicz . resolved

QQ - Is `is_desktop_android` needed if the include and code that uses it is gated behind `!BUILDFLAG(IS_ANDROID)`?

Qikai Zhong

This is needed, it is to fix the gn check. You can find the detail in the checks section of patch 8.

File chrome/browser/file_system_access/chrome_file_system_access_permission_context.cc

Line 3599, Patchset 22: browser_window_interface->GetBrowserForMigrationOnly()

Tom Lukaszewicz . resolved

nit: Not necessary here since `GetActiveTabInterface()` is on `BWI`

Qikai Zhong

Marked as resolved.

Open in Gerrit

Related details

Attention is currently required from:

Alex Carutasu
Tom Lukaszewicz

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

Tom Lukaszewicz (Gerrit)

unread,

Oct 13, 2025, 1:18:08 AM (yesterday) Oct 13

Attention needed from Alex Carutasu and Qikai Zhong

Tom Lukaszewicz voted and added 1 comment

Votes added by Tom Lukaszewicz

Code-Review

1 comment

Patchset-level comments

File-level comment, Patchset 23 (Latest):

Tom Lukaszewicz . resolved

Thanks Qikai, lgtm!

Open in Gerrit

Related details

Attention is currently required from:

Alex Carutasu
Qikai Zhong

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

Alex Carutasu (Gerrit)

unread,

Oct 13, 2025, 9:09:54 PM (5 hours ago) Oct 13

to Qikai Zhong, Tom Lukaszewicz, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, weiluanw...@google.com, dmurph+watc...@chromium.org, mgiuca...@chromium.org, oshima...@chromium.org, dmurph+wat...@chromium.org, webap...@microsoft.com, menghua...@google.com, dtraino...@chromium.org, extension...@chromium.org, kuragin+web-ap...@chromium.org, dennyh...@google.com, zelin+watch-we...@chromium.org, mac-r...@chromium.org, permissio...@chromium.org, dibyapal+wa...@chromium.org, devtools...@chromium.org, dullweb...@chromium.org, chromium-a...@chromium.org, chromeos-kio...@google.com, msrame...@chromium.org, chungshe...@google.com, mfoltz+wa...@chromium.org, cbe-cep-eng...@google.com, loyso...@chromium.org, philli...@chromium.org, byronle...@chromium.org

Attention needed from Qikai Zhong

Alex Carutasu voted and added 1 comment

Votes added by Alex Carutasu

Code-Review

1 comment

Patchset-level comments

File-level comment, Patchset 23 (Latest):

Alex Carutasu . resolved

LGTM, thanks.

Open in Gerrit

Related details

Attention is currently required from:

Qikai Zhong

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

Qikai Zhong (Gerrit)

unread,

Oct 13, 2025, 9:15:24 PM (5 hours ago) Oct 13

to Alex Carutasu, Tom Lukaszewicz, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, weiluanw...@google.com, dmurph+watc...@chromium.org, mgiuca...@chromium.org, oshima...@chromium.org, dmurph+wat...@chromium.org, webap...@microsoft.com, menghua...@google.com, dtraino...@chromium.org, extension...@chromium.org, kuragin+web-ap...@chromium.org, dennyh...@google.com, zelin+watch-we...@chromium.org, mac-r...@chromium.org, permissio...@chromium.org, dibyapal+wa...@chromium.org, devtools...@chromium.org, dullweb...@chromium.org, chromium-a...@chromium.org, chromeos-kio...@google.com, msrame...@chromium.org, chungshe...@google.com, mfoltz+wa...@chromium.org, cbe-cep-eng...@google.com, loyso...@chromium.org, philli...@chromium.org, byronle...@chromium.org

Qikai Zhong voted Commit-Queue+2

Commit-Queue

Open in Gerrit

Related details

Attention set is empty

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

Chromium LUCI CQ (Gerrit)

unread,

Oct 13, 2025, 9:38:32 PM (5 hours ago) Oct 13

to Qikai Zhong, Alex Carutasu, Tom Lukaszewicz, AyeAye, chromium...@chromium.org, weiluanw...@google.com, dmurph+watc...@chromium.org, mgiuca...@chromium.org, oshima...@chromium.org, dmurph+wat...@chromium.org, webap...@microsoft.com, menghua...@google.com, dtraino...@chromium.org, extension...@chromium.org, kuragin+web-ap...@chromium.org, dennyh...@google.com, zelin+watch-we...@chromium.org, mac-r...@chromium.org, permissio...@chromium.org, dibyapal+wa...@chromium.org, devtools...@chromium.org, dullweb...@chromium.org, chromium-a...@chromium.org, chromeos-kio...@google.com, msrame...@chromium.org, chungshe...@google.com, mfoltz+wa...@chromium.org, cbe-cep-eng...@google.com, loyso...@chromium.org, philli...@chromium.org, byronle...@chromium.org

Chromium LUCI CQ submitted the change

Change information

Commit message:

[bedrock] Migrate BrowserList begin/end Part 1.

The main content is to migrate BrowserList::begin() / BrowserList::end()
to the BrowserWindowInterface api.

Throughout this migration process, convert Browser* references to
BrowserWindowInterface* wherever feasible.

This CL migrates files in chrome/browser/[a-g]* directories as the first
part. Subsequent CLs will cover remaining directories in alphabetical
order.

Bug: 431671320

Change-Id: I44fb5685ca017b7ef2eb9df110be59caf6da2ec3

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

Reviewed-by: Tom Lukaszewicz <tl...@chromium.org>

Commit-Queue: Qikai Zhong <qikai...@microsoft.com>

Reviewed-by: Alex Carutasu <alca...@microsoft.com>

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

Files:

M chrome/browser/app_controller_mac.mm
M chrome/browser/apps/app_service/metrics/website_metrics.cc
M chrome/browser/apps/platform_apps/app_window_interactive_uitest.cc
M chrome/browser/browser_process_platform_part_ash_browsertest.cc
M chrome/browser/browsing_data/chrome_browsing_data_lifetime_manager_browsertest.cc
M chrome/browser/devtools/devtools_browser_context_manager.cc
M chrome/browser/devtools/protocol/browser_handler.cc
M chrome/browser/devtools/protocol/target_handler.cc
M chrome/browser/download/download_status_updater_win.cc
M chrome/browser/download/download_ui_controller.cc
M chrome/browser/enterprise/connectors/connectors_manager.cc
M chrome/browser/enterprise/idle/action.cc
M chrome/browser/enterprise/idle/idle_service_interactive_uitest.cc
M chrome/browser/extensions/api/management/management_apitest.cc
M chrome/browser/extensions/api/tab_groups/tab_groups_api.cc
M chrome/browser/extensions/api/tabs/tabs_event_router.cc
M chrome/browser/extensions/api/tabs/tabs_test.cc
M chrome/browser/extensions/extension_tab_util.cc
M chrome/browser/extensions/window_open_apitest.cc
M chrome/browser/fast_shutdown_browsertest.cc
M chrome/browser/file_system_access/BUILD.gn
M chrome/browser/file_system_access/chrome_file_system_access_permission_context.cc
M chrome/browser/glic/glic_metrics.cc
M chrome/browser/glic/host/context/glic_pinned_tab_manager.cc
M chrome/browser/glic/host/glic_page_handler.cc
M chrome/browser/glic/test_support/glic_test_util.cc
M chrome/browser/glic/test_support/glic_test_util.h
M chrome/browser/glic/widget/application_hotkey_delegate.cc
M chrome/browser/glic/widget/browser_conditions.cc
M chrome/browser/ui/browser.cc
M chrome/browser/ui/browser.h
M chrome/browser/ui/browser_window/public/browser_window_interface.h
M chrome/browser/ui/browser_window/test/mock_browser_window_interface.h
M chrome/browser/ui/web_applications/web_app_ui_manager_impl.cc
M chrome/browser/ui/webui/webui_util_desktop.cc

Change size: L

Delta: 35 files changed, 602 insertions(+), 380 deletions(-)

Branch: refs/heads/main

Submit Requirements:

Code-Review: +1 by Tom Lukaszewicz, +1 by Alex Carutasu

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

0 new messages