[webkit-reviews] review denied: [Bug 65897] Implement <input type=color> UI WebKit chromium part : [Attachment 113841] Patch

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied Keishi Hattori
<keishi at webkit.org>'s request for review:
Bug 65897: Implement <input type=color> UI WebKit chromium part
https://bugs.webkit.org/show_bug.cgi?id=65897

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

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=113841&action=review


> Source/WebKit/chromium/public/WebColorChooserClient.h:51
> +    virtual void didCleanup() = 0;

nit: I think using a term like "Close" here might be a bit clearer.  It seems
like
there are two events that you can receive from the color chooser.  Either, the
color was changed or the dialog was closed.

> Source/WebKit/chromium/src/ChromeClientImpl.cpp:673
> +    client->colorChooser()->setClient(colorChooserClient);

It seems like it would be better to create a new WebColorChooser for use by
WebKit.
It may be the case that you only ever want to make one color chooser visible to
the
user at a time, but by allocating a separate WebColorChooser for each
openColorChooser
call, you simplify some of the state management.  For example, your checks for
"colorChooser == client->colorChooser()->client()->colorChooser()" go away. 
Instead,
when WebKit wants to close the WebColorChooser instance, it just deletes it. 
When
WebKit wants to change the color shown in its WebColorChooser instance, it just
calls
a method on it.

So, I'd go with:

  class WebViewClient {
    ...
    virtual WebColorChooser* createColorChooser(WebColor initialColor,
WebColorChooserClient*) { return 0; }
    ...
  };