[webkit-reviews] review denied: [Bug 44650] Support element() CSS image type : [Attachment 162898] This sets the foundation for -webkit-element by adding it to the grammar, parser, and values

[bugzilla-daemon at webkit.org]

Tony Chang <tony at chromium.org> has denied Anish <bhayani at adobe.com>'s request
for review:
Bug 44650: Support element() CSS image type
https://bugs.webkit.org/show_bug.cgi?id=44650

Attachment 162898: This sets the foundation for -webkit-element by adding it to
the grammar, parser, and values
https://bugs.webkit.org/attachment.cgi?id=162898&action=review

------- Additional Comments from Tony Chang <tony at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=162898&action=review


I just skimmed the patch.

> Source/WebCore/ChangeLog:8
> +	   Updated the grammar to notice IDSEL for term
-webkit-element(#elementid). CSSParser

Please include a link to the spec.

> Source/WebCore/ChangeLog:23
> +	   * css/CSSGrammar.y:
> +	   * css/CSSParser.cpp:
> +	   (WebCore::CSSParser::parseContent):
> +	   (WebCore::CSSParser::parseFillImage):
> +	   (WebCore):
> +	   (WebCore::CSSParser::parseWebKitElement):
> +	   * css/CSSParser.h:

Please describe your changes in this section of the ChangeLog.

> Source/WTF/wtf/Platform.h:1054
> +#if PLATFORM(MAC) || PLATFORM(IOS) || PLATFORM(QT) || (PLATFORM(WIN) &&
!OS(WINCE) && !PLATFORM(WIN_CAIRO))
> +#define ENABLE_CSS_WEBKIT_ELEMENT 1

How did you pick which platforms to enable this on?  I suspect everyone will
want it disabled until it's a bit more complete.
Also, if I can bikeshed over the name, I would call it CSS_ELEMENT.

> Source/WebCore/css/CSSGrammar.y:1461
> +  | IDSEL maybe_space {
> +#if ENABLE(CSS_WEBKIT_ELEMENT)
> +	 $$.id = 0;
> +	 $$.unit = CSSPrimitiveValue::CSS_WEBKIT_ELEM;

Can you explain this change to the grammar? Would be good to note in the
ChangeLog.

> Source/WebCore/css/CSSParser.cpp:7253
> +    if (arg->unit != CSSPrimitiveValue::CSS_WEBKIT_ELEM && arg->unit !=
CSSPrimitiveValue::CSS_PARSER_HEXCOLOR)

Why CSS_PARSER_HEXCOLOR?

> Source/WebCore/css/CSSPrimitiveValue.cpp:1144
> +	   case CSS_WEBKIT_ELEM:
> +	       text = "#" + String(m_value.string);

Shouldn't this include the -webkit-element()?

> Source/WebCore/css/CSSPrimitiveValue.h:137
> +	   CSS_WEBKIT_ELEM = 116,

We normally avoid abbreviations. I would name this CSS_ELEMENT or
CSS_ELEMENT_FUNCTION.

> Source/WebCore/css/CSSPrimitiveValue.h:189
> +    bool isWebKitElement() const { return primitiveType() ==
CSS_WEBKIT_ELEM; }

Nit: isElement.  The reason for dropping WebKit is that at some point we would
drop the vendor prefix and it would just be element.

> Source/WebCore/css/WebKitCSSElementValue.cpp:42
> +    {
> +    }

Please left align.

> Source/WebCore/css/WebKitCSSElementValue.cpp:47
> +    {
> +    }

Please left align.

> Source/WebCore/css/WebKitCSSElementValue.cpp:52
> +String WebKitCSSElementValue::customCssText() const
> +    {
> +	   return "-webkit-element(" + CSSValueList::customCssText() + ")";
> +    }

Remove extra indention.

> Source/WebCore/css/WebKitCSSElementValue.cpp:57
> +PassRefPtr<WebKitCSSElementValue> WebKitCSSElementValue::cloneForCSSOM()
const
> +    {
> +	   return adoptRef(new WebKitCSSElementValue(*this));
> +    }

Ditto.

> Source/WebCore/css/WebKitCSSElementValue.cpp:60
> +void WebKitCSSElementValue::reportDescendantMemoryUsage(MemoryObjectInfo*
memoryObjectInfo) const
> +    {

Ditto.

> Source/WebCore/css/WebKitCSSElementValue.h:45
> +    public:
> +	   static PassRefPtr<WebKitCSSElementValue> create()
> +	   {
> +	       return adoptRef(new WebKitCSSElementValue());
> +	   }

Indent is wrong. Please see the coding style guide:
http://www.webkit.org/coding/coding-style.html

> Source/WebCore/css/WebKitCSSElementValue.h:55
> +	   WebKitCSSElementValue(const WebKitCSSElementValue& cloneFrom);

Nit: explicit

> LayoutTests/ChangeLog:22
> +	   * fast/css/webkit-element-parsing-expected.txt: Added.
> +	   * fast/css/webkit-element-parsing-invalid-expected.txt: Added.
> +	   * fast/css/webkit-element-parsing-invalid.html: Added.
> +	   * fast/css/webkit-element-parsing.html: Added.

Nit: I would put these in a subdirectory so it's easy for ports that disable to
feature to skip the whole directory.
Speaking of which, since you didn't enable on Chromium, I think you need to
update platform/chromium/TestExpectations

> LayoutTests/fast/css/script-tests/webkit-element-parsing-invalid.js:3
> +description("Test the parsing of the -webkit-element function.");
> +
> +// These have to be global for the test helpers to see them.

You could inline the script-tests .js files into the html file.  There's no
real benefit to having separate files for them.

> LayoutTests/fast/css/script-tests/webkit-element-parsing.js:54
> +		   "#elementA");

Shouldn't the expected text be -webkit-element(#elementA)?