New issue
Advanced search Search tips

Issue 689553 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jun 2017
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome
Pri: 3
Type: Bug



Sign in to add a comment

ActiveBrowserChanged user action triggers twice on some platforms

Project Member Reported by rkaplow@chromium.org, Feb 7 2017

Issue description

version: 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

 
#2 0x55740787af77 BrowserList::SetLastActive()
#3 0x5574079f0443 BrowserView::OnWidgetActivationChanged()
#4 0x7f92ffcf120d views::Widget::OnNativeWidgetActivationChanged()
#5 0x557407caa1f0 BrowserFrame::OnNativeWidgetActivationChanged()
#6 0x7f92ffd39520 views::DesktopNativeWidgetAura::HandleActivationChanged()
#7 0x7f92ffd60e2c views::DesktopWindowTreeHostX11::AfterActivationStateChanged()

and

#2 0x55740787af77 BrowserList::SetLastActive()
#3 0x557407caa1be BrowserFrame::OnNativeWidgetActivationChanged()
#4 0x7f92ffd39520 views::DesktopNativeWidgetAura::HandleActivationChanged()
#5 0x7f92ffd60e2c views::DesktopWindowTreeHostX11::AfterActivationStateChanged()


Basically, both BrowserView and BrowserFrame are calling SetLastActive()
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.
Labels: OS-Chrome OS-Linux OS-Windows
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.
Project Member

Comment 5 by bugdroid1@chromium.org, 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

Labels: M-61
Status: Fixed (was: Assigned)

Sign in to add a comment