Bug 114900 - checkLayout() should error out if no data-expected* attributes were found
Summary: checkLayout() should error out if no data-expected* attributes were found
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Robert Hogan
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-04-20 03:41 PDT by Robert Hogan
Modified: 2013-04-23 10:48 PDT (History)
4 users (show)

See Also:


Attachments
Patch (13.26 KB, patch)
2013-04-20 03:56 PDT, Robert Hogan
no flags Details | Formatted Diff | Diff
Patch (13.05 KB, patch)
2013-04-22 11:44 PDT, Robert Hogan
no flags Details | Formatted Diff | Diff
Patch (13.03 KB, patch)
2013-04-22 11:46 PDT, Robert Hogan
no flags Details | Formatted Diff | Diff
Patch (13.04 KB, patch)
2013-04-22 13:09 PDT, Robert Hogan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Robert Hogan 2013-04-20 03:41:39 PDT
..
Comment 1 Robert Hogan 2013-04-20 03:56:46 PDT
Created attachment 198938 [details]
Patch
Comment 2 Ojan Vafai 2013-04-21 10:46:04 PDT
Comment on attachment 198938 [details]
Patch

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.
Comment 3 Robert Hogan 2013-04-22 11:44:27 PDT
Created attachment 199055 [details]
Patch
Comment 4 Robert Hogan 2013-04-22 11:46:44 PDT
Created attachment 199056 [details]
Patch
Comment 5 Robert Hogan 2013-04-22 13:09:29 PDT
Created attachment 199094 [details]
Patch
Comment 6 WebKit Commit Bot 2013-04-23 10:48:56 PDT
Comment on attachment 199094 [details]
Patch

Clearing flags on attachment: 199094

Committed r148973: <http://trac.webkit.org/changeset/148973>
Comment 7 WebKit Commit Bot 2013-04-23 10:48:57 PDT
All reviewed patches have been landed.  Closing bug.