Avoid blocking while mapping an X11 window

Avoid blocking while mapping an X11 window

For most window managers, calling XMapWindow causes a corresponding
MapNotify event to fire, indicating it is ready for drawing. Since this
usually happens immediately, but asynchronously, chromium previously
blocked the UI thread until it got this event, to avoid calling any
further X11 commands on the window until it was ready.

Unfortunately, not all window managers fire MapNotify in all cases. For
example, the i3 window manager will not fire MapNotify for a newly
created window while you're in fullscreen mode (i.e. youtube
video). This means chromium blocks indefinitely until the user manually
exits out of fullscreen mode.

The fix here is to avoid blocking, since we cannot make the assumption
that we'll get a MapNotify event quickly. By simply not blocking on
window map, we create two issues.

The first issue occurs when dragging tabs outside a window to create a
new one. The newly created window would fail to size itself properly
according to the space given to it by the window manager. This was
because the delayed_resize_task_ would get cancelled by SetBounds during
the time the thread would have otherwise been blocked.

This issue was fixed by only cancelling the task when the bounds changed
in that function, since that's the only time it will call OnHostResized
itself.

The second issue occurs during TooltipControllerTests, where a tooltip
is shown/hidden, and then immediately calls IsVisible() on the
widget (which eventually calls it on the underlying native
window). Since the mapping is now async, IsVisible() was false until
MapNotify was called.

The fix for this was to keep track of an additional variable for when we
are waiting for a MapNotify event, and using that to make IsVisible()
true during this time (after calling MapWindow, and before getting
MapNotify).

BUG=505669

[dackerman, 2016-09-11 21:40:05]

Description was changed from

==========
Avoid blocking while mapping an X11 window

For most window managers, calling XMapWindow causes a corresponding
MapNotify event to fire, indicating it is ready for drawing. Since this
usually happens immediately, but asynchronously, chromium previously
blocked the UI thread until it got this event, to avoid calling any
further X11 commands on the window until it was ready.

Unfortunately, not all window managers fire MapNotify in all cases. For
example, the i3 window manager will not fire MapNotify for a newly
created window while you're in fullscreen mode (i.e. youtube
video). This means chromium blocks indefinitely until the user manually
exits out of fullscreen mode.

The fix here is to avoid blocking, since we cannot make the assumption
that we'll get a MapNotify event quickly. By simply not blocking on
window map, we create two issues.

The first issue occurs when dragging tabs outside a window to create a
new one. The newly created window would fail to size itself properly
according to the space given to it by the window manager. This was
because the delayed_resize_task_ would get cancelled by SetBounds during
the time the thread would have otherwise been blocked.

This issue was fixed by only cancelling the task when the bounds changed
in that function, since that's the only time it will call OnHostResized
itself.

The second issue occurs during TooltipControllerTests, where a tooltip
is shown/hidden, and then immediately calls IsVisible() on the
widget (which eventually calls it on the underlying native
window). Since the mapping is now async, IsVisible() was false until
MapNotify was called.

The fix for this was to keep track of an additional variable for when we
are waiting for a MapNotify event, and using that to make IsVisible()
true during this time (after calling MapWindow, and before getting
MapNotify).

BUG=505669
==========

to

==========
Avoid blocking while mapping an X11 window

For most window managers, calling XMapWindow causes a corresponding
MapNotify event to fire, indicating it is ready for drawing. Since this
usually happens immediately, but asynchronously, chromium previously
blocked the UI thread until it got this event, to avoid calling any
further X11 commands on the window until it was ready.

Unfortunately, not all window managers fire MapNotify in all cases. For
example, the i3 window manager will not fire MapNotify for a newly
created window while you're in fullscreen mode (i.e. youtube
video). This means chromium blocks indefinitely until the user manually
exits out of fullscreen mode.

The fix here is to avoid blocking, since we cannot make the assumption
that we'll get a MapNotify event quickly. By simply not blocking on
window map, we create two issues.

The first issue occurs when dragging tabs outside a window to create a
new one. The newly created window would fail to size itself properly
according to the space given to it by the window manager. This was
because the delayed_resize_task_ would get cancelled by SetBounds during
the time the thread would have otherwise been blocked.

This issue was fixed by only cancelling the task when the bounds changed
in that function, since that's the only time it will call OnHostResized
itself.

