[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
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.
More information about the webkit-reviews
mailing list