Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(103)

Issue 1135693006: Fix X11DesktopHandler::ActivateWindow on Unity for known user gestures

Created:
5 years, 7 months ago by johnme
Modified:
5 years, 7 months ago
CC:
chromium-reviews, michaeln, jsbell+serviceworker_chromium.org, tdanderson+views_chromium.org, serviceworker-reviews, creis+watch_chromium.org, tzik, tfarina, nasko+codewatch_chromium.org, jam, nhiroki, darin-cc_chromium.org, horo+watch_chromium.org, kinuko+serviceworker, kinuko+watch
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Fix X11DesktopHandler::ActivateWindow on Unity for known user gestures Unity in Ubuntu 14.04 ignores X11DesktopHandler::ActivateWindow's requests to raise Chrome windows by changing _NET_ACTIVE_WINDOW. Instead it treats it as a _NET_WM_STATE_DEMANDS_ATTENTION and just flashes the Launcher icon. This isn't good enough in cases where the activate request was triggered by a user gesture, for example the user clicked on a push notification. This patch changes X11DesktopHandler::ActivateWindow to indicate to the window manager when it is being called from a user gesture (see the comment added there for more details). And it plumbs the presence of the user gesture all the way from the content/ layer. In particular this patch fixes https://crbug.com/470830 on Linux. In theory the plumbing will be reused when fixing the same issue on Mac. I tried to make the fix somewhat general, by making ~ScopedBrowserShower use the user gesture path for any existing navigation with user_gesture set true in NavigateParams (see chrome/browser/ui/browser_navigator.cc). Hence the patch also fixed https://crbug.com/489256 (as chrome::ShowAboutChrome already has user_gesture set to true). However it wasn't possible to migrate all callers of activate that are in response to a user gesture (as there are hundreds of callers) - that will need to be done as follow-up patches, on a case-by-case basis. For example, this will likely allow https://crbug.com/411702 to be solved in future. BUG=470830, 489256, 411702 TEST=manual due to OS dependency, follow steps in https://crbug.com/470830 and https://crbug.com/489256

Patch Set 1 #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+132 lines, -9 lines) Patch
M chrome/browser/ui/browser.h View 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/ui/browser.cc View 1 chunk +6 lines, -0 lines 0 comments Download
M chrome/browser/ui/browser_navigator.cc View 1 chunk +4 lines, -1 line 0 comments Download
M chrome/browser/ui/views/frame/browser_view.h View 1 chunk +2 lines, -0 lines 0 comments Download
M chrome/browser/ui/views/frame/browser_view.cc View 3 chunks +21 lines, -1 line 0 comments Download
M content/browser/renderer_host/render_view_host_delegate.h View 1 chunk +3 lines, -1 line 0 comments Download
M content/browser/renderer_host/render_view_host_delegate.cc View 1 chunk +4 lines, -0 lines 0 comments Download
M content/browser/service_worker/service_worker_provider_host.cc View 1 chunk +3 lines, -2 lines 0 comments Download
M content/browser/service_worker/service_worker_version.cc View 1 chunk +3 lines, -0 lines 0 comments Download
M content/browser/web_contents/web_contents_impl.h View 1 chunk +1 line, -0 lines 0 comments Download
M content/browser/web_contents/web_contents_impl.cc View 1 chunk +5 lines, -0 lines 0 comments Download
M content/public/browser/web_contents_delegate.h View 1 chunk +2 lines, -0 lines 1 comment Download
M content/public/browser/web_contents_delegate.cc View 1 chunk +5 lines, -0 lines 0 comments Download
M ui/base/base_window.h View 2 chunks +4 lines, -0 lines 0 comments Download
M ui/base/base_window.cc View 1 chunk +8 lines, -0 lines 0 comments Download
M ui/views/widget/desktop_aura/desktop_native_widget_aura.h View 1 chunk +1 line, -0 lines 0 comments Download
M ui/views/widget/desktop_aura/desktop_native_widget_aura.cc View 1 chunk +5 lines, -0 lines 0 comments Download
M ui/views/widget/desktop_aura/desktop_window_tree_host.h View 1 chunk +1 line, -0 lines 0 comments Download
M ui/views/widget/desktop_aura/desktop_window_tree_host_win.h View 1 chunk +1 line, -0 lines 0 comments Download
M ui/views/widget/desktop_aura/desktop_window_tree_host_win.cc View 1 chunk +4 lines, -0 lines 0 comments Download
M ui/views/widget/desktop_aura/desktop_window_tree_host_x11.h View 1 chunk +1 line, -0 lines 0 comments Download
M ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc View 1 chunk +10 lines, -1 line 0 comments Download
M ui/views/widget/desktop_aura/x11_desktop_handler.h View 1 chunk +1 line, -1 line 0 comments Download
M ui/views/widget/desktop_aura/x11_desktop_handler.cc View 2 chunks +19 lines, -2 lines 0 comments Download
M ui/views/widget/native_widget_aura.h View 1 chunk +1 line, -0 lines 0 comments Download
M ui/views/widget/native_widget_aura.cc View 1 chunk +4 lines, -0 lines 0 comments Download
M ui/views/widget/native_widget_mac.h View 1 chunk +1 line, -0 lines 0 comments Download
M ui/views/widget/native_widget_mac.mm View 1 chunk +4 lines, -0 lines 0 comments Download
M ui/views/widget/native_widget_private.h View 1 chunk +1 line, -0 lines 0 comments Download
M ui/views/widget/widget.h View 1 chunk +2 lines, -0 lines 0 comments Download
M ui/views/widget/widget.cc View 1 chunk +4 lines, -0 lines 0 comments Download

Messages

Total messages: 5 (1 generated)
johnme
avi@chromium.org: Please review overall patch (as content/ and ui/ owner, you seem uniquely qualified to ...
5 years, 7 months ago (2015-05-18 13:28:49 UTC) #2
Avi (use Gerrit)
I'm OK with the idea, but turning this into two code paths is rather unfortunate. ...
5 years, 7 months ago (2015-05-18 14:59:22 UTC) #3
Elliot Glaysher
The actual x11 message changes lg, but I agree with avi@ about the duplicate interface ...
5 years, 7 months ago (2015-05-18 17:38:03 UTC) #4
johnme
5 years, 7 months ago (2015-05-22 16:42:28 UTC) #5
On 2015/05/18 14:59:22, Avi wrote:
> I'm OK with the idea, but turning this into two code paths is rather
> unfortunate.
> 
> I'm doing code searches for the callers of the Activate* functions and the
> numbers are in the low dozens. Can we break this CL into several pieces and do
> it better?
On 2015/05/18 17:38:03, Elliot Glaysher wrote:
> The actual x11 message changes lg, but I agree with avi@ about the duplicate
> interface is suboptimal.

Ok, I've gone with adding a |bool user_gesture| to the Activate functions. I've
split this up into a chain of CLs, as follows:

- https://codereview.chromium.org/1153793003 adds user_gesture to
WebContentsImpl::Activate
- https://codereview.chromium.org/1153813003 adds user_gesture to
WebContentsDelegate::ActivateContents
- https://codereview.chromium.org/1149263003 adds user_gesture to
BaseWindow::Activate
- https://codereview.chromium.org/1158523002 adds user_gesture to
BaseWindow::Show

Each patch depends on the previous patch (since the later patches use the
user_gesture parameter provided by earlier patches).

avi: Can you check that this seems reasonable? If so, I'll go ahead and do the
next patch which will update Widget::Activate in similar fashion, and finally a
patch that updates x11_desktop_handler and friends. Thanks!

Powered by Google App Engine
This is Rietveld 408576698