Bring Chrome to the front when opening a URL from a notification

Bring Chrome to the front when opening a URL from a notification

Based on crrev.com/1411243012; see crbug.com/653868#c8.

BUG=653868, 470830
Committed: https://crrev.com/6759df3b42a5d4dcd95dbefb72e2c0ce2e220730
Cr-Commit-Position: refs/heads/master@{#425576}

[Sidney San Martín, 2016-10-13 20:42:29]

Patchset #1 (id:1) has been deleted

[Sidney San Martín, 2016-10-13 20:44:53]

Description was changed from

==========
[Mac] Bring Chrome to the front when opening a URL from a notification

BUG=653868,470830
==========

to

==========
[Mac] Bring Chrome to the front when opening a URL from a notification

See crbug.com/653868#c8.

BUG=653868,470830
==========

[Sidney San Martín, 2016-10-13 20:44:53]

sdy@chromium.org changed reviewers:
+ avi@chromium.org

[Sidney San Martín, 2016-10-13 20:47:22]

Description was changed from

==========
[Mac] Bring Chrome to the front when opening a URL from a notification

See crbug.com/653868#c8.

BUG=653868,470830
==========

to

==========
[Mac] Bring Chrome to the front when opening a URL from a notification

Based on crrev.com/1411243012; see crbug.com/653868#c8.

BUG=653868,470830
==========

[Avi (use Gerrit), 2016-10-13 20:47:47]

Where does Views do something like that?

[Sidney San Martín, 2016-10-13 20:53:31]

On 2016/10/13 20:47:47, Avi wrote:
> Where does Views do something like that?

Hmm, this appears to be broken on stable, on Linux at least.

[Sidney San Martín, 2016-10-13 20:55:07]

On 2016/10/13 20:53:31, Sidney San Martín wrote:
> On 2016/10/13 20:47:47, Avi wrote:
> > Where does Views do something like that?
> 
> Hmm, this appears to be broken on stable, on Linux at least.

…same for the settings button that I fixed for Mac in crrev.com/2412983004 :(.

[Avi (use Gerrit), 2016-10-13 20:55:38]

On 2016/10/13 20:53:31, Sidney San Martín wrote:
> On 2016/10/13 20:47:47, Avi wrote:
> > Where does Views do something like that?
> 
> Hmm, this appears to be broken on stable, on Linux at least.

If this is needed for all platforms, then remove the comment about this being a
Mac thing.

[Avi (use Gerrit), 2016-10-13 20:56:15]

https://codereview.chromium.org/2420733002/diff/20001/content/browser/service...
File content/browser/service_worker/service_worker_client_utils.cc (right):

https://codereview.chromium.org/2420733002/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_client_utils.cc:170:
static_cast<WebContentsImpl*>(web_contents)->Activate();
web_contents->delegate()->ActivateContents(web_contents);

[Avi (use Gerrit), 2016-10-13 21:28:59]

https://codereview.chromium.org/2420733002/diff/20001/content/browser/service...
File content/browser/service_worker/service_worker_client_utils.cc (right):

https://codereview.chromium.org/2420733002/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_client_utils.cc:170:
static_cast<WebContentsImpl*>(web_contents)->Activate();
On 2016/10/13 20:56:15, Avi wrote:
> web_contents->delegate()->ActivateContents(web_contents);

Actually, we're in content here so it doesn't matter. Stick with what you've
got.

[Sidney San Martín, 2016-10-13 23:44:38]

Description was changed from

==========
[Mac] Bring Chrome to the front when opening a URL from a notification

Based on crrev.com/1411243012; see crbug.com/653868#c8.

BUG=653868,470830
==========

to

==========
Bring Chrome to the front when opening a URL from a notification

Based on crrev.com/1411243012; see crbug.com/653868#c8.

BUG=653868,470830
==========

[Sidney San Martín, 2016-10-13 23:45:10]

Updated.

[Avi (use Gerrit), 2016-10-13 23:46:35]

LGTM

[Sidney San Martín, 2016-10-13 23:47:03]

The CQ bit was checked by sdy@chromium.org

[commit-bot: I haz the power, 2016-10-13 23:47:48]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-14 01:37:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-14 01:37:45]

Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[johnme, 2016-10-14 14:28:21]

On 2016/10/13 20:53:31, Sidney San Martín wrote:
> Hmm, this appears to be broken on stable, on Linux at least.

The problem on Linux is somewhat different. On Linux even when you call
Activate, Unity's Window Manager will ignore it (it works on Cinnamon). The fix
is to explicitly tell X11 that we're activating on behalf of a user gesture.
https://codereview.chromium.org/1135693006/diff/1/ui/views/widget/desktop_aur...
tried to do that, but Avi wasn't keen on me adding a separate codepath. So
instead I tried adding a bool user_gesture parameter to Activate. Unfortunately,
Activate is implemented and called by a zillion classes; I got as far as 1.
https://codereview.chromium.org/1153793003, 2.
https://codereview.chromium.org/1153813003, 3.
https://codereview.chromium.org/1149263003, and 4.
https://codereview.chromium.org/1158523002. But I wasn't very confident in those
patches, since I had to inexpertly classify calls from all over the codebase as
user gesture or not. Once I realised that this user_gesture parameter would only
be relevant to Linux, and only Unity at that, I started to wonder if all the
complexity and likely bugs from misclassifying things was worth it, and
ultimately got pulled onto other things.

But it would still be nice to fix this. The challenge is to get a boolean from
FocusOnUI and OpenWindowOnUI in service_worker_client_utils.cc to
DesktopWindowTreeHostX11::Activate.

views::DesktopWindowTreeHostX11::Activate()
ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:787
Overrides: views::DesktopWindowTreeHost::Activate()
ui/views/widget/desktop_aura/desktop_window_tree_host.h:101
Called by: views::DesktopNativeWidgetAura::Activate()
ui/views/widget/desktop_aura/desktop_native_widget_aura.cc:755
Overrides: views::internal::NativeWidgetPrivate::Activate()
ui/views/widget/native_widget_private.h:190
Called by: views::Widget::Activate() ui/views/widget/widget.cc:642
Called by: BrowserView::Show() chrome/browser/ui/views/frame/browser_view.cc:655
Overrides: ui::BaseWindow::Show() ui/base/base_window.h:56
Called by: <anonymous-namespace>::ScopedBrowserShower::~ScopedBrowserShower()
chrome/browser/ui/browser_navigator.cc:304
Called by: chrome::Navigate(…) chrome/browser/ui/browser_navigator.cc:480
Called by: ChromeContentBrowserClient::OpenURL(…)
chrome/browser/chrome_content_browser_client.cc:3051
Overrides: content::ContentBrowserClient::OpenURL(…)
content/public/browser/content_browser_client.h:718
Called by:
content::service_worker_client_utils::<anonymous-namespace>::OpenWindowOnUI(…)
content/browser/service_worker/service_worker_client_utils.cc:203

views::DesktopWindowTreeHostX11::Activate()
ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:787
Overrides: views::DesktopWindowTreeHost::Activate()
ui/views/widget/desktop_aura/desktop_window_tree_host.h:101
Called by: views::DesktopNativeWidgetAura::Activate()
ui/views/widget/desktop_aura/desktop_native_widget_aura.cc:755
Overrides: views::internal::NativeWidgetPrivate::Activate()
ui/views/widget/native_widget_private.h:190
Called by: views::Widget::Activate() ui/views/widget/widget.cc:642
Called by: BrowserView::Activate()
chrome/browser/ui/views/frame/browser_view.cc:705
Overrides: ui::BaseWindow::Activate() ui/base/base_window.h:71
Called by: Browser::ActivateContents(…) chrome/browser/ui/browser.cc:1448
Overrides: content::WebContentsDelegate::ActivateContents(…)
content/public/browser/web_contents_delegate.h:130
Called by: content::WebContentsImpl::Activate()
content/browser/web_contents/web_contents_impl.cc:1709
Called by:
content::service_worker_client_utils::<anonymous-namespace>::FocusOnUI(int, int,
const std::string &)
content/browser/service_worker/service_worker_client_utils.cc:146

But there are other cases where Chrome needs to be brought to the foreground on
Linux, e.g. https://crbug.com/489256 and https://crbug.com/411702.

It's messy...

[Sidney San Martín, 2016-10-14 15:07:16]

On 2016/10/14 14:28:21, johnme wrote:
> On 2016/10/13 20:53:31, Sidney San Martín wrote:
> > Hmm, this appears to be broken on stable, on Linux at least.
> 
> The problem on Linux is somewhat different. On Linux even when you call
> Activate, Unity's Window Manager will ignore it (it works on Cinnamon). The
fix
> is to explicitly tell X11 that we're activating on behalf of a user gesture.
>
https://codereview.chromium.org/1135693006/diff/1/ui/views/widget/desktop_aur...
> tried to do that, but Avi wasn't keen on me adding a separate codepath. So
> instead I tried adding a bool user_gesture parameter to Activate.
Unfortunately,
> Activate is implemented and called by a zillion classes; I got as far as 1.
> https://codereview.chromium.org/1153793003, 2.
> https://codereview.chromium.org/1153813003, 3.
> https://codereview.chromium.org/1149263003, and 4.
> https://codereview.chromium.org/1158523002. But I wasn't very confident in
those
> patches, since I had to inexpertly classify calls from all over the codebase
as
> user gesture or not. Once I realised that this user_gesture parameter would
only
> be relevant to Linux, and only Unity at that, I started to wonder if all the
> complexity and likely bugs from misclassifying things was worth it, and
> ultimately got pulled onto other things.
> 
> But it would still be nice to fix this. The challenge is to get a boolean from
> FocusOnUI and OpenWindowOnUI in service_worker_client_utils.cc to
> DesktopWindowTreeHostX11::Activate.
> 
> views::DesktopWindowTreeHostX11::Activate()
> ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:787
> Overrides: views::DesktopWindowTreeHost::Activate()
> ui/views/widget/desktop_aura/desktop_window_tree_host.h:101
> Called by: views::DesktopNativeWidgetAura::Activate()
> ui/views/widget/desktop_aura/desktop_native_widget_aura.cc:755
> Overrides: views::internal::NativeWidgetPrivate::Activate()
> ui/views/widget/native_widget_private.h:190
> Called by: views::Widget::Activate() ui/views/widget/widget.cc:642
> Called by: BrowserView::Show()
chrome/browser/ui/views/frame/browser_view.cc:655
> Overrides: ui::BaseWindow::Show() ui/base/base_window.h:56
> Called by: <anonymous-namespace>::ScopedBrowserShower::~ScopedBrowserShower()
> chrome/browser/ui/browser_navigator.cc:304
> Called by: chrome::Navigate(…) chrome/browser/ui/browser_navigator.cc:480
> Called by: ChromeContentBrowserClient::OpenURL(…)
> chrome/browser/chrome_content_browser_client.cc:3051
> Overrides: content::ContentBrowserClient::OpenURL(…)
> content/public/browser/content_browser_client.h:718
> Called by:
> content::service_worker_client_utils::<anonymous-namespace>::OpenWindowOnUI(…)
> content/browser/service_worker/service_worker_client_utils.cc:203
> 
> views::DesktopWindowTreeHostX11::Activate()
> ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:787
> Overrides: views::DesktopWindowTreeHost::Activate()
> ui/views/widget/desktop_aura/desktop_window_tree_host.h:101
> Called by: views::DesktopNativeWidgetAura::Activate()
> ui/views/widget/desktop_aura/desktop_native_widget_aura.cc:755
> Overrides: views::internal::NativeWidgetPrivate::Activate()
> ui/views/widget/native_widget_private.h:190
> Called by: views::Widget::Activate() ui/views/widget/widget.cc:642
> Called by: BrowserView::Activate()
> chrome/browser/ui/views/frame/browser_view.cc:705
> Overrides: ui::BaseWindow::Activate() ui/base/base_window.h:71
> Called by: Browser::ActivateContents(…) chrome/browser/ui/browser.cc:1448
> Overrides: content::WebContentsDelegate::ActivateContents(…)
> content/public/browser/web_contents_delegate.h:130
> Called by: content::WebContentsImpl::Activate()
> content/browser/web_contents/web_contents_impl.cc:1709
> Called by:
> content::service_worker_client_utils::<anonymous-namespace>::FocusOnUI(int,
int,
> const std::string &)
> content/browser/service_worker/service_worker_client_utils.cc:146
> 
> But there are other cases where Chrome needs to be brought to the foreground
on
> Linux, e.g. https://crbug.com/489256 and https://crbug.com/411702.
> 
> It's messy...

I feel like I've heard someone talk about representing this kind of state on the
stack, but I don't remember where (I think avi@ was on the thread?). It actually
sounds pretty ideal for this problem, but I don't know whether this kind of
thing is done in Chrome. Apple SDKs use this concept for a few things, mostly
for animation. As an example, here's how you set up an animation on iOS:

    UIView.animate(withDuration: 5) { 
      someView.animateToNewPosition()
    }

animate(withDuration:) sets some thread-local state, calls your callback, and
then clears that state when the callback returns. I could imagine doing
something like that:

    UserGesture::callWithUserGesture([&] {
      SomeLongChainWhichEventuallyCallsActivate();
    });

Or, more generically:

    Context::callWithState({
      UserGesture::State::InUserGesture
    }, [&] {
      SomeLongChainWhichEventuallyCallsActivate();
    });

Or, more consistent with Chrome style:

    Context::ScopedState(UserGesture::State::InUserGesture);
    SomeLongChainWhichEventuallyCallsActivate();

(I apologize in advance for mediocre on-the-spot naming.) The main issue for us
is thread hopping, but if the context object is easy to copy, then making
PostTask… automatically carry it over sounds pretty straightforward. Maybe
something like this exists already? Feel free to tell me it's a bad idea.
Anything to avoid mystery boolean parameters :).

