Turn RootWindowController Menus Async

Turn RootWindowController Menus Async

Update RootWindowController to launch its context menus as async. This encompasses the chromium os desktop background, along with the shelf.

Update Widget's mouse pressed logic to account for asynchronous menus. It was performing mouse capture under the assumption that all menus were nested.

TEST=manual testing on device, ran ash_unittests, ran under an ASAN build
BUG=557130
Committed: https://crrev.com/a90d898367ba991d82dfeeb666ee2cda8ab908b0
Cr-Commit-Position: refs/heads/master@{#393867}

[jonross, 2016-05-05 15:53:00]

Hi,

Could you review this change? I switch RootWindowController menus to async, and
I made a change to Widget which always assumed nested message loops.

[jonross, 2016-05-05 15:53:20]

jonross@chromium.org changed reviewers:
+ sky@chromium.org

[jonross, 2016-05-05 15:53:20]

Hi,

Could you review this change? I switch RootWindowController menus to async, and
I made a change to Widget which always assumed nested message loops.

[jonross, 2016-05-05 15:53:23]

Hi,

Could you review this change? I switch RootWindowController menus to async, and
I made a change to Widget which always assumed nested message loops.

[sky, 2016-05-05 16:34:39]

This is subtle that it's worth tests in views as well.

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

https://codereview.chromium.org/1953753002/diff/1/ui/views/widget/widget.cc#n...
ui/views/widget/widget.cc:1192: !MenuController::GetActiveInstance()) {
I don't like having to special case the check for menus here. Would instead
checking if capture changed between the before calling OnMousePressed and after
and assume if there is capture we shouldn't change it be enough?

[jonross, 2016-05-05 17:50:42]

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

https://codereview.chromium.org/1953753002/diff/1/ui/views/widget/widget.cc#n...
ui/views/widget/widget.cc:1192: !MenuController::GetActiveInstance()) {
On 2016/05/05 16:34:39, sky wrote:
> I don't like having to special case the check for menus here. Would instead
> checking if capture changed between the before calling OnMousePressed and
after
> and assume if there is capture we shouldn't change it be enough?

I believe that could account for this. I'm not aware of other actions which
would swap capture, without the native widget becoming the new source of
capture.

Though looking at how we track capture, that would require importing
aura::client::CaptureClient, and I don't know if we want aura code popping into
widget.

I could expand NativeWidgetPrivate to do the lookup....

Thoughts?

[sky, 2016-05-05 20:54:35]

I agree, we don't want Widget to depend on aura::client. Adding a
function NativeWidget to get the current capture window seems right to
me. I believe such a function could be static.

On Thu, May 5, 2016 at 10:50 AM,  <jonross@chromium.org> wrote:
>
> https://codereview.chromium.org/1953753002/diff/1/ui/views/widget/widget.cc
> File ui/views/widget/widget.cc (right):
>
>
https://codereview.chromium.org/1953753002/diff/1/ui/views/widget/widget.cc#n...
> ui/views/widget/widget.cc:1192: !MenuController::GetActiveInstance()) {
> On 2016/05/05 16:34:39, sky wrote:
>> I don't like having to special case the check for menus here. Would
> instead
>> checking if capture changed between the before calling OnMousePressed
> and after
>> and assume if there is capture we shouldn't change it be enough?
>
> I believe that could account for this. I'm not aware of other actions
> which would swap capture, without the native widget becoming the new
> source of capture.
>
> Though looking at how we track capture, that would require importing
> aura::client::CaptureClient, and I don't know if we want aura code
> popping into widget.
>
> I could expand NativeWidgetPrivate to do the lookup....
>
> Thoughts?
>
> https://codereview.chromium.org/1953753002/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[jonross, 2016-05-06 17:40:52]

Widget updated to look for capture changes instead of verifying the
MenuController.

Added a bunch of tests.

[sky, 2016-05-06 20:03:32]

LGTM

https://codereview.chromium.org/1953753002/diff/20001/ui/views/widget/widget.cc
File ui/views/widget/widget.cc (right):

https://codereview.chromium.org/1953753002/diff/20001/ui/views/widget/widget....
ui/views/widget/widget.cc:1180: // It is possible for a View to show a context
menu on mouse-press, which
Can you make this comment more general and not entirely menu specific. Basically
we're assuming that if the capture changed during the processing of the mouse
down that a nested message loop was started and we shouldn't set internal state.

[jonross, 2016-05-09 16:50:12]

The CQ bit was checked by jonross@chromium.org

[jonross, 2016-05-09 16:50:12]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1953753002/#ps40001
(title: " ")

[jonross, 2016-05-09 16:50:25]

https://codereview.chromium.org/1953753002/diff/20001/ui/views/widget/widget.cc
File ui/views/widget/widget.cc (right):

https://codereview.chromium.org/1953753002/diff/20001/ui/views/widget/widget....
ui/views/widget/widget.cc:1180: // It is possible for a View to show a context
menu on mouse-press, which
On 2016/05/06 20:03:32, sky wrote:
> Can you make this comment more general and not entirely menu specific.
Basically
> we're assuming that if the capture changed during the processing of the mouse
> down that a nested message loop was started and we shouldn't set internal
state.

Done.

[commit-bot: I haz the power, 2016-05-09 16:50:41]

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

[commit-bot: I haz the power, 2016-05-09 18:21:41]

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

[commit-bot: I haz the power, 2016-05-09 18:21:42]

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

[jonross, 2016-05-11 00:21:05]

The CQ bit was checked by jonross@chromium.org

[jonross, 2016-05-11 00:21:06]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1953753002/#ps80001
(title: "Central Capture Tracking")

[jonross, 2016-05-11 00:21:17]

The CQ bit was unchecked by jonross@chromium.org

[jonross, 2016-05-14 16:59:57]

Hi sky,

Could you take another look at this change? Running the trybots uncovered some
issues in views_mus_unittests.

Changes:
  - DefaultCaptureClient tracks a global capture window, so different root
windows can determine that there is capture. This occurs in the
views_mus_unittests, where the launched second widget has a different root from
the one checking for capture changes.
  - WindowTreeClientImpl was receiving capture updates from the window server
after the deletion of the window.

For other potential races of WindowTreeClientImpl and test teardown I've filled
crbug.com/611984 I plan to implement the test teardown in a separate change, so
I can also address any bugs that uncovers at that time.

[sky, 2016-05-16 15:44:41]

LGTM

https://codereview.chromium.org/1953753002/diff/160001/ash/root_window_contro...
File ash/root_window_controller.cc (right):

https://codereview.chromium.org/1953753002/diff/160001/ash/root_window_contro...
ash/root_window_controller.cc:689: menu_model_(nullptr),
nit: you don't need any of these in the initializer list.

[jonross, 2016-05-16 16:53:49]

https://codereview.chromium.org/1953753002/diff/160001/ash/root_window_contro...
File ash/root_window_controller.cc (right):

https://codereview.chromium.org/1953753002/diff/160001/ash/root_window_contro...
ash/root_window_controller.cc:689: menu_model_(nullptr),
On 2016/05/16 15:44:41, sky wrote:
> nit: you don't need any of these in the initializer list.

Done.

[jonross, 2016-05-16 16:53:53]

The CQ bit was checked by jonross@chromium.org

[jonross, 2016-05-16 16:53:54]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1953753002/#ps200001
(title: " ")

[commit-bot: I haz the power, 2016-05-16 16:54:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1953753002/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1953753002/200001

[commit-bot: I haz the power, 2016-05-16 18:14:39]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2016-05-16 18:15:46]

Description was changed from

==========
Turn RootWindowController Menus Async

Update RootWindowController to launch its context menus as async. This
encompasses the chromium os desktop background, along with the shelf.

Update Widget's mouse pressed logic to account for asynchronous menus. It was
performing mouse capture under the assumption that all menus were nested.

TEST=manual testing on device, ran ash_unittests, ran under an ASAN build
BUG=557130
==========

to

==========
Turn RootWindowController Menus Async

Update RootWindowController to launch its context menus as async. This
encompasses the chromium os desktop background, along with the shelf.

Update Widget's mouse pressed logic to account for asynchronous menus. It was
performing mouse capture under the assumption that all menus were nested.

TEST=manual testing on device, ran ash_unittests, ran under an ASAN build
BUG=557130

Committed: https://crrev.com/a90d898367ba991d82dfeeb666ee2cda8ab908b0
Cr-Commit-Position: refs/heads/master@{#393867}
==========

[commit-bot: I haz the power, 2016-05-16 18:15:47]

Patchset 11 (id:??) landed as
https://crrev.com/a90d898367ba991d82dfeeb666ee2cda8ab908b0
Cr-Commit-Position: refs/heads/master@{#393867}