ARC IME: Fix crash on shutdown due to wrong focus management.

ARC IME: Fix crash on shutdown due to wrong focus management.

It didn't properly detach from the input method when ARC lost focus
and no other text input client took focus until shutdown.

BUG=673809
TEST=ArcImeServiceTest.WindowFocusTracking

Committed: https://crrev.com/10bd93d340ced0160d8faf676812ee5c9a89f0fd
Cr-Commit-Position: refs/heads/master@{#438720}

[kinaba, 2016-12-14 08:39:34]

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

[commit-bot: I haz the power, 2016-12-14 08:39:44]

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

[kinaba, 2016-12-14 08:57:34]

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

[commit-bot: I haz the power, 2016-12-14 08:57:46]

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

[kinaba, 2016-12-14 09:02:40]

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

[kinaba, 2016-12-14 09:02:40]

PTAL: hidehiko@

https://codereview.chromium.org/2570623005/diff/20001/components/arc/ime/arc_...
File components/arc/ime/arc_ime_service_unittest.cc (right):

https://codereview.chromium.org/2570623005/diff/20001/components/arc/ime/arc_...
components/arc/ime/arc_ime_service_unittest.cc:252: EXPECT_EQ(nullptr,
fake_input_method_->GetTextInputClient());
note: this assert will fail without the fix.

https://codereview.chromium.org/2570623005/diff/20001/components/arc/ime/arc_...
components/arc/ime/arc_ime_service_unittest.cc:262: EXPECT_EQ(nullptr,
fake_input_method_->GetTextInputClient());
ditto

[commit-bot: I haz the power, 2016-12-14 09:23:59]

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

[commit-bot: I haz the power, 2016-12-14 09:23:59]

Dry run: This issue passed the CQ dry run.

[hidehiko, 2016-12-14 09:25:27]

https://codereview.chromium.org/2570623005/diff/20001/components/arc/ime/arc_...
File components/arc/ime/arc_ime_service.cc (right):

https://codereview.chromium.org/2570623005/diff/20001/components/arc/ime/arc_...
components/arc/ime/arc_ime_service.cc:145: if (detaching_ime != attaching_ime) {
I'm now slightly confused. If IME is attached to the window,

if (detach != attach) {
  if (detach)
    lost_focus->GetHost()->GetInputMethod()->DetachTextInputClient(...);
  if (attach)
    gained_focus->GetHost()->GetInputMethod()->SetFocusedTextInputClient(...);
}

seems what we need here?
Is there a case focused_arc_window_ contains two or more windows?

https://codereview.chromium.org/2570623005/diff/20001/components/arc/ime/arc_...
File components/arc/ime/arc_ime_service_unittest.cc (right):

https://codereview.chromium.org/2570623005/diff/20001/components/arc/ime/arc_...
components/arc/ime/arc_ime_service_unittest.cc:154: fake_window_detector_ = new
FakeArcWindowDetector;
nit: () is missing.

[kinaba, 2016-12-14 10:33:13]

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

[commit-bot: I haz the power, 2016-12-14 10:33:22]

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, 2016-12-14 10:35:00]

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

[commit-bot: I haz the power, 2016-12-14 10:35:00]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  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...)

[kinaba, 2016-12-14 12:12:09]

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

[commit-bot: I haz the power, 2016-12-14 12:12:17]

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

[kinaba, 2016-12-14 12:12:56]

https://codereview.chromium.org/2570623005/diff/20001/components/arc/ime/arc_...
File components/arc/ime/arc_ime_service.cc (right):

