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

[bugzilla-daemon at webkit.org]

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 207257: Patch
https://bugs.webkit.org/attachment.cgi?id=207257&action=review

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


> Source/WebCore/ChangeLog:21
> +	   * 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): Always
returns true, needed to 
> +	     detach color chooser when caching a page.
> +
> +	   * html/ColorInputType.h:

line #20 is unnecessary - ColorInputType.h is logically connected to the .cpp

> Source/WebCore/ChangeLog:28
> +	   * html/HTMLInputElement.cpp:
> +	   (WebCore::HTMLInputElement::documentDidResumeFromPageCache): For
<input type='color'>, 
> +	     don't reset the element.
> +	   (WebCore::HTMLInputElement::documentWillSuspendForPageCache): For
<input type='color'>, call detach().
> +
> +	   * html/HTMLInputElement.h:

Same here...  and same for all Changelogs

> Source/WebCore/html/HTMLInputElement.cpp:1978
> +    if (!isColorControl())
> +	   return;
> +    static_cast<InputType*>(m_inputType.get())->detach();

This should be a cast to ColorInputType*

> Source/WebKit2/ChangeLog:40
> +	   * UIProcess/WebPageProxy.cpp:
> +	   (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. Reattaches 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.
> +
> +	   * UIProcess/WebPageProxy.h:
> +
> +	   * UIProcess/WebPageProxy.messages.in: Updated to include the
definition of ReattachColorChooser.

And in WK2, we often even leave the .cpp, .h, and .messages.in all paragraphed
together (no empty line between them)

> Source/WebKit2/UIProcess/WebPageProxy.cpp:2933
> +#if PLATFORM(MAC)
> +    if (!m_colorChooser)
> +	   m_colorChooser = m_pageClient->createColorChooserProxy(this,
initialColor, elementRect);
> +    else {
> +	   // Handles the case where a color picker already exists for the web
process.
> +	   reattachColorChooser(initialColor);
> +    }
> +#else

In general, I'm confused as to why this method has a completely different code
path for Mac than the code path that already exists in this method.

I don't seen any particular reason why the #if PLATFORM(MAC) code is inherently
incompatible with how you want Mac to behave.

Also, this file is not the correct place for a PLATFORM(MAC) ifdef.

> Source/WebKit2/UIProcess/WebPageProxy.cpp:2999
> +#if !PLATFORM(MAC)
>      m_colorPickerResultListener->invalidate();
>      m_colorPickerResultListener = nullptr;
>  
>      m_uiClient.hideColorPicker(this);
> +#endif // PLATFORM(MAC)

Also, this file is not the correct place for a !PLATFORM(MAC) ifdef

> Source/WebKit2/UIProcess/mac/WebColorChooserProxyMac.h:38
> +
> +#if USE(APPKIT)
> +

Wut?

> Source/WebKit2/UIProcess/mac/WebColorChooserProxyMac.h:49
> +namespace WebCore {
> +
> +class Color;
> +
> +}

l46 and l48 are unnecessary

> Source/WebKit2/UIProcess/mac/WebColorChooserProxyMac.mm:38
> +
> +#if USE(APPKIT)
> +

Wut.

> Source/WebKit2/UIProcess/mac/WebColorChooserProxyMac.mm:87
> +    // Handle the case where the color picker has been closed for an <input
type='color'> element.

Not sure this comment is useful.

> Source/WebKit2/UIProcess/mac/WebColorChooserProxyMac.mm:100
> +- (id)initWithChooser:(WebKit::WebColorChooserProxyMac*)chooser
withColor:(NSColor *)color

Instead of the WebKit::, please do a `using namespace WebKit` above the
@implmentation.

> Source/WebKit2/WebProcess/WebCoreSupport/WebChromeClient.cpp:669
>  PassOwnPtr<ColorChooser>
WebChromeClient::createColorChooser(ColorChooserClient* client, const Color&
initialColor)
>  {
> +#if !PLATFORM(MAC)
>      if (m_page->activeColorChooser())
>	   return nullptr;
> -
> +#endif
>      return adoptPtr(new WebColorChooser(m_page, client, initialColor));
>  }

It seems super peculiar to me that previously this method *always* returned a
ColorChooser, and yet your code sometimes does not.
That can't be right, can it?

Why do other implementations always want a fresh chooser, but this
implementation returns null if one already exists?

Also, this file is not the correct place for a PLATFORM(MAC) ifdef.

> Source/WebKit2/WebProcess/WebCoreSupport/WebColorChooser.cpp:86
>  void WebColorChooser::setSelectedColor(const Color& color)
>  {
>      if (!m_page)
>	   return;
> +    
> +    if (m_page->activeColorChooser() != this)
> +	   return;

It seems wrong that the inactive chooser doesn't get its color set as
requested.

If a different color chooser is now the active one, shouldn't this one's color
still be updated?

> Source/WebKit/mac/WebCoreSupport/WebChromeClient.mm:730
> +PassOwnPtr<ColorChooser>
WebChromeClient::createColorChooser(ColorChooserClient* client, const Color&
initialColor)
> +{
> +    notImplemented();
> +    return nullptr;
> +}

This seems wrong,