[webkit-reviews] review granted: [Bug 119356] [Forms: color] <input type='color'> popover color well implementation : [Attachment 207957] Patch

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has granted Ruth Fong
<ruthiecftg at gmail.com>'s request for review:
Bug 119356: [Forms: color] <input type='color'> popover color well
implementation
https://bugs.webkit.org/show_bug.cgi?id=119356

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

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=207957&action=review


> Source/WebKit2/Configurations/FeatureDefines.xcconfig:102
> +ENABLE_INPUT_TYPE_COLOR_POPOVER = ENABLE_INPUT_TYPE_COLOR_POPOVER;

You should probably update all 4 FeatureDefines.xcconfig to avoid future
confusions.

> Source/WebKit2/UIProcess/WebPageProxy.cpp:1009
> +    // Close popover color well when resizing window.

This comment does not add value.

> Source/WebKit2/UIProcess/WebPageProxy.cpp:2938
> +    // A new popover color well needs to be created (and the previous one
destroon each activation of a color element.

The comment should be in the #ifdef.

destroon? :)

Aaaaaarg, an unclosed parenthesis.

> Source/WebKit2/UIProcess/WebPageProxy.cpp:2940
> +    m_colorPicker = nil;

nil -> 0

> Source/WebKit2/UIProcess/mac/WebColorPickerMac.h:74
> +    RetainPtr<NSObject<WKColorPickerUIMac> > m_colorPickerUI;

You can remove the space between the two '>' (Yay C++ 11).

> Source/WebKit2/UIProcess/mac/WebColorPickerMac.mm:106
> +    m_colorPickerUI = nullptr;

nullptr -> nil?

> Source/WebKit2/UIProcess/mac/WebColorPickerMac.mm:150
> +	       return self;

Indentation.

> Source/WebKit2/UIProcess/mac/WebColorPickerMac.mm:200
> +	   _lastChangedByUser = YES;
> +	       return;
> +	   }

Indentation.