IME for Mus: Avoid crash in InitWindowCandidateView().

IME for Mus: Avoid crash in InitWindowCandidateView(). 

This CL fixes the crash when the candidate list is created. crbug.com/684658
tracks fixing the positioning of the candidate window when the window moves.

BUG=637416
Review-Url: https://codereview.chromium.org/2651733002
Cr-Commit-Position: refs/heads/master@{#447506}
Committed: https://chromium.googlesource.com/chromium/src/+/dfb10e743132bac81702c2a3685f4c7c782d368f

[Hadi, 2017-01-23 16:43:57]

moshayedi@chromium.org changed reviewers:
+ sadrul@chromium.org, shuchen@chromium.org

[Hadi, 2017-01-23 16:43:58]

PTAL.

[Hadi, 2017-01-23 16:44:12]

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

[commit-bot: I haz the power, 2017-01-23 16:44:25]

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

[Hadi, 2017-01-23 16:45:34]

Description was changed from

==========
IME for Mus: CandidateWindowView gets parent=nullptr in mus+ash.

Currently candidate list popup is created by the IME Driver which is implemented
by content_browser process which doesn't have access to windows of other
processes. So ash::wm::GetActiveWindow() will fail in mus+ash.

This change sets the parent for ui::ime::CandidateWindowView to nullptr when
running in mus+ash.

BUG=637416
==========

to

==========
IME for Mus: CandidateWindowView gets parent=nullptr in mus+ash.

Currently candidate list popup is created by the IME Driver which is implemented
by content_browser process which doesn't have access to windows of other
processes. So ash::wm::GetActiveWindow() will fail in mus+ash.

This change sets the parent for ui::ime::CandidateWindowView to nullptr when
running in mus+ash.

After this change, candidate list popups work in mus+ash.

BUG=637416
==========

[commit-bot: I haz the power, 2017-01-23 17:13:18]

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

[commit-bot: I haz the power, 2017-01-23 17:13:19]

Dry run: This issue passed the CQ dry run.

[sadrul, 2017-01-23 18:20:11]

https://codereview.chromium.org/2651733002/diff/1/chrome/browser/chromeos/inp...
File chrome/browser/chromeos/input_method/candidate_window_controller_impl.cc
(right):

https://codereview.chromium.org/2651733002/diff/1/chrome/browser/chromeos/inp...
chrome/browser/chromeos/input_method/candidate_window_controller_impl.cc:56:
candidate_window_view_ = new ui::ime::CandidateWindowView(parent);
Can you comment on how this candidate window will be parented in mash?

[Hadi, 2017-01-23 21:00:51]

Description was changed from

==========
IME for Mus: CandidateWindowView gets parent=nullptr in mus+ash.

Currently candidate list popup is created by the IME Driver which is implemented
by content_browser process which doesn't have access to windows of other
processes. So ash::wm::GetActiveWindow() will fail in mus+ash.

This change sets the parent for ui::ime::CandidateWindowView to nullptr when
running in mus+ash.

After this change, candidate list popups work in mus+ash.

BUG=637416
==========

to

==========
IME for Mus: Avoid crash in InitWindowCandidateView(). 

Currently ash::wm::GetActiveWindow() crashes in mus+ash, so this change simply
avoids calling it and sets the parent for CandidateWindowView as nullptr.

This won't have the proper behavior in some situations like dragging the window,

etc. But since non-latin text input is not high-priority right now, this is just
a simple change just to avoid crashing.

BUG=637416
==========

[Hadi, 2017-01-23 21:11:35]

https://codereview.chromium.org/2651733002/diff/1/chrome/browser/chromeos/inp...
File chrome/browser/chromeos/input_method/candidate_window_controller_impl.cc
(right):

https://codereview.chromium.org/2651733002/diff/1/chrome/browser/chromeos/inp...
chrome/browser/chromeos/input_method/candidate_window_controller_impl.cc:56:
candidate_window_view_ = new ui::ime::CandidateWindowView(parent);
On 2017/01/23 18:20:11, sadrul wrote:
> Can you comment on how this candidate window will be parented in mash?

Setting parent as null doesn't provide the same behavior as in ChromeOS. For
example, when dragging the window in ChromeOS the popup is also moved, but in
mus+ash the popup disappears.

This change avoids crashing and works for most cases until we find a better
solution. Non-latin text input is not high priority for near future, so I think
we can postpone for later.

I changed the title and description of this CL and added a TODO comment.

[sadrul, 2017-01-24 15:59:11]

From the CL description:

> But since non-latin text input is not high-priority right
> now, this is just a simple change just to avoid crashing.

Change this to something like:

"This CL fixes the crash when the candidate list is created. crbug.com/<new bug>
tracks fixing the positioning of the candidate window when the window moves."

With that, lgtm

https://codereview.chromium.org/2651733002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/input_method/candidate_window_controller_impl.cc
(right):

https://codereview.chromium.org/2651733002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/input_method/candidate_window_controller_impl.cc:48: //
TODO(moshayedi): crbug.com/637416. Setting parent is nullptr in mash is
Can you actually file a new bug specifically for making sure the candidate list
is moved correctly when the window containing the focused text input-client
moves?

[Hadi, 2017-01-24 19:13:55]

Description was changed from

==========
IME for Mus: Avoid crash in InitWindowCandidateView(). 

Currently ash::wm::GetActiveWindow() crashes in mus+ash, so this change simply
avoids calling it and sets the parent for CandidateWindowView as nullptr.

This won't have the proper behavior in some situations like dragging the window,

etc. But since non-latin text input is not high-priority right now, this is just
a simple change just to avoid crashing.

BUG=637416
==========

to

==========
IME for Mus: Avoid crash in InitWindowCandidateView(). 

This CL fixes the crash when the candidate list is created. crbug.com/684658
tracks fixing the positioning of the candidate window when the window moves.

BUG=637416
==========

[Hadi, 2017-01-24 19:15:04]

On 2017/01/24 15:59:11, sadrul wrote:
> From the CL description:
> 
> > But since non-latin text input is not high-priority right
> > now, this is just a simple change just to avoid crashing.
> 
> Change this to something like:
> 
> "This CL fixes the crash when the candidate list is created. crbug.com/<new
bug>
> tracks fixing the positioning of the candidate window when the window moves."
> 
> With that, lgtm
> 
>
https://codereview.chromium.org/2651733002/diff/20001/chrome/browser/chromeos...
> File chrome/browser/chromeos/input_method/candidate_window_controller_impl.cc
> (right):
> 
>
https://codereview.chromium.org/2651733002/diff/20001/chrome/browser/chromeos...
> chrome/browser/chromeos/input_method/candidate_window_controller_impl.cc:48:
//
> TODO(moshayedi): crbug.com/637416. Setting parent is nullptr in mash is
> Can you actually file a new bug specifically for making sure the candidate
list
> is moved correctly when the window containing the focused text input-client
> moves?

Done.

[Hadi, 2017-01-25 04:41:33]

shuchen@ can you please provide OWNERS review?

Thanks.

[Hadi, 2017-01-31 17:09:45]

moshayedi@chromium.org changed reviewers:
- shuchen@chromium.org

[Hadi, 2017-01-31 17:14:56]

moshayedi@chromium.org changed reviewers:
+ shuchen@chromium.org, yukishiino@chromium.org

[Hadi, 2017-01-31 17:14:56]

yukishiino@ PTAL (I think shuchen@ is OOO this week).

Thanks.

[Yuki, 2017-02-01 04:55:09]

LGTM.

Just my two cents, I think it's better to describe issues in more details (in
both of crbug.com/637416 and crbug.com/684658 ).  For example, you can link this
CL from crbug.com/684658 and/or you can mention the source file name, function
name, a link of cs.chromium.org, or how to reproduce the issue / how to confirm
that the issue is fixed, etc.

