Re-enable CJK omnibox suggest on Metro UI

Re-enable CJK omnibox suggest on Metro UI

On Metro UI, we can't get IMM32 message like WM_IME_COMPOSITION or IMN_OPENCANDIDATE/IMN_CLOSECANDIDATE.
Instead of message handling, registering callback function to TsfEventRouter which monitors TSF related event and callbacks on specific event.
To enable omnibox suggestion again for IME on Metro environment, I introduced two event callback, OnTextUpdatedByTsf which is called when the text contents are updated by TSF, and OnCandidateWindowCountChangedByTsf which is called when the number of currently opened candidate window is changed.
These callbacks are never called without Metro environment, so this CL does not affects non Metro environment including previous version of Windows.

BUG=141820, 151901
TEST=Manually done on Win8 for both Metro and classic UI.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=163233

[Yohei Yukawa, 2012-10-18 01:58:30]

lgtm

[falken, 2012-10-18 02:15:54]

lgtm

https://chromiumcodereview.appspot.com/11141019/diff/5001/chrome/browser/ui/v...
File chrome/browser/ui/views/omnibox/omnibox_view_win.cc (right):

https://chromiumcodereview.appspot.com/11141019/diff/5001/chrome/browser/ui/v...
chrome/browser/ui/views/omnibox/omnibox_view_win.cc:938: //
OnbeforePossibleChange.
nit: "OnBeforePossibleChange"

https://chromiumcodereview.appspot.com/11141019/diff/5001/chrome/browser/ui/v...
chrome/browser/ui/views/omnibox/omnibox_view_win.cc:949: // text. For example,
Baidu Japanese IME sends IMN_CLOSECANDIDATE when
In TSF you don't get IMN_CLOSECANDIDATE messages, right? Maybe this comment
should be updated for TSF.

https://chromiumcodereview.appspot.com/11141019/diff/5001/chrome/browser/ui/v...
File chrome/browser/ui/views/omnibox/omnibox_view_win.h (right):

https://chromiumcodereview.appspot.com/11141019/diff/5001/chrome/browser/ui/v...
chrome/browser/ui/views/omnibox/omnibox_view_win.h:396: // candidate window
opend is changed. This callback should not be called when
"currently opened candidate windows"

(if I understand correctly)

[Seigo Nonaka, 2012-10-18 10:02:23]

Yukawa, falken: Thank you for your review!

Adding Peter Kasting as an expert of Omnibox.
Could you please take a look?

Thanks

http://codereview.chromium.org/11141019/diff/5001/chrome/browser/ui/views/omn...
File chrome/browser/ui/views/omnibox/omnibox_view_win.cc (right):

http://codereview.chromium.org/11141019/diff/5001/chrome/browser/ui/views/omn...
chrome/browser/ui/views/omnibox/omnibox_view_win.cc:938: //
OnbeforePossibleChange.
On 2012/10/18 02:15:54, falken wrote:
> nit: "OnBeforePossibleChange"

Done.

http://codereview.chromium.org/11141019/diff/5001/chrome/browser/ui/views/omn...
chrome/browser/ui/views/omnibox/omnibox_view_win.cc:949: // text. For example,
Baidu Japanese IME sends IMN_CLOSECANDIDATE when
On 2012/10/18 02:15:54, falken wrote:
> In TSF you don't get IMN_CLOSECANDIDATE messages, right? Maybe this comment
> should be updated for TSF.

Done.

http://codereview.chromium.org/11141019/diff/5001/chrome/browser/ui/views/omn...
File chrome/browser/ui/views/omnibox/omnibox_view_win.h (right):

http://codereview.chromium.org/11141019/diff/5001/chrome/browser/ui/views/omn...
chrome/browser/ui/views/omnibox/omnibox_view_win.h:396: // candidate window
opend is changed. This callback should not be called when
On 2012/10/18 02:15:54, falken wrote:
> "currently opened candidate windows"
> 
> (if I understand correctly)

Done.

[Peter Kasting, 2012-10-18 21:21:49]

http://codereview.chromium.org/11141019/diff/12001/chrome/browser/ui/views/om...
File chrome/browser/ui/views/omnibox/omnibox_view_win.cc (right):

http://codereview.chromium.org/11141019/diff/12001/chrome/browser/ui/views/om...
chrome/browser/ui/views/omnibox/omnibox_view_win.cc:547: // Register
TsfEventRouter to catch TSF related event.
Nit: Nuke this comment, it adds nothing.

