[webkit-reviews] review denied: [Bug 92075] Implement datalist UI for input type color for Chromium : [Attachment 154834] 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 92075: Implement datalist UI for input type color for Chromium
https://bugs.webkit.org/show_bug.cgi?id=92075

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

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


> Source/WebKit/chromium/WebKit.gyp:492
>		   'src/WebCache.cpp',
> -		   'src/WebColorChooserClientImpl.cpp',
> -		   'src/WebColorChooserClientImpl.h',
> +		   'src/ColorChooserUIController.cpp',
> +		   'src/ColorChooserUIController.h',
>		   'src/WebColorName.cpp',

Should be sorted.

> Source/WebKit/chromium/src/ChromeClientImpl.cpp:699
> +    ColorChooserUIController* colorChooserUI = new
ColorChooserUIController(this, chooserClient);
> +    return adoptPtr(colorChooserUI);

The local variable 'colorChooserUI' is unnecessary.

> Source/WebKit/chromium/src/ChromeClientImpl.h:62
> +#if ENABLE(INPUT_TYPE_COLOR)
> +class WebColorChooser;
> +class WebColorChooserClient;
> +#endif

You don't need #if - #endif for class name declaration.

> Source/WebKit/chromium/src/ColorChooserUIController.cpp:48
> +enum ColorPickerPopupAction {
> +    ChooseOtherColorAction = -2,
> +    CancelAction = -1,
> +    SetValueAction = 0
> +};

Need a comment about synchronization with colorSuggestionPicker.js.

We usually use prefixes for enum symbols, not suffix.
  ColorPickerPopupActoinChooseOtherColor
  ColorPickerPopupActoinCancelAction
  ColorPickerPopupActoinSetValueAction

> Source/WebKit/chromium/src/ColorChooserUIController.cpp:69
> +    WebColor webColor = static_cast<WebColor>(color.rgb());
> +    m_chooser->setSelectedColor(webColor);

The local variable 'webColor' is unnecessary.

> Source/WebKit/chromium/src/ColorChooserUIController.h:45
> +class WebColorChooser;
> +class ChromeClientImpl;

should be sorted.

> Source/WebKit/chromium/src/ColorChooserUIController.h:56
> +    virtual void setSelectedColor(const WebCore::Color&) OVERRIDE;
> +    virtual void endChooser() OVERRIDE;
> +
> +    virtual void didChooseColor(const WebColor&) OVERRIDE;
> +    virtual void didEndChooser() OVERRIDE;

Would you add comments like "// *** functions:" like "PagePopupClient
functions:" below?

> Source/WebKit/chromium/src/ColorChooserUIController.h:64
> +    void closePopup();

This should be private?