https://codereview.chromium.org/2570623005/diff/20001/components/arc/ime/arc_...
components/arc/ime/arc_ime_service.cc:145: if (detaching_ime != attaching_ime) {
On ChromeOS as of now, all windows share the same ui::InputMethod instance,
so (detach != attach) and (detaching_ime != attaching_ime) should do the same
thing.

But that's not guaranteed in Aura in general, so I think we should not rely on
that.
For instance Mustash+MultiDisplay may (or may not; I don't know really) utilize
multiple InputMethod instances.
In such a case, we do need to do both Detach and SetFocus. My old code was wrong
in this point too.

> Is there a case focused_arc_window_ contains two or more windows?
No, its size is either 0 or 1.
It could have been a raw pointer, but I just wanted to avoid the danger of
dangling pointer (whichs is what aura::WindowTracker does.)

https://codereview.chromium.org/2570623005/diff/20001/components/arc/ime/arc_...
File components/arc/ime/arc_ime_service_unittest.cc (right):

https://codereview.chromium.org/2570623005/diff/20001/components/arc/ime/arc_...
components/arc/ime/arc_ime_service_unittest.cc:154: fake_window_detector_ = new
FakeArcWindowDetector;
On 2016/12/14 09:25:27, hidehiko wrote:
> nit: () is missing.

Done.

[commit-bot: I haz the power, 2016-12-14 13:07:07]

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

[commit-bot: I haz the power, 2016-12-14 13:07:07]

Dry run: This issue passed the CQ dry run.

[hidehiko, 2016-12-14 15:27:22]

LGTM.

https://codereview.chromium.org/2570623005/diff/20001/components/arc/ime/arc_...
File components/arc/ime/arc_ime_service.cc (right):

https://codereview.chromium.org/2570623005/diff/20001/components/arc/ime/arc_...
components/arc/ime/arc_ime_service.cc:145: if (detaching_ime != attaching_ime) {
On 2016/12/14 12:12:56, kinaba wrote:
> On ChromeOS as of now, all windows share the same ui::InputMethod instance,
> so (detach != attach) and (detaching_ime != attaching_ime) should do the same
> thing.
> 
> But that's not guaranteed in Aura in general, so I think we should not rely on
> that.
> For instance Mustash+MultiDisplay may (or may not; I don't know really)
utilize
> multiple InputMethod instances.
> In such a case, we do need to do both Detach and SetFocus. My old code was
wrong
> in this point too.
> 
> > Is there a case focused_arc_window_ contains two or more windows?
> No, its size is either 0 or 1.
> It could have been a raw pointer, but I just wanted to avoid the danger of
> dangling pointer (whichs is what aura::WindowTracker does.)

I understood. Thank you for detailed explanation.

Optional: How about;

ui::InputMethod* detaching_ime = detach ?
lost_focus->GetHost()->GetInputMethod();
ui::InputMethod* attaching_ime = ...;

if (detaching_ime != attaching_ime) {
  ...
}

GetInputMethod() implicitly depends on focused_arc_window_, which is edited in
this function. I know it's commented, I personally prefer the explicit
dependency to different calling timing. It's minor thing, I think. Up to you.

[kinaba, 2016-12-15 00:58:35]

Patchset #5 (id:80001) has been deleted

[kinaba, 2016-12-15 00:58:41]

https://codereview.chromium.org/2570623005/diff/20001/components/arc/ime/arc_...
File components/arc/ime/arc_ime_service.cc (right):

https://codereview.chromium.org/2570623005/diff/20001/components/arc/ime/arc_...
components/arc/ime/arc_ime_service.cc:145: if (detaching_ime != attaching_ime) {
On 2016/12/14 15:27:22, hidehiko wrote:
> On 2016/12/14 12:12:56, kinaba wrote:
> > On ChromeOS as of now, all windows share the same ui::InputMethod instance,
> > so (detach != attach) and (detaching_ime != attaching_ime) should do the
same
> > thing.
> > 
> > But that's not guaranteed in Aura in general, so I think we should not rely
on
> > that.
> > For instance Mustash+MultiDisplay may (or may not; I don't know really)
> utilize
> > multiple InputMethod instances.
> > In such a case, we do need to do both Detach and SetFocus. My old code was
> wrong
> > in this point too.
> > 
> > > Is there a case focused_arc_window_ contains two or more windows?
> > No, its size is either 0 or 1.
> > It could have been a raw pointer, but I just wanted to avoid the danger of
> > dangling pointer (whichs is what aura::WindowTracker does.)
> 
> I understood. Thank you for detailed explanation.
> 
> Optional: How about;
> 
> ui::InputMethod* detaching_ime = detach ?
> lost_focus->GetHost()->GetInputMethod();
> ui::InputMethod* attaching_ime = ...;
> 
> if (detaching_ime != attaching_ime) {
>   ...
> }
> 
> GetInputMethod() implicitly depends on focused_arc_window_, which is edited in
> this function. I know it's commented, I personally prefer the explicit
> dependency to different calling timing. It's minor thing, I think. Up to you.

I totally agree with your concern. Added a TODO comment on that.
That said, for this time, let me go as-is.

GetInputMethod() method is working also as a point to inject the fake IME for
testing,
so we cannot just rewrite it to the direct
lost_focus->GetHost()->GetInputMethod() call.

I probably can inject the fake IME into more deeper layer instead of our custom
way, using
https://cs.chromium.org/chromium/src/ui/base/ime/input_method_factory.h?q=Cre...
but for this, we need to add more set up code related to ash/aura/views in the
test.

Since this CL is meant to be merged to M56, I want to keep the change minimal as
possible.

[kinaba, 2016-12-15 00:59:02]

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

[commit-bot: I haz the power, 2016-12-15 00:59:35]

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, 2016-12-15 01:05:34]

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

[commit-bot: I haz the power, 2016-12-15 01:05:35]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[kinaba, 2016-12-15 02:21:16]

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

[commit-bot: I haz the power, 2016-12-15 02:21:22]

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, 2016-12-15 02:27:00]

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

[commit-bot: I haz the power, 2016-12-15 02:27:00]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[kinaba, 2016-12-15 02:53:51]

The CQ bit was checked by kinaba@chromium.org

[kinaba, 2016-12-15 02:53:52]

The patchset sent to the CQ was uploaded after l-g-t-m from
hidehiko@chromium.org
Link to the patchset: https://codereview.chromium.org/2570623005/#ps100001
(title: "Add a TODO comment on confusing usage of GetInputMethod")

[commit-bot: I haz the power, 2016-12-15 02:54:14]

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-12-15 03:03:31]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1481770431712450,
"parent_rev": "11b3a54d7e72fcc8405a114e2b3b3c6aa4a218ca", "commit_rev":
"625b90d8c042816055c49982417a00ce98120ea3"}

[commit-bot: I haz the power, 2016-12-15 03:03:55]

Description was changed from

==========
ARC IME: Fix crash on shutdown due to wrong focus management.

It didn't properly detach from the input method when ARC lost focus
and no other text input client took focus until shutdown.

BUG=673809
TEST=ArcImeServiceTest.WindowFocusTracking
==========

to

==========
ARC IME: Fix crash on shutdown due to wrong focus management.

It didn't properly detach from the input method when ARC lost focus
and no other text input client took focus until shutdown.

BUG=673809
TEST=ArcImeServiceTest.WindowFocusTracking

Review-Url: https://codereview.chromium.org/2570623005
==========

[commit-bot: I haz the power, 2016-12-15 03:03:56]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2016-12-15 03:05:55]

Description was changed from

==========
ARC IME: Fix crash on shutdown due to wrong focus management.

It didn't properly detach from the input method when ARC lost focus
and no other text input client took focus until shutdown.

BUG=673809
TEST=ArcImeServiceTest.WindowFocusTracking

Review-Url: https://codereview.chromium.org/2570623005
==========

to

==========
ARC IME: Fix crash on shutdown due to wrong focus management.

It didn't properly detach from the input method when ARC lost focus
and no other text input client took focus until shutdown.

BUG=673809
TEST=ArcImeServiceTest.WindowFocusTracking

Committed: https://crrev.com/10bd93d340ced0160d8faf676812ee5c9a89f0fd
Cr-Commit-Position: refs/heads/master@{#438720}
==========

[commit-bot: I haz the power, 2016-12-15 03:05:56]

Patchset 5 (id:??) landed as
https://crrev.com/10bd93d340ced0160d8faf676812ee5c9a89f0fd
Cr-Commit-Position: refs/heads/master@{#438720}