Athena's FocusManager

Athena's FocusManager
 This is necessary to handle non reserved accelerator.

Plus a couple of improvements
* move deps for tests to athena/DEPS
* add capture/activation controllers.
* don't set override redirect for athena_shell.

BUG=375534
TBR=ben@chromium.org

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

[Jun Mukai, 2014-06-10 01:43:05]

https://codereview.chromium.org/322503005/diff/100001/athena/input/accelerato...
File athena/input/accelerator_manager_impl.cc (right):

https://codereview.chromium.org/322503005/diff/100001/athena/input/accelerato...
athena/input/accelerator_manager_impl.cc:77: class FocusManagerDelegate : public
views::FocusManagerDelegate {
It seems like accelerator_manager_ in views::FocusManager has higher priority
than delegate_. Is it expected?

https://codereview.chromium.org/322503005/diff/100001/athena/input/accelerato...
athena/input/accelerator_manager_impl.cc:160: // FocusManager has the referene
to this object.
nit: reference

https://codereview.chromium.org/322503005/diff/100001/athena/input/accelerato...
athena/input/accelerator_manager_impl.cc:247:
accelerator_data.accelerator_flags)));
Not related to your CL, just an idea -- isn't it helpful if it prints a
LOG(ERROR) if the exactly same key combination is already in |accelerators_|?

https://codereview.chromium.org/322503005/diff/100001/athena/screen/screen_ma...
File athena/screen/screen_manager_impl.cc (right):

https://codereview.chromium.org/322503005/diff/100001/athena/screen/screen_ma...
athena/screen/screen_manager_impl.cc:110: scoped_ptr< ::wm::ScopedCaptureClient>
capture_client_;
nit: Is there a rule to put space between < and :?  I thought we just usually
write as scoped_ptr<::wm::...>

[oshima, 2014-06-10 05:18:11]

https://codereview.chromium.org/322503005/diff/100001/athena/input/accelerato...
File athena/input/accelerator_manager_impl.cc (right):

https://codereview.chromium.org/322503005/diff/100001/athena/input/accelerato...
athena/input/accelerator_manager_impl.cc:77: class FocusManagerDelegate : public
views::FocusManagerDelegate {
On 2014/06/10 01:43:05, Jun Mukai wrote:
> It seems like accelerator_manager_ in views::FocusManager has higher priority
> than delegate_. Is it expected?

Yes. If an browser window defines an accelerator, it should be executed instead.
We do, however, avoid defining the same accelerator on both side (with a few
exceptions), and cros has an unit test to catch that.
(AcceleratorTableTest.CheckDuplicatedAcceleratorsAsh)

https://codereview.chromium.org/322503005/diff/100001/athena/input/accelerato...
athena/input/accelerator_manager_impl.cc:160: // FocusManager has the referene
to this object.
On 2014/06/10 01:43:05, Jun Mukai wrote:
> nit: reference

Done.

https://codereview.chromium.org/322503005/diff/100001/athena/input/accelerato...
athena/input/accelerator_manager_impl.cc:247:
accelerator_data.accelerator_flags)));
On 2014/06/10 01:43:05, Jun Mukai wrote:
> Not related to your CL, just an idea -- isn't it helpful if it prints a
> LOG(ERROR) if the exactly same key combination is already in |accelerators_|?

ui::Accelerator has DCHECK to prevent it.
(https://code.google.com/p/chromium/codesearch#chromium/src/ui/base/accelerato...)

https://codereview.chromium.org/322503005/diff/100001/athena/screen/screen_ma...
File athena/screen/screen_manager_impl.cc (right):

https://codereview.chromium.org/322503005/diff/100001/athena/screen/screen_ma...
athena/screen/screen_manager_impl.cc:110: scoped_ptr< ::wm::ScopedCaptureClient>
capture_client_;
On 2014/06/10 01:43:05, Jun Mukai wrote:
> nit: Is there a rule to put space between < and :?  I thought we just usually
> write as scoped_ptr<::wm::...>

Believe or not, <: is reserved (replacement for "[") so you need a space there.
(like <<)

[Jun Mukai, 2014-06-10 19:32:50]

lgtm

https://codereview.chromium.org/322503005/diff/100001/athena/screen/screen_ma...
File athena/screen/screen_manager_impl.cc (right):

https://codereview.chromium.org/322503005/diff/100001/athena/screen/screen_ma...
athena/screen/screen_manager_impl.cc:110: scoped_ptr< ::wm::ScopedCaptureClient>
capture_client_;
On 2014/06/10 05:18:11, oshima wrote:
> On 2014/06/10 01:43:05, Jun Mukai wrote:
> > nit: Is there a rule to put space between < and :?  I thought we just
usually
> > write as scoped_ptr<::wm::...>
> 
> Believe or not, <: is reserved (replacement for "[") so you need a space
there.
> (like <<)

Whoa, you're right!  I've forgotten I hate that thing...

[oshima, 2014-06-10 19:38:00]

tbr'ing ben for following DEPS change that we already have in athena

ui/views (athena/input/DEPS)
ui/wm    (athena/screen/DEPS)

[oshima, 2014-06-10 19:38:03]

The CQ bit was checked by oshima@chromium.org

[commit-bot: I haz the power, 2014-06-10 19:39:11]

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

[commit-bot: I haz the power, 2014-06-11 05:04:25]

Change committed as 276267