[webkit-reviews] review denied: [Bug 92075] Implement datalist UI for input type color for Chromium : [Attachment 153993] Patch

[bugzilla-daemon at webkit.org]

Kent Tamura <tkent at chromium.org> has denied Keishi Hattori
<keishi at webkit.org>'s request for review:
Bug 92075: Implement datalist UI for input type color for Chromium
https://bugs.webkit.org/show_bug.cgi?id=92075

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

------- Additional Comments from Kent Tamura <tkent at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=153993&action=review


> Source/WebCore/ChangeLog:13
> +	   * Resources/colorPicker.css: Added.

I don't think this should be called 'color picker'.  It's confusing with color
chooser.
Maybe 'color suggestion picker' or something?

> Source/WebCore/ChangeLog:53
> +	   * html/shadow/CalendarPickerElement.cpp:
> +	   (WebCore::CalendarPickerElement::writeDocument): Modified to use
PagePopupUtil.
> +	   * page/PagePopupUtil.cpp: Added.
> +	   (WebCore): Moved helper functions from CalendarPickerElement.cpp.
> +	   (WebCore::PagePopupUtil::addJavaScriptString):
> +	   (WebCore::PagePopupUtil::addProperty):
> +	   * page/PagePopupUtil.h: Copied from
Source/WebCore/html/ColorInputType.h.
> +	   (WebCore):
> +	   (PagePopupUtil):
> +	   (WebCore::PagePopupUtil::addString):

These changes should be done in a separated patch.

> Source/WebKit/chromium/ChangeLog:53
> +	   * src/WebPagePopupImpl.h:
> +	   (WebKit::WebPagePopupImpl::hasSamePopupClient): Returns true if the
page popup client
> +	   is the same.
> +	   * src/WebViewImpl.cpp:
> +	   (WebKit::WebViewImpl::handleMouseDown): We don't want to reopen the
same popup so we
> +	   check hasSamePopupClient().

This should be done in another separated patch.

> Source/WebCore/Resources/colorPicker.css:14
> + *	  * Redistributions of source code must retain the above copyright
> + * notice, this list of conditions and the following disclaimer.
> + *	  * Redistributions in binary form must reproduce the above
> + * copyright notice, this list of conditions and the following disclaimer
> + * in the documentation and/or other materials provided with the
> + * distribution.
> + *	  * Neither the name of Google Inc. nor the names of its

Please use two-clauses version.

> Source/WebCore/Resources/colorPicker.js:14
> + *	  * Redistributions of source code must retain the above copyright
> + * notice, this list of conditions and the following disclaimer.
> + *	  * Redistributions in binary form must reproduce the above
> + * copyright notice, this list of conditions and the following disclaimer
> + * in the documentation and/or other materials provided with the
> + * distribution.
> + *	  * Neither the name of Google Inc. nor the names of its

ditto.

> Source/WebCore/Resources/colorPicker.js:51
> +function getScrollbarWidth() {
> +  if (scrollbarWidth === null) {
> +    var scrollDiv = document.createElement("div");

wrong indentation

> Source/WebCore/Resources/colorPicker.js:101
> +var DEFAULT_COLOR_PALETTE = ["#000000", "#404040", "#808080", "#c0c0c0",
"#ffffff", "#980000",
> +"#ff0000", "#ff9900", "#ffff00", "#00ff00", "#00ffff", "#4a86e8", "#0000ff",
"#9900ff",
> +"#ff00ff"];

The constant name should be CamelCase, right?
Need a comment about why DEFAULT_COLOR_PALETTE is needed.

> Source/WebCore/Resources/colorPicker.js:156
> +var SWATCH_SIZE = 24; // keep in sync with CSS
> +var SWATCHES_PER_ROW = 5;
> +var SWATCHES_MAX_ROWS = 4;

Should be CamelCase.

> Source/WebCore/Resources/colorPicker.js:170
> +    if (this.config.values.length > SWATCHES_PER_ROW * SWATCHES_MAX_ROWS)
> +	 containerWidth += getScrollbarWidth();

wrong indentation.

> Source/WebCore/WebCore.gyp/WebCore.gyp:942
> +	       '--condition=ENABLE(CALENDAR_PICKER)',

should be ENABLE(INPUT_TYPE_COLOR) && ENABLE(DATALIST_ELEMENT)

> Source/WebCore/html/ColorInputType.cpp:225
> +    HTMLDataListElement* dataList =
static_cast<HTMLDataListElement*>(element()->list());

You might want to make HTMLInputElement::dataList() public.

> Source/WebCore/html/ColorInputType.cpp:228
> +	   for (unsigned i = 0; HTMLOptionElement* option =
static_cast<HTMLOptionElement*>(options->item(i)); i++)

We prefer ++i.

> Source/WebCore/page/PagePopupUtil.h:14
> + *	  * Redistributions of source code must retain the above copyright
> + * notice, this list of conditions and the following disclaimer.
> + *	  * Redistributions in binary form must reproduce the above
> + * copyright notice, this list of conditions and the following disclaimer
> + * in the documentation and/or other materials provided with the
> + * distribution.
> + *	  * Neither the name of Google Inc. nor the names of its

Please use two-clauses version.

> Source/WebKit/chromium/src/ChromeClientImpl.cpp:1012
> -    return m_pagePopupDriver->openPagePopup(client, originBoundsInRootView);

> +    PagePopup* pp = m_pagePopupDriver->openPagePopup(client,
originBoundsInRootView);
> +    return pp;

This looks unnecessary.