ash: Start/stop observing KeyboardController via ShellObserver

ash: Start/stop observing KeyboardController via ShellObserver

- Add ScopedObserver to layout managers that observes KeyboardController
  so that they stop observing it when destructed;
- Layout managers start/stop observing KeyboardController via ShellObserver;

BUG=699765
Review-Url: https://codereview.chromium.org/2747543002
Cr-Commit-Position: refs/heads/master@{#456874}
Committed: https://chromium.googlesource.com/chromium/src/+/3816018a25a5da936828fa62a2eae92e14fe363e

[xiyuan, 2017-03-10 22:50:48]

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

[commit-bot: I haz the power, 2017-03-10 22:51:09]

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-10 22:54:24]

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

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

Dry run: Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[xiyuan, 2017-03-10 23:06:12]

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

[commit-bot: I haz the power, 2017-03-10 23:06:35]

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

[xiyuan, 2017-03-10 23:31:48]

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

[sky, 2017-03-10 23:40:47]

https://codereview.chromium.org/2747543002/diff/20001/ash/root_window_control...
File ash/root_window_controller.cc (right):

https://codereview.chromium.org/2747543002/diff/20001/ash/root_window_control...
ash/root_window_controller.cc:676:
wm_shelf_->shelf_layout_manager()->StartObservingKeyboard(
The sources an object needs to observe are generally an implementation detail of
the object. I would expect all these classes to handle adding/removing observers
as necessary rather than RootWindowController having to take care of it. WDYT of
adding ShellObserver functions for when the keyboard is activated/deactivated?
That way these classes can take care of registration themselves?

[commit-bot: I haz the power, 2017-03-10 23:41:11]

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

[commit-bot: I haz the power, 2017-03-10 23:41:12]

Dry run: This issue passed the CQ dry run.

[xiyuan, 2017-03-10 23:44:11]

https://codereview.chromium.org/2747543002/diff/20001/ash/root_window_control...
File ash/root_window_controller.cc (right):

https://codereview.chromium.org/2747543002/diff/20001/ash/root_window_control...
ash/root_window_controller.cc:676:
wm_shelf_->shelf_layout_manager()->StartObservingKeyboard(
On 2017/03/10 23:40:47, sky wrote:
> The sources an object needs to observe are generally an implementation detail
of
> the object. I would expect all these classes to handle adding/removing
observers
> as necessary rather than RootWindowController having to take care of it. WDYT
of
> adding ShellObserver functions for when the keyboard is activated/deactivated?
> That way these classes can take care of registration themselves?

Yep, think it makes more sense than exiting code. Let me give it a try and ping
back.

[xiyuan, 2017-03-11 05:45:07]

Description was changed from

==========
ash: Stop observing KeyboardController on destruction

Add ScopedObserver to layout managers that observes KeyboardController
so that they stop observing it when destructed.

BUG=699765
==========

to

==========
ash: Start/stop observing KeyboardController via ShellObserver

- Add ScopedObserver to layout managers that observes KeyboardController
  so that they stop observing it when destructed;
- Layout managers start/stop observing KeyboardController via ShellObserver;

BUG=699765
==========

[xiyuan, 2017-03-11 05:45:35]

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

[commit-bot: I haz the power, 2017-03-11 05:45: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-11 06:15:06]

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

[commit-bot: I haz the power, 2017-03-11 06:15:07]

Dry run: This issue passed the CQ dry run.

[xiyuan, 2017-03-13 16:31:05]

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

[commit-bot: I haz the power, 2017-03-13 16:31:35]

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

[xiyuan, 2017-03-13 16:36:31]

Revised to start/stop observing KeyboardController via ShellObserver. PTAL.

Thanks.

[commit-bot: I haz the power, 2017-03-13 17:15:14]

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

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

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

[xiyuan, 2017-03-13 17:23:43]

On 2017/03/13 16:36:31, xiyuan wrote:
> Revised to start/stop observing KeyboardController via ShellObserver. PTAL.
> 
> Thanks.

Please hold off reviewing PS #4. There are real test failures related to root
window filtering and I probably need to adjust some code order in
RootWindowController. Will ping back.

[xiyuan, 2017-03-13 18:00:25]

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

[commit-bot: I haz the power, 2017-03-13 18:01:11]

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-13 18:33:41]

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

[commit-bot: I haz the power, 2017-03-13 18:33:42]

Dry run: This issue passed the CQ dry run.

[xiyuan, 2017-03-13 18:34:15]

On 2017/03/13 17:23:43, xiyuan wrote:
> On 2017/03/13 16:36:31, xiyuan wrote:
> > Revised to start/stop observing KeyboardController via ShellObserver. PTAL.
> > 
> > Thanks.
> 
> Please hold off reviewing PS #4. There are real test failures related to root
> window filtering and I probably need to adjust some code order in
> RootWindowController. Will ping back.

Okay. Tests are passing now. :)

PTAL. Thanks.

[sky, 2017-03-13 19:20:48]

https://codereview.chromium.org/2747543002/diff/80001/ash/common/shelf/shelf_...
File ash/common/shelf/shelf_layout_manager.cc (right):