The second issue occurs during TooltipControllerTests, where a tooltip
is shown/hidden, and then immediately calls IsVisible() on the
widget (which eventually calls it on the underlying native
window). Since the mapping is now async, IsVisible() was false until
MapNotify was called.

The fix for this was to keep track of an additional variable for when we
are waiting for a MapNotify event, and using that to make IsVisible()
true during this time (after calling MapWindow, and before getting
MapNotify).

BUG=505669
==========

[dackerman, 2016-09-11 21:40:05]

david.w.ackerman@gmail.com changed reviewers:
+ erg@chromium.org, sadrul@chromium.org

[dackerman, 2016-09-11 21:40:05]

david.w.ackerman@gmail.com changed required reviewers:
+ erg@chromium.org

[dackerman, 2016-09-11 21:47:16]

I ran the views_unittests locally with my setup - regular master has 9 failed
tests. After my change, there are 6 failed tests (all of which are tests that
are broken on master). I am not sure how this will behave on other window
managers, so running the CI tests would be appreciated.

I also haven't added myself to AUTHORS yet, as I wasn't sure if this will
actually be accepted. I did sign the CLA though.

[Elliot Glaysher, 2016-09-12 22:22:31]

The CQ bit was checked by erg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-12 22:23:19]

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

[Elliot Glaysher, 2016-09-12 22:23:21]

erg@chromium.org changed reviewers:
+ danakj@chromium.org, thomasanderson@chromium.org

[Elliot Glaysher, 2016-09-12 22:23:21]

Started a dry CQ run for you. Also added Dana and Tom since they might have
interesting things to say about this patch.

[commit-bot: I haz the power, 2016-09-12 22:30:12]

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

[commit-bot: I haz the power, 2016-09-12 22:30:13]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Tom (Use chromium acct), 2016-09-12 22:45:07]

thomasanderson@google.com changed reviewers:
+ derat@chromium.org, thomasanderson@google.com

[Tom (Use chromium acct), 2016-09-12 22:45:07]

Thank you David for putting in this CL

I'm a bit worried about the race condition with mouse grabs during tab dragging.
 Have you tested dragging tabs in/out of windows with a few WMs?  Maybe try
"stress -c <4x # of CPUs> during tab dragging as well?

Also there was an issue about blank windows when we tried not blocking before.
https://bugs.chromium.org/p/chromium/issues/detail?id=606661
Hopefully this one doesn't resurface

https://codereview.chromium.org/2329323002/diff/1/ui/views/widget/desktop_aur...
File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc (left):

https://codereview.chromium.org/2329323002/diff/1/ui/views/widget/desktop_aur...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:1901: // Block
until our window is unmapped. This avoids a race condition when
We don't need lines 1900 to 1905 any more now that we don't
BlockUntilWindowMapped().  This means you must rename |wait_for_unmap_| too :)

You can get rid of X11EventSource::BlockUntilWindowUnmapped() too since this is
the only place it's used.

https://codereview.chromium.org/2329323002/diff/1/ui/views/widget/desktop_aur...
File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.h (right):

https://codereview.chromium.org/2329323002/diff/1/ui/views/widget/desktop_aur...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11.h:284: bool
wait_for_map_;
Since this cl means we don't wait for maps anymore, |wait_for_map_| probably
needs to be renamed.

[dackerman, 2016-09-18 23:17:41]

On 2016/09/12 22:45:07, Tom Anderson wrote:
> Thank you David for putting in this CL
> 
> I'm a bit worried about the race condition with mouse grabs during tab
dragging.
>  Have you tested dragging tabs in/out of windows with a few WMs?  Maybe try
> "stress -c <4x # of CPUs> during tab dragging as well?
> 
> Also there was an issue about blank windows when we tried not blocking before.
> https://bugs.chromium.org/p/chromium/issues/detail?id=606661
> Hopefully this one doesn't resurface
> 
>
https://codereview.chromium.org/2329323002/diff/1/ui/views/widget/desktop_aur...
> File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc (left):
> 
>
https://codereview.chromium.org/2329323002/diff/1/ui/views/widget/desktop_aur...
> ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:1901: // Block
> until our window is unmapped. This avoids a race condition when
> We don't need lines 1900 to 1905 any more now that we don't
> BlockUntilWindowMapped().  This means you must rename |wait_for_unmap_| too :)
> 
> You can get rid of X11EventSource::BlockUntilWindowUnmapped() too since this
is
> the only place it's used.
> 
>
https://codereview.chromium.org/2329323002/diff/1/ui/views/widget/desktop_aur...
> File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.h (right):
> 
>
https://codereview.chromium.org/2329323002/diff/1/ui/views/widget/desktop_aur...
> ui/views/widget/desktop_aura/desktop_window_tree_host_x11.h:284: bool
> wait_for_map_;
> Since this cl means we don't wait for maps anymore, |wait_for_map_| probably
> needs to be renamed.

