[webkit-reviews] review denied: [Bug 44650] Support element() CSS image type : [Attachment 163229] CSS Element grammar and parsing patch. Not full implementation.

[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 163229: CSS Element grammar and parsing patch. Not full
implementation.
https://bugs.webkit.org/attachment.cgi?id=163229&action=review

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


Please send an email to webkit-dev explaining that you're going to be working
on this feature.  See http://trac.webkit.org/wiki/AddingFeatures or you can use
a previous email as a template (like the email about CSS Device Adaptation).

> Source/WTF/ChangeLog:8
> +	   Reviewed by Tony Chang.

You are only supposed to fill this in after you get an r+. Most of the time,
webkit-patch or bugzilla will do it for you.  Please restore this to Reviewed
by NOBODY (OOPS!).

> Source/WebCore/ChangeLog:6
> +	   Reviewed by Tony Chang.

Please restore this to Reviewed by NOBODY (OOPS!).

> Source/WTF/wtf/Platform.h:1055
> +#if PLATFORM(MAC)
> +#define ENABLE_CSS_ELEMENT 1
> +#endif

It's more common for new features to be disabled by default on all platforms
until it is useful.  You can enable it during development by making sure that
build-webkit --enable-css-element works. See Scripts/webkitperl/FeatureList.pm.


> Source/WebCore/css/CSSElementValue.cpp:40
> +CSSElementValue::CSSElementValue()
> +    : CSSValueList(CSSElementClass, SpaceSeparator)

Why does this inherit from CSSValueList? It looks like there can only be 1 ID. 
I would probably inherit from CSSValue.

> Source/WebCore/css/CSSParser.cpp:7250
> +    RefPtr<CSSElementValue> webElem = CSSElementValue::create();

Nit: webElem -> cssElement

> Source/WebCore/css/CSSParser.h:218
> +    PassRefPtr<CSSValue> parseWebKitElement(CSSParserValueList*);

Nit: I would probably call this parseCSSElement.

> Source/WebCore/css/CSSPrimitiveValue.cpp:120
> +    case CSSPrimitiveValue:: CSS_ELEMENT_FUNCTION:

Nit: Remove the extra space after ::.

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

This should be "-webkit-element(#" + String(m_value.string) + ")". That is, if
you get the cssText, it should include the -webkit-element() part. See
CSSImageSetValue::customCssText for how this works.

> LayoutTests/ChangeLog:6
> +	   Reviewed by Tony Chang.

Please restore this to Reviewed by NOBODY (OOPS!).

> LayoutTests/ChangeLog:18
> +	   * platform/efl/Skipped:
> +	   * platform/qt/Skipped:

I think you need to skip the directory in platform/chromium/TestExpectations
too.

> LayoutTests/fast/css/cssElement/webkit-element-parsing-invalid.html:15
> +function testInvalidImageSet(description, property, rule)

Nit: You probably don't mean ImageSet here.

> LayoutTests/fast/css/cssElement/webkit-element-parsing.html:34
> +function testelementRule(description, property, rule, expectedTexts)

Nit: testelementRule -> testElementRule

> LayoutTests/fast/css/cssElement/webkit-element-parsing.html:54
> +	       subRule = elementRule[0].cssText;
> +	       shouldBe("subRule", "'" + expectedTexts + "'");

I think part of the confusion is that you're inheriting from a CSSValueList. 
If you don't inherit from list, then you don't have to use [0] and you can
check elementRule.cssText directly, which should include -webkit-element().