linux: Default to custom frame for EWMH-supporting WMs.

linux: Default to custom frame for EWMH-supporting WMs.

Change the logic for determining if we should default to
custom window frames (as opposed to using WM-drawn
titlebars):

a) Don't use custom frames for window managers that don't
   support EWMH (Extended Window Manager Hints).
b) Don't use custom frames for EWMH-supporting tiling WMs and
   a few other WMs that don't play well with custom frames.
c) Otherwise, use custom frames.

Previously, custom frames were only used for a hardcoded set
of EWMH-supporting window managers.

BUG=345482
Committed: https://crrev.com/8d3a769f091ad5db1907aa7e5b46ca2c4ac7052d
Cr-Commit-Position: refs/heads/master@{#295975}

[Daniel Erat, 2014-09-16 23:48:33]

derat@chromium.org changed reviewers:
+ erg@chromium.org, mgiuca@chromium.org

[Daniel Erat, 2014-09-16 23:48:34]

you will never escape, elliot. we won't let you.

https://codereview.chromium.org/578443004/diff/1/ui/base/x/x11_util.cc
File ui/base/x/x11_util.cc (right):

https://codereview.chromium.org/578443004/diff/1/ui/base/x/x11_util.cc#newcod...
ui/base/x/x11_util.cc:1017: // frames.
at least, i'm assuming that their omission from the old list was intentional.

it took some digging, but in http://crbug.com/15861 tony suggested that kwin was
intentional. i'm less sure about icewm.

[Elliot Glaysher, 2014-09-16 23:59:36]

lgtm

https://codereview.chromium.org/578443004/diff/1/ui/base/x/x11_util.cc
File ui/base/x/x11_util.cc (right):

https://codereview.chromium.org/578443004/diff/1/ui/base/x/x11_util.cc#newcod...
ui/base/x/x11_util.cc:1017: // frames.
On 2014/09/16 23:48:34, Daniel Erat wrote:
> at least, i'm assuming that their omission from the old list was intentional.
> 
> it took some digging, but in http://crbug.com/15861 tony suggested that kwin
was
> intentional. i'm less sure about icewm.

We've had problems with icewm in the past, at least in a testing context. I
assume it is intentional.

[Matt Giuca, 2014-09-17 00:56:37]

It looks like this CL is doing a lot of things at once, so it's a bit hard to
review. It's hard to see which changes are refactoring/cleanup and what are
actual semantic changes. Let's go through them:

1. Removing WM_CHROME_OS -- is this going to change behaviour, or is the name
"chromeos-wm" no longer emitted by GetWindowManagerName (so you're just cleaning
up)?
2. Refactoring RecordLinuxWindowManager.
3. Adding 8 new window managers to the UMA records.
4. Inverting the logic in GetCustomFramePrefDefault so it's true by default.

I've carefully compared the logic of the new and old GetCustomFramePrefDefault
and it looks like it will behave exactly the same as before for all explicitly
known window managers. The only behavioural difference is that WM_UNKNOWN window
managers (that report their name) will now return true, not false. I don't see
how this achieves your stated goal of "Avoid custom window frames for tiling
window managers". (It seems like the WMs that default to custom frames after
this patch will be a superset of those before this patch.) (Which WMs were
getting custom frames before that shouldn't have been?) So unless I'm missing
something... not lgtm.

Can I suggest moving changes #1, #2 and #3 into a separate CL -- essentially
that patch is just refactoring/cleanup along with the UMA additions which are
harmless. (Unless the WM_CHROME_OS change is actually changing some behaviour,
in which case it should be in its own CL as well.) Then it will be much easier
to reason about #4 which will be making a behavioural change.

[Matt Giuca, 2014-09-17 01:09:16]

Forgot to press M.

https://codereview.chromium.org/578443004/diff/1/chrome/browser/metrics/chrom...
File chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc (right):

https://codereview.chromium.org/578443004/diff/1/chrome/browser/metrics/chrom...
chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc:148: case
ui::WM_UNKNOWN:       return UMA_LINUX_WINDOW_MANAGER_OTHER;
I don't think this conforms to the style guide. Can you put each return
statement on its own line?

https://codereview.chromium.org/578443004/diff/1/ui/base/x/x11_util.cc
File ui/base/x/x11_util.cc (right):

https://codereview.chromium.org/578443004/diff/1/ui/base/x/x11_util.cc#newcod...
ui/base/x/x11_util.cc:998: // assume that it doesn't want custom frames.
I'm not sure why you make this assumption. If the WM doesn't support EWMH then
we don't know anything about it, so we could just do the default behaviour
(which is custom frames) and let the user change it if it's wrong.

[Daniel Erat, 2014-09-18 16:07:44]

On 2014/09/17 01:09:16, Matt Giuca wrote:
> Forgot to press M.
> 
>
https://codereview.chromium.org/578443004/diff/1/chrome/browser/metrics/chrom...
> File chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc
(right):
> 
>
https://codereview.chromium.org/578443004/diff/1/chrome/browser/metrics/chrom...
> chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc:148: case
> ui::WM_UNKNOWN:       return UMA_LINUX_WINDOW_MANAGER_OTHER;
> I don't think this conforms to the style guide. Can you put each return
> statement on its own line?
> 
> https://codereview.chromium.org/578443004/diff/1/ui/base/x/x11_util.cc
> File ui/base/x/x11_util.cc (right):
> 
>
https://codereview.chromium.org/578443004/diff/1/ui/base/x/x11_util.cc#newcod...
> ui/base/x/x11_util.cc:998: // assume that it doesn't want custom frames.
> I'm not sure why you make this assumption. If the WM doesn't support EWMH then
> we don't know anything about it, so we could just do the default behaviour
> (which is custom frames) and let the user change it if it's wrong.

you're right; "_Explicitly_ avoid custom window frames for tiling window
managers" would've been a more accurate title. i believe that the i3 complaint
at
http://www.reddit.com/r/linux/comments/207zr3/google_to_replace_gtk_with_its_...
was just due to a period of time where the linux aura version of chrome used the
custom frame unconditionally. the only functional change here should be using
custom frames by default for not-known-to-be-tiling EWMH-supporting window
managers.

pulling the non-GetCustomFramePrefDefault() code into a separate change makes
sense to me; i'll make that change.

regarding WM_CHROME_OS, chromeos-wm is long dead (ever since chrome os switched
to aura) and is never returned by GetWindowManagerName().

[Daniel Erat, 2014-09-18 16:07:58]

https://codereview.chromium.org/578443004/diff/1/chrome/browser/metrics/chrom...
File chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc (right):

https://codereview.chromium.org/578443004/diff/1/chrome/browser/metrics/chrom...
chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc:148: case
ui::WM_UNKNOWN:       return UMA_LINUX_WINDOW_MANAGER_OTHER;
On 2014/09/17 01:09:15, Matt Giuca wrote:
> I don't think this conforms to the style guide. Can you put each return
> statement on its own line?

this style is fairly common in chromium, afaict:

% git grep 'case.*return' | fgrep -c .cc
806

the "Loops and Conditionals" section of
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Horizontal_Wh...
includes an example of "case 2: break;", implying that putting code to the right
of the case is permitted.

the argument could be made that there should only be a single space to the right
of the colon, but lining up the return statements is more readable and seems to
be done frequently in chromium.

https://codereview.chromium.org/578443004/diff/1/ui/base/x/x11_util.cc
File ui/base/x/x11_util.cc (right):

https://codereview.chromium.org/578443004/diff/1/ui/base/x/x11_util.cc#newcod...
ui/base/x/x11_util.cc:998: // assume that it doesn't want custom frames.
On 2014/09/17 01:09:16, Matt Giuca wrote:
> I'm not sure why you make this assumption. If the WM doesn't support EWMH then
> we don't know anything about it, so we could just do the default behaviour
> (which is custom frames) and let the user change it if it's wrong.

if the WM doesn't support EWMH, then it doesn't understand _NET_WM_MOVERESIZE
messages, which i believe means that trying to move a window by dragging it
around won't do anything. using system decorations seems like the right default
for this case.

[Daniel Erat, 2014-09-18 17:02:23]

split the detection and metrics changes into
https://codereview.chromium.org/585553003/ (and also fixed an embarrassing typo
where ratpoison was misdetected as WM_OPENBOX)