Thanks for the quick feedback! Just FYI I think I can only work on this on
weekends now, so sorry if this CL moves somewhat slowly.

I did try stress -c 8, and couldn't break anything on my machine (verified all
CPUs pegged). I dragged tabs from one window to another, outside to create a new
one, single-tab windows to others, etc. Everything seemed to work fine.

I created an Ubuntu 14.04 machine using VirtualBox and ran my debug build on it
(via shared folder). It was super-slow due to VM+debug+linked directory (which
is probably good for smoking out races), but the tab dragging also worked fine.
Menus worked as well (from the bug you linked).  One difference between my CL
and the previous one is that mine doesn't call anything immediately on the map
event - it always does it async. Maybe that had to do with it?

I _might_ be able to try Enlightenment on this Ubuntu VM since it was mentioned
in the other bug, but i'm not sure how much work it will be.

[dackerman, 2016-09-23 14:12:37]

oops, I didn't realize I had to click "Publish+Mail Comments" :-/

https://codereview.chromium.org/2329323002/diff/1/ui/views/widget/desktop_aur...
File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc (left):

https://codereview.chromium.org/2329323002/diff/1/ui/views/widget/desktop_aur...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:1901: // Block
until our window is unmapped. This avoids a race condition when
On 2016/09/12 22:45:07, Tom Anderson wrote:
> We don't need lines 1900 to 1905 any more now that we don't
> BlockUntilWindowMapped().  This means you must rename |wait_for_unmap_| too :)
> 
> You can get rid of X11EventSource::BlockUntilWindowUnmapped() too since this
is
> the only place it's used.

