[webkit-reviews] review granted: [Bug 114900] checkLayout() should error out if no data-expected* attributes were found : [Attachment 198938] Patch

Sun Apr 21 10:47:45 PDT 2013

Ojan Vafai <ojan at chromium.org> has granted Robert Hogan <robert at webkit.org>'s
request for review:
Bug 114900: checkLayout() should error out if no data-expected* attributes were
found
https://bugs.webkit.org/show_bug.cgi?id=114900

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

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

This is great.

Mostly a bunch of style nits. Only things I'd definitely like to see changed to
not output the error to the console as it's just redundant and to include FAIL
in the error text. Would be nice if you addressed the nits as well.

> LayoutTests/fast/check-layout-error-no-attributes.html:1
> +<!doctype html>

Nit: standard doctype is <!DOCTYPE html>

> LayoutTests/fast/check-layout-error-no-attributes.html:3
> +  <head>

Nit: you can just leave the <head> element out.

> LayoutTests/fast/check-layout-error-no-attributes.html:11
> +    <p> webkit.org/b/114900: checkLayout() should error out if the layout
wasn't checked. </p>
> +	<span style="background-color: green;">xxx <span
style="background-color:blue;" data-total-invalid-attribute=0>yyy</span></span>

> +    <script>
> +	   checkLayout('span > span');
> +    </script>

This indentation is inconsistent.

> LayoutTests/resources/check-layout.js:31
> +    layout.checked = layout.checked || result;

Nit: layout.checked |= result;

> LayoutTests/resources/check-layout.js:37
> +    var layout = { checked: false };

Nit: Calling this "layout" seems a bit confusing to me. How about..."output"?

> LayoutTests/resources/check-layout.js:195
> +	   checkedLayout = checkExpectedValues(node.parentNode, failures) ||
checkedLayout;
> +	   checkedLayout = checkSubtreeExpectedValues(node, failures) ||
checkedLayout;

I prefer |= in cases like this.

> LayoutTests/resources/check-layout.js:207
> +	   document.body.innerHTML = "No valid data-* attributes found in
selector list : " + selectorList;

Please prefix this with "FAIL: " to make it even more clear the test is
failing.

> LayoutTests/resources/check-layout.js:208
> +	   console.error("No valid data-* attributes found in :" +
selectorList);

I don't think the console error add value on top of the innerHTML.