[Matt Giuca, 2014-09-19 01:01:38]

Thanks for splitting this out. Can you rebase this CL off of your other one so
it's easy to see the remaining changes? (And change the CL description to
match.)

On 2014/09/18 16:07:44, Daniel Erat wrote:
> you're right; "_Explicitly_ avoid custom window frames for tiling window
> managers" would've been a more accurate title. i believe that the i3 complaint
> at
>
http://www.reddit.com/r/linux/comments/207zr3/google_to_replace_gtk_with_its_...
> was just due to a period of time where the linux aura version of chrome used
the
> custom frame unconditionally. the only functional change here should be using
> custom frames by default for not-known-to-be-tiling EWMH-supporting window
> managers.

I think the CL description should be very clear about what the functional
changes are (and you can use subsequent paragraphs to explain internal logic
changes). I would title this CL: "Linux: unknown window managers that support
EWMH default to custom frames". Really that is the only functional change (all
known WMs, and non-EWMH-supporting WMs, are going to stay the same, as far as I
can tell).

> regarding WM_CHROME_OS, chromeos-wm is long dead (ever since chrome os
switched
> to aura) and is never returned by GetWindowManagerName().

Acknowledged.

https://codereview.chromium.org/578443004/diff/1/ui/base/x/x11_util.cc
File ui/base/x/x11_util.cc (right):

