mustash: Use ui::chromeos::EventRewriter in mus

mustash: Use ui::chromeos::EventRewriter in mus

Use ui::chromeos::EventRewriter in mus for Chrome OS build.
It will convert F1~12 keys to media function keys.

BUG=693180
Review-Url: https://codereview.chromium.org/2712963003
Cr-Commit-Position: refs/heads/master@{#458938}
Committed: https://chromium.googlesource.com/chromium/src/+/17134c6c3b7ef0d87c09c6148340ee98b05e8b0d

[Peng, 2017-02-23 16:44:59]

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

[Peng, 2017-02-23 16:45:27]

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

[Peng, 2017-02-23 16:45:28]

Hi Scott, PTAL. Thanks

[commit-bot: I haz the power, 2017-02-23 16:45: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, 2017-02-23 17:00:37]

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

[commit-bot: I haz the power, 2017-02-23 17:00:38]

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

[Peng, 2017-02-23 18:18:58]

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

[commit-bot: I haz the power, 2017-02-23 18:19:56]

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, 2017-02-23 18:44:29]

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

[commit-bot: I haz the power, 2017-02-23 18:44:30]

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

[sky, 2017-02-23 20:23:09]

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

[sky, 2017-02-23 20:23:10]

What's the advantage of this vs changing services/ui/ws/event_dispatcher.cc
directly to support rewriting? That seems simpler.

[Peng, 2017-02-23 20:26:17]

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

[commit-bot: I haz the power, 2017-02-23 20:26:43]

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

[Peng, 2017-02-23 20:30:53]

On 2017/02/23 20:23:10, sky wrote:
> What's the advantage of this vs changing services/ui/ws/event_dispatcher.cc
> directly to support rewriting? That seems simpler.

To share the ChromeOS event rewriters in //chrome/browser/chromeos/events (those
code will be refactored to 
a common place which can be used by browser and //services/ui), we have to
provide a ui::EventSource. See[1].
That's why we need this CL.

https://cs.chromium.org/chromium/src/chrome/browser/chromeos/events/event_rew...

[commit-bot: I haz the power, 2017-02-23 21:27:30]

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

[commit-bot: I haz the power, 2017-02-23 21:27:31]

Dry run: This issue passed the CQ dry run.

[sky, 2017-02-23 21:35:21]

I think you could structure the common code so it only needs
ui::Event. But I would like to hear Sadrul weigh in on this as he
knows the event processing code better than I.

  -Scott

