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

[bugzilla-daemon at webkit.org]

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


Darin Fisher (:fishd, Google) <fishd at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #58749|review?                     |review-
               Flag|                            |




--- Comment #3 from Darin Fisher (:fishd, Google) <fishd at chromium.org>  2010-06-14 21:44:57 PST ---
(From update of attachment 58749)
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

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