http://codereview.chromium.org/11141019/diff/12001/chrome/browser/ui/views/om...
chrome/browser/ui/views/omnibox/omnibox_view_win.cc:934:
OnAfterPossibleChangeInternal(true);
What about the very first time we reach here, will we have called
OnBeforePossibleChange() somehow before that?  Or are we guaranteed that that
doesn't matter?

http://codereview.chromium.org/11141019/diff/12001/chrome/browser/ui/views/om...
chrome/browser/ui/views/omnibox/omnibox_view_win.cc:950: if
(model()->user_input_in_progress())
Nit: Combine else and if as "else if"

http://codereview.chromium.org/11141019/diff/12001/chrome/browser/ui/views/om...
chrome/browser/ui/views/omnibox/omnibox_view_win.cc:990: if
(base::win::IsTsfAwareRequired()) {
Nit: Seems better to check for tsf_event_router_ not being NULL, that way if we
want to change when we init it we'll only need to change one place.  (3 places)

http://codereview.chromium.org/11141019/diff/12001/chrome/browser/ui/views/om...
chrome/browser/ui/views/omnibox/omnibox_view_win.cc:992: } else {
Nit: No else after return

http://codereview.chromium.org/11141019/diff/12001/chrome/browser/ui/views/om...
File chrome/browser/ui/views/omnibox/omnibox_view_win.h (right):

http://codereview.chromium.org/11141019/diff/12001/chrome/browser/ui/views/om...
chrome/browser/ui/views/omnibox/omnibox_view_win.h:393: void
OnTextUpdatedByTsf();
Nit: Use "TSF" in place of "Tsf" everywhere.  (It would be nice to rename
TsfEventRouter too.)

http://codereview.chromium.org/11141019/diff/12001/chrome/browser/ui/views/om...
chrome/browser/ui/views/omnibox/omnibox_view_win.h:528:
scoped_refptr<ui::TsfEventRouter> tsf_event_router_;
Why does this need to be refcounted?  Owning it directly and nuking the weak
pointer factory would be better.

[Seigo Nonaka, 2012-10-19 08:50:01]

http://codereview.chromium.org/11141019/diff/12001/chrome/browser/ui/views/om...
File chrome/browser/ui/views/omnibox/omnibox_view_win.cc (right):

http://codereview.chromium.org/11141019/diff/12001/chrome/browser/ui/views/om...
chrome/browser/ui/views/omnibox/omnibox_view_win.cc:547: // Register
TsfEventRouter to catch TSF related event.
On 2012/10/18 21:21:49, Peter Kasting wrote:
> Nit: Nuke this comment, it adds nothing.

Done.

http://codereview.chromium.org/11141019/diff/12001/chrome/browser/ui/views/om...
chrome/browser/ui/views/omnibox/omnibox_view_win.cc:934:
OnAfterPossibleChangeInternal(true);
In the current implementation, it does not matter, but in the future this may
not be the case. So, I changed OnCreate to call OnBeforePossibleChange to
guarantee it is at least called before any OnAfterPossibleChange call.

On 2012/10/18 21:21:49, Peter Kasting wrote:
> What about the very first time we reach here, will we have called
> OnBeforePossibleChange() somehow before that?  Or are we guaranteed that that
> doesn't matter?

http://codereview.chromium.org/11141019/diff/12001/chrome/browser/ui/views/om...
chrome/browser/ui/views/omnibox/omnibox_view_win.cc:950: if
(model()->user_input_in_progress())
On 2012/10/18 21:21:49, Peter Kasting wrote:
> Nit: Combine else and if as "else if"

Done.

http://codereview.chromium.org/11141019/diff/12001/chrome/browser/ui/views/om...
chrome/browser/ui/views/omnibox/omnibox_view_win.cc:990: if
(base::win::IsTsfAwareRequired()) {
On 2012/10/18 21:21:49, Peter Kasting wrote:
> Nit: Seems better to check for tsf_event_router_ not being NULL, that way if
we
> want to change when we init it we'll only need to change one place.  (3
places)

Done.

http://codereview.chromium.org/11141019/diff/12001/chrome/browser/ui/views/om...
chrome/browser/ui/views/omnibox/omnibox_view_win.cc:992: } else {
On 2012/10/18 21:21:49, Peter Kasting wrote:
> Nit: No else after return

Done.

http://codereview.chromium.org/11141019/diff/12001/chrome/browser/ui/views/om...
File chrome/browser/ui/views/omnibox/omnibox_view_win.h (right):

http://codereview.chromium.org/11141019/diff/12001/chrome/browser/ui/views/om...
chrome/browser/ui/views/omnibox/omnibox_view_win.h:393: void
OnTextUpdatedByTsf();
I've renamed this function.

Is it a style convention to use uppercase for acronyms like this? I checked the
style guide and could not find anything definitive, but it uses "Url" in an
example. For the TsfEventRouter, I have been using "Tsf" prefix for all TSF
related modules.  So I'd like to keep this naming or change it in another CL.

Thanks

On 2012/10/18 21:21:49, Peter Kasting wrote:
> Nit: Use "TSF" in place of "Tsf" everywhere.  (It would be nice to rename
> TsfEventRouter too.)

http://codereview.chromium.org/11141019/diff/12001/chrome/browser/ui/views/om...
chrome/browser/ui/views/omnibox/omnibox_view_win.h:528:
scoped_refptr<ui::TsfEventRouter> tsf_event_router_;
The reason of scoped_refptr is TsfEventRouter is COM object. However I've
removed weak pointer factory by introducing Observer class.
On 2012/10/18 21:21:49, Peter Kasting wrote:
> Why does this need to be refcounted?  Owning it directly and nuking the weak
> pointer factory would be better.

[Peter Kasting, 2012-10-19 18:40:57]

LGTM, but TsfEventRouter has some serious issues that need to be resolved in a
subsequent change.  I filed http://crbug.com/156867 about these.

I do think you should change Tsf to TSF, but doing that as a standalone change
is fine (indeed, preferred).

http://codereview.chromium.org/11141019/diff/16005/chrome/browser/ui/views/om...
File chrome/browser/ui/views/omnibox/omnibox_view_win.cc (right):

http://codereview.chromium.org/11141019/diff/16005/chrome/browser/ui/views/om...
chrome/browser/ui/views/omnibox/omnibox_view_win.cc:563: if (tsf_event_router_)
Tiny nit: This should probably go above the paint patcher bit, since that object
was inited first (and it's best to shut things down in the reverse order of
startup)

http://codereview.chromium.org/11141019/diff/16005/chrome/browser/ui/views/om...
chrome/browser/ui/views/omnibox/omnibox_view_win.cc:1417: // to call
OnBeforePossibleChange at the beginning of an edit session, if we
Nit: Slightly clearer:

  // When TSF is enabled, OnTextUpdated() may be called without any previous
  // call that would have indicated the start of an editing session.  In order
  // to guarantee we've always called OnBeforePossibleChange() before
  // OnAfterPossibleChange(), we therefore call that here.  Note that multiple
  // (i.e. unmatched) calls to this function in a row are safe.

http://codereview.chromium.org/11141019/diff/16005/chrome/browser/ui/views/om...
chrome/browser/ui/views/omnibox/omnibox_view_win.cc:1421:
OnBeforePossibleChange();
You should only do this when |tsf_event_router_| is non-NULL.

http://codereview.chromium.org/11141019/diff/16005/chrome/browser/ui/views/om...
File chrome/browser/ui/views/omnibox/omnibox_view_win.h (right):

http://codereview.chromium.org/11141019/diff/16005/chrome/browser/ui/views/om...
chrome/browser/ui/views/omnibox/omnibox_view_win.h:334: //
TsfEventRouter::Observer override.
Nit: If the IMM32 comment is useful, put it in the TsfEventRouter::Observer
declarations.  Then declare the overrides this way:

  // TsfEventRouter::Observer:
  virtual void OnTextUpdated() OVERRIDE;
  virtual void OnCandidateWindowCountChanged(size_t window_count) OVERRIDE;

(Frankly, the other overrides here should do the same, but you need not fix
those.)

http://codereview.chromium.org/11141019/diff/16005/ui/base/ime/win/tsf_event_...
File ui/base/ime/win/tsf_event_router.cc (right):

http://codereview.chromium.org/11141019/diff/16005/ui/base/ime/win/tsf_event_...
ui/base/ime/win/tsf_event_router.cc:234: // An observer to be notify the events.
Nit: This comment doesn't make sense, and doesn't actually add anything anyway. 
Remove.

http://codereview.chromium.org/11141019/diff/16005/ui/base/ime/win/tsf_event_...
File ui/base/ime/win/tsf_event_router.h (right):

http://codereview.chromium.org/11141019/diff/16005/ui/base/ime/win/tsf_event_...
ui/base/ime/win/tsf_event_router.h:33: // Called when the number of currently
opend candidate windowsd is changed.
Nit: opend -> open, windowsd -> windows, is changed -> changes

http://codereview.chromium.org/11141019/diff/16005/ui/base/ime/win/tsf_event_...
ui/base/ime/win/tsf_event_router.h:47: // Factory function, creates a new
instance and retunrns ownership.
Nit: While here: and retunrns ownership -> which the caller owns

[Seigo Nonaka, 2012-10-22 03:10:54]

Sure, thanks. I will fix soon.

On 2012/10/19 18:40:57, Peter Kasting wrote:
> LGTM, but TsfEventRouter has some serious issues that need to be resolved in a
> subsequent change.  I filed http://crbug.com/156867 about these.
> 
> I do think you should change Tsf to TSF, but doing that as a standalone change
> is fine (indeed, preferred).
> 
>
http://codereview.chromium.org/11141019/diff/16005/chrome/browser/ui/views/om...
> File chrome/browser/ui/views/omnibox/omnibox_view_win.cc (right):
> 
>
http://codereview.chromium.org/11141019/diff/16005/chrome/browser/ui/views/om...
> chrome/browser/ui/views/omnibox/omnibox_view_win.cc:563: if
(tsf_event_router_)
> Tiny nit: This should probably go above the paint patcher bit, since that
object
> was inited first (and it's best to shut things down in the reverse order of
> startup)
> 
>
http://codereview.chromium.org/11141019/diff/16005/chrome/browser/ui/views/om...
> chrome/browser/ui/views/omnibox/omnibox_view_win.cc:1417: // to call
> OnBeforePossibleChange at the beginning of an edit session, if we
> Nit: Slightly clearer:
> 
>   // When TSF is enabled, OnTextUpdated() may be called without any previous
>   // call that would have indicated the start of an editing session.  In order
>   // to guarantee we've always called OnBeforePossibleChange() before
>   // OnAfterPossibleChange(), we therefore call that here.  Note that multiple
>   // (i.e. unmatched) calls to this function in a row are safe.
> 
>
http://codereview.chromium.org/11141019/diff/16005/chrome/browser/ui/views/om...
> chrome/browser/ui/views/omnibox/omnibox_view_win.cc:1421:
> OnBeforePossibleChange();
> You should only do this when |tsf_event_router_| is non-NULL.
> 
>
http://codereview.chromium.org/11141019/diff/16005/chrome/browser/ui/views/om...
> File chrome/browser/ui/views/omnibox/omnibox_view_win.h (right):
> 
>
http://codereview.chromium.org/11141019/diff/16005/chrome/browser/ui/views/om...
> chrome/browser/ui/views/omnibox/omnibox_view_win.h:334: //
> TsfEventRouter::Observer override.
> Nit: If the IMM32 comment is useful, put it in the TsfEventRouter::Observer
> declarations.  Then declare the overrides this way:
> 
>   // TsfEventRouter::Observer:
>   virtual void OnTextUpdated() OVERRIDE;
>   virtual void OnCandidateWindowCountChanged(size_t window_count) OVERRIDE;
> 
> (Frankly, the other overrides here should do the same, but you need not fix
> those.)
> 
>
http://codereview.chromium.org/11141019/diff/16005/ui/base/ime/win/tsf_event_...
> File ui/base/ime/win/tsf_event_router.cc (right):
> 
>
http://codereview.chromium.org/11141019/diff/16005/ui/base/ime/win/tsf_event_...
> ui/base/ime/win/tsf_event_router.cc:234: // An observer to be notify the
events.
> Nit: This comment doesn't make sense, and doesn't actually add anything
anyway. 
> Remove.
> 
>
http://codereview.chromium.org/11141019/diff/16005/ui/base/ime/win/tsf_event_...
> File ui/base/ime/win/tsf_event_router.h (right):
> 
>
http://codereview.chromium.org/11141019/diff/16005/ui/base/ime/win/tsf_event_...
> ui/base/ime/win/tsf_event_router.h:33: // Called when the number of currently
> opend candidate windowsd is changed.
> Nit: opend -> open, windowsd -> windows, is changed -> changes
> 
>
http://codereview.chromium.org/11141019/diff/16005/ui/base/ime/win/tsf_event_...
> ui/base/ime/win/tsf_event_router.h:47: // Factory function, creates a new
> instance and retunrns ownership.
> Nit: While here: and retunrns ownership -> which the caller owns

[Seigo Nonaka, 2012-10-22 03:11:03]

http://codereview.chromium.org/11141019/diff/16005/chrome/browser/ui/views/om...
File chrome/browser/ui/views/omnibox/omnibox_view_win.cc (right):

http://codereview.chromium.org/11141019/diff/16005/chrome/browser/ui/views/om...
chrome/browser/ui/views/omnibox/omnibox_view_win.cc:563: if (tsf_event_router_)
On 2012/10/19 18:40:57, Peter Kasting wrote:
> Tiny nit: This should probably go above the paint patcher bit, since that
object
> was inited first (and it's best to shut things down in the reverse order of
> startup)

Done.

http://codereview.chromium.org/11141019/diff/16005/chrome/browser/ui/views/om...
chrome/browser/ui/views/omnibox/omnibox_view_win.cc:1417: // to call
OnBeforePossibleChange at the beginning of an edit session, if we
On 2012/10/19 18:40:57, Peter Kasting wrote:
> Nit: Slightly clearer:
> 
>   // When TSF is enabled, OnTextUpdated() may be called without any previous
>   // call that would have indicated the start of an editing session.  In order
>   // to guarantee we've always called OnBeforePossibleChange() before
>   // OnAfterPossibleChange(), we therefore call that here.  Note that multiple
>   // (i.e. unmatched) calls to this function in a row are safe.

Done.

http://codereview.chromium.org/11141019/diff/16005/chrome/browser/ui/views/om...
chrome/browser/ui/views/omnibox/omnibox_view_win.cc:1421:
OnBeforePossibleChange();
On 2012/10/19 18:40:57, Peter Kasting wrote:
> You should only do this when |tsf_event_router_| is non-NULL.

Done.

http://codereview.chromium.org/11141019/diff/16005/chrome/browser/ui/views/om...
File chrome/browser/ui/views/omnibox/omnibox_view_win.h (right):

http://codereview.chromium.org/11141019/diff/16005/chrome/browser/ui/views/om...
chrome/browser/ui/views/omnibox/omnibox_view_win.h:334: //
TsfEventRouter::Observer override.
On 2012/10/19 18:40:57, Peter Kasting wrote:
> Nit: If the IMM32 comment is useful, put it in the TsfEventRouter::Observer
> declarations.  Then declare the overrides this way:
> 
>   // TsfEventRouter::Observer:
>   virtual void OnTextUpdated() OVERRIDE;
>   virtual void OnCandidateWindowCountChanged(size_t window_count) OVERRIDE;
> 
> (Frankly, the other overrides here should do the same, but you need not fix
> those.)

Done.

http://codereview.chromium.org/11141019/diff/16005/ui/base/ime/win/tsf_event_...
File ui/base/ime/win/tsf_event_router.cc (right):

http://codereview.chromium.org/11141019/diff/16005/ui/base/ime/win/tsf_event_...
ui/base/ime/win/tsf_event_router.cc:234: // An observer to be notify the events.
On 2012/10/19 18:40:57, Peter Kasting wrote:
> Nit: This comment doesn't make sense, and doesn't actually add anything
anyway. 
> Remove.

Done.

http://codereview.chromium.org/11141019/diff/16005/ui/base/ime/win/tsf_event_...
File ui/base/ime/win/tsf_event_router.h (right):

http://codereview.chromium.org/11141019/diff/16005/ui/base/ime/win/tsf_event_...
ui/base/ime/win/tsf_event_router.h:33: // Called when the number of currently
opend candidate windowsd is changed.
On 2012/10/19 18:40:57, Peter Kasting wrote:
> Nit: opend -> open, windowsd -> windows, is changed -> changes

Done.

http://codereview.chromium.org/11141019/diff/16005/ui/base/ime/win/tsf_event_...
ui/base/ime/win/tsf_event_router.h:47: // Factory function, creates a new
instance and retunrns ownership.
On 2012/10/19 18:40:57, Peter Kasting wrote:
> Nit: While here: and retunrns ownership -> which the caller owns

Done.

[commit-bot: I haz the power, 2012-10-22 03:11:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nona@chromium.org/11141019/24001

[commit-bot: I haz the power, 2012-10-22 06:39:36]

Change committed as 163233