[webkit-reviews] review denied: [Bug 73038] Fix Aspect Ratio Property Inheritance And Make the Computed Value Equal the Specified Value : [Attachment 116458] Patch

Wed Nov 23 18:46:17 PST 2011

Ojan Vafai <ojan at chromium.org> has denied Fady Samuel <fsamuel at chromium.org>'s
request for review:
Bug 73038: Fix Aspect Ratio Property Inheritance And Make the Computed Value
Equal the Specified Value
https://bugs.webkit.org/show_bug.cgi?id=73038

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

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

C++ code is all fine except for the include. A bunch of nits about the test.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:56
> +#include "RenderStyle.h"

Is this include necessary?

> LayoutTests/fast/css/aspect-ratio-inheritance.html:1
> +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">

should use the html doctype
<!DOCTYPE html>

> LayoutTests/fast/css/aspect-ratio-inheritance.html:4
> +    <link rel="stylesheet" href="../../fast/js/resources/js-test-style.css">

You don't need the stylesheet. It's automatically inserted. Also, I'd drop the
head element as well and just use a div instead of the body element.

> LayoutTests/fast/css/aspect-ratio-inheritance.html:9
> +  <body style="-webkit-aspect-ratio: 1 / 2;">
> +    <div id="aspectRatioTest"></div>

There's no official style for webkit tests, but it makes sense to be consistent
where we can. In this case, 4-space indent.

Also, typically we don't indent the top-level elements since every page has
them. So, the html, head, body elements and their immediate children are
usually not indented.

> LayoutTests/fast/css/aspect-ratio-inheritance.html:21
> +	 <script>
> +	     function testComputedValue(elementId, value, styleAttribute)
> +	     {
> +		 var div = document.getElementById(elementId);
> +		 div.style[styleAttribute] = value;
> +		 var computedValue =
window.getComputedStyle(div)[styleAttribute];
> +		 return computedValue;
> +	     }
> +	     shouldBeEqualToString('testComputedValue("aspectRatioTest", "1 /
4", "-webkit-aspect-ratio")', '1/4');
> +	     shouldBeEqualToString('testComputedValue("aspectRatioTest",
"inherit", "-webkit-aspect-ratio")', '1/2');
> +	     shouldBeEqualToString('testComputedValue("aspectRatioTest",
"none", "-webkit-aspect-ratio")', 'none');
> +	 </script>

This is indented more than the script element below it.