I didn't remove this since I don't know in what scenario this might run, and
it's not necessary to fix this issue. I felt that the risk of creating a new bug
by removing it (when it wasn't necessary) was higher than the benefit of
removing all blocking.

Do you think there's a decent chance leaving this here would make things
actively worse? If not, it seems safer to make the smallest change possible
(especially in light of how hard this area of the code is to test/verify). That
way, we'll have more fine-grained control if things need to be reverted.

https://codereview.chromium.org/2329323002/diff/1/ui/views/widget/desktop_aur...
File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.h (right):

https://codereview.chromium.org/2329323002/diff/1/ui/views/widget/desktop_aur...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11.h:284: bool
wait_for_map_;
On 2016/09/12 22:45:07, Tom Anderson wrote:
> Since this cl means we don't wait for maps anymore, |wait_for_map_| probably
> needs to be renamed.

How about |is_waiting_for_map_|, |expecting_map_notify_|, or
|map_window_called_|? Maybe some combination of these?  In a sense it is
"waiting" for the event to happen, but not blocking the thread. Also, you didn't
mention it, but does it make sense to include this it in the visibility check
like I did?

[Tom (Use chromium acct), 2016-09-23 18:35:49]

https://codereview.chromium.org/2329323002/diff/1/ui/views/widget/desktop_aur...
File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc (left):

https://codereview.chromium.org/2329323002/diff/1/ui/views/widget/desktop_aur...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:1901: // Block
until our window is unmapped. This avoids a race condition when
On 2016/09/23 14:12:37, dackerman wrote:
> On 2016/09/12 22:45:07, Tom Anderson wrote:
> > We don't need lines 1900 to 1905 any more now that we don't
> > BlockUntilWindowMapped().  This means you must rename |wait_for_unmap_| too
:)
> > 
> > You can get rid of X11EventSource::BlockUntilWindowUnmapped() too since this
> is
> > the only place it's used.
> 
> I didn't remove this since I don't know in what scenario this might run, and
> it's not necessary to fix this issue. I felt that the risk of creating a new
bug
> by removing it (when it wasn't necessary) was higher than the benefit of
> removing all blocking.
> 
> Do you think there's a decent chance leaving this here would make things
> actively worse? If not, it seems safer to make the smallest change possible
> (especially in light of how hard this area of the code is to test/verify).
That
> way, we'll have more fine-grained control if things need to be reverted.

This code was only recently added to fix the problem where we indefinitely
BlockUntilWindowMapped, although it clearly didn't work in all situations. 
Since this CL removes BlockUntilWindowMapped, we don't need it any more, and it
only introduces the problem of waiting forever in BlockUntilWindowUnmapped
instead.

https://codereview.chromium.org/2329323002/diff/1/ui/views/widget/desktop_aur...
File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.h (right):

https://codereview.chromium.org/2329323002/diff/1/ui/views/widget/desktop_aur...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11.h:284: bool
wait_for_map_;
On 2016/09/23 14:12:37, dackerman wrote:
> On 2016/09/12 22:45:07, Tom Anderson wrote:
> > Since this cl means we don't wait for maps anymore, |wait_for_map_| probably
> > needs to be renamed.
> 
> How about |is_waiting_for_map_|, |expecting_map_notify_|, or
> |map_window_called_|? Maybe some combination of these?  In a sense it is
> "waiting" for the event to happen, but not blocking the thread. Also, you
didn't
> mention it, but does it make sense to include this it in the visibility check
> like I did?

It does make sense to add the check for wait_for_map_ in the visibility check
since that way, a window will be IsVisible() right after a Show(), which matches
the current behavior with the blocking.

However, I've changed my mind about this.  We should instead remove both the
wait_for_map_ and wait_for_unmap_ logic and just introduce is_visible_.

is_visible_ will be initialized to false.
It will be set to true where you have "wait_for_map_ = true"
It will be set to false where we currently have "wait_for_unmap = true"
IsVisible() should just return is_visible_
Change the comment on window_mapped_ (line 278) to reflect that it means the
window is mapped w.r.t. the X server.
Add a comment on is_visible_ to reflect that it means the window is visible
w.r.t. Aura.

[dackerman, 2016-09-25 19:42:27]

The CQ bit was checked by david.w.ackerman@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2016-09-25 19:42:30]

Dry run: 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-09-25 19:42:31]

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

[commit-bot: I haz the power, 2016-09-25 19:42:32]

Dry run: All required reviewers (with asterisk prefixes) have not yet approved
this CL.

No L-G-T-M from a valid reviewer yet. 
CQ run can only be started by full committers or once the patch has
received an L-G-T-M from a full committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[dackerman, 2016-09-25 19:45:35]

david.w.ackerman@gmail.com changed reviewers:
+ david.w.ackerman@gmail.com
- David.W.Ackerman@gmail.com

[dackerman, 2016-09-25 19:45:36]

I removed the BlockUntilWindowUnmapped and simplified the visibility check as
requested (it definitely looks much better now!). I also reran the automated
tests and tried to break things manually both on my Archlinux setup and the
Ubuntu 14.04 VirtualBox instance - everything seems to be working fine (moving
tabs & windows around, popup windows during fullscreen, menus being drawn
properly).

I think I am ready to merge it unless there are any more concerns.

https://codereview.chromium.org/2329323002/diff/1/ui/views/widget/desktop_aur...
File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc (left):

