[webkit-reviews] review denied: [Bug 71644] Change ColorChooser from singleton to ordinary object : [Attachment 113817] Patch

[bugzilla-daemon at webkit.org]

Kent Tamura <tkent at chromium.org> has denied Keishi Hattori
<keishi at webkit.org>'s request for review:
Bug 71644: Change ColorChooser from singleton to ordinary object
https://bugs.webkit.org/show_bug.cgi?id=71644

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

------- Additional Comments from Kent Tamura <tkent at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=113817&action=review


> Source/WebCore/page/Chrome.h:161
> +	   void openColorChooser(PassRefPtr<ColorChooser>, const Color&);
> +	   void cleanupColorChooser(PassRefPtr<ColorChooser>);
> +	   void setSelectedColorInColorChooser(PassRefPtr<ColorChooser>, const
Color&);

Passing PassRefPtr<> as a function argument is wrong in many cases.
If an implementation of these function want to have an ownership of this
ColorChooser, these function should have a ColorChooser* argument, and the
implementation should assign the raw pointer to RefPtr<ColorChooser>.

RefPtr<ColorChooser> m_colorChooser;
ChromeClientFoo::openColorChooser(ColorChooser* chooser, const Color&) {
    m_colorChooser = chooser;
    ...
}

> Source/WebCore/platform/ColorChooser.cpp:56
> +	   m_chooser->disconnectClient();
> +	 m_chooser.clear();

Wrong indentation.

> Source/WebCore/platform/ColorChooser.h:48
> +    PassRefPtr<ColorChooser> chooser() { return m_chooser; }

Returning PassRefPtr<> from non-factory method is wrong in many cases.
ColorChooser* is enough in this case.

You have code like the following in this patch:

  if (chrome && chooser())
       chrome->setSelectedColorInColorChooser(chooser(), valueAsColor());

This calls ref() twice and deref() twice, and they are unnecessary at all.