mus: Introduce AcceleratorRegistrar and AcceleratorHandler.

mus: Introduce AcceleratorRegistrar and AcceleratorHandler.

. An app can request an AcceleratorRegistrar from the window manager, and
  install some accelerators. The app must install an AcceleratorHandler to
  process these accelerators.
. The AcceleratorRegistrar implementation in the window manager makes sure
  that the accelerator ids from different apps do not conflict with each
  other.
. When an AcceleratorHandler is disconnected, the accelerators its supposed
  to handle are automatically unregistered from mus.

BUG=548363
Committed: https://crrev.com/669040467ae91e1bc48cd24ae6afd64860e8d47a
Cr-Commit-Position: refs/heads/master@{#362915}

[sadrul, 2015-12-01 00:10:33]

Description was changed from

==========
mus: Introduce AcceleratorRegistrar and AcceleratorHandler.

...

BUG=548363
==========

to

==========
mus: Introduce AcceleratorRegistrar and AcceleratorHandler.

BUG=548363
==========

[sadrul, 2015-12-01 00:12:21]

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

[sadrul, 2015-12-01 00:12:21]

This introduces AcceleratorRegistrar and AcceleratorHandler inside mus. However,
mus itself doesn't do anything with these. mash-wm is the one that actually
provides the implementation. So maybe I should move these out of mus and into
mash/wm instead?

[sadrul, 2015-12-01 00:29:07]

Description was changed from

==========
mus: Introduce AcceleratorRegistrar and AcceleratorHandler.

BUG=548363
==========

to

==========
mus: Introduce AcceleratorRegistrar and AcceleratorHandler.

. An app can request an AcceleratorRegistrar from the window manager, and
  install some accelerators and AcceleratorHandlers.
. The AcceleratorRegistrar implementation in the window manager makes sure
  that the accelerator ids from different apps do not conflict with each
  other.
. When an AcceleratorRegistrar proxy is destroyed, the accelerators
  registered by the app through the proxy are automatically unregistered
  from mus.

BUG=548363
==========

[sadrul, 2015-12-01 00:29:52]

Updated the CL description to briefly explain how this works.

[Ben Goodger (Google), 2015-12-01 03:47:33]

https://codereview.chromium.org/1488713002/diff/40001/components/mus/public/i...
File components/mus/public/interfaces/accelerator_registrar.mojom (right):

https://codereview.chromium.org/1488713002/diff/40001/components/mus/public/i...
components/mus/public/interfaces/accelerator_registrar.mojom:14: interface
AcceleratorRegistrar {
While it's clever to control the lifetime of namespaces by the lifetime of this
object, and bind the lifetime of this object to the lifetime of the pipe, it
feels a bit heavyweight that a client has to keep this pipe open effectively
forever. I wonder if there's a way to have a single registry and scope the
lifetime of the entries to the AcceleratorHandler pipe which must remain open
for obvious reasons?

[sadrul, 2015-12-01 15:11:40]

Description was changed from

==========
mus: Introduce AcceleratorRegistrar and AcceleratorHandler.

. An app can request an AcceleratorRegistrar from the window manager, and
  install some accelerators and AcceleratorHandlers.
. The AcceleratorRegistrar implementation in the window manager makes sure
  that the accelerator ids from different apps do not conflict with each
  other.
. When an AcceleratorRegistrar proxy is destroyed, the accelerators
  registered by the app through the proxy are automatically unregistered
  from mus.

BUG=548363
==========

to

==========
mus: Introduce AcceleratorRegistrar and AcceleratorHandler.

. An app can request an AcceleratorRegistrar from the window manager, and
  install some accelerators and AcceleratorHandlers.
. The AcceleratorRegistrar implementation in the window manager makes sure
  that the accelerator ids from different apps do not conflict with each
  other.
. When an AcceleratorHandler is disconnected, the accelerators its supposed
  to handle are automatically unregistered from mus.

BUG=548363
==========

[sadrul, 2015-12-01 15:17:24]

https://codereview.chromium.org/1488713002/diff/40001/components/mus/public/i...
File components/mus/public/interfaces/accelerator_registrar.mojom (right):

https://codereview.chromium.org/1488713002/diff/40001/components/mus/public/i...
components/mus/public/interfaces/accelerator_registrar.mojom:14: interface
AcceleratorRegistrar {
On 2015/12/01 03:47:32, Ben Goodger (Google) wrote:
> While it's clever to control the lifetime of namespaces by the lifetime of
this
> object, and bind the lifetime of this object to the lifetime of the pipe, it
> feels a bit heavyweight that a client has to keep this pipe open effectively
> forever. I wonder if there's a way to have a single registry and scope the
> lifetime of the entries to the AcceleratorHandler pipe which must remain open
> for obvious reasons?

I have changed this so that (1) the lifetime of the namespace is tied with the
lifetime of the AcceleratorRegistrar, i.e. different clients still have
different namespace. I think we still need this to avoid accelerator id
conflicts from different clients. (2) the accelerators are removed when the
associated AcceleratorHandler is destroyed. So the client no longer needs to
keep the AcceleratorRegistrar alive, and can destroy it as soon as all the
accelerators are installed. The accelerators will work correctly as long as the
AcceleratorHandlers are kept alive.

I have updated the CL description accordingly.

[Ben Goodger (Google), 2015-12-01 16:01:46]

https://codereview.chromium.org/1488713002/diff/80001/mash/wm/accelerator_reg...
File mash/wm/accelerator_registrar_impl.cc (right):

https://codereview.chromium.org/1488713002/diff/80001/mash/wm/accelerator_reg...
mash/wm/accelerator_registrar_impl.cc:53: binding_.Unbind();
is this necessary?

https://codereview.chromium.org/1488713002/diff/80001/mash/wm/accelerator_reg...
mash/wm/accelerator_registrar_impl.cc:67: mus::mojom::AcceleratorHandlerPtr
handler,
just thinking about this... passing a new handler here almost makes the id
redundant.

given the (current) cost of message pipes it may be desirable to think about
reducing the number in use. in that sense perhaps it makes sense to have a
SetHandler() method on this interface that allows one handler to be set for all
accelerators for this connection.

[sadrul, 2015-12-01 19:18:05]

https://codereview.chromium.org/1488713002/diff/80001/mash/wm/accelerator_reg...
File mash/wm/accelerator_registrar_impl.cc (right):

https://codereview.chromium.org/1488713002/diff/80001/mash/wm/accelerator_reg...
mash/wm/accelerator_registrar_impl.cc:53: binding_.Unbind();
On 2015/12/01 16:01:45, Ben Goodger (Google) wrote:
> is this necessary?

My initial thought was this wouldn't be necessary. But looks like without this,
binding_.is_bound() on line 93 always returns true.

https://codereview.chromium.org/1488713002/diff/80001/mash/wm/accelerator_reg...
mash/wm/accelerator_registrar_impl.cc:67: mus::mojom::AcceleratorHandlerPtr
handler,
On 2015/12/01 16:01:45, Ben Goodger (Google) wrote:
> just thinking about this... passing a new handler here almost makes the id
> redundant.
> 
> given the (current) cost of message pipes it may be desirable to think about
> reducing the number in use. in that sense perhaps it makes sense to have a
> SetHandler() method on this interface that allows one handler to be set for
all
> accelerators for this connection. 

Nice. That makes sense, yeah. Done.

https://codereview.chromium.org/1488713002/diff/120001/mash/wm/accelerator_re...
File mash/wm/accelerator_registrar_impl.cc (right):

https://codereview.chromium.org/1488713002/diff/120001/mash/wm/accelerator_re...
mash/wm/accelerator_registrar_impl.cc:64: // handler and install new
accelerators).
Not entirely sure about this. It would be unusual for the client to keep the
Registrar connection open, install a Handler, destroy it, and install another
Handler. But nothing stops the client from doing it. So I am thinking might as
well make sure it works correctly.

[sadrul, 2015-12-01 19:23:43]

Description was changed from

==========
mus: Introduce AcceleratorRegistrar and AcceleratorHandler.

. An app can request an AcceleratorRegistrar from the window manager, and
  install some accelerators and AcceleratorHandlers.
. The AcceleratorRegistrar implementation in the window manager makes sure
  that the accelerator ids from different apps do not conflict with each
  other.
. When an AcceleratorHandler is disconnected, the accelerators its supposed
  to handle are automatically unregistered from mus.

BUG=548363
==========

to

==========
mus: Introduce AcceleratorRegistrar and AcceleratorHandler.

. An app can request an AcceleratorRegistrar from the window manager, and
  install some accelerators. The app must install an AcceleratorHandler to
  process these accelerators.
. The AcceleratorRegistrar implementation in the window manager makes sure
  that the accelerator ids from different apps do not conflict with each
  other.
. When an AcceleratorHandler is disconnected, the accelerators its supposed
  to handle are automatically unregistered from mus.

BUG=548363
==========

[Ben Goodger (Google), 2015-12-01 19:25:05]

lgtm

https://codereview.chromium.org/1488713002/diff/120001/mash/wm/accelerator_re...
File mash/wm/accelerator_registrar_impl.cc (right):

https://codereview.chromium.org/1488713002/diff/120001/mash/wm/accelerator_re...
mash/wm/accelerator_registrar_impl.cc:64: // handler and install new
accelerators).
On 2015/12/01 19:18:05, sadrul wrote:
> Not entirely sure about this. It would be unusual for the client to keep the
> Registrar connection open, install a Handler, destroy it, and install another
> Handler. But nothing stops the client from doing it. So I am thinking might as
> well make sure it works correctly.

SGTM

https://codereview.chromium.org/1488713002/diff/120001/mash/wm/window_manager...
File mash/wm/window_manager_application.cc (right):

https://codereview.chromium.org/1488713002/diff/120001/mash/wm/window_manager...
mash/wm/window_manager_application.cc:135: window_tree_host_.get(),
++accelerator_registrar_count, request.Pass(),
check on overflow of accelerator_registrar_count?

[Ben Goodger (Google), 2015-12-01 20:32:59]

On 2015/12/01 19:25:05, Ben Goodger (Google) wrote:
> lgtm
> 
>
https://codereview.chromium.org/1488713002/diff/120001/mash/wm/accelerator_re...
> File mash/wm/accelerator_registrar_impl.cc (right):
> 
>
https://codereview.chromium.org/1488713002/diff/120001/mash/wm/accelerator_re...
> mash/wm/accelerator_registrar_impl.cc:64: // handler and install new
> accelerators).
> On 2015/12/01 19:18:05, sadrul wrote:
> > Not entirely sure about this. It would be unusual for the client to keep the
> > Registrar connection open, install a Handler, destroy it, and install
another
> > Handler. But nothing stops the client from doing it. So I am thinking might
as
> > well make sure it works correctly.
> 
> SGTM
> 
>
https://codereview.chromium.org/1488713002/diff/120001/mash/wm/window_manager...
> File mash/wm/window_manager_application.cc (right):
> 
>
https://codereview.chromium.org/1488713002/diff/120001/mash/wm/window_manager...
> mash/wm/window_manager_application.cc:135: window_tree_host_.get(),
> ++accelerator_registrar_count, request.Pass(),
> check on overflow of accelerator_registrar_count?

btw an apptest for this would be nice (second cl fine)

[sadrul, 2015-12-02 00:57:42]

https://codereview.chromium.org/1488713002/diff/120001/mash/wm/window_manager...
File mash/wm/window_manager_application.cc (right):

https://codereview.chromium.org/1488713002/diff/120001/mash/wm/window_manager...
mash/wm/window_manager_application.cc:135: window_tree_host_.get(),
++accelerator_registrar_count, request.Pass(),
On 2015/12/01 19:25:04, Ben Goodger (Google) wrote:
> check on overflow of accelerator_registrar_count?

Done (silently restart from 0 instead of overflowing. Added a comment to
explain).

[Ben Goodger (Google), 2015-12-02 00:59:45]

https://codereview.chromium.org/1488713002/diff/140001/mash/wm/window_manager...
File mash/wm/window_manager_application.cc (right):

https://codereview.chromium.org/1488713002/diff/140001/mash/wm/window_manager...
mash/wm/window_manager_application.cc:139: // install accelerators.
how is this enforced? you don't seem to prevent registration.

[sadrul, 2015-12-02 19:14:26]

https://codereview.chromium.org/1488713002/diff/140001/mash/wm/window_manager...
File mash/wm/window_manager_application.cc (right):

https://codereview.chromium.org/1488713002/diff/140001/mash/wm/window_manager...
mash/wm/window_manager_application.cc:139: // install accelerators.
On 2015/12/02 00:59:45, Ben Goodger (Google) wrote:
> how is this enforced? you don't seem to prevent registration.