[Sidney San Martín, 2016-10-14 15:21:54]

On 2016/10/14 15:07:16, Sidney San Martín wrote:
> animate(withDuration:)

Correction: animate(withDuration:animations:)
https://developer.apple.com/reference/uikit/uiview/1622418-animate

Also, if it was confusing, the "context" thing I described was a stack, or a
bunch of little stacks. Setting a property only shadows that particular
property.

[Sidney San Martín, 2016-10-14 17:24:48]

+mlamouri to make sure it's right to remove this special case from the test.

[Sidney San Martín, 2016-10-14 17:25:13]

sdy@chromium.org changed reviewers:
+ mlamouri@chromium.org

[Sidney San Martín, 2016-10-14 17:25:13]

*Actually* +mlamouri to make sure it's right to remove this special case from
the test.

[mlamouri (slow - plz ping), 2016-10-15 20:35:18]

On 2016/10/14 at 17:25:13, sdy wrote:
> *Actually* +mlamouri to make sure it's right to remove this special case from
the test.

lgtm

[Sidney San Martín, 2016-10-15 20:48:18]

The CQ bit was checked by sdy@chromium.org

[Sidney San Martín, 2016-10-15 20:48:19]

The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org
Link to the patchset: https://codereview.chromium.org/2420733002/#ps60001
(title: "Kill a Mac special case in this test")

