[webkit-reviews] review granted: [Bug 116446] [CSS Exclusions] property parsing tests should be revised : [Attachment 202476] Patch

[bugzilla-daemon at webkit.org]

Alexandru Chiculita <achicu at adobe.com> has granted Hans Muller
<giles_joplin at yahoo.com>'s request for review:
Bug 116446: [CSS Exclusions] property parsing tests should be revised
https://bugs.webkit.org/show_bug.cgi?id=116446

Attachment 202476: Patch
https://bugs.webkit.org/attachment.cgi?id=202476&action=review

------- Additional Comments from Alexandru Chiculita <achicu at adobe.com>
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, [ ... ]);