[webkit-reviews] review denied: [Bug 95930] Ensure variables are resolved for specialized CSS primitive value types. : [Attachment 162664] Patch

[bugzilla-daemon at webkit.org]

Tony Chang <tony at chromium.org> has denied Luke Macpherson
<macpherson at chromium.org>'s request for review:
Bug 95930: Ensure variables are resolved for specialized CSS primitive value
types.
https://bugs.webkit.org/show_bug.cgi?id=95930

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

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


> Source/WebCore/ChangeLog:22
> +	   * css/CSSBasicShapes.cpp:
> +	   (WebCore::buildRectangleString):
> +	   (WebCore::CSSBasicShapeRectangle::cssText):
> +	   (WebCore):
> +	   (WebCore::CSSBasicShapeRectangle::serializeResolvingVariables):
> +	   (WebCore::CSSBasicShapeRectangle::hasVariableReference):
> +	   (WebCore::buildCircleString):
> +	   (WebCore::CSSBasicShapeCircle::cssText):

Please list a description of your change here.	For example, you don't need to
fill in all the hasVariableReference and serializeResolvingVariables sections,
but in the first one you could mention the changes you're making.  The changes
in CSSPrimitiveValue.cpp and StyleResolver.cpp could use some explanation too.

> Source/WebCore/css/CSSBasicShapes.cpp:151
> +String CSSBasicShapePolygon::cssText() const
> +{
>      StringBuilder result;

The duplicated code with serializeResolvingVariables is unfortunate.  Maybe you
could make a helper function that takes a Vector<String> and
cssTest/serializeResolvingVariables would pass in the constructed Vector?

> Source/WebCore/css/CSSBasicShapes.cpp:160
> +    for (unsigned i = 0; i < m_values.size(); i += 2) {

Nit: unsigned -> size_t

> Source/WebCore/css/CSSBasicShapes.cpp:185
>      for (unsigned i = 0; i < m_values.size(); i += 2) {

Nit: unsigned -> size_t

> Source/WebCore/css/CSSBasicShapes.cpp:200
> +    for (unsigned i = 0; i < m_values.size(); i++) {

Nits: unsigned -> size_t, i++ -> ++i

> Source/WebCore/css/Pair.h:53
> +    static String generateCssString(const String& first, const String&
second)

Can this be private? Also, it should be generateCSSString.

> Source/WebCore/css/Pair.h:55
> +	   if (first == second)

Comparing strings seems like it would be slower than comparing
CSSPrimitiveValues.  I would probably just get rid of generateCSSString and
duplicate the 'if values are the same don't append' logic since that's only 3
lines of code and will be different for serializeResolvingVariables.

> Source/WebCore/css/Pair.h:69
> +	   return
generateCssString(first()->customSerializeResolvingVariables(variables),
> +			    
second()->customSerializeResolvingVariables(variables));

Nit: Indention here is weird.

> Source/WebCore/css/Rect.h:77
> +    static String generateCssString(const String& top, const String& right,
const String& bottom, const String& left)

private, generateCSSString.

> Source/WebCore/css/Rect.h:93
> +	   return
generateCssString(top()->customSerializeResolvingVariables(variables),
> +			    
right()->customSerializeResolvingVariables(variables),
> +			    
bottom()->customSerializeResolvingVariables(variables),
> +			    
left()->customSerializeResolvingVariables(variables));

Nit: Indention is weird.

> Source/WebCore/css/Rect.h:108
> +    static String generateCssString(const String& top, const String& right,
const String& bottom, const String& left)

private, generateCSSString.

> LayoutTests/ChangeLog:8
> +	   Add tests that use specialized CSS values (eg. pairs, quads).

Can you add tests for all the different shapes (rect, basic shape rect, circle,
ellipse, polygon)?

> LayoutTests/fast/css/variables/root-background-size.html:4
> +try { internals.settings.setCSSVariablesEnabled(true); } catch (e) {}

Why is this in a try/catch?  Shouldn't we be checking for window.internals?

> LayoutTests/fast/css/variables/var-inside-pair.html:4
> +internals.settings.setCSSVariablesEnabled(true);

Please wrap this in a window.internals check.

> LayoutTests/fast/css/variables/var-inside-quad.html:4
> +internals.settings.setCSSVariablesEnabled(true);

Please wrap this in a window.internals check.