I found no background info nor discussions there.

[Hadi, 2017-02-01 13:32:58]

The CQ bit was checked by moshayedi@chromium.org

[Hadi, 2017-02-01 13:32:58]

The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org
Link to the patchset: https://codereview.chromium.org/2651733002/#ps40001
(title: "addressed feedback.")

[commit-bot: I haz the power, 2017-02-01 13:33:15]

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

[Hadi, 2017-02-01 13:35:09]

On 2017/02/01 04:55:09, Yuki wrote:
> Just my two cents, I think it's better to describe issues in more details (in
> both of crbug.com/637416 and crbug.com/684658 ).  For example, you can link
this
> CL from crbug.com/684658 and/or you can mention the source file name, function
> name, a link of http://cs.chromium.org, or how to reproduce the issue / how to
confirm
> that the issue is fixed, etc.
> 
> I found no background info nor discussions there.

Thanks Yuki. Sorry for lack of background or discussions. I'll definitely keep
your feedback in mind and try to write better issues.

[commit-bot: I haz the power, 2017-02-01 14:04:00]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1485955978346890,
"parent_rev": "17c21fefe0e32acf1196c71882e10277baf406c8", "commit_rev":
"dfb10e743132bac81702c2a3685f4c7c782d368f"}

[commit-bot: I haz the power, 2017-02-01 14:04:32]

Description was changed from

==========
IME for Mus: Avoid crash in InitWindowCandidateView(). 

This CL fixes the crash when the candidate list is created. crbug.com/684658
tracks fixing the positioning of the candidate window when the window moves.

BUG=637416
==========

to

==========
IME for Mus: Avoid crash in InitWindowCandidateView(). 

This CL fixes the crash when the candidate list is created. crbug.com/684658
tracks fixing the positioning of the candidate window when the window moves.

BUG=637416

Review-Url: https://codereview.chromium.org/2651733002
Cr-Commit-Position: refs/heads/master@{#447506}
Committed:
https://chromium.googlesource.com/chromium/src/+/dfb10e743132bac81702c2a3685f...
==========

[commit-bot: I haz the power, 2017-02-01 14:04:33]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/dfb10e743132bac81702c2a3685f...