Summary: | [CSSRegions]Implement NamedFlow::name attribute | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Mihnea Ovidenie <mihnea> | ||||||
Component: | CSS | Assignee: | 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
Mihnea Ovidenie
2012-02-27 01:30:21 PST
Created attachment 143313 [details]
The implementation and the test
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. 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 on attachment 143558 [details]
Patch and test, second try
Looks great. r=me
Comment on attachment 143558 [details] Patch and test, second try Clearing flags on attachment: 143558 Committed r118178: <http://trac.webkit.org/changeset/118178> All reviewed patches have been landed. Closing bug. |