https://codereview.chromium.org/2329323002/diff/1/ui/views/widget/desktop_aur...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11.cc:1901: // Block
until our window is unmapped. This avoids a race condition when
On 2016/09/23 18:35:48, Tom Anderson wrote:
> On 2016/09/23 14:12:37, dackerman wrote:
> > On 2016/09/12 22:45:07, Tom Anderson wrote:
> > > We don't need lines 1900 to 1905 any more now that we don't
> > > BlockUntilWindowMapped().  This means you must rename |wait_for_unmap_|
too
> :)
> > > 
> > > You can get rid of X11EventSource::BlockUntilWindowUnmapped() too since
this
> > is
> > > the only place it's used.
> > 
> > I didn't remove this since I don't know in what scenario this might run, and
> > it's not necessary to fix this issue. I felt that the risk of creating a new
> bug
> > by removing it (when it wasn't necessary) was higher than the benefit of
> > removing all blocking.
> > 
> > Do you think there's a decent chance leaving this here would make things
> > actively worse? If not, it seems safer to make the smallest change possible
> > (especially in light of how hard this area of the code is to test/verify).
> That
> > way, we'll have more fine-grained control if things need to be reverted.
> 
> This code was only recently added to fix the problem where we indefinitely
> BlockUntilWindowMapped, although it clearly didn't work in all situations. 
> Since this CL removes BlockUntilWindowMapped, we don't need it any more, and
it
> only introduces the problem of waiting forever in BlockUntilWindowUnmapped
> instead.

Done.

https://codereview.chromium.org/2329323002/diff/1/ui/views/widget/desktop_aur...
File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.h (right):

https://codereview.chromium.org/2329323002/diff/1/ui/views/widget/desktop_aur...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11.h:284: bool
wait_for_map_;
On 2016/09/23 18:35:48, Tom Anderson wrote:
> On 2016/09/23 14:12:37, dackerman wrote:
> > On 2016/09/12 22:45:07, Tom Anderson wrote:
> > > Since this cl means we don't wait for maps anymore, |wait_for_map_|
probably
> > > needs to be renamed.
> > 
> > How about |is_waiting_for_map_|, |expecting_map_notify_|, or
> > |map_window_called_|? Maybe some combination of these?  In a sense it is
> > "waiting" for the event to happen, but not blocking the thread. Also, you
> didn't
> > mention it, but does it make sense to include this it in the visibility
check
> > like I did?
> 
> It does make sense to add the check for wait_for_map_ in the visibility check
> since that way, a window will be IsVisible() right after a Show(), which
matches
> the current behavior with the blocking.
> 
> However, I've changed my mind about this.  We should instead remove both the
> wait_for_map_ and wait_for_unmap_ logic and just introduce is_visible_.
> 
> is_visible_ will be initialized to false.
> It will be set to true where you have "wait_for_map_ = true"
> It will be set to false where we currently have "wait_for_unmap = true"
> IsVisible() should just return is_visible_
> Change the comment on window_mapped_ (line 278) to reflect that it means the
> window is mapped w.r.t. the X server.
> Add a comment on is_visible_ to reflect that it means the window is visible
> w.r.t. Aura.

Done.

[Tom (Use chromium acct), 2016-09-26 17:26:04]

lgtm, but I have no OWNERS power here.  You'll need erg@ or sadrul@ to review

https://codereview.chromium.org/2329323002/diff/20001/ui/events/platform/x11/...
File ui/events/platform/x11/x11_event_source.h (left):

https://codereview.chromium.org/2329323002/diff/20001/ui/events/platform/x11/...
ui/events/platform/x11/x11_event_source.h:68: void BlockUntilWindowMapped(XID
window);
This should be done in the future, but I just want to document it here for now. 
There are only 2 remaining places that calls BlockUntilWindowMapped, and those
are in x11_window_base.cc and x11_whole_screen_move_loop.cc.  The former is just
like this case, and should be removed.  The other is done on an
override-redirect window, so we don't need to block for a map.

This means we can remove BlockUntilWindowMapped and BlockOnWindowStructureEvent.

https://codereview.chromium.org/2329323002/diff/20001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/desktop_window_tree_host_x11.h (right):

https://codereview.chromium.org/2329323002/diff/20001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_window_tree_host_x11.h:278: // Whether the
window is mapped with respect to the X11.
nit: "with respect to the X11" -> "with respect to the X11 server"

[Elliot Glaysher, 2016-09-26 20:43:23]

lgtm owners stamp

[Tom (Use chromium acct), 2016-09-27 00:51:34]

The CQ bit was checked by thomasanderson@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-09-27 00:52:17]

Dry run: 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-09-27 01:03:04]

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

[commit-bot: I haz the power, 2016-09-27 01:03:05]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[sadrul, 2016-09-27 01:05:39]

lgtm!

[dackerman, 2016-09-29 13:38:05]

On 2016/09/27 01:05:39, sadrul wrote:
> lgtm!

Great to see the lgtms, but in the trybot results I see a couple red boxes - it
looks like the following is failing:
ManagePasswordsBubbleViewTest.AutoSigninNoFocus
GlobalCommandsApiTest.GlobalCommand
WidgetTest.MinimumSizeConstraints

