[bedrock] Migrate chrome_visibility_observer.cc away from BrowserListObserver. [chromium/src : main]
Glenn Hartmann (Gerrit)
New activity on the change![]( "Open in Gerrit")
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
gwsq (Gerrit)
Message from gwsq![]( "Open in Gerrit")
Reviewer source(s):
lucn...@google.com is from context(analysis/uma/chrome-metrics.gwsq)
Related details
- Luc Nguyen
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Luc Nguyen (Gerrit)
Luc Nguyen added 2 comments![]( "Open in Gerrit")
void OnBrowserActivated(BrowserWindowInterface* browser) override;
void OnBrowserDeactivated(BrowserWindowInterface* browser) override;
void OnBrowserClosed(BrowserWindowInterface* browser) override;sanity check: i'm not exactly familiar with the migration from `BrowserListObserver` to `BrowserCollectionObserver` -- is the latter intended to be the exact same behaviour as the former? i'm looking at when the `BrowserListObserver` functions are called vs `BrowserCollectionObserver` and i can't seem to find any trivial link between the two
if possible, could you possibly share code pointers on how exactly those two are linked?
for context, this class (`ChromeVisibilityObserver`) backs one of our most crucial metric (`Session.TotalDuration`) on Desktop so any behaviour change (however small it may seem, especially one that might not get picked up by our existing tests) would need to be vetted first
// process. (Ideally we would want the observer to start after
// g_browser_process is initialized, but before a browser instance is created
// - but this is difficult or impossible with the way InProcessBrowserTest
// currently works). So we start by closing that browser and replacing it with
// one of our own, which the observer will know about.hmm... so IIUC, `BrowserCollectionObserver` currently doesn't have support for observing the actual/first browser in a browsertest? that feels like a pretty big hole in terms of test coverage... is there any work being done to support this?
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 2 comments![]( "Open in Gerrit")
void OnBrowserActivated(BrowserWindowInterface* browser) override;
void OnBrowserDeactivated(BrowserWindowInterface* browser) override;
void OnBrowserClosed(BrowserWindowInterface* browser) override;sanity check: i'm not exactly familiar with the migration from `BrowserListObserver` to `BrowserCollectionObserver` -- is the latter intended to be the exact same behaviour as the former? i'm looking at when the `BrowserListObserver` functions are called vs `BrowserCollectionObserver` and i can't seem to find any trivial link between the two
if possible, could you possibly share code pointers on how exactly those two are linked?
for context, this class (`ChromeVisibilityObserver`) backs one of our most crucial metric (`Session.TotalDuration`) on Desktop so any behaviour change (however small it may seem, especially one that might not get picked up by our existing tests) would need to be vetted first
sanity check: i'm not exactly familiar with the migration from `BrowserListObserver` to `BrowserCollectionObserver` -- is the latter intended to be the exact same behaviour as the former?
They are intended to be equivalent, yes.
i'm looking at when the `BrowserListObserver` functions are called vs `BrowserCollectionObserver` and i can't seem to find any trivial link between the two
if possible, could you possibly share code pointers on how exactly those two are linked?
Yeah, the relationship is not trivial. In the old world, BrowserList was the only list of browsers, and it was globally-scoped.
However, there are many cases when we only need to observe browsers of a given profile rather than all browsers. So in the new world, each profile owns its own browsers, via BrowserManagerService (https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/browser_manager_service.h). Each BrowserManagerService implements ProfileBrowserCollection, which, as the name implies, is an observable collection of browsers in that profile.
GlobalBrowserCollection observes every ProfileBrowserCollection to combine all the browsers into a global list which should be equivalent to the original BrowserList concept. That means there's a bit of extra indirection, hence why it's not immediately obvious that BrowserList and GlobalBrowserCollection behave the same.
I think the situation is most easily conveyed with diagrams, so here you go: https://drive.google.com/drive/folders/1UmTddiO5sNe-ABbUrua9HieShdscVWcO?usp=sharing
// process. (Ideally we would want the observer to start after
// g_browser_process is initialized, but before a browser instance is created
// - but this is difficult or impossible with the way InProcessBrowserTest
// currently works). So we start by closing that browser and replacing it with
// one of our own, which the observer will know about.hmm... so IIUC, `BrowserCollectionObserver` currently doesn't have support for observing the actual/first browser in a browsertest? that feels like a pretty big hole in terms of test coverage... is there any work being done to support this?
I did some more research, and did manage to find a way to make this work. It's not too pretty, though.
Related details
- Luc Nguyen
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. |
Luc Nguyen (Gerrit)
Luc Nguyen voted and added 2 comments![]( "Open in Gerrit")
Votes added by Luc Nguyen
| Code-Review | +1 |
2 comments
void OnBrowserActivated(BrowserWindowInterface* browser) override;
void OnBrowserDeactivated(BrowserWindowInterface* browser) override;
void OnBrowserClosed(BrowserWindowInterface* browser) override;Glenn Hartmannsanity check: i'm not exactly familiar with the migration from `BrowserListObserver` to `BrowserCollectionObserver` -- is the latter intended to be the exact same behaviour as the former? i'm looking at when the `BrowserListObserver` functions are called vs `BrowserCollectionObserver` and i can't seem to find any trivial link between the two
if possible, could you possibly share code pointers on how exactly those two are linked?
for context, this class (`ChromeVisibilityObserver`) backs one of our most crucial metric (`Session.TotalDuration`) on Desktop so any behaviour change (however small it may seem, especially one that might not get picked up by our existing tests) would need to be vetted first
sanity check: i'm not exactly familiar with the migration from `BrowserListObserver` to `BrowserCollectionObserver` -- is the latter intended to be the exact same behaviour as the former?
They are intended to be equivalent, yes.
i'm looking at when the `BrowserListObserver` functions are called vs `BrowserCollectionObserver` and i can't seem to find any trivial link between the two
if possible, could you possibly share code pointers on how exactly those two are linked?
Yeah, the relationship is not trivial. In the old world, BrowserList was the only list of browsers, and it was globally-scoped.
However, there are many cases when we only need to observe browsers of a given profile rather than all browsers. So in the new world, each profile owns its own browsers, via BrowserManagerService (https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/browser_manager_service.h). Each BrowserManagerService implements ProfileBrowserCollection, which, as the name implies, is an observable collection of browsers in that profile.
GlobalBrowserCollection observes every ProfileBrowserCollection to combine all the browsers into a global list which should be equivalent to the original BrowserList concept. That means there's a bit of extra indirection, hence why it's not immediately obvious that BrowserList and GlobalBrowserCollection behave the same.
I think the situation is most easily conveyed with diagrams, so here you go: https://drive.google.com/drive/folders/1UmTddiO5sNe-ABbUrua9HieShdscVWcO?usp=sharing
- Glenn Hartmann
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Glenn Hartmann (Gerrit)
Glenn Hartmann voted Commit-Queue+2![]( "Open in Gerrit")
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
[bedrock] Migrate chrome_visibility_observer.cc away from BrowserListObserver.
This migration is part of project bedrock to reduce the dependencies on
Browser and BrowserList. See https://crbug.com/431671320 for more info.
- M chrome/browser/metrics/desktop_session_duration/chrome_visibility_observer.cc
- M chrome/browser/metrics/desktop_session_duration/chrome_visibility_observer.h
- M chrome/browser/metrics/desktop_session_duration/chrome_visibility_observer_interactive_uitest.cc
- M chrome/browser/metrics/desktop_session_duration/touch_ui_controller_stats_tracker_unittest.cc
Code-Review: +1 by Luc Nguyen
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |