Bug 71644

Created attachment 113817 [details] Patch

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.

Created attachment 113825 [details] Patch

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

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.

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

Created attachment 113831 [details] Patch

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

Created attachment 113839 [details] Patch

Comment on attachment 113839 [details] Patch Clearing flags on attachment: 113839 Committed r99403: <http://trac.webkit.org/changeset/99403>

All reviewed patches have been landed. Closing bug.