Bug 114900

Summary:

checkLayout() should error out if no data-expected* attributes were found

Product:

WebKit

Reporter:

Robert Hogan <robert>

Component:

Tools / Tests

Assignee:

Robert Hogan <robert>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

commit-queue, ojan, robert, tony

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	none

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	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
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.