[webkit-reviews] review denied: [Bug 65897] Implement <input type=color> UI WebKit chromium part : [Attachment 103723] changed selectedColor to colorSelected

[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 103723: changed selectedColor to colorSelected
https://bugs.webkit.org/attachment.cgi?id=103723&action=review

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


> Source/WebKit/chromium/public/WebColorChooserListener.h:39
> +class WebColorChooserListener {

please add a protected destructor, just like WebFileChooserCompletion.

also, i think it would be helpful to use the same naming convention as
other interfaces.  instead of a Listener, call this a Completion.

 WebColorChooserCompletion

> Source/WebKit/chromium/public/WebColorChooserListener.h:41
> +    // Called when user selects a color. On the Mac this would be called
every

nit: "Called when [the] user selects a color."

I would be careful about documenting platform specific behavior here.  That
can change as new versions of the OS are released.  I wouldn't be surprised
to find Linux desktops behaving like Macs in the future.  Maybe this comment
should just say something more generic, like "Depending on the platform, this
method may either be called multiple times to preview the selected color, or
just once when the user has made their final color choice."

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

nit: we use the convention of prefix-naming event methods either did* or will*,
and
so in this case I think didSelectColor would be best.

actually, to be like WebFileChooserCompletion, I'd call this didChooseColor.

> Source/WebKit/chromium/public/WebViewClient.h:179
>  

nit: please preserve the two new line whitespace gap above section separators.

> Source/WebKit/chromium/public/WebViewClient.h:185
> +    virtual void openColorChooser(WebColor, WebColorChooserListener*) { }

nit: I think you should name the first parameter defaultColor.	That should
make
the code more self-documenting.

nit: call this runColorChooser, just like runFileChooser

> Source/WebKit/chromium/public/WebViewClient.h:187
> +    // This method tries to closes the color chooser interface. The color

nit: "tries to closes" -> "tries to close"

no need to say "interface" here.  it is sufficient to refer to it as a "color
chooser".

> Source/WebKit/chromium/public/WebViewClient.h:188
> +    // chooser would not close if the color chooser's listener is is tied to


nit: "is is"

I'm having a hard time understanding this last sentence.  It seems like this
method can only refer to a color chooser that was created by this WebView.
So, how can "the color chooser's listener" be tied to another tab?  I'm very
confused.

> Source/WebKit/chromium/public/WebViewClient.h:197
>      // Dialogs -------------------------------------------------------------


nit: please preserve the two new line whitespace gap above section separators.