Bug 79645

Summary: [CSSRegions]Implement NamedFlow::name attribute
Product: WebKit Reporter: Mihnea Ovidenie <mihnea>
Component: CSSAssignee: Mihnea Ovidenie <mihnea>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, abucur, ojan, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 57312, 87640    
Attachments:
Description Flags
The implementation and the test
kling: review-
Patch and test, second try none

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
> +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.
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.