[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