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

[bugzilla-daemon at webkit.org]

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

------- Additional Comments from Sam Weinig <sam at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=206514&action=review


> Source/WebKit2/UIProcess/WebPageProxy.cpp:582
> +	   m_colorChooser->endChooser();
>	   m_colorChooser->invalidate();

Should invalidate just implicitly endChooser()? If not, why not?

> Source/WebKit2/UIProcess/WebPageProxy.cpp:2974
>  #if ENABLE(INPUT_TYPE_COLOR)
>  void WebPageProxy::showColorChooser(const WebCore::Color& initialColor,
const IntRect& elementRect)
>  {
> +#if PLATFORM(MAC)
> +    m_colorChooser = m_pageClient->createColorChooserProxy(this,
initialColor, elementRect);
> +#else
>      ASSERT(!m_colorChooser);
>  

Why is this mac specific?  We should really try to keep the platform
differences to a minimum at this layer.

> Source/WebKit2/UIProcess/mac/WebColorChooserProxyMac.h:86
> +// A Listener class to act as a event target for NSColorPanel and send
> +// the results to the C++ class, WebColorChooserProxyMac.
> + at interface WKColorPanelMac : NSObject<NSWindowDelegate> {
> +    
> + at private
> +    BOOL _lastChangedByUser;
> +    WebKit::WebColorChooserProxyMac* _chooser;
> +}
> +
> +- (id)initWithChooser:(WebKit::WebColorChooserProxyMac*)chooser
withColor:(NSColor *)color;
> +
> +- (void)didChooseColor:(NSColorPanel *)panel;
> +
> +// Sets color to the NSColorPanel as a non user change.
> +- (void)setColor:(NSColor *)color;
> +
> + at end
> +

Does this need to be in the header?

> Source/WebKit2/UIProcess/mac/WebColorChooserProxyMac.mm:52
> +WebColorChooserProxyMac::~WebColorChooserProxyMac()
> +{
> +    if (m_panel)
> +	   endChooser();
> +}

The destructor should come after the constructor.

> Source/WebKit2/UIProcess/mac/WebColorChooserProxyMac.mm:58
>
+WebColorChooserProxyMac::WebColorChooserProxyMac(WebColorChooserProxy::Client*
client, const WebCore::Color& initialColor)
> +    : WebColorChooserProxy(client)
> +{
> +    m_panel = adoptNS([[WKColorPanelMac alloc] initWithChooser:this
withColor:nsColor(initialColor)]);
> +}

can the initialization of m_panel be in the initializer list?

> Source/WebKit2/UIProcess/mac/WebColorChooserProxyMac.mm:63
> +    if (m_panel)
> +	   m_panel = nullptr;

This branch seems unnecessary.

> Source/WebKit2/UIProcess/mac/WebColorChooserProxyMac.mm:91
> +    if ((self = [super init])) {
> +	   _chooser = chooser;

We traditionally use the early return form of this.

> Source/WebKit2/WebKit2.xcodeproj/project.pbxproj:2506
> +		BCBD38FA125BAB9A00D2C29F /* WebPageProxy.messages.in */ = {isa
= PBXFileReference; fileEncoding = 4; lastKnownFileType = text; lineEnding = 0;
path = WebPageProxy.messages.in; sourceTree = "<group>"; };
> +		BCBD3912125BB1A800D2C29F /* WebPageProxyMessageReceiver.cpp */
= {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType =
sourcecode.cpp.cpp; lineEnding = 0; path = WebPageProxyMessageReceiver.cpp;
sourceTree = "<group>"; xcLanguageSpecificationIdentifier = xcode.lang.cpp; };
> +		BCBD3913125BB1A800D2C29F /* WebPageProxyMessages.h */ = {isa =
PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h;
lineEnding = 0; path = WebPageProxyMessages.h; sourceTree = "<group>";
xcLanguageSpecificationIdentifier = xcode.lang.objcpp; };

These seem like unrelated changes.

> Source/WebKit2/WebProcess/WebCoreSupport/WebChromeClient.cpp:-665
> -    if (m_page->activeColorChooser())
> -	   return nullptr;

Are you sure others are not relying on this?