Summary: | [CSS Exclusions] property parsing tests should be revised | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Hans Muller <giles_joplin> | ||||||
Component: | CSS | Assignee: | Hans Muller <giles_joplin> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | bjonesbe, commit-queue | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Hans Muller
2013-05-20 10:35:50 PDT
Created attachment 202476 [details]
Patch
Comment on attachment 202476 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=202476&action=review Thanks Hans! Might have been easier to review if you did this in two different patches :) > LayoutTests/fast/exclusions/parsing-wrap-shape-inside.html:-7 > -<script src="script-tests/parsing-wrap-shape-inside.js"></script> Is parsing-wrap-shape-inside.js still in use? You don't seem to remove it in this patch. > LayoutTests/fast/exclusions/parsing/script-tests/parsing-shape-outside.js:19 > +invalidShapeValues.forEach(function(value, i, a) { Might be good to add a comment explaining where invalidShapeValues is defined. The same for the other "shared" variables. > LayoutTests/fast/exclusions/parsing/script-tests/parsing-test-utils.js:21 > + nit: I think there's no need for empty lines between the comment and the code. It makes me think the comment is about the whole file. > LayoutTests/fast/exclusions/parsing/script-tests/parsing-wrap-flow.js:8 > +[["-webkit-wrap-flow", "auto", "auto"], nit:I think you can remove the space between the comment and the data. It was confusing initially. Also, might be useful to just use something like the following pattern instead: // ["property", "value", "expectedValue"], ["-webkit-wrap-flow", "both", "both"], > LayoutTests/fast/exclusions/parsing/script-tests/parsing-wrap-flow.js:20 > +].forEach(function(args, i, a) { > + testShapeSpecifiedProperty.apply(null, args); > +}); Looks like you are doing the for-each a lot of times. It might be better to just add a helper for that one too. For example: runFunctionWithArgumentsSet(testShapeSpecifiedProperty, [ ... ]); Created attachment 202620 [details]
Patch
Made all of the suggested changes.
Comment on attachment 202620 [details] Patch Clearing flags on attachment: 202620 Committed r150542: <http://trac.webkit.org/changeset/150542> All reviewed patches have been landed. Closing bug. |