Implement BrowserCollectionObserver and ProfileBrowsers. [chromium/src : main]
Glenn Hartmann (Gerrit)
Glenn Hartmann voted and added 1 comment![]( "Open in Gerrit")
Votes added by Glenn Hartmann
| Commit-Queue | +1 |
1 comment
Naming is still undecided (see https://docs.google.com/document/d/1aQRPDX9RjWHE48rAHntsV64VU1-4uZO_nkL6A7ohr2k/edit?tab=t.5u9lkywxkk2n#bookmark=id.vwfubuu0wrbt) but I think everything else is ready for review
Related details
- Tom Lukaszewicz
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tom Lukaszewicz (Gerrit)
Tom Lukaszewicz added 7 comments![]( "Open in Gerrit")
Overall looks great!
BrowserManagerServiceFactory::GetForProfile(GetProfile())
->OnBrowserDidBecomeActive(this);Since `BrowserManagerService` owns `BrowserWindowInterface` instances would it work to register listeners on the specific instances each service is responsible for to avoid relying on `Browser` to deliver these updates?
Dealing with the callback subscription handles seems like it might be annoying but the upside is we wouldn't need to expose `BrowserManagerService::OnBrowserDidBecomeActive/Inactive()`
// ProfileBrowserstrivial nit: We typically add a trailing colon by convention
void CloseBrowser(Profile* profile, Browser* browser) {
BrowserManagerServiceFactory::GetForProfile(profile)->DeleteBrowser(
browser);
}nit: We should be able to use `InProcessBrowserTest::CloseBrowserSynchronously()`
void ActivatePrimaryBrowser(Browser* const secondary_browser) {I wonder if we should attempt to activate browsers using the [`BrowserActivationWaiter`](https://source.chromium.org/chromium/chromium/src/+/main:chrome/test/base/interactive_test_utils.h;l=42;drc=e44b92356d4568bf5f3d7a78ed75d2356b3d2871) (this does mean it would need to be an interactive_ui_test however).
static ProfileBrowsers* GetForProfile(Profile* profile);nit: We should add a virtual defaulted destructor also.
class ProfileBrowsers {nit: Could we add a brief comment describing the responsibility / functionality of ProfileBrowsers?
Related details
- Glenn Hartmann
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. |
Glenn Hartmann (Gerrit)
Glenn Hartmann added 7 comments![]( "Open in Gerrit")
BrowserManagerServiceFactory::GetForProfile(GetProfile())
->OnBrowserDidBecomeActive(this);Since `BrowserManagerService` owns `BrowserWindowInterface` instances would it work to register listeners on the specific instances each service is responsible for to avoid relying on `Browser` to deliver these updates?
Dealing with the callback subscription handles seems like it might be annoying but the upside is we wouldn't need to expose `BrowserManagerService::OnBrowserDidBecomeActive/Inactive()`
hmm, good point. It is a bit of a pain to manage the subscriptions, but I gave it a go.
btw, why does BrowserManagerService never remove anything from its `browsers_` vector? Just because we don't expect it to ever grow very big?
trivial nit: We typically add a trailing colon by convention
Done
// TODO: is it possible for |browser_ptr| to get deleted part-way through?any thoughts on this in particular?
void CloseBrowser(Profile* profile, Browser* browser) {
BrowserManagerServiceFactory::GetForProfile(profile)->DeleteBrowser(
browser);
}nit: We should be able to use `InProcessBrowserTest::CloseBrowserSynchronously()`
Done
void ActivatePrimaryBrowser(Browser* const secondary_browser) {I wonder if we should attempt to activate browsers using the [`BrowserActivationWaiter`](https://source.chromium.org/chromium/chromium/src/+/main:chrome/test/base/interactive_test_utils.h;l=42;drc=e44b92356d4568bf5f3d7a78ed75d2356b3d2871) (this does mean it would need to be an interactive_ui_test however).
I tried, but couldn't get it working. Maybe I'm doing something wrong, but here's a minimal repro of my issue: https://chromium-review.googlesource.com/c/chromium/src/+/7127225
That consistently times out on my machine.
static ProfileBrowsers* GetForProfile(Profile* profile);nit: We should add a virtual defaulted destructor also.
Done
nit: Could we add a brief comment describing the responsibility / functionality of ProfileBrowsers?
- Tom Lukaszewicz
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. |
Tom Lukaszewicz (Gerrit)
Tom Lukaszewicz voted and added 4 comments![]( "Open in Gerrit")
Votes added by Tom Lukaszewicz
| Code-Review | +1 |
4 comments
Still looks good - just a few nits
struct BrowserAndSubscriptions {nit: It's probably sufficient to have this be a pair of `std::pair<std::unique_ptr<Browser>, std::vector<base::CallbackListSubscription>>` instead (since we don't really care about differentiating between the subscriptions, we just need to hang onto the handles).
// TODO: is it possible for |browser_ptr| to get deleted part-way through?any thoughts on this in particular?
It's technically possible though should be very unlikely (there aren't any actions I'm aware of that would reasonably lead to the destruction of that same browser being created).
If we wanted to be extra careful though we could get a WeakPtr to the BWI before notifying and check this before notifying any clients if its end up going away.
void ActivatePrimaryBrowser(Browser* const secondary_browser) {Glenn HartmannI wonder if we should attempt to activate browsers using the [`BrowserActivationWaiter`](https://source.chromium.org/chromium/chromium/src/+/main:chrome/test/base/interactive_test_utils.h;l=42;drc=e44b92356d4568bf5f3d7a78ed75d2356b3d2871) (this does mean it would need to be an interactive_ui_test however).
I tried, but couldn't get it working. Maybe I'm doing something wrong, but here's a minimal repro of my issue: https://chromium-review.googlesource.com/c/chromium/src/+/7127225
That consistently times out on my machine.
Interesting - there does seem to be an issue with this on linux (see crbug.com/356183782). I'd add a TODO to this method to consider re-writing this as an interactive_ui_test once native window activation issues have been addressed (your current approach is great for now though)
Related details
- Glenn Hartmann
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. |
Glenn Hartmann (Gerrit)
Glenn Hartmann voted and added 4 comments![]( "Open in Gerrit")
Votes added by Glenn Hartmann
| Commit-Queue | +1 |
4 comments
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
nit: It's probably sufficient to have this be a pair of `std::pair<std::unique_ptr<Browser>, std::vector<base::CallbackListSubscription>>` instead (since we don't really care about differentiating between the subscriptions, we just need to hang onto the handles).
Done
// TODO: is it possible for |browser_ptr| to get deleted part-way through?Tom Lukaszewiczany thoughts on this in particular?
It's technically possible though should be very unlikely (there aren't any actions I'm aware of that would reasonably lead to the destruction of that same browser being created).
If we wanted to be extra careful though we could get a WeakPtr to the BWI before notifying and check this before notifying any clients if its end up going away.
Done
void ActivatePrimaryBrowser(Browser* const secondary_browser) {Glenn HartmannI wonder if we should attempt to activate browsers using the [`BrowserActivationWaiter`](https://source.chromium.org/chromium/chromium/src/+/main:chrome/test/base/interactive_test_utils.h;l=42;drc=e44b92356d4568bf5f3d7a78ed75d2356b3d2871) (this does mean it would need to be an interactive_ui_test however).
Tom LukaszewiczI tried, but couldn't get it working. Maybe I'm doing something wrong, but here's a minimal repro of my issue: https://chromium-review.googlesource.com/c/chromium/src/+/7127225
That consistently times out on my machine.
Interesting - there does seem to be an issue with this on linux (see crbug.com/356183782). I'd add a TODO to this method to consider re-writing this as an interactive_ui_test once native window activation issues have been addressed (your current approach is great for now though)
- Tom Lukaszewicz
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. |
Glenn Hartmann (Gerrit)
Glenn Hartmann added 1 comment![]( "Open in Gerrit")
BrowserManagerServiceFactory::GetForProfile(GetProfile())
->OnBrowserDidBecomeActive(this);Glenn HartmannSince `BrowserManagerService` owns `BrowserWindowInterface` instances would it work to register listeners on the specific instances each service is responsible for to avoid relying on `Browser` to deliver these updates?
Dealing with the callback subscription handles seems like it might be annoying but the upside is we wouldn't need to expose `BrowserManagerService::OnBrowserDidBecomeActive/Inactive()`
hmm, good point. It is a bit of a pain to manage the subscriptions, but I gave it a go.
btw, why does BrowserManagerService never remove anything from its `browsers_` vector? Just because we don't expect it to ever grow very big?
Done