[webkit-reviews] review denied: [Bug 118516] % unit heights don't work if parent block height is set in vh : [Attachment 211321] Patch

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has denied gur.trio at gmail.com's request
for review:
Bug 118516: % unit heights don't work if parent block height is set in vh
https://bugs.webkit.org/show_bug.cgi?id=118516

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

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=211321&action=review


First round: some style issues:

> Source/WebCore/rendering/RenderBox.cpp:2748
> +	       // We need to adjust for min/max height because this method does
not
> +	       // handle the min/max of the current block, its caller does. So
the
> +	       // return value from the recursive call will not have been
adjusted
> +	       // yet.

WebKit comments are not limited to the 80 characters mark.

> LayoutTests/ChangeLog:9
> +	   * fast/css/viewport-percentage-compute-box-height-expected.txt:
Added.
> +	   * fast/css/viewport-percentage-compute-box-height.html: Added.

Do we already have test coverage for viewport width?
If not, that is a good opportunity to add it, even if it is not directly
related to your patch.

> LayoutTests/fast/css/viewport-percentage-compute-box-height.html:7
> +		   description('Tests that % unit heights work if parent block
height is set in vh');

Missing period.
The description should also describe what a correct result looks like (for
manual testing).

> LayoutTests/fast/css/viewport-percentage-compute-box-height.html:8
> +		  
shouldBeNonZero("document.getElementById('inner').offsetHeight");

This should be testing an exact value if testRunner is present.

Tests are run in a 800*600px viewport, you can rely on that for testing.

> LayoutTests/fast/css/viewport-percentage-compute-box-height.html:9
> +		   isSuccessfullyParsed();

You should not do this!
Instead, insert js-test-post.js