Bug 116446 – [CSS Exclusions] property parsing tests should be revised

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Hans Muller]

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.

[Hans Muller]

Created attachment 202476 [details]
Patch

[Alexandru Chiculita]

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]

Created attachment 202620 [details]
Patch

Made all of the suggested changes.

[WebKit Commit Bot]

Comment on attachment 202620 [details]
Patch

Clearing flags on attachment: 202620

Committed r150542: <http://trac.webkit.org/changeset/150542>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.