New method: InputMethod::IsCandidatePopupOpen()

New method: InputMethod::IsCandidatePopupOpen()

Supports a new method {ui,views}::InputMethod::IsCandidatePopupOpen(),
which returns true if a candidate window is open.

BUG=245578
TEST=Test manually.

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

[Yuki, 2013-06-19 09:00:57]

Could you review this CL?

Cheers,
Yuki Shiino

[Seigo Nonaka, 2013-06-19 09:15:51]

https://codereview.chromium.org/17112021/diff/5001/ui/base/ime/input_method.h
File ui/base/ime/input_method.h (right):

https://codereview.chromium.org/17112021/diff/5001/ui/base/ime/input_method.h...
ui/base/ime/input_method.h:153: virtual bool IsPopupOpen() const = 0;
IsPopupOpen seems not clear for me.
How about IsCandidateWindowOpen or IsInputMethodUIActive?

By the way, is it okay to get only open or not?
I guess you are trying to integrate omnibox suggestion and candidate window.
introducing observer is more helpful?

https://codereview.chromium.org/17112021/diff/5001/ui/base/ime/input_method_t...
File ui/base/ime/input_method_tsf.cc (right):

https://codereview.chromium.org/17112021/diff/5001/ui/base/ime/input_method_t...
ui/base/ime/input_method_tsf.cc:102: *handled = FALSE;
Is it okay not to use TsfEventRouter for pure TSF stuff?

[Yohei Yukawa, 2013-06-19 09:23:07]

Just FYI.

https://codereview.chromium.org/17112021/diff/1/ui/base/ime/input_method_tsf.cc
File ui/base/ime/input_method_tsf.cc (right):

https://codereview.chromium.org/17112021/diff/1/ui/base/ime/input_method_tsf....
ui/base/ime/input_method_tsf.cc:56: case WM_IME_NOTIFY:
I guess WM_IME_NOTIFY is not supported on Metro mode, where IMM32-TSF
emulation layer is disabled. I think it is OK to keep
InputMethodTSF::IsPopupOpen as just a stub that always returns false,
as far as we will fix it in some follow-up CLs.

FYI, if you want to support InputMethodTSF::IsPopupOpen() in the TSF way,
you can receive appropriate notifications via
TSFEventRouterObserver::OnCandidateWindowCountChanged. You can see our
previous work in chrome/browser/ui/views/omnibox/omnibox_view_win.cc
about how to use the event.

[Yuki, 2013-06-19 09:35:54]

https://codereview.chromium.org/17112021/diff/5001/ui/base/ime/input_method.h
File ui/base/ime/input_method.h (right):

https://codereview.chromium.org/17112021/diff/5001/ui/base/ime/input_method.h...
ui/base/ime/input_method.h:153: virtual bool IsPopupOpen() const = 0;
On 2013/06/19 09:15:51, Seigo Nonaka wrote:
> IsPopupOpen seems not clear for me.
> How about IsCandidateWindowOpen or IsInputMethodUIActive?
> 
> By the way, is it okay to get only open or not?
> I guess you are trying to integrate omnibox suggestion and candidate window.
> introducing observer is more helpful?

For name, I'd like to support in near future not only candidate windows, but
also IME suggest or anything similar.  So I thought "CandidateWindow" is too
specific.  From the view point of UI component, I think it's best to say
"Popup".  I'd like to cover any sort of popups, which would conflict with
Omnibox popup.

For observer, it's not helpful this time.  At
OmniboxPopupContentsView::UpdatePopupAppearance(), I'd like to check if any IME
popups are open or not.  So a simple getter method is more useful than a
observer.

https://codereview.chromium.org/17112021/diff/5001/ui/base/ime/input_method_t...
File ui/base/ime/input_method_tsf.cc (right):

https://codereview.chromium.org/17112021/diff/5001/ui/base/ime/input_method_t...
ui/base/ime/input_method_tsf.cc:102: *handled = FALSE;
On 2013/06/19 09:15:51, Seigo Nonaka wrote:
> Is it okay not to use TsfEventRouter for pure TSF stuff?

I'll support it in a separate CL later, as Yukawa-san suggested.

[Seigo Nonaka, 2013-06-20 05:29:10]