Ah, you are right. We do not actually do the right thing in
WindowTreeHostImpl/EventDispatcher when a dup accelerator is registered. I have
put up https://codereview.chromium.org/1487333004/, which we can use here to
notify the client when registering an accelerator fails.

However, I am not sure if this completely addresses this issue. For example,
some accelerators of the second client with the dup namespace can successfully
be registered, while attempting to register some others may fail (it depends on
what acceletators the first client had installed). I feel like that's a
reasonable expectation for the client, but I could be convinced otherwise. WDYT?

[Ben Goodger (Google), 2015-12-02 20:07:08]

On 2015/12/02 19:14:26, sadrul wrote:
>
https://codereview.chromium.org/1488713002/diff/140001/mash/wm/window_manager...
> File mash/wm/window_manager_application.cc (right):
> 
>
https://codereview.chromium.org/1488713002/diff/140001/mash/wm/window_manager...
> mash/wm/window_manager_application.cc:139: // install accelerators.
> On 2015/12/02 00:59:45, Ben Goodger (Google) wrote:
> > how is this enforced? you don't seem to prevent registration.
> 
> Ah, you are right. We do not actually do the right thing in
> WindowTreeHostImpl/EventDispatcher when a dup accelerator is registered. I
have
> put up https://codereview.chromium.org/1487333004/, which we can use here to
> notify the client when registering an accelerator fails.
> 
> However, I am not sure if this completely addresses this issue. For example,
> some accelerators of the second client with the dup namespace can successfully
> be registered, while attempting to register some others may fail (it depends
on
> what acceletators the first client had installed). I feel like that's a
> reasonable expectation for the client, but I could be convinced otherwise.
WDYT?