On Thu, Feb 23, 2017 at 12:30 PM,  <penghuang@chromium.org> wrote:
> On 2017/02/23 20:23:10, sky wrote:
>> What's the advantage of this vs changing
>> services/ui/ws/event_dispatcher.cc
>> directly to support rewriting? That seems simpler.
>
> To share the ChromeOS event rewriters in //chrome/browser/chromeos/events
> (those
> code will be refactored to
> a common place which can be used by browser and //services/ui), we have to
> provide a ui::EventSource. See[1].
> That's why we need this CL.
>
>
https://cs.chromium.org/chromium/src/chrome/browser/chromeos/events/event_rew...
>
> https://codereview.chromium.org/2712963003/

-- 
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.

[sadrul, 2017-02-24 15:59:54]

I think I would prefer that we use ui::EventSource etc. for dispatching events,
since it already has support for event-rewriters. It would of course be possible
to still use EventRewriter etc. without using the rest of the event-dispatch
mechanism, but I can't think of a good reason for doing that.

However, if we are going to use the event-dispatch mechanism, then we should do
it right, i.e. we should provide appropriate EventTargeter etc. (which should be
pretty straight-forward too)

https://codereview.chromium.org/2712963003/diff/40001/services/ui/ws/display.cc
File services/ui/ws/display.cc (right):

https://codereview.chromium.org/2712963003/diff/40001/services/ui/ws/display....
services/ui/ws/display.cc:278: NOTREACHED();
These should have proper implementations.

[Peng, 2017-02-27 18:54:24]

Patchset #4 (id:60001) has been deleted

[Peng, 2017-02-28 16:45:46]

Patchset #4 (id:80001) has been deleted

[Peng, 2017-02-28 16:47:30]

https://codereview.chromium.org/2712963003/diff/40001/services/ui/ws/display.cc
File services/ui/ws/display.cc (right):

https://codereview.chromium.org/2712963003/diff/40001/services/ui/ws/display....
services/ui/ws/display.cc:278: NOTREACHED();
On 2017/02/24 15:59:54, sadrul wrote:
> These should have proper implementations.

I did little research and found out it is not easy to use ui::EventTarget and
ui::EventTargeter to do the targeting in mus. (Becasus mus handles event
asynchronously, but ui::EventTarget and ui::EventTargeter don't support it).
Talked offline with sadrul, we decide to only use ui::EventSource for now, and
may revisit later.

[sky, 2017-02-28 17:54:27]

https://codereview.chromium.org/2712963003/diff/40001/services/ui/ws/display.cc
File services/ui/ws/display.cc (right):

https://codereview.chromium.org/2712963003/diff/40001/services/ui/ws/display....
services/ui/ws/display.cc:278: NOTREACHED();
On 2017/02/28 16:47:30, Peng wrote:
> On 2017/02/24 15:59:54, sadrul wrote:
> > These should have proper implementations.
> 
> I did little research and found out it is not easy to use ui::EventTarget and
> ui::EventTargeter to do the targeting in mus. (Becasus mus handles event
> asynchronously, but ui::EventTarget and ui::EventTargeter don't support it).
> Talked offline with sadrul, we decide to only use ui::EventSource for now, and
> may revisit later.

It feels a bit hacky to try to use parts of event without the others. My
understanding is you want to do this change to use EventRewriters. Is there
another way to plumb in the rewriter that doesn't require making the display a
ui::EventProcessor?

[Peng, 2017-02-28 18:14:29]

https://codereview.chromium.org/2712963003/diff/40001/services/ui/ws/display.cc
File services/ui/ws/display.cc (right):

https://codereview.chromium.org/2712963003/diff/40001/services/ui/ws/display....
services/ui/ws/display.cc:278: NOTREACHED();
On 2017/02/28 17:54:26, sky wrote:
> On 2017/02/28 16:47:30, Peng wrote:
> > On 2017/02/24 15:59:54, sadrul wrote:
> > > These should have proper implementations.
> > 
> > I did little research and found out it is not easy to use ui::EventTarget
and
> > ui::EventTargeter to do the targeting in mus. (Becasus mus handles event
> > asynchronously, but ui::EventTarget and ui::EventTargeter don't support it).
> > Talked offline with sadrul, we decide to only use ui::EventSource for now,
and
> > may revisit later.
> 
> It feels a bit hacky to try to use parts of event without the others. My
> understanding is you want to do this change to use EventRewriters. Is there
> another way to plumb in the rewriter that doesn't require making the display a
> ui::EventProcessor?

We can let PlatformDisplayDefault to implement the ui::EventProcessor or add a
new class PlatformDisplayDefaultEventProcessor which implements
ui::EventProcessor. WDYT?

[sky, 2017-02-28 20:35:46]

I'm not sure that's much better. What is relying on ui::EventProcessor
to use the rewriters?

On Tue, Feb 28, 2017 at 10:14 AM,  <penghuang@chromium.org> wrote:
>
>
https://codereview.chromium.org/2712963003/diff/40001/services/ui/ws/display.cc
> File services/ui/ws/display.cc (right):
>
>
https://codereview.chromium.org/2712963003/diff/40001/services/ui/ws/display....
> services/ui/ws/display.cc:278: NOTREACHED();
> On 2017/02/28 17:54:26, sky wrote:
>> On 2017/02/28 16:47:30, Peng wrote:
>> > On 2017/02/24 15:59:54, sadrul wrote:
>> > > These should have proper implementations.
>> >
>> > I did little research and found out it is not easy to use
> ui::EventTarget and
>> > ui::EventTargeter to do the targeting in mus. (Becasus mus handles
> event
>> > asynchronously, but ui::EventTarget and ui::EventTargeter don't
> support it).
>> > Talked offline with sadrul, we decide to only use ui::EventSource
> for now, and
>> > may revisit later.
>>
>> It feels a bit hacky to try to use parts of event without the others.
> My
>> understanding is you want to do this change to use EventRewriters. Is
> there
>> another way to plumb in the rewriter that doesn't require making the
> display a
>> ui::EventProcessor?
>
> We can let PlatformDisplayDefault to implement the ui::EventProcessor or
> add a new class PlatformDisplayDefaultEventProcessor which implements
> ui::EventProcessor. WDYT?
>
> https://codereview.chromium.org/2712963003/

-- 
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.

[sky, 2017-02-28 20:36:05]

Also, Sadrul knows this area better than I. So, if he thinks what you
have here is best, go for it.

On Tue, Feb 28, 2017 at 12:35 PM, Scott Violet <sky@chromium.org> wrote:
> I'm not sure that's much better. What is relying on ui::EventProcessor
> to use the rewriters?
>
> On Tue, Feb 28, 2017 at 10:14 AM,  <penghuang@chromium.org> wrote:
>>
>>
https://codereview.chromium.org/2712963003/diff/40001/services/ui/ws/display.cc
>> File services/ui/ws/display.cc (right):
>>
>>
https://codereview.chromium.org/2712963003/diff/40001/services/ui/ws/display....
>> services/ui/ws/display.cc:278: NOTREACHED();
>> On 2017/02/28 17:54:26, sky wrote:
>>> On 2017/02/28 16:47:30, Peng wrote:
>>> > On 2017/02/24 15:59:54, sadrul wrote:
>>> > > These should have proper implementations.
>>> >
>>> > I did little research and found out it is not easy to use
>> ui::EventTarget and
>>> > ui::EventTargeter to do the targeting in mus. (Becasus mus handles
>> event
>>> > asynchronously, but ui::EventTarget and ui::EventTargeter don't
>> support it).
>>> > Talked offline with sadrul, we decide to only use ui::EventSource
>> for now, and
>>> > may revisit later.
>>>
>>> It feels a bit hacky to try to use parts of event without the others.
>> My
>>> understanding is you want to do this change to use EventRewriters. Is
>> there
>>> another way to plumb in the rewriter that doesn't require making the
>> display a
>>> ui::EventProcessor?
>>
>> We can let PlatformDisplayDefault to implement the ui::EventProcessor or
>> add a new class PlatformDisplayDefaultEventProcessor which implements
>> ui::EventProcessor. WDYT?
>>
>> https://codereview.chromium.org/2712963003/

-- 
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.

[sadrul, 2017-03-06 17:11:36]

On 2017/02/28 20:36:05, sky wrote:
> Also, Sadrul knows this area better than I. So, if he thinks what you
> have here is best, go for it.

Peng and I discussed about this offline. Because of the asynchronous nature of
event dispatch in the window server, using the current mechanism for
event-dispatch ui/events/ may be a fair amount of work. I still think we should
do that, although that can happen separately so that we can start using the
rewriters sooner.

Landing this change before https://codereview.chromium.org/2724913002/ isn't
going to be useful, so let's wait for that one to land first. In the meantime, I
will see if I can come up with a strategy to update the dispatch mechanism so
that it can be used easily in mus-ws.

[sky, 2017-03-06 18:51:28]

I'm actually fine with keeping mus doing it's own dispatch. I feel like if
we try to convert it to use ui/events we'll likely end up with a more
complex system that for the most part isn't used.

  -Scott

On Mon, Mar 6, 2017 at 9:11 AM, <sadrul@chromium.org> wrote:

> On 2017/02/28 20:36:05, sky wrote:
> > Also, Sadrul knows this area better than I. So, if he thinks what you
> > have here is best, go for it.
>
> Peng and I discussed about this offline. Because of the asynchronous
> nature of
> event dispatch in the window server, using the current mechanism for
> event-dispatch ui/events/ may be a fair amount of work. I still think we
> should
> do that, although that can happen separately so that we can start using the
> rewriters sooner.
>
> Landing this change before https://codereview.chromium.org/2724913002/
> isn't
> going to be useful, so let's wait for that one to land first. In the
> meantime, I
> will see if I can come up with a strategy to update the dispatch mechanism
> so
> that it can be used easily in mus-ws.
>
> https://codereview.chromium.org/2712963003/
>

-- 
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.

[Peng, 2017-03-14 18:56:16]

Description was changed from

==========
mustash: Using ui::EventSource in mus for dispatching events.

Let ui::PlatformDisplayDefault implement ui::EventSource, so we can support
event rewriters in mus. In ChromeOS, we need event rewriter to convert F1~12
to chromebook function keys.

BUG=693180
==========

to

==========
mustash: Using ui::chromeos::EventRewriter in mus

Let ui::PlatformDisplayDefault implement ui::EventSource, so we can
support event rewriters in mus. And use ui::chromeos::EventRewriter
in mus to convert F1~12 to chromebook function keys.

BUG=693180
==========

[Peng, 2017-03-14 18:57:40]

I updated this CL. With CL, the ui::chromeos::EventRewriter will be used
in mus to convert chromeos media function keys. PTAL. Thanks.

On 2017/03/06 18:51:28, sky wrote:
> I'm actually fine with keeping mus doing it's own dispatch. I feel like if
> we try to convert it to use ui/events we'll likely end up with a more
> complex system that for the most part isn't used.
> 
>   -Scott
> 
> On Mon, Mar 6, 2017 at 9:11 AM, <mailto:sadrul@chromium.org> wrote:
> 
> > On 2017/02/28 20:36:05, sky wrote:
> > > Also, Sadrul knows this area better than I. So, if he thinks what you
> > > have here is best, go for it.
> >
> > Peng and I discussed about this offline. Because of the asynchronous
> > nature of
> > event dispatch in the window server, using the current mechanism for
> > event-dispatch ui/events/ may be a fair amount of work. I still think we
> > should
> > do that, although that can happen separately so that we can start using the
> > rewriters sooner.
> >
> > Landing this change before https://codereview.chromium.org/2724913002/
> > isn't
> > going to be useful, so let's wait for that one to land first. In the
> > meantime, I
> > will see if I can come up with a strategy to update the dispatch mechanism
> > so
> > that it can be used easily in mus-ws.
> >
> > https://codereview.chromium.org/2712963003/
> >
> 
> -- 
> 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 mailto:chromium-reviews+unsubscribe@chromium.org.

[sky, 2017-03-14 20:34:10]

On 2017/03/14 18:57:40, Peng wrote:
> I updated this CL. With CL, the ui::chromeos::EventRewriter will be used
> in mus to convert chromeos media function keys. PTAL. Thanks.
> 
> On 2017/03/06 18:51:28, sky wrote:
> > I'm actually fine with keeping mus doing it's own dispatch. I feel like if
> > we try to convert it to use ui/events we'll likely end up with a more
> > complex system that for the most part isn't used.
> > 
> >   -Scott
> > 
> > On Mon, Mar 6, 2017 at 9:11 AM, <mailto:sadrul@chromium.org> wrote:
> > 
> > > On 2017/02/28 20:36:05, sky wrote:
> > > > Also, Sadrul knows this area better than I. So, if he thinks what you
> > > > have here is best, go for it.
> > >
> > > Peng and I discussed about this offline. Because of the asynchronous
> > > nature of
> > > event dispatch in the window server, using the current mechanism for
> > > event-dispatch ui/events/ may be a fair amount of work. I still think we
> > > should
> > > do that, although that can happen separately so that we can start using
the
> > > rewriters sooner.
> > >
> > > Landing this change before https://codereview.chromium.org/2724913002/
> > > isn't
> > > going to be useful, so let's wait for that one to land first. In the
> > > meantime, I
> > > will see if I can come up with a strategy to update the dispatch mechanism
> > > so
> > > that it can be used easily in mus-ws.
> > >
> > > https://codereview.chromium.org/2712963003/
> > >
> > 
> > -- 
> > 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 mailto:chromium-reviews+unsubscribe@chromium.org.

Because of the different code path events take in mus I'm inclined to not use
EventProcessor, EventTarget and the like in mus. Instead of this change could
WMS call EventRewriter directly?

[Peng, 2017-03-15 19:03:51]

Description was changed from

==========
mustash: Using ui::chromeos::EventRewriter in mus

Let ui::PlatformDisplayDefault implement ui::EventSource, so we can
support event rewriters in mus. And use ui::chromeos::EventRewriter
in mus to convert F1~12 to chromebook function keys.

BUG=693180
==========

to

==========
mustash: Use ui::chromeos::EventRewriter in mus

Use ui::chromeos::EventRewriter in mus for Chrome OS build.
It will convert F1~12 keys to media function keys.

BUG=693180
==========

[Peng, 2017-03-15 19:07:35]

Hi Scott, I updated CL to not use ui::EventSource. PTAL. Thanks.

[sky, 2017-03-15 19:48:12]

Thanks for cleaning this up!

How about test coverage?

https://codereview.chromium.org/2712963003/diff/140001/services/ui/ws/BUILD.gn
File services/ui/ws/BUILD.gn (right):

https://codereview.chromium.org/2712963003/diff/140001/services/ui/ws/BUILD.g...
services/ui/ws/BUILD.gn:155: public_deps += [ "//ui/chromeos/events" ]
I was concerned about what this pulls in, and don't see it a directory with this
name. Is it right?

https://codereview.chromium.org/2712963003/diff/140001/services/ui/ws/display.cc
File services/ui/ws/display.cc (right):

https://codereview.chromium.org/2712963003/diff/140001/services/ui/ws/display...
services/ui/ws/display.cc:279: base::ScopedClosureRunner scoped_runner(
Do we really need a ScopedClosureRunner? Can you call OnUserActivity right away?
If the event is discarded do we still want to call OnUserActivity?

https://codereview.chromium.org/2712963003/diff/140001/services/ui/ws/display...
services/ui/ws/display.cc:288: auto status = ui::EVENT_REWRITE_CONTINUE;
Please don't overuse auto, it's make the code harder to read. I would prefer if
you didn't use auto in any of the cases you have here.

[sadrul, 2017-03-16 05:05:40]

https://codereview.chromium.org/2712963003/diff/160001/services/ui/ws/display...
File services/ui/ws/display_manager.h (right):

https://codereview.chromium.org/2712963003/diff/160001/services/ui/ws/display...
services/ui/ws/display_manager.h:104: std::unique_ptr<ui::EventRewriter>
event_rewriter_;
Note that we will need to add more rewriters (accessibility adds several
rewriters). It would be better to maintain a list here.

[Daniel Erat, 2017-03-16 16:11:03]

https://codereview.chromium.org/2712963003/diff/160001/services/ui/ws/display.cc
File services/ui/ws/display.cc (right):

https://codereview.chromium.org/2712963003/diff/160001/services/ui/ws/display...
services/ui/ws/display.cc:292: // |event_rewriter| is null for non-ChromeOS
build.
nit: non-Chrome-OS (since there's a space between "Chrome" and "OS" usually)

[sky, 2017-03-16 16:11:43]

https://codereview.chromium.org/2712963003/diff/160001/services/ui/ws/display...
File services/ui/ws/display_manager.h (right):

https://codereview.chromium.org/2712963003/diff/160001/services/ui/ws/display...
services/ui/ws/display_manager.h:104: std::unique_ptr<ui::EventRewriter>
event_rewriter_;
On 2017/03/16 05:05:40, sadrul wrote:
> Note that we will need to add more rewriters (accessibility adds several
> rewriters). It would be better to maintain a list here.

Will we need to support rewriters from different services? If so, we need to
make sure we come up with a declarative way to specify what those rewriters do
to avoid having to call out too much during dispatch. This can be done later,
not in this patch.

[Peng, 2017-03-16 17:28:58]

https://codereview.chromium.org/2712963003/diff/140001/services/ui/ws/BUILD.gn
File services/ui/ws/BUILD.gn (right):

https://codereview.chromium.org/2712963003/diff/140001/services/ui/ws/BUILD.g...
services/ui/ws/BUILD.gn:155: public_deps += [ "//ui/chromeos/events" ]
On 2017/03/15 19:48:11, sky wrote:
> I was concerned about what this pulls in, and don't see it a directory with
this
> name. Is it right?

It will be added by CL https://codereview.chromium.org/2724913002/ . It is still
under review.

https://codereview.chromium.org/2712963003/diff/140001/services/ui/ws/display.cc
File services/ui/ws/display.cc (right):

https://codereview.chromium.org/2712963003/diff/140001/services/ui/ws/display...
services/ui/ws/display.cc:279: base::ScopedClosureRunner scoped_runner(
On 2017/03/15 19:48:11, sky wrote:
> Do we really need a ScopedClosureRunner? Can you call OnUserActivity right
away?
> If the event is discarded do we still want to call OnUserActivity?

This function has several return statements, using ScopedClosureRunner may make
code simple, and it will be unlikely to have bugs in future.

I guess calling OnUserActivity() is for tracing last user event, right?
Maybe use it for keep screen on. In that case, I think we will want to keep the
screen on by any event, even if the event is consumed by event rewriter. For
example, the sticky keys controller may consume the CTRL release event and send
it back to UI later.

https://codereview.chromium.org/2712963003/diff/140001/services/ui/ws/display...
services/ui/ws/display.cc:288: auto status = ui::EVENT_REWRITE_CONTINUE;
On 2017/03/15 19:48:11, sky wrote:
> Please don't overuse auto, it's make the code harder to read. I would prefer
if
> you didn't use auto in any of the cases you have here.

Done.

https://codereview.chromium.org/2712963003/diff/160001/services/ui/ws/display.cc
File services/ui/ws/display.cc (right):

https://codereview.chromium.org/2712963003/diff/160001/services/ui/ws/display...
services/ui/ws/display.cc:292: // |event_rewriter| is null for non-ChromeOS
build.
On 2017/03/16 16:11:03, Daniel Erat wrote:
> nit: non-Chrome-OS (since there's a space between "Chrome" and "OS" usually)

non-ChromeOS vs non-chrome-os [1] vs [2]. Seems non-ChromeOS is  more popular in
chromium source code.

[1] https://cs.chromium.org/search/?q=non-chromeos+package:%5Echromium$&type=cs

[2] https://cs.chromium.org/search/?q=non-chrome-os+package:%5Echromium$&type=cs

[Peng, 2017-03-16 17:36:12]

On 2017/03/16 16:11:43, sky wrote:
>
https://codereview.chromium.org/2712963003/diff/160001/services/ui/ws/display...
> File services/ui/ws/display_manager.h (right):
> 
>
https://codereview.chromium.org/2712963003/diff/160001/services/ui/ws/display...
> services/ui/ws/display_manager.h:104: std::unique_ptr<ui::EventRewriter>
> event_rewriter_;
> On 2017/03/16 05:05:40, sadrul wrote:
> > Note that we will need to add more rewriters (accessibility adds several
> > rewriters). It would be better to maintain a list here.
> 
> Will we need to support rewriters from different services? If so, we need to
> make sure we come up with a declarative way to specify what those rewriters do
> to avoid having to call out too much during dispatch. This can be done later,
> not in this patch.

See [1] [2]. In chrome, several  rewriters are used (media function keys,
Keyboard driven, spoken feedback,
TouchExplorationController, ect). To support multiple rewriters, we need
duplicated more code
from ui::EventSource, and test cases as well. Probably it is better to just use
the ui::EventSource in
mus directly, or maybe refactor ui::EventSource a little bit, make it more
lightweight for mus. WDYT?


[1]
https://cs.chromium.org/chromium/src/chrome/browser/chromeos/chrome_browser_m...
[2]
https://cs.chromium.org/chromium/src/ui/chromeos/touch_exploration_controller...

[sadrul, 2017-03-16 17:37:26]

https://codereview.chromium.org/2712963003/diff/160001/services/ui/ws/display...
File services/ui/ws/display_manager.h (right):

https://codereview.chromium.org/2712963003/diff/160001/services/ui/ws/display...
services/ui/ws/display_manager.h:104: std::unique_ptr<ui::EventRewriter>
event_rewriter_;
On 2017/03/16 16:11:43, sky wrote:
> On 2017/03/16 05:05:40, sadrul wrote:
> > Note that we will need to add more rewriters (accessibility adds several
> > rewriters). It would be better to maintain a list here.
> 
> Will we need to support rewriters from different services? If so, we need to
> make sure we come up with a declarative way to specify what those rewriters do
> to avoid having to call out too much during dispatch. This can be done later,
> not in this patch.

I actually think those services should simply live in the window server itself,
i.e. window server itself should provide explicit accessibility support (that
relates to windowing/event systems). In other words, I think we should use the
existing event-rewriters here.

[sky, 2017-03-16 20:23:08]

https://codereview.chromium.org/2712963003/diff/140001/services/ui/ws/display.cc
File services/ui/ws/display.cc (right):

https://codereview.chromium.org/2712963003/diff/140001/services/ui/ws/display...
services/ui/ws/display.cc:279: base::ScopedClosureRunner scoped_runner(
On 2017/03/16 17:28:57, Peng wrote:
> On 2017/03/15 19:48:11, sky wrote:
> > Do we really need a ScopedClosureRunner? Can you call OnUserActivity right
> away?
> > If the event is discarded do we still want to call OnUserActivity?
> 
> This function has several return statements, using ScopedClosureRunner may
make
> code simple, and it will be unlikely to have bugs in future.
> 
> I guess calling OnUserActivity() is for tracing last user event, right?
> Maybe use it for keep screen on. In that case, I think we will want to keep
the
> screen on by any event, even if the event is consumed by event rewriter. For
> example, the sticky keys controller may consume the CTRL release event and
send
> it back to UI later.

Sorry for not being clear. I wasn't suggesting you don't call OnUserActivity.
Rather I'm asking if you can call OnUserActivity() directly right here and no
ScopedClosureRunner.

https://codereview.chromium.org/2712963003/diff/160001/services/ui/ws/display...
File services/ui/ws/display_manager.h (right):

https://codereview.chromium.org/2712963003/diff/160001/services/ui/ws/display...
services/ui/ws/display_manager.h:104: std::unique_ptr<ui::EventRewriter>
event_rewriter_;
On 2017/03/16 17:37:26, sadrul wrote:
> On 2017/03/16 16:11:43, sky wrote:
> > On 2017/03/16 05:05:40, sadrul wrote:
> > > Note that we will need to add more rewriters (accessibility adds several
> > > rewriters). It would be better to maintain a list here.
> > 
> > Will we need to support rewriters from different services? If so, we need to
> > make sure we come up with a declarative way to specify what those rewriters
do
> > to avoid having to call out too much during dispatch. This can be done
later,
> > not in this patch.
> 
> I actually think those services should simply live in the window server
itself,
> i.e. window server itself should provide explicit accessibility support (that
> relates to windowing/event systems). In other words, I think we should use the
> existing event-rewriters here.

SGTM. I assume then mus grows an API to turn on the rewriters?

[sky, 2017-03-16 20:29:53]

https://codereview.chromium.org/2712963003/diff/160001/services/ui/ws/BUILD.gn
File services/ui/ws/BUILD.gn (right):

https://codereview.chromium.org/2712963003/diff/160001/services/ui/ws/BUILD.g...
services/ui/ws/BUILD.gn:155: public_deps += [ "//ui/chromeos/events" ]
I want to make sure we don't end up pulling in any unwanted deps. Can you add:
  assert_no_deps = [
    "//ash
    "//content/public/browser",
    "//content/public/common",
    "//ui/aura",
    "//ui/views",
  ]

[Peng, 2017-03-17 20:09:11]

Patchset #8 (id:180001) has been deleted

[Peng, 2017-03-19 00:13:23]

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

[commit-bot: I haz the power, 2017-03-19 00:13: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, 2017-03-19 00:23:27]

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

[commit-bot: I haz the power, 2017-03-19 00:23:28]

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

[Peng, 2017-03-20 15:08:29]

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

[commit-bot: I haz the power, 2017-03-20 15:08:44]

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, 2017-03-20 15:17:33]

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

[commit-bot: I haz the power, 2017-03-20 15:17:34]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[sky, 2017-03-20 15:42:13]

https://codereview.chromium.org/2712963003/diff/220001/services/ui/ws/cursor_...
File services/ui/ws/cursor_unittest.cc (right):

https://codereview.chromium.org/2712963003/diff/220001/services/ui/ws/cursor_...
services/ui/ws/cursor_unittest.cc:95:
(void)static_cast<PlatformDisplayDelegate*>(display)
Why the (void) here?

https://codereview.chromium.org/2712963003/diff/220001/services/ui/ws/platfor...
File services/ui/ws/platform_display_default.cc (right):

https://codereview.chromium.org/2712963003/diff/220001/services/ui/ws/platfor...
services/ui/ws/platform_display_default.cc:163:
SendEventToSink(const_cast<ui::Event*>(&event));
Don't cast away the const here. Instead make this take a ui::Event* event.

https://codereview.chromium.org/2712963003/diff/220001/services/ui/ws/test_ut...
File services/ui/ws/test_utils.h (right):

https://codereview.chromium.org/2712963003/diff/220001/services/ui/ws/test_ut...
services/ui/ws/test_utils.h:142:
display_->OnEventFromSource(const_cast<ui::Event*>(&event));
Same comment here about not casting away const.

https://codereview.chromium.org/2712963003/diff/220001/ui/aura/window_tree_ho...
File ui/aura/window_tree_host.cc (right):

https://codereview.chromium.org/2712963003/diff/220001/ui/aura/window_tree_ho...
ui/aura/window_tree_host.cc:75: return dispatcher();
return dispatcher_.get;

https://codereview.chromium.org/2712963003/diff/220001/ui/aura/window_tree_ho...
ui/aura/window_tree_host.cc:333: return event_sink();
return disptacher_.get();

https://codereview.chromium.org/2712963003/diff/220001/ui/events/event_sink.h
File ui/events/event_sink.h (right):

https://codereview.chromium.org/2712963003/diff/220001/ui/events/event_sink.h...
ui/events/event_sink.h:21: WARN_UNUSED_RESULT = 0;
Given mus seems to not care about the return value, is it really necessary to
have WARN_UNUSED_RESULT?

[Peng, 2017-03-20 17:24:07]

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

[Peng, 2017-03-20 17:24:14]

https://codereview.chromium.org/2712963003/diff/220001/services/ui/ws/cursor_...
File services/ui/ws/cursor_unittest.cc (right):

https://codereview.chromium.org/2712963003/diff/220001/services/ui/ws/cursor_...
services/ui/ws/cursor_unittest.cc:95:
(void)static_cast<PlatformDisplayDelegate*>(display)
On 2017/03/20 15:42:12, sky wrote:
> Why the (void) here?

Done.

https://codereview.chromium.org/2712963003/diff/220001/services/ui/ws/platfor...
File services/ui/ws/platform_display_default.cc (right):

https://codereview.chromium.org/2712963003/diff/220001/services/ui/ws/platfor...
services/ui/ws/platform_display_default.cc:163:
SendEventToSink(const_cast<ui::Event*>(&event));
On 2017/03/20 15:42:12, sky wrote:
> Don't cast away the const here. Instead make this take a ui::Event* event.

Done.

https://codereview.chromium.org/2712963003/diff/220001/services/ui/ws/test_ut...
File services/ui/ws/test_utils.h (right):

https://codereview.chromium.org/2712963003/diff/220001/services/ui/ws/test_ut...
services/ui/ws/test_utils.h:142:
display_->OnEventFromSource(const_cast<ui::Event*>(&event));
On 2017/03/20 15:42:12, sky wrote:
> Same comment here about not casting away const.

Done.

https://codereview.chromium.org/2712963003/diff/220001/ui/aura/window_tree_ho...
File ui/aura/window_tree_host.cc (right):

https://codereview.chromium.org/2712963003/diff/220001/ui/aura/window_tree_ho...
ui/aura/window_tree_host.cc:75: return dispatcher();
On 2017/03/20 15:42:12, sky wrote:
> return dispatcher_.get;

Done.

https://codereview.chromium.org/2712963003/diff/220001/ui/aura/window_tree_ho...
ui/aura/window_tree_host.cc:333: return event_sink();
On 2017/03/20 15:42:12, sky wrote:
> return disptacher_.get();

Done.

https://codereview.chromium.org/2712963003/diff/220001/ui/events/event_sink.h
File ui/events/event_sink.h (right):

https://codereview.chromium.org/2712963003/diff/220001/ui/events/event_sink.h...
ui/events/event_sink.h:21: WARN_UNUSED_RESULT = 0;
On 2017/03/20 15:42:13, sky wrote:
> Given mus seems to not care about the return value, is it really necessary to
> have WARN_UNUSED_RESULT?

Remove it. Done

[commit-bot: I haz the power, 2017-03-20 17:24:53]

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, 2017-03-20 17:45:32]

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

[commit-bot: I haz the power, 2017-03-20 17:45:33]

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_...)

[Peng, 2017-03-20 18:19:30]

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

[commit-bot: I haz the power, 2017-03-20 18:20: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, 2017-03-20 19:08:45]

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

[commit-bot: I haz the power, 2017-03-20 19:08:47]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[sky, 2017-03-20 20:03:25]

LGTM

https://codereview.chromium.org/2712963003/diff/240001/services/ui/ws/cursor_...
File services/ui/ws/cursor_unittest.cc (right):

https://codereview.chromium.org/2712963003/diff/240001/services/ui/ws/cursor_...
services/ui/ws/cursor_unittest.cc:95:
ignore_result(static_cast<PlatformDisplayDelegate*>(display)
You no longer need the ignore_result here.

[Peng, 2017-03-20 20:25:09]

penghuang@chromium.org changed reviewers:
+ dtseng@chromium.org, jamescook@chromium.org

[Peng, 2017-03-20 20:25:10]

+jamescook@chromium.org for
extensions/shell/browser/shell_desktop_controller_aura.cc

+dtseng@chromium.org for
content/browser/accessibility/touch_accessibility_aura_browsertest.cc

Hi James and David, PTAL. Thanks.

https://codereview.chromium.org/2712963003/diff/240001/services/ui/ws/cursor_...
File services/ui/ws/cursor_unittest.cc (right):

https://codereview.chromium.org/2712963003/diff/240001/services/ui/ws/cursor_...
services/ui/ws/cursor_unittest.cc:95:
ignore_result(static_cast<PlatformDisplayDelegate*>(display)
On 2017/03/20 20:03:25, sky wrote:
> You no longer need the ignore_result here.

Done

[James Cook, 2017-03-20 20:33:20]

extensions/shell lgtm

[Peng, 2017-03-20 20:34:00]

sadrul, I updated this CL. PTAL. Thanks.

[Peng, 2017-03-20 21:16:39]

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

[commit-bot: I haz the power, 2017-03-20 21:17:14]

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, 2017-03-20 22:54:19]

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

[commit-bot: I haz the power, 2017-03-20 22:54:20]

Dry run: This issue passed the CQ dry run.

[David Tseng, 2017-03-21 18:15:36]

lgtm

c/b/c/accessibility lgtm

[sadrul, 2017-03-22 19:39:17]

lgtm

[sadrul, 2017-03-22 19:40:10]

https://codereview.chromium.org/2712963003/diff/270001/ui/events/event_sink.h
File ui/events/event_sink.h (right):

https://codereview.chromium.org/2712963003/diff/270001/ui/events/event_sink.h...
ui/events/event_sink.h:20: virtual EventDispatchDetails OnEventFromSource(Event*
event) = 0;
Actually, can you use WARN_UNUSED_RESULT here?

[sadrul, 2017-03-22 19:52:37]

https://codereview.chromium.org/2712963003/diff/220001/ui/events/event_sink.h
File ui/events/event_sink.h (right):

https://codereview.chromium.org/2712963003/diff/220001/ui/events/event_sink.h...
ui/events/event_sink.h:21: WARN_UNUSED_RESULT = 0;
On 2017/03/20 15:42:13, sky wrote:
> Given mus seems to not care about the return value, is it really necessary to
> have WARN_UNUSED_RESULT?

This has been useful to prevent uaf in aura when windows/hosts are destroyed
during the processing of an event. I would prefer that we have this, and use
ignore_result() where we don't need the return value.

[Peng, 2017-03-22 20:16:19]

https://codereview.chromium.org/2712963003/diff/270001/ui/events/event_sink.h
File ui/events/event_sink.h (right):

https://codereview.chromium.org/2712963003/diff/270001/ui/events/event_sink.h...
ui/events/event_sink.h:20: virtual EventDispatchDetails OnEventFromSource(Event*
event) = 0;
On 2017/03/22 19:40:10, sadrul wrote:
> Actually, can you use WARN_UNUSED_RESULT here?

Done.

[Peng, 2017-03-22 20:16:32]

The CQ bit was checked by penghuang@chromium.org

[Peng, 2017-03-22 20:16:32]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org,
sadrul@chromium.org, jamescook@chromium.org, dtseng@chromium.org
Link to the patchset: https://codereview.chromium.org/2712963003/#ps290001
(title: "Address review issue")

[commit-bot: I haz the power, 2017-03-22 20:17:44]

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, 2017-03-22 20:31:49]

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

[commit-bot: I haz the power, 2017-03-22 20:31:51]

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

[Peng, 2017-03-22 20:51:23]

The CQ bit was checked by penghuang@chromium.org

[Peng, 2017-03-22 20:51:24]

The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org,
jamescook@chromium.org, dtseng@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2712963003/#ps310001
(title: "Fix build issues.")

[commit-bot: I haz the power, 2017-03-22 20:52:27]

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, 2017-03-23 00:01:12]

CQ is committing da patch.

Bot data: {"patchset_id": 310001, "attempt_start_ts": 1490215883890590,
"parent_rev": "086c46157f8d1c5386c9bcab1c1f81f6d106e817", "commit_rev":
"17134c6c3b7ef0d87c09c6148340ee98b05e8b0d"}

[commit-bot: I haz the power, 2017-03-23 00:02:03]

Description was changed from

==========
mustash: Use ui::chromeos::EventRewriter in mus

Use ui::chromeos::EventRewriter in mus for Chrome OS build.
It will convert F1~12 keys to media function keys.

BUG=693180
==========

to

==========
mustash: Use ui::chromeos::EventRewriter in mus

Use ui::chromeos::EventRewriter in mus for Chrome OS build.
It will convert F1~12 keys to media function keys.

BUG=693180

Review-Url: https://codereview.chromium.org/2712963003
Cr-Commit-Position: refs/heads/master@{#458938}
Committed:
https://chromium.googlesource.com/chromium/src/+/17134c6c3b7ef0d87c09c6148340...
==========

[commit-bot: I haz the power, 2017-03-23 00:02:05]

Committed patchset #14 (id:310001) as
https://chromium.googlesource.com/chromium/src/+/17134c6c3b7ef0d87c09c6148340...