|Summary:||[CSSRegions]Implement NamedFlow::name attribute|
|Product:||WebKit||Reporter:||Mihnea Ovidenie <mihnea>|
|Component:||CSS||Assignee:||Mihnea Ovidenie <mihnea>|
|Severity:||Normal||CC:||abarth, abucur, ojan, webkit.review.bot|
|Version:||528+ (Nightly build)|
|Bug Depends on:|
|Bug Blocks:||57312, 87640|
Description Mihnea Ovidenie 2012-02-27 01:30:21 PST
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 <firstname.lastname@example.org> > + [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.