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

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


I'm going to make the radical suggestion that given this new architecture, you
could probably drop the word "Session" from everything, and it would all still
make sense.  You might need to change "endSession" to "endChooser"

> Source/WebCore/html/ColorInputType.cpp:130
> +	 session()->setSelectedColor(valueAsColor());

nit: indentation

> Source/WebCore/html/ColorInputType.cpp:150
> +void ColorInputType::newColorChooserSession(const Color& initialColor)

since this method returns early if there is already a session, perhaps it's
name
should have the IfNecessary suffix?  also, I think 'create' would be a more
canonical prefix:  createColorChooserSessionIfNecessary

> Source/WebCore/loader/EmptyClients.h:195
> +    ColorChooserSession*
createColorChooserSession(ColorChooserSessionClient*, const Color&) { return 0;
}

perhaps this should return PassOwnPtr?

> Source/WebCore/platform/ColorChooserSessionClient.h:18
> +    ColorChooserSession* session() { return m_session.get(); }

Why does the ColorChooserSession need to be stored on the *Client?  That is a
very
uncommon design.  It seems like ColorInputType should have a m_session instead.


> Source/WebCore/platform/ColorChooserSessionClient.h:19
> +    void setSession(ColorChooserSession* session) { m_session =
adoptPtr(session); }

nit: parameter should be PassOwnPtr<ColorChooserSession>