[commit-bot: I haz the power, 2016-10-15 20:48:22]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-15 22:09:05]

Description was changed from

==========
Bring Chrome to the front when opening a URL from a notification

Based on crrev.com/1411243012; see crbug.com/653868#c8.

BUG=653868,470830
==========

to

==========
Bring Chrome to the front when opening a URL from a notification

Based on crrev.com/1411243012; see crbug.com/653868#c8.

BUG=653868,470830
==========

[commit-bot: I haz the power, 2016-10-15 22:09:09]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-10-15 22:10:46]

Description was changed from

==========
Bring Chrome to the front when opening a URL from a notification

Based on crrev.com/1411243012; see crbug.com/653868#c8.

BUG=653868,470830
==========

to

==========
Bring Chrome to the front when opening a URL from a notification

Based on crrev.com/1411243012; see crbug.com/653868#c8.

BUG=653868,470830

Committed: https://crrev.com/6759df3b42a5d4dcd95dbefb72e2c0ce2e220730
Cr-Commit-Position: refs/heads/master@{#425576}
==========

[commit-bot: I haz the power, 2016-10-15 22:10:48]

Patchset 3 (id:??) landed as
https://crrev.com/6759df3b42a5d4dcd95dbefb72e2c0ce2e220730
Cr-Commit-Position: refs/heads/master@{#425576}