Bug 71644 – Change ColorChooser from singleton to ordinary object

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Keishi Hattori]

Changing WebCore::ColorChooser from a singleton to an ordinary object can broaden how browsers implement the color chooser interface.

[Keishi Hattori]

Created attachment 113817 [details]
Patch

[Kent Tamura]

Comment on attachment 113817 [details]
Patch

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.

[Keishi Hattori]

Created attachment 113825 [details]
Patch

[Keishi Hattori]

(In reply to comment #2)
> 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>.

I removed PassRefPtr everywhere except ColorChooser::create

> Wrong indentation.

Done.

[Kent Tamura]

Comment on attachment 113825 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=113825&action=review

> Source/WebCore/ChangeLog:30
> +        * platform/ColorChooser.cpp: Styled after FileChooser

What does this mean?

> Source/WebCore/html/ColorInputType.h:61
>      virtual void didChooseColor(const Color&);
> -    virtual bool isColorInputType() const;
> +    virtual void didCleanup();

nit: We had better add OVERRIDE.

> LayoutTests/fast/forms/color/input-color-onchange-event.html:29
> +eventSender.mouseMoveTo(10, 10);
> +eventSender.mouseDown();
> +eventSender.mouseUp();

We had better show a manual testing instruction if there is no eventSender.

[Keishi Hattori]

(In reply to comment #5)
> (From update of attachment 113825 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=113825&action=review
> 
> > Source/WebCore/ChangeLog:30
> > +        * platform/ColorChooser.cpp: Styled after FileChooser
> 
> What does this mean?
I was trying to say that I named the methods similar to FileChooser. It was unnecessary, I removed this.

> > Source/WebCore/html/ColorInputType.h:61
> >      virtual void didChooseColor(const Color&);
> > -    virtual bool isColorInputType() const;
> > +    virtual void didCleanup();
> 
> nit: We had better add OVERRIDE.
Done.

> > LayoutTests/fast/forms/color/input-color-onchange-event.html:29
> > +eventSender.mouseMoveTo(10, 10);
> > +eventSender.mouseDown();
> > +eventSender.mouseUp();
> 
> We had better show a manual testing instruction if there is no eventSender.
Done.

[Keishi Hattori]

Created attachment 113831 [details]
Patch

[WebKit Review Bot]

Comment on attachment 113831 [details]
Patch

Rejecting attachment 113831 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1

NOBODY (OOPS!) found in /mnt/git/webkit-commit-queue/LayoutTests/ChangeLog does not appear to be a valid reviewer according to committers.py.
ERROR: /mnt/git/webkit-commit-queue/LayoutTests/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://queues.webkit.org/results/10338614

[Keishi Hattori]

Created attachment 113839 [details]
Patch

[WebKit Review Bot]

Comment on attachment 113839 [details]
Patch

Clearing flags on attachment: 113839

Committed r99403: <http://trac.webkit.org/changeset/99403>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.