Bug 40608 – [chromium]Refactor input method related APIs.

• Component: WebKit API
• Status: RESOLVED FIXED

[James Su]

In order to fix following chromium issues and for future input method feature enhancements, some refactory should be done for input method related APIs.
http://crbug.com/23219
http://crbug.com/41876
http://crbug.com/44529
http://crbug.com/46326

New API contains:
1. Full support of WebKit's CompositionUnderline feature
2. Ability to get text input type of focused node, rather than just true and false.
3. Split handleCompositionEvent() and queryInputMethodStatus() methods into multiple independent methods to make them clearer and simpler.
4. Move all input method related things into WebWidget/WebWidgetClient

I'll upload the patch shortly

[James Su]

Created attachment 58749 [details]
Proposed patch.

[WebKit Review Bot]

Attachment 58749 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3334147

[Darin Fisher (:fishd, Google)]

Comment on attachment 58749 [details]
Proposed patch.

WebCore/page/FocusController.cpp:599
 +      // input method can still have chance to finish the ongoing composition
nit: "can still have _a_ chance to..."

WebCore/page/FocusController.cpp: 
 +          m_page->editorClient()->setInputMethodState(false);
can you please explain why you are removing these calls to setInputMethodState?
it is not obvious from the context here.

WebKit/chromium/public/WebCompositionCommand.h:36
 +  // DEPRECIATED. It's not used anymore.
