Bug 79645 - [CSSRegions]Implement NamedFlow::name attribute
Summary: [CSSRegions]Implement NamedFlow::name attribute
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mihnea Ovidenie
Depends on:
Blocks: 57312 87640
  Show dependency treegraph
Reported: 2012-02-27 01:30 PST by Mihnea Ovidenie
Modified: 2012-05-28 02:13 PDT (History)
4 users (show)

See Also:

The implementation and the test (5.60 KB, patch)
2012-05-22 09:19 PDT, Andrei Bucur
kling: review-
Details | Formatted Diff | Diff
Patch and test, second try (5.62 KB, patch)
2012-05-23 06:41 PDT, Andrei Bucur
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Comment 1 Andrei Bucur 2012-05-22 09:19:57 PDT
Created attachment 143313 [details]
The implementation and the test
Comment 2 Andreas Kling 2012-05-23 03:59:31 PDT
Comment on attachment 143313 [details]
The implementation and the test

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@adobe.com>
> +        [CSSRegions]Implement NamedFlow::name attribute

Missing newline between these rows.

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

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.
Comment 3 Andrei Bucur 2012-05-23 06:41:29 PDT
Created attachment 143558 [details]
Patch and test, second try

Patch with updated test case using Andreas suggestions. It looks better, I have to agree :).
Comment 4 Andreas Kling 2012-05-23 06:43:59 PDT
Comment on attachment 143558 [details]
Patch and test, second try

Looks great. r=me
Comment 5 WebKit Review Bot 2012-05-23 07:25:43 PDT
Comment on attachment 143558 [details]
Patch and test, second try

Clearing flags on attachment: 143558

Committed r118178: <http://trac.webkit.org/changeset/118178>
Comment 6 WebKit Review Bot 2012-05-23 07:25:57 PDT
All reviewed patches have been landed.  Closing bug.