[webkit-reviews] review denied: [Bug 64937] Add more text input types for chromium : [Attachment 101591] Fix the style issue

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied Peng Huang
<shawn.p.huang at gmail.com>'s request for review:
Bug 64937: Add more text input types for chromium
https://bugs.webkit.org/show_bug.cgi?id=64937

Attachment 101591: Fix the style issue
https://bugs.webkit.org/attachment.cgi?id=101591&action=review

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=101591&action=review


> Source/WebKit/chromium/public/WebTextInputType.h:47
> +    WebTextInputTypeSearch,

These ones seem a bit analogous to WebTextInputTypePassword, but that one
has a rather verbose comment, and these ones do not.  It seems like replicating

that comment for each of these, and just changing password to email, etc.,
would
be overkill.  Maybe you should re-organize this so that it looks more like so:

  // Input caret is in a specific input field, an input method may be used only
if
  // it's suitable for the specific input field.
  WebTextInputTypePassword,
  WebTextInputTypeEmail,
  WebTextInputTypeNumber,
  WebTextInputTypeTelephone,
  WebTextInputTypeURL,

^^^ no need for new lines between them, but you can still separate
WebTextInputTypeNone and WebTextInputTypeText with new lines because
those are conceptually quite different from the group of specific
input types.

> Source/WebKit/chromium/src/WebViewImpl.cpp:1485
> +	       if (input.isPasswordField())

maybe this should be a method on WebInputElement?

also, when we are inside the WebKit implementation, we ordinarily just
use WebCore directly instead of creating a WebKit API type in order to
use the WebKit API.  the code is often a bit lighterweight when we use
WebCore directly.  have you considered that?