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

[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 108838: rebased
https://bugs.webkit.org/attachment.cgi?id=108838&action=review

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


> Source/WebKit/chromium/public/WebColorChooserCompletion.h:44
> +    virtual void didChooseColor(WebColor) = 0;

How do we know when we can delete the WebColorChooserCompletion instance?
In the case of WebFileChooserCompletion, we know that we can delete the
instance when didChooseFile is called because it will only be called once.
Since you need to support many calls to didChooseColor, it seems like you
need a finalization method.

You seem to be getting around this by making the WebColorChooserCompletion
be allocated as a singleton.

> Source/WebKit/chromium/public/WebViewClient.h:188
> +    // This method does cleanup on the color chooser interface.

I think it would be much cleaner to define a WebColorChooser interface, and on
that interface you could put methods like "close()" and "setSelectedColor()".

I think you should hide the detail that Chrome will only show one color chooser

globally for all tabs at any given time.  The API here should not mandate such
an implementation.

How about a design like this:

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

  class WebColorChooser {
  public:
      virtual ~WebColorChooser() {}  // Destroys the color chooser.
      virtual void setSelectedColor(WebColor) = 0;  // Updates the color shown
in the chooser.
  };

  class WebColorChooserClient {
  public:
      virtual void didChooseColor(WebColor) = 0;  // Can be called multiple
times to preview the chosen color.  Can be reverted via didCancel.
      virtual void didAccept() = 0;  // Done choosing color, and user accepts
chosen color.
      virtual void didCancel() = 0;  // Done choosing color, and user wants to
revert back to the original color.
  protected:
      ~WebColorChooserClient() {}
  };

In this design, the WebColorChooser should be destroyed by the client once
didAccept or didCancel is called,
and at that point it is safe to destroy the WebColorChooserClient instance.  If
setSelectedColor is called
after didAccept/didCancel, then it will be ignored.

An alternative design might add methods on WebColorChooser to show and hide the
dialog, and then the Client
interface might have didShow and didHide methods.  It might also need a
didRevert method to indicate when the
previewed color choice should be reverted.