Enable accelerators on web activity window

Enable accelerators on web activity window

* AcceleratorManager::CreateForFocusManager allows windows to define their own accelerators.
* Added reload/forward/back accelerators.

BUG=380438
TBR=sadrul@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=276978

[Jun Mukai, 2014-06-11 22:07:51]

generally speaking, I still don't get the point why ui::AcceleratorManager (or
just FocusManager) isn't used directly.  What's essential in
athena::AcceleratorManagerImpl?  Can't it be ported to ui::AcceleratorManager?

https://codereview.chromium.org/332443005/diff/80001/athena/input/accelerator...
File athena/input/accelerator_manager_impl.cc (right):

https://codereview.chromium.org/332443005/diff/80001/athena/input/accelerator...
athena/input/accelerator_manager_impl.cc:166: explicit
FocusManagerWrapper(views::FocusManager* focus_manager)
Can we make FocusManager being a subclass of ui::AcceleratorManager instead?

https://codereview.chromium.org/332443005/diff/80001/athena/input/accelerator...
File athena/input/accelerator_manager_impl.h (right):

https://codereview.chromium.org/332443005/diff/80001/athena/input/accelerator...
athena/input/accelerator_manager_impl.h:40: explicit
AcceleratorManagerImpl(AcceleratorWrapper* wrapper);
I am not sure what this means.  Who can call this method?
Can it be a private constructor?

https://codereview.chromium.org/332443005/diff/80001/athena/input/accelerator...
athena/input/accelerator_manager_impl.h:68: class InternalData;
nit: internal class declaration usually appears right after 'private:' (same as
typedefs and enums, IIUC)

[oshima, 2014-06-11 22:46:43]

> generally speaking, I still don't get the point why ui::AcceleratorManager (or
> just FocusManager) isn't used directly.  What's essential in
athena::AcceleratorManagerImpl?

It hides details such as nested message loop, repeat handing, reserved
handlings, that you need to dup if you use ui::AcceleratorManager/FocusManager
directly.

>  Can't it be ported to ui::AcceleratorManager?

It can't depend on vies/aura/wm

https://codereview.chromium.org/332443005/diff/80001/athena/input/accelerator...
File athena/input/accelerator_manager_impl.cc (right):

https://codereview.chromium.org/332443005/diff/80001/athena/input/accelerator...
athena/input/accelerator_manager_impl.cc:166: explicit
FocusManagerWrapper(views::FocusManager* focus_manager)
On 2014/06/11 22:07:51, Jun Mukai wrote:
> Can we make FocusManager being a subclass of ui::AcceleratorManager instead?

what do you mean? Focus manager is defined in views (views/focus/focus_manager),
and a focus manager doesn't have is-a relation to AcceleratorManager.

https://codereview.chromium.org/332443005/diff/80001/athena/input/accelerator...
File athena/input/accelerator_manager_impl.h (right):

https://codereview.chromium.org/332443005/diff/80001/athena/input/accelerator...
athena/input/accelerator_manager_impl.h:40: explicit
AcceleratorManagerImpl(AcceleratorWrapper* wrapper);
On 2014/06/11 22:07:51, Jun Mukai wrote:
> I am not sure what this means.

Sorry I didn't understand your question.


>  Who can call this method?
> Can it be a private constructor?

This is called in AcceleratorManager::CreateForFocusmanager(). I did have a
static method to create CreateForFocusmanager with private ctor, but I thought
it's overkill. I can change it back if you want. let me know.

https://codereview.chromium.org/332443005/diff/80001/athena/input/accelerator...
athena/input/accelerator_manager_impl.h:68: class InternalData;
On 2014/06/11 22:07:51, Jun Mukai wrote:
> nit: internal class declaration usually appears right after 'private:' (same
as
> typedefs and enums, IIUC)

Done.

[Jun Mukai, 2014-06-11 23:13:22]

https://codereview.chromium.org/332443005/diff/80001/athena/input/accelerator...
File athena/input/accelerator_manager_impl.cc (right):

https://codereview.chromium.org/332443005/diff/80001/athena/input/accelerator...
athena/input/accelerator_manager_impl.cc:166: explicit
FocusManagerWrapper(views::FocusManager* focus_manager)
On 2014/06/11 22:46:43, oshima wrote:
> On 2014/06/11 22:07:51, Jun Mukai wrote:
> > Can we make FocusManager being a subclass of ui::AcceleratorManager instead?
> 
> what do you mean? Focus manager is defined in views
(views/focus/focus_manager),
> and a focus manager doesn't have is-a relation to AcceleratorManager.

Why not?  views::FocusManager creates its own AcceleratorManager in ctor and
provides really similar APIs of accelerator_manager. What is the reason
views::FocusManager must not inherit AcceleratorManager?  It surely manages
accelerators, right?


You created the wrapper class just because FocusManager mostly behaves like
ui::AcceleratorManager but you cannot use the same interface.  My suggestion is
to make the interface same. So you don't have to create the wrapper anymore,
AcceleratorManagerImpl just receives ui::AcceleratorManager* for ctor parameter.

https://codereview.chromium.org/332443005/diff/80001/athena/input/accelerator...
File athena/input/accelerator_manager_impl.h (right):

https://codereview.chromium.org/332443005/diff/80001/athena/input/accelerator...
athena/input/accelerator_manager_impl.h:40: explicit
AcceleratorManagerImpl(AcceleratorWrapper* wrapper);
On 2014/06/11 22:46:43, oshima wrote:
> On 2014/06/11 22:07:51, Jun Mukai wrote:
> > I am not sure what this means.
> 
> Sorry I didn't understand your question.
> 
> 
> >  Who can call this method?
> > Can it be a private constructor?
> 
> This is called in AcceleratorManager::CreateForFocusmanager(). I did have a
> static method to create CreateForFocusmanager with private ctor, but I thought
> it's overkill. I can change it back if you want. let me know.

Ah, it must be called from a static method of its superclass and that's why it's
public.

I don't think this is a good design, but can't find better ones.

[Jun Mukai, 2014-06-11 23:16:10]

https://codereview.chromium.org/332443005/diff/80001/athena/input/accelerator...
File athena/input/accelerator_manager_impl.cc (right):

https://codereview.chromium.org/332443005/diff/80001/athena/input/accelerator...
athena/input/accelerator_manager_impl.cc:166: explicit
FocusManagerWrapper(views::FocusManager* focus_manager)
On 2014/06/11 23:13:22, Jun Mukai wrote:
> On 2014/06/11 22:46:43, oshima wrote:
> > On 2014/06/11 22:07:51, Jun Mukai wrote:
> > > Can we make FocusManager being a subclass of ui::AcceleratorManager
instead?
> > 
> > what do you mean? Focus manager is defined in views
> (views/focus/focus_manager),
> > and a focus manager doesn't have is-a relation to AcceleratorManager.
> 
> Why not?  views::FocusManager creates its own AcceleratorManager in ctor and
> provides really similar APIs of accelerator_manager. What is the reason
> views::FocusManager must not inherit AcceleratorManager?  It surely manages
> accelerators, right?
> 
> 
> You created the wrapper class just because FocusManager mostly behaves like
> ui::AcceleratorManager but you cannot use the same interface.  My suggestion
is
> to make the interface same. So you don't have to create the wrapper anymore,
> AcceleratorManagerImpl just receives ui::AcceleratorManager* for ctor
parameter.

Or -- stop the AcceleratorWrapper class, but creates a FocusManagerWrapper class
which is like:
class FocusManagerWrapper : public ui::AcceleratorManager {
 public:
   ...
};

Or, add AcceleratorManager* accelerator_manager() { return
accelerator_manager_.get(); } to views::FocusManager and use it.

[oshima, 2014-06-12 17:06:38]

https://codereview.chromium.org/332443005/diff/80001/athena/input/accelerator...
File athena/input/accelerator_manager_impl.cc (right):

https://codereview.chromium.org/332443005/diff/80001/athena/input/accelerator...
athena/input/accelerator_manager_impl.cc:166: explicit
FocusManagerWrapper(views::FocusManager* focus_manager)
On 2014/06/11 23:13:22, Jun Mukai wrote:
> On 2014/06/11 22:46:43, oshima wrote:
> > On 2014/06/11 22:07:51, Jun Mukai wrote:
> > > Can we make FocusManager being a subclass of ui::AcceleratorManager
instead?
> > 
> > what do you mean? Focus manager is defined in views
> (views/focus/focus_manager),
> > and a focus manager doesn't have is-a relation to AcceleratorManager.
> 
> Why not?  views::FocusManager creates its own AcceleratorManager in ctor and
> provides really similar APIs of accelerator_manager. What is the reason
> views::FocusManager must not inherit AcceleratorManager?  It surely manages
> accelerators, right?

Using inheritance for has-a relation is considered bad design. I can discuss f2f
but you can also find a lot of article if you search "inheritance composition".

Also AcceleratorManager is not designed to be extended (No virtual dtor). 

> 
> 
> You created the wrapper class just because FocusManager mostly behaves like
> ui::AcceleratorManager but you cannot use the same interface.

It is good reason, at least better than alternating hierarchy just for this.

>  My suggestion is
> to make the interface same. So you don't have to create the wrapper anymore,
> AcceleratorManagerImpl just receives ui::AcceleratorManager* for ctor
parameter.

One possible approach is to separate ui::AcceleratorManager to be an interface,
and provide a separate implementation class, and use that interface in
FocusManager, and then delegate the call to AcceleratorManager impl. But I think
it's overkill.

Yet aonther issue is the object ownership. ui::AcceleratorManager for global
accelerator is owned here, while the focus manager's accelerator manager is
owned by the focus manager. You can have a flag to tell the ownership, but I
believe the current design is better than other options.

https://codereview.chromium.org/332443005/diff/80001/athena/input/accelerator...
athena/input/accelerator_manager_impl.cc:166: explicit
FocusManagerWrapper(views::FocusManager* focus_manager)
On 2014/06/11 23:16:10, Jun Mukai wrote:
> On 2014/06/11 23:13:22, Jun Mukai wrote:
> > On 2014/06/11 22:46:43, oshima wrote:
> > > On 2014/06/11 22:07:51, Jun Mukai wrote:
> > > > Can we make FocusManager being a subclass of ui::AcceleratorManager
> instead?
> > > 
> > > what do you mean? Focus manager is defined in views
> > (views/focus/focus_manager),
> > > and a focus manager doesn't have is-a relation to AcceleratorManager.
> > 
> > Why not?  views::FocusManager creates its own AcceleratorManager in ctor and
> > provides really similar APIs of accelerator_manager. What is the reason
> > views::FocusManager must not inherit AcceleratorManager?  It surely manages
> > accelerators, right?
> > 
> > 
> > You created the wrapper class just because FocusManager mostly behaves like
> > ui::AcceleratorManager but you cannot use the same interface.  My suggestion
> is
> > to make the interface same. So you don't have to create the wrapper anymore,
> > AcceleratorManagerImpl just receives ui::AcceleratorManager* for ctor
> parameter.
> 
> Or -- stop the AcceleratorWrapper class, but creates a FocusManagerWrapper
class
> which is like:
> class FocusManagerWrapper : public ui::AcceleratorManager {
>  public:
>    ...
> };
> 
> Or, add AcceleratorManager* accelerator_manager() { return
> accelerator_manager_.get(); } to views::FocusManager and use it.

AcceleratorManager is internals state because FocusManager::ProcessAccelerator
does different thing than ui::AcceleratorManager::Process. Your design exposes
both which is error prone.

[Jun Mukai, 2014-06-12 17:59:04]

https://codereview.chromium.org/332443005/diff/120001/athena/content/web_acti...
File athena/content/web_activity.cc (right):

https://codereview.chromium.org/332443005/diff/120001/athena/content/web_acti...
athena/content/web_activity.cc:34: web_view_->GetFocusManager()).Pass();
Right now CreateForFocusManager is only used here, and it is mostly for the
utility of IsRegistered().
Am I wrong?  Are you planning to expand accelerator_manager_ for other use?


The restructure of this CL's size just for using IsRegistered() sounds like an
overkill.  Why not:
- implement IsRegistered here (or create a static utility function)
- or, put it into views::FocusManager

[oshima, 2014-06-12 18:15:42]

On 2014/06/12 17:59:04, Jun Mukai wrote:
>
https://codereview.chromium.org/332443005/diff/120001/athena/content/web_acti...
> File athena/content/web_activity.cc (right):
> 
>
https://codereview.chromium.org/332443005/diff/120001/athena/content/web_acti...
> athena/content/web_activity.cc:34: web_view_->GetFocusManager()).Pass();
> Right now CreateForFocusManager is only used here, and it is mostly for the
> utility of IsRegistered().
> Am I wrong?  Are you planning to expand accelerator_manager_ for other use?
> 
> 
> The restructure of this CL's size just for using IsRegistered() sounds like an
> overkill.  Why not:
> - implement IsRegistered here (or create a static utility function)
> - or, put it into views::FocusManager

IsRegistered is also used in accelerator_manager_impl.cc. 

This will also be used for app activity window.

[Jun Mukai, 2014-06-12 20:21:22]

lgtm

Understood the design.

I still don't like the wrapper design (I want it private if possible), but
cannot think of anything better.

[oshima, 2014-06-12 21:11:18]

Ok I made the constructor private. This makes the wrapper class effectively
private because it's not possible to use this opaque class outside of this
class.

does this look ok to you?

[Jun Mukai, 2014-06-12 21:30:31]

lgtm++

https://codereview.chromium.org/332443005/diff/140001/athena/input/accelerato...
File athena/input/accelerator_manager_impl.h (right):

https://codereview.chromium.org/332443005/diff/140001/athena/input/accelerato...
athena/input/accelerator_manager_impl.h:34: class AcceleratorWrapper;
then I think you should make this class decl to private

[oshima, 2014-06-12 22:35:23]

https://codereview.chromium.org/332443005/diff/140001/athena/input/accelerato...
File athena/input/accelerator_manager_impl.h (right):

https://codereview.chromium.org/332443005/diff/140001/athena/input/accelerato...
athena/input/accelerator_manager_impl.h:34: class AcceleratorWrapper;
On 2014/06/12 21:30:31, Jun Mukai wrote:
> then I think you should make this class decl to private

I unfortunately can't without exposing internal class as a friend that are
currently defined in anonymous namespace.

[oshima, 2014-06-12 22:35:28]

The CQ bit was checked by oshima@chromium.org

[commit-bot: I haz the power, 2014-06-12 22:37:58]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/oshima@chromium.org/332443005/140001

[Jun Mukai, 2014-06-12 22:50:26]

https://codereview.chromium.org/332443005/diff/140001/athena/input/accelerato...
File athena/input/accelerator_manager_impl.h (right):

https://codereview.chromium.org/332443005/diff/140001/athena/input/accelerato...
athena/input/accelerator_manager_impl.h:34: class AcceleratorWrapper;
On 2014/06/12 22:35:23, oshima wrote:
> On 2014/06/12 21:30:31, Jun Mukai wrote:
> > then I think you should make this class decl to private
> 
> I unfortunately can't without exposing internal class as a friend that are
> currently defined in anonymous namespace. 

Confused. Which one exactly you need to expose, and which class has to be
friend??  As I understand the code, AcceleratorWrapper is referred only from
AcceleratorManagerImpl, then it can be private. Am I missing something?

[oshima, 2014-06-12 23:11:01]

https://codereview.chromium.org/332443005/diff/140001/athena/input/accelerato...
File athena/input/accelerator_manager_impl.h (right):

https://codereview.chromium.org/332443005/diff/140001/athena/input/accelerato...
athena/input/accelerator_manager_impl.h:34: class AcceleratorWrapper;
On 2014/06/12 22:50:26, Jun Mukai wrote:
> On 2014/06/12 22:35:23, oshima wrote:
> > On 2014/06/12 21:30:31, Jun Mukai wrote:
> > > then I think you should make this class decl to private
> > 
> > I unfortunately can't without exposing internal class as a friend that are
> > currently defined in anonymous namespace. 
> 
> Confused. Which one exactly you need to expose, and which class has to be
> friend??  As I understand the code, AcceleratorWrapper is referred only from
> AcceleratorManagerImpl, then it can be private. Am I missing something?

Subclasses (UIAcceleratorManagerWrapper, FocusManagerWrapper) need to acccess
it.

[Jun Mukai, 2014-06-12 23:14:25]

On 2014/06/12 23:11:01, oshima wrote:
>
https://codereview.chromium.org/332443005/diff/140001/athena/input/accelerato...
> File athena/input/accelerator_manager_impl.h (right):
> 
>
https://codereview.chromium.org/332443005/diff/140001/athena/input/accelerato...
> athena/input/accelerator_manager_impl.h:34: class AcceleratorWrapper;
> On 2014/06/12 22:50:26, Jun Mukai wrote:
> > On 2014/06/12 22:35:23, oshima wrote:
> > > On 2014/06/12 21:30:31, Jun Mukai wrote:
> > > > then I think you should make this class decl to private
> > > 
> > > I unfortunately can't without exposing internal class as a friend that are
> > > currently defined in anonymous namespace. 
> > 
> > Confused. Which one exactly you need to expose, and which class has to be
> > friend??  As I understand the code, AcceleratorWrapper is referred only from
> > AcceleratorManagerImpl, then it can be private. Am I missing something?
> 
> Subclasses (UIAcceleratorManagerWrapper, FocusManagerWrapper) need to acccess
> it.

Aaah. I see. What about declaring subclasses in header?  I don't like this idea,
just saying...

still lgtm (just in case)

[oshima, 2014-06-13 00:03:11]

The CQ bit was unchecked by oshima@chromium.org

[oshima, 2014-06-13 00:09:09]

tbr'ing sadul for the following deps change

ui/views/controls/webview to ui/views in athena/content/

[oshima, 2014-06-13 00:09:13]

The CQ bit was checked by oshima@chromium.org

[commit-bot: I haz the power, 2014-06-13 00:10:47]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/oshima@chromium.org/332443005/160001

[oshima, 2014-06-13 00:12:40]

On 2014/06/12 23:14:25, Jun Mukai wrote:
> On 2014/06/12 23:11:01, oshima wrote:
> >
>
https://codereview.chromium.org/332443005/diff/140001/athena/input/accelerato...
> > File athena/input/accelerator_manager_impl.h (right):
> > 
> >
>
https://codereview.chromium.org/332443005/diff/140001/athena/input/accelerato...
> > athena/input/accelerator_manager_impl.h:34: class AcceleratorWrapper;
> > On 2014/06/12 22:50:26, Jun Mukai wrote:
> > > On 2014/06/12 22:35:23, oshima wrote:
> > > > On 2014/06/12 21:30:31, Jun Mukai wrote:
> > > > > then I think you should make this class decl to private
> > > > 
> > > > I unfortunately can't without exposing internal class as a friend that
are
> > > > currently defined in anonymous namespace. 
> > > 
> > > Confused. Which one exactly you need to expose, and which class has to be
> > > friend??  As I understand the code, AcceleratorWrapper is referred only
from
> > > AcceleratorManagerImpl, then it can be private. Am I missing something?
> > 
> > Subclasses (UIAcceleratorManagerWrapper, FocusManagerWrapper) need to
acccess
> > it.
> 
> Aaah. I see. What about declaring subclasses in header?  I don't like this
idea,
> just saying...
> 
> still lgtm (just in case)

That's probably worse because renaming internal class requires changing header,
for example.
Another option is to make it protected and define subclasses in the subclass of
AcceleratorManagerImpl (w/o instance)
but that'd be overkill.

[oshima, 2014-06-13 00:20:20]

The CQ bit was unchecked by oshima@chromium.org

[oshima, 2014-06-13 00:20:21]

The CQ bit was checked by oshima@chromium.org

[commit-bot: I haz the power, 2014-06-13 00:24:07]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/oshima@chromium.org/332443005/160001

[commit-bot: I haz the power, 2014-06-13 08:48:43]

Change committed as 276978

[sadrul, 2014-06-13 09:37:20]

lgtm