On 2013/06/19 09:35:54, Yuki wrote:
> https://codereview.chromium.org/17112021/diff/5001/ui/base/ime/input_method.h
> File ui/base/ime/input_method.h (right):
> 
>
https://codereview.chromium.org/17112021/diff/5001/ui/base/ime/input_method.h...
> ui/base/ime/input_method.h:153: virtual bool IsPopupOpen() const = 0;
> On 2013/06/19 09:15:51, Seigo Nonaka wrote:
> > IsPopupOpen seems not clear for me.
> > How about IsCandidateWindowOpen or IsInputMethodUIActive?
> > 
> > By the way, is it okay to get only open or not?
> > I guess you are trying to integrate omnibox suggestion and candidate window.
> > introducing observer is more helpful?
> 
> For name, I'd like to support in near future not only candidate windows, but
> also IME suggest or anything similar.  So I thought "CandidateWindow" is too
> specific.  From the view point of UI component, I think it's best to say
> "Popup".  I'd like to cover any sort of popups, which would conflict with
> Omnibox popup.
I still don't think "Popup" is good for this case. Developer may confuse what is
the Popup window of input method.

Currently there are already some popup windows around text inputs e.g. candidate
window, infolist window and popup menu for touch text selection.
In addition to this, more text input popup may be introduced in future, e.g.
input method indicator or virtual keyboard, handwriting window.
I think the behavior of IsPopupOpen is not trivial for that windows. And some of
them can not be handled only by input methods.

If you want not to be specific, you should introduce more general class like
TextInputPopupObserver.
> 
> For observer, it's not helpful this time.  At
> OmniboxPopupContentsView::UpdatePopupAppearance(), I'd like to check if any
IME
> popups are open or not.  So a simple getter method is more useful than a
> observer.
> 
>
https://codereview.chromium.org/17112021/diff/5001/ui/base/ime/input_method_t...
> File ui/base/ime/input_method_tsf.cc (right):
> 
>
https://codereview.chromium.org/17112021/diff/5001/ui/base/ime/input_method_t...
> ui/base/ime/input_method_tsf.cc:102: *handled = FALSE;
> On 2013/06/19 09:15:51, Seigo Nonaka wrote:
> > Is it okay not to use TsfEventRouter for pure TSF stuff?
> 
> I'll support it in a separate CL later, as Yukawa-san suggested.

[Yuki, 2013-06-20 06:36:49]

Thanks for the review comment.
As we discussed offline, I renamed the method name from IsPopupOpen() to
IsCandidatePopupOpen().

Thanks,
Yuki Shiino

[Seigo Nonaka, 2013-06-20 07:38:54]

lgtm

[Yuki, 2013-06-20 07:43:05]

sky: Could you review this CL as an owner of
ui/views/win/hwnd_message_handler.h ?

I'd like to handle WM_IME_NOTIFY to detect whether a IME candidate window is
open or not.

Cheers,
Yuki Shiino

[sky, 2013-06-20 17:35:36]

LGTM

[commit-bot: I haz the power, 2013-06-21 02:57:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yukishiino@chromium.org/17112021/18001

[Yohei Yukawa, 2013-06-21 06:45:46]

https://codereview.chromium.org/17112021/diff/18001/ui/base/ime/input_method_...
File ui/base/ime/input_method_tsf.cc (right):

https://codereview.chromium.org/17112021/diff/18001/ui/base/ime/input_method_...
ui/base/ime/input_method_tsf.cc:65: case WM_IME_NOTIFY:
Wait.

Please do not handle WM_IME_NOTIFY when TSF is enabled.
It was not a right way as I commented previously.

[Yuki, 2013-06-21 06:55:02]

On 2013/06/21 06:45:46, Yohei Yukawa wrote:
>
https://codereview.chromium.org/17112021/diff/18001/ui/base/ime/input_method_...
> File ui/base/ime/input_method_tsf.cc (right):
> 
>
https://codereview.chromium.org/17112021/diff/18001/ui/base/ime/input_method_...
> ui/base/ime/input_method_tsf.cc:65: case WM_IME_NOTIFY:
> Wait.
> 
> Please do not handle WM_IME_NOTIFY when TSF is enabled.
> It was not a right way as I commented previously.

Sorry, removed TSF code and will add it in a separate CL.

[commit-bot: I haz the power, 2013-06-21 06:56:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yukishiino@chromium.org/17112021/61001

[commit-bot: I haz the power, 2013-06-21 14:05:28]

Retried try job too often on linux_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-06-21 14:22:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yukishiino@chromium.org/17112021/104001

[commit-bot: I haz the power, 2013-06-21 16:40:02]

Change committed as 207858