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

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

> Source/WebCore/html/ColorInputType.cpp:116
> +    element()->setValue(color.serialized(), true);

Because this line dispatches a 'change' event and a JavaScript code can run,
it's possible that the JavaScript code change the input type and this
ColorInputType instance might be deleted. We must not access 'this' after this
line.

> Source/WebCore/html/ColorInputType.cpp:120
> +	  
element()->document()->page()->chrome()->runUpdateColorPanel(Color(element()->v
alue()));

Why do we need to call runUpdateColorPanel() in colorChange()? colorChanged()
is called from the color panel, isn't it?

> Source/WebCore/page/Chrome.cpp:461
> +void Chrome::runUpdateColorPanel(const Color color)
> +{
> +    m_client->runUpdateColorPanel(color);

The name 'runUpdateColorPanel' sounds curious.
'updateSelectedColorOfColorPanel'?

> Source/WebCore/page/ChromeClient.h:228
> +	   virtual void runColorPanel() = 0;

You need to add a comment of the function behavior.
What should we do if runColorPanel() is called twice?
When should we close the color panel opened by runColorPanel()?
How to obtain ColorChooser instance to which the color panel send the selected
color?