Set the WM_CLASS property of X11 windows in Linux Aura build.

Set the WM_CLASS property of X11 windows in Linux Aura build.

This sets the WM_CLASS for both browser and app windows, mimicking the
behaviour of the existing GTK implementation. Now app windows have the
correct grouping, icon and title in the Ubuntu Unity window manager.

Also, .desktop shortcut files generated from the Aura build now have the
appropriate StartupWMClass key set (which was previously only set by the
GTK build).

BUG=174743
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=220801

[Matt Giuca, 2013-08-23 05:32:15]

erg: Overall, and for OWNERS:
chrome/browser/shell_integration_{linux,unittest}.cc,
ui/views/widget/desktop_aura/desktop_root_window_host_x11.cc
sky: chrome/browser/shell_integration.*, chrome/browser/ui/views/*,
ui/views/widget/widget.h
benwells: chrome/browser/web_applications/web_app.*
davemoore: ui/base/x/x11_util.*

I couldn't find any existing tests for the main classes involved
(NativeAppWindowViews and BrowserFrame). Any suggestions about where I can
insert tests for the new functionality, or is testing this stuff considered "too
hard"?

https://codereview.chromium.org/23093020/diff/1/chrome/browser/ui/views/frame...
File chrome/browser/ui/views/frame/browser_frame.cc (right):

https://codereview.chromium.org/23093020/diff/1/chrome/browser/ui/views/frame...
chrome/browser/ui/views/frame/browser_frame.cc:81: #if defined(OS_LINUX)
FYI: This code is mostly copied from chrome/browser/ui/gtk/browser_window_gtk.cc
(BrowserWindowGtk::Init).

[Elliot Glaysher, 2013-08-23 18:59:17]

lgtm

In general, most of the X11 interaction isn't tested very well because of the
asynchronicity of the connection, which makes it really difficult to test in a
non-flaky way. If you have one small piece where you know that you aren't round
tripping with the X server, write a test for it, but if not, it's ok.

[sky, 2013-08-23 19:41:48]

https://codereview.chromium.org/23093020/diff/1/chrome/browser/shell_integrat...
File chrome/browser/shell_integration.h (right):

https://codereview.chromium.org/23093020/diff/1/chrome/browser/shell_integrat...
chrome/browser/shell_integration.h:152: static std::string
GetProgramClassName();
Should this be ifdef'd?

[benwells, 2013-08-26 00:10:28]

https://codereview.chromium.org/23093020/diff/1/chrome/browser/shell_integrat...
File chrome/browser/shell_integration.h (right):

https://codereview.chromium.org/23093020/diff/1/chrome/browser/shell_integrat...
chrome/browser/shell_integration.h:152: static std::string
GetProgramClassName();
On 2013/08/23 19:41:48, sky wrote:
> Should this be ifdef'd?

or moved to shell_integration_linux.h?

https://codereview.chromium.org/23093020/diff/1/chrome/browser/web_applicatio...
File chrome/browser/web_applications/web_app.h (right):

https://codereview.chromium.org/23093020/diff/1/chrome/browser/web_applicatio...
chrome/browser/web_applications/web_app.h:100: // On Linux, windows that
correspond to web apps need to have a deterministic
Should the ifdef become #if defined(OS_LINUX) instead of being removed?

[Matt Giuca, 2013-08-26 04:46:09]

https://codereview.chromium.org/23093020/diff/1/chrome/browser/shell_integrat...
File chrome/browser/shell_integration.h (right):

https://codereview.chromium.org/23093020/diff/1/chrome/browser/shell_integrat...
chrome/browser/shell_integration.h:152: static std::string
GetProgramClassName();
On 2013/08/26 00:10:29, benwells wrote:
> On 2013/08/23 19:41:48, sky wrote:
> > Should this be ifdef'd?
> 
> or moved to shell_integration_linux.h?

Done.

https://codereview.chromium.org/23093020/diff/1/chrome/browser/web_applicatio...
File chrome/browser/web_applications/web_app.h (right):

https://codereview.chromium.org/23093020/diff/1/chrome/browser/web_applicatio...
chrome/browser/web_applications/web_app.h:100: // On Linux, windows that
correspond to web apps need to have a deterministic
On 2013/08/26 00:10:29, benwells wrote:
> Should the ifdef become #if defined(OS_LINUX) instead of being removed?

Done.

[Matt Giuca, 2013-08-26 04:48:21]

erg: PTAL at shell_integration_linux.{h,cc}, since I moved code from
shell_integration.{h,cc} into there. Thanks.

[Elliot Glaysher, 2013-08-26 17:58:00]

slgtm

[benwells, 2013-08-27 01:06:19]

c/b/web_applications lgtm

[Matt Giuca, 2013-08-29 01:05:00]

sky, davemoore: ping.

[sky, 2013-08-29 15:33:47]

https://codereview.chromium.org/23093020/diff/13001/chrome/browser/ui/views/f...
File chrome/browser/ui/views/frame/browser_frame.cc (right):

https://codereview.chromium.org/23093020/diff/13001/chrome/browser/ui/views/f...
chrome/browser/ui/views/frame/browser_frame.cc:84: #if defined(OS_LINUX)
Can this be moved to BrowserDesktopRootWindowHostX11 so that you don't need to
extend InitParams? BrowserDesktopRootWindowHostX11 is a DesktopRootWindowHostX11
and has a pointer to BrowserView.

[Matt Giuca, 2013-08-30 02:47:33]

https://codereview.chromium.org/23093020/diff/13001/chrome/browser/ui/views/f...
File chrome/browser/ui/views/frame/browser_frame.cc (right):

https://codereview.chromium.org/23093020/diff/13001/chrome/browser/ui/views/f...
chrome/browser/ui/views/frame/browser_frame.cc:84: #if defined(OS_LINUX)
Good point. I've tried this and it worked OK for browser windows, but I can't
find the equivalent for native app windows.

BrowserDesktopRootWindowHostX11 is a DesktopRootWindowHostX11, but there is
nothing equivalent for NativeAppWindowViews. I don't think there is a Linux-only
code path that is specific to V2 app windows where I could insert the code which
I currently have in native_app_window_views.cc.

[sky, 2013-08-30 16:58:20]

Based on your comments I assumed you only needed this for browsers. If you need
it for apps then this approach is fine. LGTM

[sky, 2013-08-30 16:58:43]

By find I should say I don't particularly like the platform specific initparams,
but I can't think of a better way.

[Matt Giuca, 2013-09-01 23:44:36]

+derat: ui/base/x/x11_util.* (since I haven't gotten a response from DaveMoore).
(Technically I already have OWNERS from sky but it's always good to have more
specific reviewer.)

sky: Yes, I share your aversion to adding this to InitParams, but I also can't
think of another way to do it.

[Daniel Erat, 2013-09-02 00:54:30]

LGTM for ui/base

[commit-bot: I haz the power, 2013-09-02 01:00:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mgiuca@chromium.org/23093020/13001

[commit-bot: I haz the power, 2013-09-02 05:26:40]

Change committed as 220801