ActiveBrowserChanged user action triggers twice on some platforms |
|||
Issue descriptionversion: 58.0.3000.4 Switching windows triggers the user action ActiveBrowserChanged twice, which can be seen on chrome://user-actions. This occurs on Linux and Windows, but *not* Mac. This means any stats related to it are doubled on these platforms
,
Jun 15 2017
I think we can just get rid of the code in BrowserFrame for this. BrowserFrame will always have a BrowserView and the BrowserView will always observe its widget (via GetWidget()->AddObserver(this) call in InitViews()), so it will always get the OnWidgetActivationChanged() notifications - and it needs them for other things already. Will prepare a CL for this.
,
Jun 15 2017
,
Jun 15 2017
There's a separate bug where ActiveBrowserChanged is logged twice when opening a new window. This one doesn't seem to be platform specific - I can repro it on Mac too. I filed crbug.com/733701 to track that one.
,
Jun 30 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/ba8e10dc8119f1bd7d2f365b2b3078f68aeb16cf commit ba8e10dc8119f1bd7d2f365b2b3078f68aeb16cf Author: asvitkine <asvitkine@chromium.org> Date: Fri Jun 30 14:20:45 2017 Fix duplicate logging of ActiveBrowserChanged action on views. This was caused by redundant calls to BrowserList::SetLastActive, one from BrowserFrame and another from BrowserView: views::DesktopNativeWidgetAura::HandleActivationChanged() BrowserFrame::OnNativeWidgetActivationChanged() BrowserList::SetLastActive() and: views::DesktopNativeWidgetAura::HandleActivationChanged() BrowserFrame::OnNativeWidgetActivationChanged() views::Widget::OnNativeWidgetActivationChanged() BrowserView::OnWidgetActivationChanged() BrowserList::SetLastActive() The change removes the call from BrowserFrame since it seems redundant with the one in BrowserView which observes its widget (via GetWidget()->AddObserver(this) call in InitViews()), so it will always get the OnWidgetActivationChanged() notifications - and it needs them for other things already. Also fixes ChromeVisibilityObserverBrowserTest on Windows which was fragile to the ordering of active/inactive events (and just happened to work due to the duplicate events). That test was also made unnecessarily complex by https://codereview.chromium.org/2591783002 so this change reverts most of that CL and simplifies things via RunLoop::RunUntilIdle(). BUG= 689553 TEST=Open two windows and then load chrome://user-actions. Switch windows and observe only a single instance of ActiveBrowserChanged logged per window switch. Review-Url: https://codereview.chromium.org/2939943004 Cr-Commit-Position: refs/heads/master@{#483695} [modify] https://crrev.com/ba8e10dc8119f1bd7d2f365b2b3078f68aeb16cf/chrome/browser/metrics/desktop_session_duration/chrome_visibility_observer.h [modify] https://crrev.com/ba8e10dc8119f1bd7d2f365b2b3078f68aeb16cf/chrome/browser/metrics/desktop_session_duration/chrome_visibility_observer_browsertest.cc [modify] https://crrev.com/ba8e10dc8119f1bd7d2f365b2b3078f68aeb16cf/chrome/browser/ui/views/frame/browser_frame.cc [modify] https://crrev.com/ba8e10dc8119f1bd7d2f365b2b3078f68aeb16cf/chrome/browser/ui/views/frame/browser_frame.h
,
Jun 30 2017
|
|||
►
Sign in to add a comment |
|||
Comment 1 by asvitk...@chromium.org
, Jun 15 2017