desktop_window_tree_host_x11.cc: do not return immediately from DesktopWindowTreeHostX11::OnWMState…

Linux: fix saving window state in fluxbox.

DesktopWindowTreeHostX11::OnWMStateUpdated(): assume that none of the
_NET_WM_STATE atoms are set if the _NET_WM_STATE window property is missing.
Fluxbox removes the _NET_WM_STATE window property if none of the _NET_WM_STATE
atoms are set.

BUG=437816, 411705
Committed: https://crrev.com/6783cdb2f65a63ca7e83d9e53527fa2d64ce4dd3
Cr-Commit-Position: refs/heads/master@{#306595}

[Lukasz Jagielski, 2014-12-01 15:42:55]

ljagielski@opera.com changed reviewers:
+ erg@chromium.org, pkotwicz@chromium.org

[Lukasz Jagielski, 2014-12-01 15:42:55]

Hi,
PTAL at my CL proposition.
Regards,
Łukasz

[Elliot Glaysher, 2014-12-01 18:40:50]

The patch is good, but the description needs work. Something like:

Linux: Fix saving window bounds in fluxbox.

Some description about how fluxbox unsets _NET_WM_STATE.

[pkotwicz, 2014-12-01 19:11:25]

https://codereview.chromium.org/764373002/diff/1/ui/views/widget/desktop_aura...
File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc (right):

https://codereview.chromium.org/764373002/diff/1/ui/views/widget/desktop_aura...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:1227: std::vector<
::Atom> atom_list;
Can you please also add a comment in the code as to why the early return is bad?

[pkotwicz, 2014-12-02 02:22:08]

Added crbug.com/411705 to this change list because this change list fixes that
bug too

[pkotwicz, 2014-12-02 17:12:02]

LGTM with comments.

I slightly edited your change list description to make it clearer

Sorry that I was rude and edited the "change list description" in place. I will
put the "clearer change list description" as a comment in the future and not
modify the original.

https://codereview.chromium.org/764373002/diff/20001/ui/views/widget/desktop_...
File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc (right):

https://codereview.chromium.org/764373002/diff/20001/ui/views/widget/desktop_...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:1229: // remove the
_NET_WM_STATE property when no _NET_WM_STATE_* atom is set.
How about: "Ignore the return value of ui::GetAtomArrayProperty(). Fluxbox
removes the _NET_WM_STATE property when no _NET_WM_STATE atoms are set."

Aside: I am unsure whether ui::GetAtomArrayProperty() should succeed when the
requested atom is not set. There are not enough uses of
ui::GetAtomArrayProperty() to make a strong case one way or the other.

[Elliot Glaysher, 2014-12-02 18:12:29]

lgtm if Peter is happy with it.

[Lukasz Jagielski, 2014-12-02 22:29:33]

https://codereview.chromium.org/764373002/diff/20001/ui/views/widget/desktop_...
File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc (right):

https://codereview.chromium.org/764373002/diff/20001/ui/views/widget/desktop_...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:1229: // remove the
_NET_WM_STATE property when no _NET_WM_STATE_* atom is set.
On 2014/12/02 17:12:02, pkotwicz wrote:
> How about: "Ignore the return value of ui::GetAtomArrayProperty(). Fluxbox
> removes the _NET_WM_STATE property when no _NET_WM_STATE atoms are set."
> 
> Aside: I am unsure whether ui::GetAtomArrayProperty() should succeed when the
> requested atom is not set. There are not enough uses of
> ui::GetAtomArrayProperty() to make a strong case one way or the other.

Yeah, my English needs some polishing (hope not to Polish it instead). That
comment has to wait until tomorrow though.

[Lukasz Jagielski, 2014-12-03 12:50:28]

The CQ bit was checked by ljagielski@opera.com

[commit-bot: I haz the power, 2014-12-03 12:51:01]

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

[commit-bot: I haz the power, 2014-12-03 13:34:33]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2014-12-03 13:35:27]

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