Summary: | [v8] The last portion of CSS GC work: fixing fast/dom/StyleSheet/gc-stylesheet-wrapper.html | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | anton muhin <antonm> | ||||||||
Component: | New Bugs | Assignee: | anton muhin <antonm> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | abarth, ager, commit-queue, dglazkov, eric, japhet, jorlow, levin | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Other | ||||||||||
OS: | OS X 10.5 | ||||||||||
Attachments: |
|
Description
anton muhin
2010-12-15 11:34:28 PST
Created attachment 76691 [details]
Patch
Comment on attachment 76691 [details]
Patch
WebCore uses 4 space indent. I'm not sure how this passed the style queue.
Comment on attachment 76691 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=76691&action=review > WebCore/bindings/v8/V8GCController.cpp:371 > + if (object) { Early return is strongly preferred in WebKit. Created attachment 76701 [details]
Patch
(In reply to comment #2) > (From update of attachment 76691 [details]) > WebCore uses 4 space indent. I'm not sure how this passed the style queue. Thanks a lot, Eric, fixed. I am surprised as well Fixed too, but webkit-patch upload takes a lot of time on my MBP, please, wait for 3rd patch (In reply to comment #3) > (From update of attachment 76691 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=76691&action=review > > > WebCore/bindings/v8/V8GCController.cpp:371 > > + if (object) { > > Early return is strongly preferred in WebKit. Created attachment 76705 [details]
Patch
(In reply to comment #5) > (In reply to comment #2) > > (From update of attachment 76691 [details] [details]) > > WebCore uses 4 space indent. I'm not sure how this passed the style queue. > > Thanks a lot, Eric, fixed. I am surprised as well check-webkit-style doesn't check indents really. It requires a bit of "intelligence" to do this correctly even at a simple level. For example, a(b, c d); Would be indented correctly even though d would not be indented at a multiple of 4. You need to detect a statement end basically and then check to see that the indent is a multiple of 4. (To check that it is the correct multiple of 4 is more complicated because there are special rules for switch (for example).) Comment on attachment 76705 [details]
Patch
Don't you need to remove it from the skipped list? Looks sane enough to me, but I don't fully understand the consequences of this change not being particularly well versed in V8.
Mads, can you do an unofficial review of this please? This looks good to me. Thanks a lot, Mads. Guys, may I get r+, land this thing and finish CSS GC cleanup? (In reply to comment #11) > This looks good to me. Comment on attachment 76705 [details] Patch Clearing flags on attachment: 76705 Committed r74260: <http://trac.webkit.org/changeset/74260> All reviewed patches have been landed. Closing bug. |