RESOLVED FIXED 114900
checkLayout() should error out if no data-expected* attributes were found 
https://bugs.webkit.org/show_bug.cgi?id=114900
Summary checkLayout() should error out if no data-expected* attributes were found
Robert Hogan
Reported 2013-04-20 03:41:39 PDT
..
Attachments
Patch (13.26 KB, patch)
2013-04-20 03:56 PDT, Robert Hogan 		no flags
DetailsFormatted DiffDiff
Patch (13.05 KB, patch)
2013-04-22 11:44 PDT, Robert Hogan 		no flags
DetailsFormatted DiffDiff
Patch (13.03 KB, patch)
2013-04-22 11:46 PDT, Robert Hogan 		no flags
DetailsFormatted DiffDiff
Patch (13.04 KB, patch)
2013-04-22 13:09 PDT, Robert Hogan 		no flags
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Robert Hogan
Comment 1 2013-04-20 03:56:46 PDT
Created attachment 198938 [details] Patch
Ojan Vafai
Comment 2 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.
Robert Hogan
Comment 3 2013-04-22 11:44:27 PDT
Created attachment 199055 [details] Patch
Robert Hogan
Comment 4 2013-04-22 11:46:44 PDT
Created attachment 199056 [details] Patch
Robert Hogan
Comment 5 2013-04-22 13:09:29 PDT
Created attachment 199094 [details] Patch
WebKit Commit Bot
Comment 6 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>
WebKit Commit Bot
Comment 7 2013-04-23 10:48:57 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.