https://codereview.chromium.org/2747543002/diff/80001/ash/common/shelf/shelf_...
ash/common/shelf/shelf_layout_manager.cc:412: if (!keyboard_controller ||
Do you need the null check here? Also, given you seem to need the RootWindow in
all overrides of OnVirtualKeyboardStateChanged() wdyt of adding it as a
parameter to the function?

[xiyuan, 2017-03-13 21:41:15]

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

[commit-bot: I haz the power, 2017-03-13 21:41:41]

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

[xiyuan, 2017-03-13 22:09:46]

https://codereview.chromium.org/2747543002/diff/80001/ash/common/shelf/shelf_...
File ash/common/shelf/shelf_layout_manager.cc (right):

https://codereview.chromium.org/2747543002/diff/80001/ash/common/shelf/shelf_...
ash/common/shelf/shelf_layout_manager.cc:412: if (!keyboard_controller ||
On 2017/03/13 19:20:47, sky wrote:
> Do you need the null check here? Also, given you seem to need the RootWindow
in
> all overrides of OnVirtualKeyboardStateChanged() wdyt of adding it as a
> parameter to the function?

Done. Removed the null check and passing RootWindow as an arg to
OnVirtualKeyboardStateChanged().

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

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

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

Dry run: 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_...)

[sky, 2017-03-14 03:23:50]

Looking good! One minor suggestion in hopes of reducing boilerplate.

https://codereview.chromium.org/2747543002/diff/100001/ash/common/wm/workspac...
File ash/common/wm/workspace/workspace_layout_manager.cc (right):

https://codereview.chromium.org/2747543002/diff/100001/ash/common/wm/workspac...
ash/common/wm/workspace/workspace_layout_manager.cc:336: if (root_window !=
root_window_)
As this code exists in a handful of places, WDYT of a function like:

UpdateKeyboardObserverFromStateChanged(bool activated,
WmWindow* keyboard_root_window,
WmWindow* root_window,
keyboard_observer).

All the overrides would have the single call to
UpdateKeyboardObserverFromStateChanged, eg:
UpdateKeyboardObserverFromStated(activated, root_window, root_window_,
&keyboard_observer);

And UpdateKeyboardObserverFromStateChanged would go in the file
keyboard_observer_register?

[xiyuan, 2017-03-14 20:36:35]

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

[commit-bot: I haz the power, 2017-03-14 20:37:19]

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

[xiyuan, 2017-03-14 21:25:55]

Done and also migrated LockLayoutManager to use similar code as other layout
manager.

https://codereview.chromium.org/2747543002/diff/100001/ash/common/wm/workspac...
File ash/common/wm/workspace/workspace_layout_manager.cc (right):

https://codereview.chromium.org/2747543002/diff/100001/ash/common/wm/workspac...
ash/common/wm/workspace/workspace_layout_manager.cc:336: if (root_window !=
root_window_)
On 2017/03/14 03:23:50, sky wrote:
> As this code exists in a handful of places, WDYT of a function like:
> 
> UpdateKeyboardObserverFromStateChanged(bool activated,
> WmWindow* keyboard_root_window,
> WmWindow* root_window,
> keyboard_observer).
> 
> All the overrides would have the single call to
> UpdateKeyboardObserverFromStateChanged, eg:
> UpdateKeyboardObserverFromStated(activated, root_window, root_window_,
> &keyboard_observer);
> 
> And UpdateKeyboardObserverFromStateChanged would go in the file
> keyboard_observer_register?

Good idea. Done.

[sky, 2017-03-14 21:34:03]

Nice! LGTM

[commit-bot: I haz the power, 2017-03-14 22:18:15]

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

[commit-bot: I haz the power, 2017-03-14 22:18:16]

Dry run: This issue passed the CQ dry run.

[xiyuan, 2017-03-14 22:39:36]

The CQ bit was checked by xiyuan@chromium.org

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

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-14 22:50:17]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1489531176664310,
"parent_rev": "3d9e8afb33950f06b047aab296e143e45ab576ce", "commit_rev":
"3816018a25a5da936828fa62a2eae92e14fe363e"}

[commit-bot: I haz the power, 2017-03-14 22:51:01]

Description was changed from

==========
ash: Start/stop observing KeyboardController via ShellObserver

- Add ScopedObserver to layout managers that observes KeyboardController
  so that they stop observing it when destructed;
- Layout managers start/stop observing KeyboardController via ShellObserver;

BUG=699765
==========

to

==========
ash: Start/stop observing KeyboardController via ShellObserver

- Add ScopedObserver to layout managers that observes KeyboardController
  so that they stop observing it when destructed;
- Layout managers start/stop observing KeyboardController via ShellObserver;

BUG=699765

Review-Url: https://codereview.chromium.org/2747543002
Cr-Commit-Position: refs/heads/master@{#456874}
Committed:
https://chromium.googlesource.com/chromium/src/+/3816018a25a5da936828fa62a2ea...
==========

[commit-bot: I haz the power, 2017-03-14 22:51:02]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/3816018a25a5da936828fa62a2ea...