Are these actual failures that are caused by my CL, or potential existing
issues? If the former, I'll look into them (although I think
WidgetTest.MinimumSizeConstraints at least fails on master for even my local
setup). If the latter, how do I merge this? Is there documentation on it?

[Tom (Use chromium acct), 2016-09-29 17:09:43]

On 2016/09/29 13:38:05, dackerman wrote:
> On 2016/09/27 01:05:39, sadrul wrote:
> > lgtm!
> 
> Great to see the lgtms, but in the trybot results I see a couple red boxes -
it
> looks like the following is failing:
> ManagePasswordsBubbleViewTest.AutoSigninNoFocus
> GlobalCommandsApiTest.GlobalCommand
> WidgetTest.MinimumSizeConstraints
> 
> Are these actual failures that are caused by my CL, or potential existing
> issues? 

Most likely we just need to rerun the tests

> If the former, I'll look into them (although I think
> WidgetTest.MinimumSizeConstraints at least fails on master for even my local
> setup). If the latter, how do I merge this? Is there documentation on it?

However one thing you do need to change for sure is add yourself to the AUTHORS
file, you can do that in the next patch set.

** Presubmit Warnings **
David.W.Ackerman@gmail.com is not in AUTHORS file. If you are a new contributor,
please visit
http://www.chromium.org/developers/contributing-code and read the "Legal"
section
If you are a chromite, verify the contributor signed the CLA.

[Tom (Use chromium acct), 2016-09-29 22:40:50]

The CQ bit was checked by thomasanderson@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-09-29 22:41:23]

Dry run: 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-09-29 22:52:09]

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

[commit-bot: I haz the power, 2016-09-29 22:52:09]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[dackerman, 2016-09-30 13:58:17]

The CQ bit was checked by david.w.ackerman@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2016-09-30 13:58:37]

Dry run: 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-09-30 15:12:14]

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

[commit-bot: I haz the power, 2016-09-30 15:12:15]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Tom (Use chromium acct), 2016-10-01 22:18:53]

The CQ bit was checked by thomasanderson@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-01 22:19:02]

Dry run: 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-01 22:59:22]

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

[commit-bot: I haz the power, 2016-10-01 22:59:23]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[dackerman, 2016-10-02 23:15:34]

The CQ bit was checked by david.w.ackerman@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-02 23:15:45]

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

[dackerman, 2016-10-02 23:20:00]

On 2016/09/29 17:09:43, Tom Anderson wrote:
> On 2016/09/29 13:38:05, dackerman wrote:
> > On 2016/09/27 01:05:39, sadrul wrote:
> > > lgtm!
> > 
> > Great to see the lgtms, but in the trybot results I see a couple red boxes -
> it
> > looks like the following is failing:
> > ManagePasswordsBubbleViewTest.AutoSigninNoFocus
> > GlobalCommandsApiTest.GlobalCommand
> > WidgetTest.MinimumSizeConstraints
> > 
> > Are these actual failures that are caused by my CL, or potential existing
> > issues? 
> 
> Most likely we just need to rerun the tests
> 
> > If the former, I'll look into them (although I think
> > WidgetTest.MinimumSizeConstraints at least fails on master for even my local
> > setup). If the latter, how do I merge this? Is there documentation on it?
> 
> However one thing you do need to change for sure is add yourself to the
AUTHORS
> file, you can do that in the next patch set.
> 
> ** Presubmit Warnings **
> mailto:David.W.Ackerman@gmail.com is not in AUTHORS file. If you are a new
contributor,
> please visit
> http://www.chromium.org/developers/contributing-code and read the "Legal"
> section
> If you are a chromite, verify the contributor signed the CLA.

So I think the GlobalCommandsApiTest.GlobalCommand test was actually failing
(broken after my change, not on master according to my local setup). It seemed
that it was just firing key events too quickly (i.e. before mapping happened)...
I fixed it in a way that feels wrong, so please advise if you think there's a
better way.  There's also a tab dragging test that seems to be failing here
(TabDragging/DetachToBrowserTabDragControllerTest.DeleteTabsWhileDetached/0) -
but on my local setup it also fails in the same way on master, and I am not able
to understand what is actually happening to figure out how to fix it.

[commit-bot: I haz the power, 2016-10-02 23:27:36]

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

[commit-bot: I haz the power, 2016-10-02 23:27:37]

Dry run: Exceeded global retry quota

