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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Dec 14 11:09:40 PST 2011

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

Attachment 118768: new patch

------- 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
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
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>

More information about the webkit-reviews mailing list