[webkit-reviews] review denied: [Bug 121422] CSS Unit vh, vw, vmin and vmax in box-shadow are not applied. : [Attachment 212117] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Sep 19 20:46:27 PDT 2013

Darin Adler <darin at apple.com> has denied gur.trio at gmail.com's request for
Bug 121422: CSS Unit vh, vw, vmin and vmax in box-shadow are not applied.

Attachment 212117: Patch

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=212117&action=review

New feature looks pretty good. Some style mistakes that need to be fixed.

Also, do the tests cover all the code paths?S

> Source/WebCore/css/StyleResolver.cpp:4068
> +int StyleResolver::getViewportPercentageLength(CSSPrimitiveValue* item, int

I’m not sure this function is a good idea. Instead I suggest we have four small
helper functions, viewportHeight and viewportWidth functions and min and max
ones. At each call site we already know which of these four cases we are in, so
it's awkward to pass the value in and check the item inside this function.

> Source/WebCore/css/StyleResolver.cpp:4083
> +}
>  } // namespace WebCore

Should have a blank line here.

> Source/WebCore/css/StyleResolver.h:588
> +    int getViewportPercentageLength(CSSPrimitiveValue* item, int factor);

This is not right for WebKit coding style. The prefix get should not be used
for a function that returns a value. And the argument name "item" should be
omitted. Further "factor" is not a specific enough name to make clear what the
argument is. I think maybe percentage is what is should be named.

> LayoutTests/fast/css/shadow-viewport-units.html:10
> +    var a = document.getElementById("a");	

Should not add trailing spaces.

> LayoutTests/fast/css/shadow-viewport-units.html:12
> +    document.body.appendChild(output);	

Should not add trailing spaces.

> LayoutTests/fast/css/shadow-viewport-units.html:13
> +    if (window.getComputedStyle(a).textShadow == "none")	    

Should not add trailing spaces.Should not add trailing spaces.

> LayoutTests/fast/css/shadow-viewport-units.html:15
> +    else	

Should not add trailing spaces.

More information about the webkit-reviews mailing list