nit: DEPRECIATED -> DEPRECATED, and "It's not used anymore" is redundant
with that, so you can just leave the comment as "// DEPRECATED"
WebKit/chromium/public/WebCompositionUnderline.h:41
 +  class WebCompositionUnderline {
this should be a struct since all of its member variables are public

WebKit/chromium/public/WebCompositionUnderline.h:40
 +  // CompositionUnderline struct.
we generally avoid mentioning WebCore in API comments since the point
of the WebKit API is to make it unnecessary to know anything about
WebCore.

WebKit/chromium/public/WebCompositionUnderline.h:47
 +          , thick(0) { }
thick is a bool type, so please use false instead of 0.

WebKit/chromium/public/WebCompositionUnderline.h:68
 +    + LF
webkit files should not have svn:eol-style

WebKit/chromium/public/WebTextInputType.h:58
 +    + LF
no svn:eol-style

WebKit/chromium/public/WebViewClient.h:128
 +      // DEPRECIATED: replaced by WebWidgetClient::resetInputMethod().
nit: DEPRECIATED -> DEPRECATED

WebKit/chromium/public/WebWidget.h:82
 +      // DEPRECIATED. It's replaced by setComposition() and confirmComposition().
DEPRECIATED -> DEPRECATED

WebKit/chromium/public/WebWidget.h:100
 +      // Called to inform the WebWidget to Confirm an ongoing composition.
nit: Confirm -> confirm

WebKit/chromium/public/WebWidget.h:104
 +      // DEPRECIATED. It's replaced by textInputType() and caretBounds().
DEPRECIATED -> DEPRECATED

WebKit/chromium/public/WebWidget.h:112
 +      virtual WebRect caretBounds() = 0;
should this be named caretOrSelectionBounds?

WebKit/chromium/src/WebCompositionUnderlineConversion.h:44
 +  class CompositionUnderlineBuilder : public WebCore::CompositionUnderline {
this file should be named after this class, and you should create a separate file for each class.

WebKit/chromium/src/WebPopupMenuImpl.cpp:233
 +  // DEPRECIATED, will be removed later.
nit: DEPRECIATED -> DEPRECATED

WebKit/chromium/src/WebPopupMenuImpl.cpp:253
 +  // DEPRECIATED, will be removed later.
ditto

WebKit/chromium/src/WebPopupMenuImpl.h:70
 +      // DEPRECIATED, will be removed later.
ditto

WebKit/chromium/src/WebPopupMenuImpl.h:80
 +      // DEPRECIATED, will be removed later.
ditto

WebKit/chromium/src/WebViewImpl.cpp:1195
 +  // DEPRECIATED, will be removed later.
ditto

WebKit/chromium/src/WebViewImpl.cpp:1211
 +          WebVector<WebCompositionUnderline> underlines(static_cast<size_t>(3));
can you also do 3U instead of static_cast<size_t>(3) ?

WebKit/chromium/src/WebViewImpl.cpp:1227
 +      if (command == WebKit::WebCompositionCommandDiscard) {
nit: webkit style is to not use {}'s around single line statements

WebKit/chromium/src/WebViewImpl.cpp:1268
 +      if (!text.length() || m_suppressNextKeypressEvent) {
can you instead write text.isEmpty()?

WebKit/chromium/src/WebViewImpl.cpp:1314
 +  // DEPRECIATED, will be removed later.
ditto

WebKit/chromium/src/WebViewImpl.cpp:1290
 +  bool WebViewImpl::confirmComposition()
perhaps it would be helpful if confirmComposition took a WebNode corresponding
to the node being edited?  that way you could confirm that the same node is
still being edited when this method is called?  that might be superior to
having to guess.

WebKit/chromium/src/WebViewImpl.cpp:1370
 +      if (controller->isInPasswordField()) {
same nit about excluding {}s around single line statements

WebKit/chromium/src/WebViewImpl.cpp:1372
 +      } else if (node->shouldUseInputMethod()) {
ditto

WebKit/chromium/src/WebViewImpl.cpp:1393
 +      if (controller->isCaret()) {
same nit about {}s

WebKit/chromium/src/WebViewImpl.h:94
 +      // DEPRECIATED, will be removed later.
ditto

WebKit/chromium/src/WebViewImpl.h:107
 +      // DEPRECIATED, will be removed later.
ditto

[James Su]

Created attachment 58756 [details]
Updated patch according to review feedbacks.

[WebKit Review Bot]

Attachment 58756 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebKit/chromium/src/CompositionUnderlineVectorBuilder.cpp:32:  Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebKit/chromium/src/CompositionUnderlineVectorBuilder.cpp:33:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 2 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[James Su]

Created attachment 58757 [details]
Fix code style.

[James Su]

(In reply to comment #3)
> (From update of attachment 58749 [details])
> WebCore/page/FocusController.cpp:599
>  +      // input method can still have chance to finish the ongoing composition
> nit: "can still have _a_ chance to..."
Done

> 
> WebCore/page/FocusController.cpp: 
>  +          m_page->editorClient()->setInputMethodState(false);
> can you please explain why you are removing these calls to setInputMethodState?
> it is not obvious from the context here.
It's only necessary to call this method once in FocusController::setFocusedNode(). As it's already called above, this call is redundant.

> 
> WebKit/chromium/public/WebCompositionCommand.h:36
>  +  // DEPRECIATED. It's not used anymore.
> nit: DEPRECIATED -> DEPRECATED, and "It's not used anymore" is redundant
> with that, so you can just leave the comment as "// DEPRECATED"
Done

> WebKit/chromium/public/WebCompositionUnderline.h:41
>  +  class WebCompositionUnderline {
> this should be a struct since all of its member variables are public
Done

> 
> WebKit/chromium/public/WebCompositionUnderline.h:40
>  +  // CompositionUnderline struct.
> we generally avoid mentioning WebCore in API comments since the point
> of the WebKit API is to make it unnecessary to know anything about
> WebCore.
Done

> 
> WebKit/chromium/public/WebCompositionUnderline.h:47
>  +          , thick(0) { }
> thick is a bool type, so please use false instead of 0.
Done

> 
> WebKit/chromium/public/WebCompositionUnderline.h:68
>  +    + LF
> webkit files should not have svn:eol-style
Done

> 
> WebKit/chromium/public/WebTextInputType.h:58
>  +    + LF
> no svn:eol-style
Done

> 
> WebKit/chromium/public/WebViewClient.h:128
>  +      // DEPRECIATED: replaced by WebWidgetClient::resetInputMethod().
> nit: DEPRECIATED -> DEPRECATED
> 
Done

> WebKit/chromium/public/WebWidget.h:82
>  +      // DEPRECIATED. It's replaced by setComposition() and confirmComposition().
> DEPRECIATED -> DEPRECATED
> 
Done

> WebKit/chromium/public/WebWidget.h:100
>  +      // Called to inform the WebWidget to Confirm an ongoing composition.
> nit: Confirm -> confirm
> 
Done

> WebKit/chromium/public/WebWidget.h:104
>  +      // DEPRECIATED. It's replaced by textInputType() and caretBounds().
> DEPRECIATED -> DEPRECATED
> 
Done

> WebKit/chromium/public/WebWidget.h:112
>  +      virtual WebRect caretBounds() = 0;
> should this be named caretOrSelectionBounds?
> 
Done

> WebKit/chromium/src/WebCompositionUnderlineConversion.h:44
>  +  class CompositionUnderlineBuilder : public WebCore::CompositionUnderline {
> this file should be named after this class, and you should create a separate file for each class.
Done

> 
> WebKit/chromium/src/WebPopupMenuImpl.cpp:233
>  +  // DEPRECIATED, will be removed later.
> nit: DEPRECIATED -> DEPRECATED
Done

> 
> WebKit/chromium/src/WebPopupMenuImpl.cpp:253
>  +  // DEPRECIATED, will be removed later.
> ditto
Done

> 
> WebKit/chromium/src/WebPopupMenuImpl.h:70
>  +      // DEPRECIATED, will be removed later.
> ditto
Done

> 
> WebKit/chromium/src/WebPopupMenuImpl.h:80
>  +      // DEPRECIATED, will be removed later.
> ditto
Done

> 
> WebKit/chromium/src/WebViewImpl.cpp:1195
>  +  // DEPRECIATED, will be removed later.
> ditto
Done

> 
> WebKit/chromium/src/WebViewImpl.cpp:1211
>  +          WebVector<WebCompositionUnderline> underlines(static_cast<size_t>(3));
> can you also do 3U instead of static_cast<size_t>(3) ?
gcc doesn't like 3U here :(

> 
> WebKit/chromium/src/WebViewImpl.cpp:1227
>  +      if (command == WebKit::WebCompositionCommandDiscard) {
> nit: webkit style is to not use {}'s around single line statements
Done

> 
> WebKit/chromium/src/WebViewImpl.cpp:1268
>  +      if (!text.length() || m_suppressNextKeypressEvent) {
> can you instead write text.isEmpty()?
Done

> 
> WebKit/chromium/src/WebViewImpl.cpp:1314
>  +  // DEPRECIATED, will be removed later.
> ditto
Done

> 
> WebKit/chromium/src/WebViewImpl.cpp:1290
>  +  bool WebViewImpl::confirmComposition()
> perhaps it would be helpful if confirmComposition took a WebNode corresponding
> to the node being edited?  that way you could confirm that the same node is
> still being edited when this method is called?  that might be superior to
> having to guess.
Adding WebNode here would make chromium's code more complicated and make WebWidget depend on WebNode.

> 
> WebKit/chromium/src/WebViewImpl.cpp:1370
>  +      if (controller->isInPasswordField()) {
> same nit about excluding {}s around single line statements
Done

> 
> WebKit/chromium/src/WebViewImpl.cpp:1372
>  +      } else if (node->shouldUseInputMethod()) {
> ditto
Done

> 
> WebKit/chromium/src/WebViewImpl.cpp:1393
>  +      if (controller->isCaret()) {
> same nit about {}s
Done

> 
> WebKit/chromium/src/WebViewImpl.h:94
>  +      // DEPRECIATED, will be removed later.
> ditto
Done

> 
> WebKit/chromium/src/WebViewImpl.h:107
>  +      // DEPRECIATED, will be removed later.
> ditto
Done

[WebKit Review Bot]

Attachment 58757 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3277173

[Tony Chang]

Comment on attachment 58757 [details]
Fix code style.

WebCore/page/FocusController.cpp:600
 +      // session.
FWIW, this sounds like a functional change, even though the ChangeLog says this isn't a functional change.

WebKit/chromium/src/WebViewImpl.h:100
 +      virtual bool setComposition(
Should this return more than just a bool?  For example, I think the cancel composition work would be easier if this returned an enum (COMPOSITION_SET, COMPOSITION_CANCELED, or KEYPRESS_CANCELED) or something.  A bool is probably fine if there's a way to query what the current composition text is (e.g., if a keydown is prevented, setComposition can return false and the render view can query the webview to see what the current composition text should be set to in the browser process).

[James Su]

(In reply to comment #9)
> (From update of attachment 58757 [details])
> WebCore/page/FocusController.cpp:600
>  +      // session.
> FWIW, this sounds like a functional change, even though the ChangeLog says this isn't a functional change.
Or maybe I should say there is no new functionality?

> 
> WebKit/chromium/src/WebViewImpl.h:100
>  +      virtual bool setComposition(
> Should this return more than just a bool?  For example, I think the cancel composition work would be easier if this returned an enum (COMPOSITION_SET, COMPOSITION_CANCELED, or KEYPRESS_CANCELED) or something.  A bool is probably fine if there's a way to query what the current composition text is (e.g., if a keydown is prevented, setComposition can return false and the render view can query the webview to see what the current composition text should be set to in the browser process).
Returning boolean should be enough. This method should only return false when the input method has some composition text but the webkit can't accept it by any reason, then the only thing can be done is to cancel the input method's composition text.

[Tony Chang]

(In reply to comment #10)
> Returning boolean should be enough. This method should only return false when the input method has some composition text but the webkit can't accept it by any reason, then the only thing can be done is to cancel the input method's composition text.

Oh, I remembered why I wanted to return an enum.  Ojan suggested that we try to cancel each letter, rather than the whole composition if the site calls preventDefault().  I'm not sure this is going to work on all platforms, so maybe there should be different return types for canceling the whole composition vs canceling a single character (in which case on OSX, we'll need to reset the composition state in the browser)?

[Tony Chang]

Also, I think a bool is fine for now, we can refactor this later.

[James Su]

(In reply to comment #11)
> (In reply to comment #10)
> > Returning boolean should be enough. This method should only return false when the input method has some composition text but the webkit can't accept it by any reason, then the only thing can be done is to cancel the input method's composition text.
> 
> Oh, I remembered why I wanted to return an enum.  Ojan suggested that we try to cancel each letter, rather than the whole composition if the site calls preventDefault().  I'm not sure this is going to work on all platforms, so maybe there should be different return types for canceling the whole composition vs canceling a single character (in which case on OSX, we'll need to reset the composition state in the browser)?

I don't think it's possible to cancel individual letter of a composition session, because you can't know if the internal state of the input method will be changed by a key event, even the composition text stays unchanged. In another word, there is no way for us to let the input method revert to a previous middle state. The only thing we can do is to cancel the ongoing composition session in whole which will reset the input method to its initial state.

[Tony Chang]

(In reply to comment #13)

> I don't think it's possible to cancel individual letter of a composition session, because you can't know if the internal state of the input method will be changed by a key event, even the composition text stays unchanged. In another word, there is no way for us to let the input method revert to a previous middle state. The only thing we can do is to cancel the ongoing composition session in whole which will reset the input method to its initial state.

Ok, I'm going to use that argument to close out http://crbug.com/9883 :)

[Darin Fisher (:fishd, Google)]

Comment on attachment 58757 [details]
Fix code style.

WebKit/chromium/public/WebTextInputType.h:47
 +      // TODO(suzhe): Add more text input types when necessary, eg. Number,
nit: In WebKit style, use FIXME: instead of TODO(suzhe):

WebKit/chromium/src/WebViewImpl.cpp:1369
 +      // TODO(suzhe): Support more text input types when necessary, eg. Number,
same nit about FIXME: instead of TODO(suzhe):

[James Su]

Created attachment 59008 [details]
Updated patch according to review feedbacks.

[Eric Seidel (no email)]

Comment on attachment 58757 [details]
Fix code style.

Cleared Darin Fisher's review+ from obsolete attachment 58757 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[James Su]

(In reply to comment #17)
> (From update of attachment 58757 [details])
> Cleared Darin Fisher's review+ from obsolete attachment 58757 [details] so that this bug does not appear in http://webkit.org/pending-commit.

What else should I do in order to commit the patch? Seems this bug is still in the list.

[WebKit Commit Bot]

Comment on attachment 59008 [details]
Updated patch according to review feedbacks.

Clearing flags on attachment: 59008

Committed r61484: <http://trac.webkit.org/changeset/61484>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.