SGTM. lgtm

[sadrul, 2015-12-03 04:49:13]

The CQ bit was checked by sadrul@chromium.org

[sadrul, 2015-12-03 04:49:13]

The patchset sent to the CQ was uploaded after l-g-t-m from ben@chromium.org
Link to the patchset: https://codereview.chromium.org/1488713002/#ps160001
(title: "tot.merge")

[commit-bot: I haz the power, 2015-12-03 04:50:11]

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

[commit-bot: I haz the power, 2015-12-03 05:25:10]

Description was changed from

==========
mus: Introduce AcceleratorRegistrar and AcceleratorHandler.

. An app can request an AcceleratorRegistrar from the window manager, and
  install some accelerators. The app must install an AcceleratorHandler to
  process these accelerators.
. The AcceleratorRegistrar implementation in the window manager makes sure
  that the accelerator ids from different apps do not conflict with each
  other.
. When an AcceleratorHandler is disconnected, the accelerators its supposed
  to handle are automatically unregistered from mus.

BUG=548363
==========

to

==========
mus: Introduce AcceleratorRegistrar and AcceleratorHandler.

. An app can request an AcceleratorRegistrar from the window manager, and
  install some accelerators. The app must install an AcceleratorHandler to
  process these accelerators.
. The AcceleratorRegistrar implementation in the window manager makes sure
  that the accelerator ids from different apps do not conflict with each
  other.
. When an AcceleratorHandler is disconnected, the accelerators its supposed
  to handle are automatically unregistered from mus.

BUG=548363
==========

[commit-bot: I haz the power, 2015-12-03 05:25:11]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2015-12-03 05:25:49]

Description was changed from

==========
mus: Introduce AcceleratorRegistrar and AcceleratorHandler.

. An app can request an AcceleratorRegistrar from the window manager, and
  install some accelerators. The app must install an AcceleratorHandler to
  process these accelerators.
. The AcceleratorRegistrar implementation in the window manager makes sure
  that the accelerator ids from different apps do not conflict with each
  other.
. When an AcceleratorHandler is disconnected, the accelerators its supposed
  to handle are automatically unregistered from mus.

BUG=548363
==========

to

==========
mus: Introduce AcceleratorRegistrar and AcceleratorHandler.

. An app can request an AcceleratorRegistrar from the window manager, and
  install some accelerators. The app must install an AcceleratorHandler to
  process these accelerators.
. The AcceleratorRegistrar implementation in the window manager makes sure
  that the accelerator ids from different apps do not conflict with each
  other.
. When an AcceleratorHandler is disconnected, the accelerators its supposed
  to handle are automatically unregistered from mus.

BUG=548363

Committed: https://crrev.com/669040467ae91e1bc48cd24ae6afd64860e8d47a
Cr-Commit-Position: refs/heads/master@{#362915}
==========

[commit-bot: I haz the power, 2015-12-03 05:25:50]

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