[webkit-reviews] review denied: [Bug 61276] <input type=color> Mac UI behaviour : [Attachment 207142] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Jul 19 15:07:51 PDT 2013


Brady Eidson <beidson at apple.com> has denied Ruth Fong <ruthiecftg at gmail.com>'s
request for review:
Bug 61276: <input type=color> Mac UI behaviour
https://bugs.webkit.org/show_bug.cgi?id=61276

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

------- Additional Comments from Brady Eidson <beidson at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=207142&action=review


This review is ChangeLog Nazi-ism only, will continue reviewing code later...

> Source/JavaScriptCore/ChangeLog:5
> +	   https://bugs.webkit.org/show_bug.cgi?id=61276
> +	   <rdar://problem/10269922>

These can be on the same line (and repeat for each ChangeLog)

> Source/WebCore/ChangeLog:10
> +	   Tests to be added in a later patch.
> +	   No new tests (OOPS!).

Can't leave "OOPS!" in the log, the patch will be rejected.  Just delete line
10.

> Source/WebCore/ChangeLog:24
> +	   * html/ColorInputType.cpp:
> +	   (WebCore::ColorInputType::handleDOMActivateEvent): Updated to
reattach
> +	   the color picker if a color picker has already been shown for an
element.
> +
> +	   (WebCore::ColorInputType::shouldResetOnDocumentActivation): Added.
> +	   Returns true; needed to detach color chooser when caching a page.

I've never seen the style of paragraphing different functions within the same
file, and I'm not a fan of it.	I'd remove the empty line #22

> Source/WebCore/ChangeLog:27
> +	   * html/ColorInputType.h:
> +	   * html/HTMLInputElement.cpp:

These two lines, however, should be paragraphed!

> Source/WebCore/ChangeLog:32
> +	   (WebCore::HTMLInputElement::documentDidResumeFromPageCache):
Updated.
> +	   For <input type='color'>, don't reset the element.
> +
> +	   (WebCore::HTMLInputElement::documentWillSuspendForPageCache): Added.

> +	   For <input type='color'>, call detach().

Same here - line #30 grumble!

> Source/WebCore/ChangeLog:35
> +	   * html/HTMLInputElement.h:
> +	   * platform/ColorChooser.h:

And same here, these two should be separated!

> Source/WebKit2/ChangeLog:8
> +	   Implemented <input type='color'> on Mac using the
> +	   native color picker. 

No reason to split this up in multiple lines.

> Source/WebKit2/ChangeLog:40
> +	   (WebKit::WebPageProxy::showColorChooser): Added mac-specific
handling
> +	   that allows showColorChooser to be called when first clicking on an
> +	   <input type='color'> element. (Previously, showColorChooser assumed
> +	   that m_colorChooser, which is owned by the web page, is null; 
> +	   disallowing multiple <input type='color'> elements on the same web
page.)
> +
> +	   (WebKit::WebPageProxy::reattachColorChooser): Added.
> +	   Reattached an <input type='color'> element (represented by
m_colorChooser)
> +	   to the color picker.
> +
> +	   (WebKit::WebPageProxy::didEndColorChooser): Added preprocessing for 

> +	   non-Mac platforms that have more color UI objects to clean up.

You can guess my comment here.

> Source/WebKit2/ChangeLog:56
> +	   (WebKit::WebColorChooserProxyMac::reattachColorChooser):
> +	   WebColorChooserProxyMac contains a reference to a WKColorPanelMac
object
> +	   and is responsible for communicating between the WebProcess and
> +	   the color picker UI.

In some descriptions you start on the same line as the method name then wrap. 
In others the entire description is newlined.  I can't tell why the difference.


More information about the webkit-reviews mailing list