https://codereview.chromium.org/578443004/diff/1/ui/base/x/x11_util.cc#newcod...
ui/base/x/x11_util.cc:998: // assume that it doesn't want custom frames.
On 2014/09/18 16:07:58, Daniel Erat wrote:
> On 2014/09/17 01:09:16, Matt Giuca wrote:
> > I'm not sure why you make this assumption. If the WM doesn't support EWMH
then
> > we don't know anything about it, so we could just do the default behaviour
> > (which is custom frames) and let the user change it if it's wrong.
> 
> if the WM doesn't support EWMH, then it doesn't understand _NET_WM_MOVERESIZE
> messages, which i believe means that trying to move a window by dragging it
> around won't do anything. using system decorations seems like the right
default
> for this case.

Acknowledged.

[Daniel Erat, 2014-09-19 16:58:52]

i was holding off on reuploading until the other change had been reviewed; sorry
if that was unclear. i've diffed this change against the other one and updated
the description now.

[Daniel Erat, 2014-09-19 16:59:55]

https://codereview.chromium.org/578443004/diff/40001/ui/base/x/x11_util.cc
File ui/base/x/x11_util.cc (right):

https://codereview.chromium.org/578443004/diff/40001/ui/base/x/x11_util.cc#ne...
ui/base/x/x11_util.cc:999: // is needed for titlebar-dragging-initiated window
movement.
will change this to "frame-dragging-initiated" for clarity

[Matt Giuca, 2014-09-22 00:18:01]

lgtm, thanks for your patience on this.

Can you change the CL description, so where it says:
"b) Don't use custom frames for EWMH-supporting tiling WMs."
add, "and a few other WMs that don't play well with custom frames". (or your own
wording).

[Daniel Erat, 2014-09-22 14:43:17]

sure, i've used your wording.

[Daniel Erat, 2014-09-22 14:43:21]

The CQ bit was checked by derat@chromium.org

[commit-bot: I haz the power, 2014-09-22 14:44:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/578443004/60001

[commit-bot: I haz the power, 2014-09-22 15:32:45]

Committed patchset #4 (id:60001) as 4ef88ad7f610899f19d6276b936808d39b5bd458

[commit-bot: I haz the power, 2014-09-22 15:33:26]

Patchset 4 (id:??) landed as
https://crrev.com/8d3a769f091ad5db1907aa7e5b46ca2c4ac7052d
Cr-Commit-Position: refs/heads/master@{#295975}