[dackerman, 2016-10-03 00:04:23]

The CQ bit was checked by david.w.ackerman@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-03 00:04:31]

Dry run: 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-03 00:15:20]

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

[commit-bot: I haz the power, 2016-10-03 00:15:21]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[dackerman, 2016-10-03 04:53:16]

The CQ bit was checked by david.w.ackerman@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-03 04:53:26]

Dry run: 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-03 05:54:55]

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

[commit-bot: I haz the power, 2016-10-03 05:54:56]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Tom (Use chromium acct), 2016-10-03 18:23:19]

On 2016/10/02 23:20:00, dackerman wrote:
> On 2016/09/29 17:09:43, Tom Anderson wrote:
> > On 2016/09/29 13:38:05, dackerman wrote:
> > > On 2016/09/27 01:05:39, sadrul wrote:
> > > > lgtm!
> > > 
> > > Great to see the lgtms, but in the trybot results I see a couple red boxes
-
> > it
> > > looks like the following is failing:
> > > ManagePasswordsBubbleViewTest.AutoSigninNoFocus
> > > GlobalCommandsApiTest.GlobalCommand
> > > WidgetTest.MinimumSizeConstraints
> > > 
> > > Are these actual failures that are caused by my CL, or potential existing
> > > issues? 
> > 
> > Most likely we just need to rerun the tests
> > 
> > > If the former, I'll look into them (although I think
> > > WidgetTest.MinimumSizeConstraints at least fails on master for even my
local
> > > setup). If the latter, how do I merge this? Is there documentation on it?
> > 
> > However one thing you do need to change for sure is add yourself to the
> AUTHORS
> > file, you can do that in the next patch set.
> > 
> > ** Presubmit Warnings **
> > mailto:David.W.Ackerman@gmail.com is not in AUTHORS file. If you are a new
> contributor,
> > please visit
> > http://www.chromium.org/developers/contributing-code and read the "Legal"
> > section
> > If you are a chromite, verify the contributor signed the CLA.
> 
> So I think the GlobalCommandsApiTest.GlobalCommand test was actually failing
> (broken after my change, not on master according to my local setup). It seemed
> that it was just firing key events too quickly (i.e. before mapping
happened)...
> I fixed it in a way that feels wrong, so please advise if you think there's a
> better way.  There's also a tab dragging test that seems to be failing here
> (TabDragging/DetachToBrowserTabDragControllerTest.DeleteTabsWhileDetached/0) -
> but on my local setup it also fails in the same way on master, and I am not
able
> to understand what is actually happening to figure out how to fix it.

Ok, I've taken a look and it appears the failure in
WidgetTest.MinimumSizeConstraints is here
https://cs.chromium.org/chromium/src/ui/views/widget/widget_unittest.cc?rcl=0...

GetNativeWidgetMinimumContentSize() calls XGetWMNormalHints here
https://cs.chromium.org/chromium/src/ui/views/test/widget_test_aura.cc?rcl=0&...

But, we set the WmNormalHints in DesktopWindowTreeHostX11::UpdateMinAndMaxSize
which is supposed to get called after receiving a MapNotify here:
https://cs.chromium.org/chromium/src/ui/views/widget/desktop_aura/desktop_win...

It looks like xfwm actually has problems with WM_NORMAL_HINTS getting changed
after a map.  I also peeked at the GTK source code and they set WM_NORMAL_HINTS
before mapping.

A blame points to this revision
https://chromium.googlesource.com/chromium/src/+/c07049637%5E%21/#F5
It looks like it was simply a mistake to add the call to UpdateMinAndMaxSize
after the map.

Please remove UpdateMinAndMaxSize() from the MapNotify and add it in
DesktopWindowTreeHostX11::MapWindow() before the XMapWindow()

[dackerman, 2016-10-15 15:13:23]

The CQ bit was checked by david.w.ackerman@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-15 15:13:33]

Dry run: 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 15:47:17]

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

[commit-bot: I haz the power, 2016-10-15 15:47:19]

Dry run: Exceeded global retry quota

[Tom (Use chromium acct), 2016-10-28 20:12:05]

The CQ bit was checked by thomasanderson@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-28 20:12:43]

Dry run: 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-28 22:14:07]

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

[commit-bot: I haz the power, 2016-10-28 22:14:08]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)