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