Bug 116446 - [CSS Exclusions] property parsing tests should be revised
Summary: [CSS Exclusions] property parsing tests should be revised
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Hans Muller
Depends on:
Reported: 2013-05-20 10:35 PDT by Hans Muller
Modified: 2013-05-22 14:22 PDT (History)
2 users (show)

See Also:

Patch (89.00 KB, patch)
2013-05-21 16:04 PDT, Hans Muller
achicu: review+
achicu: commit-queue-
Details | Formatted Diff | Diff
Patch (112.09 KB, patch)
2013-05-22 13:56 PDT, Hans Muller
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hans Muller 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.
Comment 1 Hans Muller 2013-05-21 16:04:12 PDT
Created attachment 202476 [details]
Comment 2 Alexandru Chiculita 2013-05-22 10:56:01 PDT
Comment on attachment 202476 [details]

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, [ ... ]);
Comment 3 Hans Muller 2013-05-22 13:56:11 PDT
Created attachment 202620 [details]

Made all of the suggested changes.
Comment 4 WebKit Commit Bot 2013-05-22 14:22:33 PDT
Comment on attachment 202620 [details]

Clearing flags on attachment: 202620

Committed r150542: <http://trac.webkit.org/changeset/150542>
Comment 5 WebKit Commit Bot 2013-05-22 14:22:34 PDT
All reviewed patches have been landed.  Closing bug.