[Webkit-unassigned] [Bug 40608] [chromium]Refactor input method related APIs.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=40608





--- Comment #7 from James Su <suzhe at chromium.org>  2010-06-14 23:44:31 PST ---
(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

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.