Summary: | [chromium]Refactor input method related APIs. | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | James Su <suzhe> | ||||||||||
Component: | WebKit API | Assignee: | Nobody <webkit-unassigned> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | commit-queue, dglazkov, fishd, hbono, jshin, tony, webkit.review.bot | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | All | ||||||||||||
OS: | All | ||||||||||||
Attachments: |
|
Description
James Su
2010-06-14 20:19:40 PDT
Created attachment 58749 [details]
Proposed patch.
Attachment 58749 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/3334147 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
Created attachment 58756 [details]
Updated patch according to review feedbacks.
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.
Created attachment 58757 [details]
Fix code style.
(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 Attachment 58757 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/3277173 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).
(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. (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)? Also, I think a bool is fine for now, we can refactor this later. (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. (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 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):
Created attachment 59008 [details]
Updated patch according to review feedbacks.
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. (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 on attachment 59008 [details] Updated patch according to review feedbacks. Clearing flags on attachment: 59008 Committed r61484: <http://trac.webkit.org/changeset/61484> All reviewed patches have been landed. Closing bug. |