[webkit-reviews] review denied: [Bug 61275] <input type=color> Mac UI appearance : [Attachment 94406] Implements the UI appearance for Mac

[bugzilla-daemon at webkit.org]

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

Attachment 94406: Implements the UI appearance for Mac
https://bugs.webkit.org/attachment.cgi?id=94406&action=review

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

> LayoutTests/fast/forms/color/input-appearance-color.html:7
> +<h2>Different Font Sizes</h2>
> +<input type="color" list value="#FF0000" class="big-font">
> +<input type="color" list value="#FF0000" class="middle-font">
> +<input type="color" list value="#FF0000" class="small-font">

These test make no sense because there are no class definitions for big-font,
middle-font, and small-font.
You should write what appearance is correct.  e.g. The appearance should be
identical for various font sizes.

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:22837
>				E1BE512E0CF6C512002EA959 /* XSLTUnicodeSort.h
in Headers */,
>				977E2E0F12F0FC9C00C13379 /* XSSFilter.h in
Headers */,
>				FD537353137B651800008DCE /* ZeroPole.h in
Headers */,
> +				C3EAEBB6138A2E7A00FB055F /*
ColorSwatchElement.h in Headers */,

Should be sorted.

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:25512
>				E1BE512D0CF6C512002EA959 /* XSLTUnicodeSort.cpp
in Sources */,
>				977E2E0E12F0FC9C00C13379 /* XSSFilter.cpp in
Sources */,
>				FD537352137B651800008DCE /* ZeroPole.cpp in
Sources */,
> +				C3EAEBB7138A2E7A00FB055F /*
ColorSwatchElement.cpp in Sources */,

ditto.

> Source/WebCore/css/html.css:598
> +input[type="color"][list] {
> +    -webkit-appearance: menulist;
> +    width: 88px;
> +    height: 23px;
> +}

Should be wrapped with ENABLE_DATALIST

> Source/WebCore/css/html.css:619
> +input[type="color"][list]::-webkit-color-swatch {
> +    border: 1px solid #000000;
> +    left: 8px;
> +    right: 24px;
> +}

ditto.

> Source/WebCore/html/ColorInputType.cpp:98
> +RenderObject* ColorInputType::createRenderer(RenderArena* arena,
RenderStyle*) const
> +{
> +    return new (arena) RenderBlock(element());
> +}

Do you need to override this?
InputType::createRenderer() is not enough?

> Source/WebCore/html/shadow/ColorSwatchElement.h:44
> +class ColorSwatchElement : public HTMLDivElement {

Will you add a lot of code to ColorSwatchElement and ColorSwatchWrapperElement?

If so, we should use separated source fiels.
If no, we should move ElementWithPseudoId in html/ValidationMessage.cpp to
html/shadow/, and use it.

> Source/WebCore/rendering/RenderThemeMac.mm:1369
> +    HTMLInputElement* inputElement = static_cast<HTMLInputElement*>
(o->node()->parentNode()->parentNode()->shadowHost());

Remove a space between '>' and '('.
This line causes a crash if non-shadow element has the webkit-appearance.  <p
style="-webkit-appearance:color-swatch"></p>
Also, you should use o->node()->shadowAncestorNode().
Don't use one-letter variables such as 'o' 'r'.