[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]

Darin Adler <darin at apple.com> has denied gur.trio at gmail.com's request for
review:
Bug 121422: CSS Unit vh, vw, vmin and vmax in box-shadow are not applied.
https://bugs.webkit.org/show_bug.cgi?id=121422

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

------- 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
factor)

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.