[webkit-reviews] review denied: [Bug 62619] Implement <input type=color> UI behavior WebCore part : [Attachment 102767] new 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 62619: Implement <input type=color> UI behavior WebCore part
https://bugs.webkit.org/show_bug.cgi?id=62619

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

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


> Source/WebCore/WebCore.xcodeproj/project.pbxproj:19091
>				E1E1BEFF115FF6FB006F52CA /*
WindowsKeyboardCodes.h */,
> +				C3EEE07713E7F9DE005F7460 /* ColorChooser.h */,
> +				C3EEE07813E7F9DE005F7460 /* ColorChooser.cpp
*/,

They should be in alphabetical order.

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:20281
>				41E1B1D10FF5986900576B3B /* AbstractWorker.h in
Headers */,
> +				C3EEE07913E7F9DE005F7460 /* ColorChooser.h in
Headers */,
>				29A8122E0FBB9C1D00510293 /*
AccessibilityARIAGridCell.h in Headers */,

ditto.

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:26071
>				BCAB418113E356E800D8AAF3 /* Region.cpp in
Sources */,
> +				C3EEE07A13E7F9DE005F7460 /* ColorChooser.cpp in
Sources */,

ditto.

> Source/WebCore/css/html.css:640
>      width: 100%;
> -    height: 100%
> +    height: 100%;

This change is unrelated to this bug.

> Source/WebCore/html/ColorInputType.cpp:147
> +    ExceptionCode ec;
> +    setValueAsColor(color, ec);
> +}

We shouldn't ignore the resultant ExceptionCode.
Please see the comment about setValueAsColor() in ColorInputType.h

> Source/WebCore/html/ColorInputType.h:53
> +    virtual void setValueAsColor(const Color&, ExceptionCode&) const;

We should have the default argument for ExceptionCode&.
  virtual void setValueAsColor(const Color&, ExceptionCode& =
ASSERT_NO_EXCEPTION) const;
Then, you can omit the ExceptionCode argument for call sites.

> Source/WebCore/html/ColorInputType.h:60
> +
> +    virtual void colorSelected(const Color&);

Please add a comment that this function is for ColorChooserClient.

> Source/WebCore/html/HTMLInputElement.h:157
> +    Color valueAsColor() const;
> +    void setValueAsColor(const Color&, ExceptionCode&);

Why we need them in HTMLInputElement?

> Source/WebCore/page/Chrome.h:163
> +	   void runOpenColorChooser(ColorChooser*, const Color&);
> +	   void runCloseColorChooser();
> +	   void runSetSelectedColorInColorChooser(const Color&);

I still don't like these function names.  'run' + verb sounds very strange.
I guess "runOpenPanel" means "run an OpenPanel dialog".  So we don't need to
follow it.

> Source/WebCore/platform/ColorChooser.h:60
> +    ColorChooser()
> +    : m_client(0)

Indentation is wrong.