[webkit-reviews] review denied: [Bug 62619] Implement <input type=color> UI behavior WebCore part : [Attachment 102900] fixed 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 102900: fixed patch
https://bugs.webkit.org/attachment.cgi?id=102900&action=review

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


> Source/WebCore/html/ColorInputType.cpp:128
> +void ColorInputType::willRemove()
> +{
> +    if (ColorChooser::chooser()->client() == this) {
> +	   if (Chrome* chrome = this->chrome())
> +	       chrome->closeColorChooser();

I think Node::willRemove() is wrong for this task.
We should close the chooser when the element disappears.  e.g. display:none;

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

I realized that valueAsColor() and setValueAsColor() were used only by
ColorInputType.
So,
 - we dont need InputType::valueAsColor() and setValueAsColor().
 - They should not be virtual.
 - They should be private.
 - setValueAsColor() doesn't need the ExceptionCode& argument at all.