mus: Disregard windows that explicitly set can_accept_events to be false when sending events.

mus: Disregard windows that explicitly set can_accept_events to be false when sending events.

BUG=588311
TEST=mus_ws_unittests

Committed: https://crrev.com/7207207e4326fbc04fa2d226a2c85bafde3c9518
Cr-Commit-Position: refs/heads/master@{#405298}

[riajiang, 2016-07-04 23:10:05]

Description was changed from

==========
mus: Disregard windows that explicitly set accept_events to be false
when sending events

BUG=
==========

to

==========
mus: Disregard windows that explicitly set accept_events to be false when
sending events.

This is part of the touch hud app for mustash
(https://codereview.chromium.org/2092343002/).
==========

[riajiang, 2016-07-04 23:38:19]

riajiang@chromium.org changed reviewers:
+ msw@chromium.org, sadrul@chromium.org

[riajiang, 2016-07-05 16:28:48]

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

[riajiang, 2016-07-05 17:30:14]

riajiang@chromium.org changed reviewers:
- msw@chromium.org

[sky, 2016-07-06 16:21:25]

https://codereview.chromium.org/2118383002/diff/20001/services/ui/public/cpp/...
File services/ui/public/cpp/window.h (right):

https://codereview.chromium.org/2118383002/diff/20001/services/ui/public/cpp/...
services/ui/public/cpp/window.h:228: void SetAcceptEvents(bool accept_events);
nit: this is a bad location as GetName is more debugging. Maybe move by other
setter type functions, perhaps 212(ish)?

https://codereview.chromium.org/2118383002/diff/20001/services/ui/public/inte...
File services/ui/public/interfaces/window_tree.mojom (right):

https://codereview.chromium.org/2118383002/diff/20001/services/ui/public/inte...
services/ui/public/interfaces/window_tree.mojom:237: SetAcceptEvents(uint32
window_id, bool accept_events);
Please add description. I also recommend moving this around 225ish as it's
effects OnWindowInputEvent.

https://codereview.chromium.org/2118383002/diff/20001/services/ui/ws/server_w...
File services/ui/ws/server_window.h (right):

https://codereview.chromium.org/2118383002/diff/20001/services/ui/ws/server_w...
services/ui/ws/server_window.h:184: void SetAcceptEvents(bool accept_events) {
accept_events_ = accept_events; }
set_accept_events and accept_events() const (see style guide for general
naming).

https://codereview.chromium.org/2118383002/diff/20001/services/ui/ws/window_t...
File services/ui/ws/window_tree.cc (right):

https://codereview.chromium.org/2118383002/diff/20001/services/ui/ws/window_t...
services/ui/ws/window_tree.cc:352:
GetWindowByClientId(ClientWindowId(transport_window_id));
nit: spacing should be 4 here. Run git cl format to get the right thing.

https://codereview.chromium.org/2118383002/diff/20001/services/ui/ws/window_t...
services/ui/ws/window_tree.cc:354: window->SetAcceptEvents(accept_events);
You should also make sure the client can actually change this. See SetFocus how
it calls to CanSetFocus for what I mean.

https://codereview.chromium.org/2118383002/diff/20001/ui/views/mus/native_wid...
File ui/views/mus/native_widget_mus.cc (right):

https://codereview.chromium.org/2118383002/diff/20001/ui/views/mus/native_wid...
ui/views/mus/native_widget_mus.cc:548: if (accept_events == false)
window_->SetAcceptEvents(false);
nit: in general we don't use single line conditionals like this.

Also, you shouldn't need a new parameter for accept_events as it should be the
same as InitParams::accept_events. So, move this call to 
InitNativeWidget and use the value from InitParams.

[riajiang, 2016-07-08 17:59:59]

https://codereview.chromium.org/2118383002/diff/20001/services/ui/public/cpp/...
File services/ui/public/cpp/window.h (right):

https://codereview.chromium.org/2118383002/diff/20001/services/ui/public/cpp/...
services/ui/public/cpp/window.h:228: void SetAcceptEvents(bool accept_events);
On 2016/07/06 16:21:25, sky wrote:
> nit: this is a bad location as GetName is more debugging. Maybe move by other
> setter type functions, perhaps 212(ish)?

Done.

https://codereview.chromium.org/2118383002/diff/20001/services/ui/public/inte...
File services/ui/public/interfaces/window_tree.mojom (right):

https://codereview.chromium.org/2118383002/diff/20001/services/ui/public/inte...
services/ui/public/interfaces/window_tree.mojom:237: SetAcceptEvents(uint32
window_id, bool accept_events);
On 2016/07/06 16:21:25, sky wrote:
> Please add description. I also recommend moving this around 225ish as it's
> effects OnWindowInputEvent.

Done.

https://codereview.chromium.org/2118383002/diff/20001/services/ui/ws/server_w...
File services/ui/ws/server_window.h (right):

https://codereview.chromium.org/2118383002/diff/20001/services/ui/ws/server_w...
services/ui/ws/server_window.h:184: void SetAcceptEvents(bool accept_events) {
accept_events_ = accept_events; }
On 2016/07/06 16:21:25, sky wrote:
> set_accept_events and accept_events() const (see style guide for general
> naming).

Done.

https://codereview.chromium.org/2118383002/diff/20001/services/ui/ws/window_t...
File services/ui/ws/window_tree.cc (right):

https://codereview.chromium.org/2118383002/diff/20001/services/ui/ws/window_t...
services/ui/ws/window_tree.cc:352:
GetWindowByClientId(ClientWindowId(transport_window_id));
On 2016/07/06 16:21:25, sky wrote:
> nit: spacing should be 4 here. Run git cl format to get the right thing.

Done.

https://codereview.chromium.org/2118383002/diff/20001/services/ui/ws/window_t...
services/ui/ws/window_tree.cc:354: window->SetAcceptEvents(accept_events);
On 2016/07/06 16:21:25, sky wrote:
> You should also make sure the client can actually change this. See SetFocus
how
> it calls to CanSetFocus for what I mean.

Done.

https://codereview.chromium.org/2118383002/diff/20001/ui/views/mus/native_wid...
File ui/views/mus/native_widget_mus.cc (right):

https://codereview.chromium.org/2118383002/diff/20001/ui/views/mus/native_wid...
ui/views/mus/native_widget_mus.cc:548: if (accept_events == false)
window_->SetAcceptEvents(false);
On 2016/07/06 16:21:25, sky wrote:
> nit: in general we don't use single line conditionals like this.
> 
> Also, you shouldn't need a new parameter for accept_events as it should be the
> same as InitParams::accept_events. So, move this call to 
> InitNativeWidget and use the value from InitParams.

Done.

[sky, 2016-07-08 20:35:00]

Looking good, just nits.

https://codereview.chromium.org/2118383002/diff/60001/services/ui/public/cpp/...
File services/ui/public/cpp/window.h (right):

https://codereview.chromium.org/2118383002/diff/60001/services/ui/public/cpp/...
services/ui/public/cpp/window.h:213: void SetAcceptEvents(bool accept_events);
nit: SetCanAcceptEvents.

https://codereview.chromium.org/2118383002/diff/60001/services/ui/public/inte...
File services/ui/public/interfaces/window_tree.mojom (right):

https://codereview.chromium.org/2118383002/diff/60001/services/ui/public/inte...
services/ui/public/interfaces/window_tree.mojom:226: // Set whether to accept
events for the given window.
whether the specified window can accept events.

https://codereview.chromium.org/2118383002/diff/60001/services/ui/ws/server_w...
File services/ui/ws/server_window.h (right):

https://codereview.chromium.org/2118383002/diff/60001/services/ui/ws/server_w...
services/ui/ws/server_window.h:154: void set_accept_events(bool accept_events) {
accept_events_ = accept_events; }
can_accept. The reason I'm requesting the 'can' is that when you look at a
function like accept_events(), it reads as though it's a verb, when that isn't
the case.

[sadrul, 2016-07-09 14:51:22]

https://codereview.chromium.org/2118383002/diff/60001/ui/views/mus/native_wid...
File ui/views/mus/native_widget_mus.cc (right):

https://codereview.chromium.org/2118383002/diff/60001/ui/views/mus/native_wid...
ui/views/mus/native_widget_mus.cc:686:
window_->SetAcceptEvents(params.accept_events);
You can do this unconditionally (like SetCanFocus above). Although I suppose it
would make sense to teach ui::Window to not send any IPC if the setting is not
changing?

[riajiang, 2016-07-11 15:48:38]

https://codereview.chromium.org/2118383002/diff/60001/services/ui/public/cpp/...
File services/ui/public/cpp/window.h (right):

https://codereview.chromium.org/2118383002/diff/60001/services/ui/public/cpp/...
services/ui/public/cpp/window.h:213: void SetAcceptEvents(bool accept_events);
On 2016/07/08 20:35:00, sky (OOO until 7-18) wrote:
> nit: SetCanAcceptEvents.

Done.

https://codereview.chromium.org/2118383002/diff/60001/services/ui/public/inte...
File services/ui/public/interfaces/window_tree.mojom (right):

https://codereview.chromium.org/2118383002/diff/60001/services/ui/public/inte...
services/ui/public/interfaces/window_tree.mojom:226: // Set whether to accept
events for the given window.
On 2016/07/08 20:35:00, sky (OOO until 7-18) wrote:
> whether the specified window can accept events.

Done.

https://codereview.chromium.org/2118383002/diff/60001/services/ui/ws/server_w...
File services/ui/ws/server_window.h (right):

https://codereview.chromium.org/2118383002/diff/60001/services/ui/ws/server_w...
services/ui/ws/server_window.h:154: void set_accept_events(bool accept_events) {
accept_events_ = accept_events; }
On 2016/07/08 20:35:00, sky (OOO until 7-18) wrote:
> can_accept. The reason I'm requesting the 'can' is that when you look at a
> function like accept_events(), it reads as though it's a verb, when that isn't
> the case.

Done.

https://codereview.chromium.org/2118383002/diff/60001/ui/views/mus/native_wid...
File ui/views/mus/native_widget_mus.cc (right):

https://codereview.chromium.org/2118383002/diff/60001/ui/views/mus/native_wid...
ui/views/mus/native_widget_mus.cc:686:
window_->SetAcceptEvents(params.accept_events);
On 2016/07/09 14:51:22, sadrul wrote:
> You can do this unconditionally (like SetCanFocus above). Although I suppose
it
> would make sense to teach ui::Window to not send any IPC if the setting is not
> changing?

Changed it to do the checking in window_tree_client before it sends IPC calls.

[riajiang, 2016-07-11 20:17:09]

riajiang@chromium.org changed reviewers:
+ msw@chromium.org

[riajiang, 2016-07-11 20:17:10]

+msw, since sky is OOO

[riajiang, 2016-07-11 20:17:26]

Description was changed from

==========
mus: Disregard windows that explicitly set accept_events to be false when
sending events.

This is part of the touch hud app for mustash
(https://codereview.chromium.org/2092343002/).
==========

to

==========
mus: Disregard windows that explicitly set can_accept_events to be false when
sending events.

This is part of the touch hud app for mustash
(https://codereview.chromium.org/2092343002/).
==========

[msw, 2016-07-11 20:36:19]

Sadrul is likely a more qualified reviewer, but since I'm still in the
services/ui/OWNERS file, I can probably review/stamp as needed after Sadrul is
satisfied.

[sadrul, 2016-07-12 17:32:33]

https://codereview.chromium.org/2118383002/diff/80001/services/ui/public/cpp/...
File services/ui/public/cpp/lib/window_tree_client.cc (right):

https://codereview.chromium.org/2118383002/diff/80001/services/ui/public/cpp/...
services/ui/public/cpp/lib/window_tree_client.cc:713: can_accept_events_ =
can_accept_events;
This is wrong. Some window in a WindowTreeClient may want to accept events,
while some other windows in the same WTC may not. |can_accept_events_| should be
something tracked in mus::Window, not in WindowTreeClient.

https://codereview.chromium.org/2118383002/diff/80001/services/ui/public/cpp/...
File services/ui/public/cpp/window.h (right):

https://codereview.chromium.org/2118383002/diff/80001/services/ui/public/cpp/...
services/ui/public/cpp/window.h:212: // Set whether this window accepts events.
*Sets

https://codereview.chromium.org/2118383002/diff/80001/services/ui/public/inte...
File services/ui/public/interfaces/window_tree.mojom (right):

https://codereview.chromium.org/2118383002/diff/80001/services/ui/public/inte...
services/ui/public/interfaces/window_tree.mojom:227: SetCanAcceptEvents(uint32
window_id, bool accept_events);
Update the doc to also explain that if a Window is not accepting events, that
means none of its descendent windows would be able to accept events either.

https://codereview.chromium.org/2118383002/diff/80001/services/ui/ws/window_f...
File services/ui/ws/window_finder.cc (right):

https://codereview.chromium.org/2118383002/diff/80001/services/ui/ws/window_f...
services/ui/ws/window_finder.cc:28: gfx::Point* location) {
return nullptr if (!window->can_accept_events())

https://codereview.chromium.org/2118383002/diff/80001/services/ui/ws/window_t...
File services/ui/ws/window_tree.cc (right):

https://codereview.chromium.org/2118383002/diff/80001/services/ui/ws/window_t...
services/ui/ws/window_tree.cc:355:
window->set_can_accept_events(can_accept_events);
Add a TODO here to check that there aren't any entries in |event_queue_| for
|window|

[riajiang, 2016-07-12 18:46:41]

https://codereview.chromium.org/2118383002/diff/80001/services/ui/public/cpp/...
File services/ui/public/cpp/lib/window_tree_client.cc (right):

https://codereview.chromium.org/2118383002/diff/80001/services/ui/public/cpp/...
services/ui/public/cpp/lib/window_tree_client.cc:713: can_accept_events_ =
can_accept_events;
On 2016/07/12 17:32:33, sadrul wrote:
> This is wrong. Some window in a WindowTreeClient may want to accept events,
> while some other windows in the same WTC may not. |can_accept_events_| should
be
> something tracked in mus::Window, not in WindowTreeClient.

Done. Moved to ui::Window.

https://codereview.chromium.org/2118383002/diff/80001/services/ui/public/cpp/...
File services/ui/public/cpp/window.h (right):

https://codereview.chromium.org/2118383002/diff/80001/services/ui/public/cpp/...
services/ui/public/cpp/window.h:212: // Set whether this window accepts events.
On 2016/07/12 17:32:33, sadrul wrote:
> *Sets

Done.

https://codereview.chromium.org/2118383002/diff/80001/services/ui/public/inte...
File services/ui/public/interfaces/window_tree.mojom (right):

https://codereview.chromium.org/2118383002/diff/80001/services/ui/public/inte...
services/ui/public/interfaces/window_tree.mojom:227: SetCanAcceptEvents(uint32
window_id, bool accept_events);
On 2016/07/12 17:32:33, sadrul wrote:
> Update the doc to also explain that if a Window is not accepting events, that
> means none of its descendent windows would be able to accept events either.

Done.

https://codereview.chromium.org/2118383002/diff/80001/services/ui/ws/window_f...
File services/ui/ws/window_finder.cc (right):

https://codereview.chromium.org/2118383002/diff/80001/services/ui/ws/window_f...
services/ui/ws/window_finder.cc:28: gfx::Point* location) {
On 2016/07/12 17:32:33, sadrul wrote:
> return nullptr if (!window->can_accept_events())

Done.

https://codereview.chromium.org/2118383002/diff/80001/services/ui/ws/window_t...
File services/ui/ws/window_tree.cc (right):

https://codereview.chromium.org/2118383002/diff/80001/services/ui/ws/window_t...
services/ui/ws/window_tree.cc:355:
window->set_can_accept_events(can_accept_events);
On 2016/07/12 17:32:33, sadrul wrote:
> Add a TODO here to check that there aren't any entries in |event_queue_| for
> |window|

Done.

[sadrul, 2016-07-12 19:44:19]

lgtm

https://codereview.chromium.org/2118383002/diff/100001/services/ui/public/cpp...
File services/ui/public/cpp/lib/window.cc (right):

https://codereview.chromium.org/2118383002/diff/100001/services/ui/public/cpp...
services/ui/public/cpp/lib/window.cc:451: if (client_ && can_accept_events_ !=
can_accept_events) {
Prefer early return.

  if (can_accept_events_ == can_accept_events)
    return;
  can_accept_events_ = can_accept_events;
  if (client_)
    client_->...

https://codereview.chromium.org/2118383002/diff/100001/services/ui/public/cpp...
File services/ui/public/cpp/window_tree_client.h (right):

https://codereview.chromium.org/2118383002/diff/100001/services/ui/public/cpp...
services/ui/public/cpp/window_tree_client.h:206: void SetCanAcceptEvents(Id
window_id, bool can_accept_events);
Move above, close to SetCanFocus()

[sadrul, 2016-07-12 19:44:56]

Update the CL description with BUG=...

[riajiang, 2016-07-12 20:41:04]

Description was changed from

==========
mus: Disregard windows that explicitly set can_accept_events to be false when
sending events.

This is part of the touch hud app for mustash
(https://codereview.chromium.org/2092343002/).
==========

to

==========
mus: Disregard windows that explicitly set can_accept_events to be false when
sending events.

This is part of the touch hud app for mustash
(https://codereview.chromium.org/2092343002/).

BUG=588311
TEST=mus_ws_unittests
==========

[riajiang, 2016-07-12 20:51:40]

https://codereview.chromium.org/2118383002/diff/100001/services/ui/public/cpp...
File services/ui/public/cpp/lib/window.cc (right):

https://codereview.chromium.org/2118383002/diff/100001/services/ui/public/cpp...
services/ui/public/cpp/lib/window.cc:451: if (client_ && can_accept_events_ !=
can_accept_events) {
On 2016/07/12 19:44:19, sadrul wrote:
> Prefer early return.
> 
>   if (can_accept_events_ == can_accept_events)
>     return;
>   can_accept_events_ = can_accept_events;
>   if (client_)
>     client_->...

Done.

https://codereview.chromium.org/2118383002/diff/100001/services/ui/public/cpp...
File services/ui/public/cpp/window_tree_client.h (right):

https://codereview.chromium.org/2118383002/diff/100001/services/ui/public/cpp...
services/ui/public/cpp/window_tree_client.h:206: void SetCanAcceptEvents(Id
window_id, bool can_accept_events);
On 2016/07/12 19:44:19, sadrul wrote:
> Move above, close to SetCanFocus()

Done.

[msw, 2016-07-12 22:07:23]

lgtm with nits.

https://codereview.chromium.org/2118383002/diff/120001/services/ui/public/cpp...
File services/ui/public/cpp/tests/test_window_tree.h (right):

https://codereview.chromium.org/2118383002/diff/120001/services/ui/public/cpp...
services/ui/public/cpp/tests/test_window_tree.h:108: void
SetCanAcceptEvents(uint32_t window_id, bool can_accept_events) override;
nit: order to match WindowTree decl (ditto for definition).

https://codereview.chromium.org/2118383002/diff/120001/services/ui/public/cpp...
File services/ui/public/cpp/window.h (right):

https://codereview.chromium.org/2118383002/diff/120001/services/ui/public/cpp...
services/ui/public/cpp/window.h:346: // Only call SetCanAcceptEvents if the new
setting is different from
nit: move this sentence up to |SetCanAcceptEvents|.

https://codereview.chromium.org/2118383002/diff/120001/services/ui/public/cpp...
services/ui/public/cpp/window.h:347: // the current one. Initialize it to be
true since server window will
nit: "Initialized to true to match ServerWindow".

https://codereview.chromium.org/2118383002/diff/120001/services/ui/public/cpp...
services/ui/public/cpp/window.h:348: // also initialize can_accept_events_ to be
true.
nit: |can_accept_events_| (if you keep this)

https://codereview.chromium.org/2118383002/diff/120001/services/ui/ws/server_...
File services/ui/ws/server_window.h (right):

https://codereview.chromium.org/2118383002/diff/120001/services/ui/ws/server_...
services/ui/ws/server_window.h:154: void set_can_accept_events(bool
can_accept_events) {
optional nit for one-liner:
  void set_can_accept_events(bool value) { can_accept_events_ = value; }

https://codereview.chromium.org/2118383002/diff/120001/services/ui/ws/server_...
services/ui/ws/server_window.h:231: bool can_accept_events_;
nit: order just after can_focus_; optionally init to true here?

https://codereview.chromium.org/2118383002/diff/120001/services/ui/ws/window_...
File services/ui/ws/window_finder.cc (right):

https://codereview.chromium.org/2118383002/diff/120001/services/ui/ws/window_...
services/ui/ws/window_finder.cc:37: if (!child->can_accept_events())
optional nit: combine with if statement above.

https://codereview.chromium.org/2118383002/diff/120001/services/ui/ws/window_...
File services/ui/ws/window_tree.h (right):

https://codereview.chromium.org/2118383002/diff/120001/services/ui/ws/window_...
services/ui/ws/window_tree.h:170: void SetCanAcceptEvents(Id
transport_window_id,
nit: reorder to match decl and keep overrides together.
(maybe add a separate synchronous wrapper if needed?)

https://codereview.chromium.org/2118383002/diff/120001/services/ui/ws/window_...
File services/ui/ws/window_tree_unittest.cc (right):

https://codereview.chromium.org/2118383002/diff/120001/services/ui/ws/window_...
services/ui/ws/window_tree_unittest.cc:1254: EXPECT_EQ(true,
window->can_accept_events());
nit: EXPECT_TRUE

https://codereview.chromium.org/2118383002/diff/120001/services/ui/ws/window_...
services/ui/ws/window_tree_unittest.cc:1256: EXPECT_EQ(false,
window->can_accept_events());
nit: EXPECT_FALSE

[riajiang, 2016-07-13 00:01:44]

riajiang@chromium.org changed reviewers:
+ dcheng@chromium.org

[riajiang, 2016-07-13 00:01:46]

+dcheng for the mojom

https://codereview.chromium.org/2118383002/diff/120001/services/ui/public/cpp...
File services/ui/public/cpp/tests/test_window_tree.h (right):

https://codereview.chromium.org/2118383002/diff/120001/services/ui/public/cpp...
services/ui/public/cpp/tests/test_window_tree.h:108: void
SetCanAcceptEvents(uint32_t window_id, bool can_accept_events) override;
On 2016/07/12 22:07:23, msw wrote:
> nit: order to match WindowTree decl (ditto for definition).

Done.

https://codereview.chromium.org/2118383002/diff/120001/services/ui/public/cpp...
File services/ui/public/cpp/window.h (right):

https://codereview.chromium.org/2118383002/diff/120001/services/ui/public/cpp...
services/ui/public/cpp/window.h:346: // Only call SetCanAcceptEvents if the new
setting is different from
On 2016/07/12 22:07:23, msw wrote:
> nit: move this sentence up to |SetCanAcceptEvents|.

Done.

https://codereview.chromium.org/2118383002/diff/120001/services/ui/public/cpp...
services/ui/public/cpp/window.h:347: // the current one. Initialize it to be
true since server window will
On 2016/07/12 22:07:23, msw wrote:
> nit: "Initialized to true to match ServerWindow".

Done.

https://codereview.chromium.org/2118383002/diff/120001/services/ui/ws/server_...
File services/ui/ws/server_window.h (right):

https://codereview.chromium.org/2118383002/diff/120001/services/ui/ws/server_...
services/ui/ws/server_window.h:154: void set_can_accept_events(bool
can_accept_events) {
On 2016/07/12 22:07:23, msw wrote:
> optional nit for one-liner:
>   void set_can_accept_events(bool value) { can_accept_events_ = value; }

Done.

https://codereview.chromium.org/2118383002/diff/120001/services/ui/ws/server_...
services/ui/ws/server_window.h:231: bool can_accept_events_;
On 2016/07/12 22:07:23, msw wrote:
> nit: order just after can_focus_; optionally init to true here?

Moved to be after can_focus_. Since can_focus_ is also initialized in the ctor,
I think it would be more consistent to leave it there?

https://codereview.chromium.org/2118383002/diff/120001/services/ui/ws/window_...
File services/ui/ws/window_finder.cc (right):

https://codereview.chromium.org/2118383002/diff/120001/services/ui/ws/window_...
services/ui/ws/window_finder.cc:37: if (!child->can_accept_events())
On 2016/07/12 22:07:23, msw wrote:
> optional nit: combine with if statement above.

Done.

https://codereview.chromium.org/2118383002/diff/120001/services/ui/ws/window_...
File services/ui/ws/window_tree.h (right):

https://codereview.chromium.org/2118383002/diff/120001/services/ui/ws/window_...
services/ui/ws/window_tree.h:170: void SetCanAcceptEvents(Id
transport_window_id,
On 2016/07/12 22:07:23, msw wrote:
> nit: reorder to match decl and keep overrides together.
> (maybe add a separate synchronous wrapper if needed?)

Moved to after SetCanFocus and added a function in test::WindowTreeTestApi to
access this (now private)

https://codereview.chromium.org/2118383002/diff/120001/services/ui/ws/window_...
File services/ui/ws/window_tree_unittest.cc (right):

https://codereview.chromium.org/2118383002/diff/120001/services/ui/ws/window_...
services/ui/ws/window_tree_unittest.cc:1254: EXPECT_EQ(true,
window->can_accept_events());
On 2016/07/12 22:07:23, msw wrote:
> nit: EXPECT_TRUE

Done.

https://codereview.chromium.org/2118383002/diff/120001/services/ui/ws/window_...
services/ui/ws/window_tree_unittest.cc:1256: EXPECT_EQ(false,
window->can_accept_events());
On 2016/07/12 22:07:23, msw wrote:
> nit: EXPECT_FALSE

Done.

[msw, 2016-07-13 01:04:19]

lgtm with just one more optional nit; thanks.

https://codereview.chromium.org/2118383002/diff/120001/services/ui/ws/server_...
File services/ui/ws/server_window.h (right):

https://codereview.chromium.org/2118383002/diff/120001/services/ui/ws/server_...
services/ui/ws/server_window.h:231: bool can_accept_events_;
On 2016/07/13 00:01:46, riajiang wrote:
> On 2016/07/12 22:07:23, msw wrote:
> > nit: order just after can_focus_; optionally init to true here?
> 
> Moved to be after can_focus_. Since can_focus_ is also initialized in the
ctor,
> I think it would be more consistent to leave it there?

Acknowledged.

https://codereview.chromium.org/2118383002/diff/140001/services/ui/public/cpp...
File services/ui/public/cpp/window.h (right):

https://codereview.chromium.org/2118383002/diff/140001/services/ui/public/cpp...
services/ui/public/cpp/window.h:213: // Only calls ui::WindowTreeClient's
SetCanAcceptEvents if the new
nit: Ah, this was a description of the function's implementation, rather than a
suggestion for usage; I'd say this comment can be removed altogether; no-op on
setting to an existing value is pretty common and not particularly worth calling
out explicitly here.

[dcheng, 2016-07-13 14:32:59]

mojo lgtm

https://codereview.chromium.org/2118383002/diff/140001/services/ui/public/int...
File services/ui/public/interfaces/window_tree.mojom (right):

https://codereview.chromium.org/2118383002/diff/140001/services/ui/public/int...
services/ui/public/interfaces/window_tree.mojom:242: // window is not accepting
events, none of its descendent windows would
Super minor nit: a slightly more standard phrasing is "If a window does not
accept events, none of its descendant windows accept events either."

[riajiang, 2016-07-13 18:03:49]

https://codereview.chromium.org/2118383002/diff/140001/services/ui/public/cpp...
File services/ui/public/cpp/window.h (right):

https://codereview.chromium.org/2118383002/diff/140001/services/ui/public/cpp...
services/ui/public/cpp/window.h:213: // Only calls ui::WindowTreeClient's
SetCanAcceptEvents if the new
On 2016/07/13 01:04:19, msw wrote:
> nit: Ah, this was a description of the function's implementation, rather than
a
> suggestion for usage; I'd say this comment can be removed altogether; no-op on
> setting to an existing value is pretty common and not particularly worth
calling
> out explicitly here.

Removed

https://codereview.chromium.org/2118383002/diff/140001/services/ui/public/int...
File services/ui/public/interfaces/window_tree.mojom (right):

https://codereview.chromium.org/2118383002/diff/140001/services/ui/public/int...
services/ui/public/interfaces/window_tree.mojom:242: // window is not accepting
events, none of its descendent windows would
On 2016/07/13 14:32:59, dcheng wrote:
> Super minor nit: a slightly more standard phrasing is "If a window does not
> accept events, none of its descendant windows accept events either."

Done.

[riajiang, 2016-07-13 18:05:16]

The CQ bit was checked by riajiang@chromium.org

[riajiang, 2016-07-13 18:05:17]

The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org,
msw@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2118383002/#ps160001
(title: "Fix comments")

[commit-bot: I haz the power, 2016-07-13 18:05:40]

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-07-13 19:31:12]

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

[commit-bot: I haz the power, 2016-07-13 19:31:13]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[riajiang, 2016-07-13 19:40:19]

The CQ bit was checked by riajiang@chromium.org

[commit-bot: I haz the power, 2016-07-13 19:42:45]

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-07-13 21:38:28]

Description was changed from

==========
mus: Disregard windows that explicitly set can_accept_events to be false when
sending events.

This is part of the touch hud app for mustash
(https://codereview.chromium.org/2092343002/).

BUG=588311
TEST=mus_ws_unittests
==========

to

==========
mus: Disregard windows that explicitly set can_accept_events to be false when
sending events.

This is part of the touch hud app for mustash
(https://codereview.chromium.org/2092343002/).

BUG=588311
TEST=mus_ws_unittests
==========

[commit-bot: I haz the power, 2016-07-13 21:38:29]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-07-13 21:38:42]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-13 21:41:24]

Description was changed from

==========
mus: Disregard windows that explicitly set can_accept_events to be false when
sending events.

This is part of the touch hud app for mustash
(https://codereview.chromium.org/2092343002/).

BUG=588311
TEST=mus_ws_unittests
==========

to

==========
mus: Disregard windows that explicitly set can_accept_events to be false when
sending events.

This is part of the touch hud app for mustash
(https://codereview.chromium.org/2092343002/).

BUG=588311
TEST=mus_ws_unittests

Committed: https://crrev.com/7207207e4326fbc04fa2d226a2c85bafde3c9518
Cr-Commit-Position: refs/heads/master@{#405298}
==========

[commit-bot: I haz the power, 2016-07-13 21:41:26]

Patchset 9 (id:??) landed as
https://crrev.com/7207207e4326fbc04fa2d226a2c85bafde3c9518
Cr-Commit-Position: refs/heads/master@{#405298}

[riajiang, 2016-07-18 14:43:25]

Description was changed from

==========
mus: Disregard windows that explicitly set can_accept_events to be false when
sending events.

This is part of the touch hud app for mustash
(https://codereview.chromium.org/2092343002/).

BUG=588311
TEST=mus_ws_unittests

Committed: https://crrev.com/7207207e4326fbc04fa2d226a2c85bafde3c9518
Cr-Commit-Position: refs/heads/master@{#405298}
==========

to

==========
mus: Disregard windows that explicitly set can_accept_events to be false when
sending events.

BUG=588311
TEST=mus_ws_unittests

Committed: https://crrev.com/7207207e4326fbc04fa2d226a2c85bafde3c9518
Cr-Commit-Position: refs/heads/master@{#405298}
==========