[webkit-reviews] review denied: [Bug 84351] [Chromium] datalist: Inconsistent behavior of HTMLInputElement::list : [Attachment 138024] Patch

[bugzilla-daemon at webkit.org]

Kent Tamura <tkent at chromium.org> has denied Keishi Hattori
<keishi at webkit.org>'s request for review:
Bug 84351: [Chromium] datalist: Inconsistent behavior of HTMLInputElement::list
https://bugs.webkit.org/show_bug.cgi?id=84351

Attachment 138024: Patch
https://bugs.webkit.org/attachment.cgi?id=138024&action=review

------- Additional Comments from Kent Tamura <tkent at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=138024&action=review


> Source/WebCore/ChangeLog:38
> +	   * rendering/RenderThemeChromiumSkia.h:
> +	   (RenderThemeChromiumSkia):
> +
> +2012-04-19  Keishi Hattori  <keishi at webkit.org>
> +
> +	   [Chromium] datalist: Inconsistent behavior of HTMLInputElement::list

> +	   https://bugs.webkit.org/show_bug.cgi?id=84351

Duplicated ChangeLog entries

> Source/WebCore/html/ColorInputType.cpp:164
> +    Document* document = element()->document();
> +    RefPtr<RenderTheme> theme = document->page() ? document->page()->theme()
: RenderTheme::defaultTheme();
> +    return theme->supportsDataListUI(formControlType());

Repeating this code is not good.
How about adding a static function to InputType and call it from
FooInputType::shouldRespectListAttribute, or implementing this in InputType and
overriding in non-supported types?

> Source/WebCore/rendering/RenderTheme.h:134
> +    // A method asking if the theme is able to show datalist suggestions.

In this case, "theme is able to show ..." is not correct; "The platform is able
to show ..."?
You should explain what is "type" argument.
We should mention what types should be supported as per the standard.

> Source/WebCore/rendering/RenderThemeChromiumMac.h:35
> +    virtual bool supportsDataListUI(const AtomicString& type) const;

should append OVERRIDE.

> Source/WebCore/rendering/RenderThemeChromiumMac.mm:78
> +    return type == InputTypeNames::text() || type ==
InputTypeNames::search() || type == InputTypeNames::url()
> +	   || type == InputTypeNames::telephone() || type ==
InputTypeNames::email() || type == InputTypeNames::number();

We should add a FIXME comment about unsupported types.

> Source/WebCore/rendering/RenderThemeChromiumSkia.cpp:133
> +bool RenderThemeChromiumSkia::supportsDataListUI(const AtomicString& type)
const
> +{
> +    return type == InputTypeNames::text() || type ==
InputTypeNames::search() || type == InputTypeNames::url()
> +	   || type == InputTypeNames::telephone() || type ==
InputTypeNames::email() || type == InputTypeNames::number();
> +}

We shouldn't repeat the same code.
We might want a file for such chromium-common code. 
RenderThemeChromiumCommon.h?

> Source/WebCore/rendering/RenderThemeChromiumSkia.h:54
> +	   virtual bool supportsDataListUI(const AtomicString& type) const;

should append OVERRIDE