[webkit-reviews] review denied: [Bug 40608] [chromium]Refactor input method related APIs. : [Attachment 58749] Proposed patch.

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied James Su
<suzhe at chromium.org>'s request for review:
Bug 40608: [chromium]Refactor input method related APIs.
https://bugs.webkit.org/show_bug.cgi?id=40608

Attachment 58749: Proposed patch.
https://bugs.webkit.org/attachment.cgi?id=58749&action=review

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
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