[webkit-reviews] review denied: [Bug 106479] Available height should respect min and max height : [Attachment 181970] Patch

Wed Jan 9 13:17:56 PST 2013

Ojan Vafai <ojan at chromium.org> has denied Robert Hogan <robert at webkit.org>'s
request for review:
Bug 106479: Available height should respect min and max height
https://bugs.webkit.org/show_bug.cgi?id=106479

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

------- Additional Comments from Ojan Vafai <ojan at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=181970&action=review

The C++ side is fine. Please cleanup the tests and then I'm happy to r+.

> LayoutTests/fast/block/percent-top-respects-max-height.html:1
> +<!DOCTYPE html>

Can you fix up the indentation so that it's consistent and 4 spaces?

> LayoutTests/fast/block/percent-top-respects-max-height.html:4
> +		<style type="text/css">

nit: no need to include the type here.

> LayoutTests/fast/block/percent-top-respects-max-height.html:29
> +			.container	{
> +				width:	250px;
> +				background:	green;
> +				margin-right:	20px;
> +		   position: relative;
> +			}
> +			.box	{
> +				height: 100px;
> +				position:	relative;
> +				top:	50%;
> +				background:	blue;
> +			}
> +			.ref	{
> +				height: 100px;
> +				width:	250px;
> +				position: absolute;
> +				top:	150px;
> +				background:	red;
> +			}
> +			.hasheight	{
> +				height: 600px;
> +			}
> +			.hassmallermaxheight	{
> +				max-height:	300px;
> +			}

Can you reduce this to the minimal CSS necessary? e.g. do you need the
margin-right or the position: relative on .box? Do you need any of these
widths?

> LayoutTests/fast/block/percent-top-respects-max-height.html:40
> +		<div class="container hasheight hassmallermaxheight">

Can you just make these all one class?

> LayoutTests/fast/block/percent-top-respects-max-height.html:46
> +	       if (document.getElementById("box").offsetTop == 150)

Can you make this a check-layout.js test instead? Then you won't need custom
code for this and the failure output is more helpful.

> LayoutTests/fast/block/percent-top-respects-min-height.html:1
> +<!DOCTYPE html>

Ditto all the comments for the above test. Or, you could just test both cases
in a single test.