arc: Use exo::WMHelper to abserve window focus.

arc: Use exo::WMHelper to abserve window focus.

In mus+ash, toplevel windows don't share the same focus_client,
so we have to use exo::WMHelper for handling window focus.

BUG=637914
Committed: https://crrev.com/aa981039ea66bac847c08c6d68e5cd011f3a1c2f
Cr-Commit-Position: refs/heads/master@{#413433}

[Peng, 2016-08-19 14:34:30]

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

[commit-bot: I haz the power, 2016-08-19 14:34:47]

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

[Peng, 2016-08-19 14:37:08]

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

[Peng, 2016-08-19 14:37:09]

Hi Yusuke, PTAL. Thanks.

[commit-bot: I haz the power, 2016-08-19 15:27:19]

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

[commit-bot: I haz the power, 2016-08-19 15:27:20]

Dry run: This issue passed the CQ dry run.

[Yusuke Sato, 2016-08-20 01:25:37]

yusukes@chromium.org changed reviewers:
+ kinaba@chromium.org

[Yusuke Sato, 2016-08-20 01:25:38]

Sorry for the delay.

cc: kinaba FYI.

https://codereview.chromium.org/2259243002/diff/1/components/arc/ime/arc_ime_...
File components/arc/ime/arc_ime_service.cc (right):

https://codereview.chromium.org/2259243002/diff/1/components/arc/ime/arc_ime_...
components/arc/ime/arc_ime_service.cc:57: if (is_focus_observer_installed_ &&
exo::WMHelper::GetInstance())
Is the is_focus_observer_installed_ check really necessary? base::ObserverList
just ignores unknown pointers.

https://codereview.chromium.org/2259243002/diff/1/components/arc/ime/arc_ime_...
components/arc/ime/arc_ime_service.cc:94:
exo::WMHelper::GetInstance()->AddFocusObserver(this);
Can't we move this to somewhere else (e.g. the constructor) which is executed
only once so we can remove is_focus_observer_installed_ member variable?

https://codereview.chromium.org/2259243002/diff/1/components/arc/ime/arc_ime_...
File components/arc/ime/arc_ime_service.h (right):

https://codereview.chromium.org/2259243002/diff/1/components/arc/ime/arc_ime_...
components/arc/ime/arc_ime_service.h:1: // Copyright 2016 The Chromium Authors.
All rights reserved.
Please test the CL on non mus+ash environment too. ARC++ is already in
production.

[Peng, 2016-08-20 13:09:23]

https://codereview.chromium.org/2259243002/diff/1/components/arc/ime/arc_ime_...
File components/arc/ime/arc_ime_service.cc (right):

https://codereview.chromium.org/2259243002/diff/1/components/arc/ime/arc_ime_...
components/arc/ime/arc_ime_service.cc:57: if (is_focus_observer_installed_ &&
exo::WMHelper::GetInstance())
On 2016/08/20 01:25:38, Yusuke Sato (ooo Aug 8) wrote:
> Is the is_focus_observer_installed_ check really necessary? base::ObserverList
> just ignores unknown pointers.

As I explained at line 94. It is difficult to remove
is_focus_observer_installed_, so checking it here shouldn't hurt anything.

https://codereview.chromium.org/2259243002/diff/1/components/arc/ime/arc_ime_...
components/arc/ime/arc_ime_service.cc:94:
exo::WMHelper::GetInstance()->AddFocusObserver(this);
On 2016/08/20 01:25:38, Yusuke Sato (ooo Aug 8) wrote:
> Can't we move this to somewhere else (e.g. the constructor) which is executed
> only once so we can remove is_focus_observer_installed_ member variable?

The constructor doesn't work, because this class is created before exo
initialization. That why we have to delay the observer installation to here.

https://codereview.chromium.org/2259243002/diff/1/components/arc/ime/arc_ime_...
File components/arc/ime/arc_ime_service.h (right):

https://codereview.chromium.org/2259243002/diff/1/components/arc/ime/arc_ime_...
components/arc/ime/arc_ime_service.h:1: // Copyright 2016 The Chromium Authors.
All rights reserved.
On 2016/08/20 01:25:38, Yusuke Sato (ooo Aug 8) wrote:
> Please test the CL on non mus+ash environment too. ARC++ is already in
> production.

I tested exo with weston-terminal, and other wayland clients with both non
mus+ash and mus+ash environments. they work fine. But without this CL, exo
crashes because ArcImeService assumes all
arc windows share the same focus client, but it is not not true for mus+ash.

[kinaba, 2016-08-22 01:59:09]

> cc: kinaba FYI.

Verified that it works well with ARC++ IME.

[Yusuke Sato, 2016-08-22 05:53:53]

lgtm

[Peng, 2016-08-22 11:13:43]

On 2016/08/22 01:59:09, kinaba wrote:
> > cc: kinaba FYI.
> 
> Verified that it works well with ARC++ IME.

thanks for helping me test it in arc++

[Peng, 2016-08-22 11:13:54]

The CQ bit was checked by penghuang@chromium.org

[commit-bot: I haz the power, 2016-08-22 11:14:04]

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, 2016-08-22 12:43:44]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-08-22 12:45:30]

Description was changed from

==========
arc: Use exo::WMHelper to abserve window focus.

In mus+ash, toplevel windows don't share the same focus_client,
so we have to use exo::WMHelper for handling window focus.

BUG=637914
==========

to

==========
arc: Use exo::WMHelper to abserve window focus.

In mus+ash, toplevel windows don't share the same focus_client,
so we have to use exo::WMHelper for handling window focus.

BUG=637914

Committed: https://crrev.com/aa981039ea66bac847c08c6d68e5cd011f3a1c2f
Cr-Commit-Position: refs/heads/master@{#413433}
==========

[commit-bot: I haz the power, 2016-08-22 12:45:31]

Patchset 1 (id:??) landed as
https://crrev.com/aa981039ea66bac847c08c6d68e5cd011f3a1c2f
Cr-Commit-Position: refs/heads/master@{#413433}