[webkit-reviews] review denied: [Bug 79645] [CSSRegions]Implement NamedFlow::name attribute : [Attachment 143313] The implementation and the test

[bugzilla-daemon at webkit.org]

Andreas Kling <kling at webkit.org> has denied Andrei Bucur <abucur at adobe.com>'s
request for review:
Bug 79645: [CSSRegions]Implement NamedFlow::name attribute
https://bugs.webkit.org/show_bug.cgi?id=79645

Attachment 143313: The implementation and the test
https://bugs.webkit.org/attachment.cgi?id=143313&action=review

------- Additional Comments from Andreas Kling <kling at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=143313&action=review


Patch itself looks fine, but let's make the test a little more consistent with
the rest of our layout tests.

Note that there's a helper script to create new tests in
Tools/Scripts/make-new-script-test :)

> LayoutTests/ChangeLog:2
> +2012-05-22  Andrei Bucur  <abucur at adobe.com>
> +	   [CSSRegions]Implement NamedFlow::name attribute

Missing newline between these rows.

> LayoutTests/fast/regions/webkit-named-flow-name-expected.txt:7
> +PASS
> +PASS
> +PASS

We should try to write tests that tell you a bit more about what's going
right/wrong.

For example, this kind of construct in your test below:
assert(namedFlow.name == "flow-name", "The name should be 'flow-name' when
there are no regions to flow into");

Could be replaced by our existing JS test helpers:
shouldBe("namedFlow.name", "'flow-name'");

As for the explanation of what's supposed to happen, that can just be a comment
in the JS, or you can log them using the debug("foo bar baz"); helper.