Bug 116446

Summary: [CSS Exclusions] property parsing tests should be revised
Product: WebKit Reporter: Hans Muller <giles_joplin>
Component: CSSAssignee: 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 Flags
Patch
achicu: review+, achicu: commit-queue-
Patch none

Hans Muller
Reported 2013-05-20 10:35:50 PDT
Several problems with the (old) parsing tests have been pointed out: - The shape-inside,shape-outside variations are nearly identical. The common code should be factored out. - There's excessive and needless stringification in many of the test functions. - Some of the functions have unused arguments. - There are various style problems. - The parsing tests should be segregated in a subdirectory.
Attachments
Patch (89.00 KB, patch)
2013-05-21 16:04 PDT, Hans Muller 		achicu: review+
achicu: commit-queue-
DetailsFormatted DiffDiff
Patch (112.09 KB, patch)
2013-05-22 13:56 PDT, Hans Muller 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Hans Muller
Comment 1 2013-05-21 16:04:12 PDT
Created attachment 202476 [details] Patch
Alexandru Chiculita
Comment 2 2013-05-22 10:56:01 PDT
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, [ ... ]);
Hans Muller
Comment 3 2013-05-22 13:56:11 PDT
Created attachment 202620 [details] Patch Made all of the suggested changes.
WebKit Commit Bot
Comment 4 2013-05-22 14:22:33 PDT
Comment on attachment 202620 [details] Patch Clearing flags on attachment: 202620 Committed r150542: <http://trac.webkit.org/changeset/150542>
WebKit Commit Bot
Comment 5 2013-05-22 14:22:34 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.