Bug 40608 - [chromium]Refactor input method related APIs.
Summary: [chromium]Refactor input method related APIs.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit API (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-06-14 20:19 PDT by James Su
Modified: 2010-06-19 06:32 PDT (History)
7 users (show)

See Also:


Attachments
Proposed patch. (33.81 KB, patch)
2010-06-14 20:41 PDT, James Su
fishd: review-
Details | Formatted Diff | Diff
Updated patch according to review feedbacks. (35.49 KB, patch)
2010-06-14 23:29 PDT, James Su
no flags Details | Formatted Diff | Diff
Fix code style. (35.49 KB, patch)
2010-06-14 23:35 PDT, James Su
no flags Details | Formatted Diff | Diff
Updated patch according to review feedbacks. (35.53 KB, patch)
2010-06-17 10:30 PDT, James Su
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description James Su 2010-06-14 20:19:40 PDT
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
Comment 1 James Su 2010-06-14 20:41:15 PDT
Created attachment 58749 [details]
Proposed patch.
Comment 2 WebKit Review Bot 2010-06-14 21:28:17 PDT
Attachment 58749 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3334147
Comment 3 Darin Fisher (:fishd, Google) 2010-06-14 21:44:57 PDT
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
Comment 4 James Su 2010-06-14 23:29:36 PDT
Created attachment 58756 [details]
Updated patch according to review feedbacks.
Comment 5 WebKit Review Bot 2010-06-14 23:31:33 PDT
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.
Comment 6 James Su 2010-06-14 23:35:52 PDT
Created attachment 58757 [details]
Fix code style.
Comment 7 James Su 2010-06-14 23:44:31 PDT
(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
Comment 8 WebKit Review Bot 2010-06-15 00:33:58 PDT
Attachment 58757 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3277173
Comment 9 Tony Chang 2010-06-15 00:36:46 PDT
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).
Comment 10 James Su 2010-06-15 10:57:25 PDT
(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.
Comment 11 Tony Chang 2010-06-15 16:48:04 PDT
(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)?
Comment 12 Tony Chang 2010-06-15 16:48:27 PDT
Also, I think a bool is fine for now, we can refactor this later.
Comment 13 James Su 2010-06-15 17:04:17 PDT
(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.
Comment 14 Tony Chang 2010-06-15 17:13:21 PDT
(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 :)
Comment 15 Darin Fisher (:fishd, Google) 2010-06-17 10:20:32 PDT
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):
Comment 16 James Su 2010-06-17 10:30:55 PDT
Created attachment 59008 [details]
Updated patch according to review feedbacks.
Comment 17 Eric Seidel (no email) 2010-06-17 15:29:03 PDT
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.
Comment 18 James Su 2010-06-17 16:13:23 PDT
(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.
Comment 19 WebKit Commit Bot 2010-06-19 06:32:45 PDT
Comment on attachment 59008 [details]
Updated patch according to review feedbacks.

Clearing flags on attachment: 59008

Committed r61484: <http://trac.webkit.org/changeset/61484>
Comment 20 WebKit Commit Bot 2010-06-19 06:32:51 PDT
All reviewed patches have been landed.  Closing bug.