Bug 51121 – [v8] The last portion of CSS GC work: fixing fast/dom/StyleSheet/gc-stylesheet-wrapper.html

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[anton muhin]

[v8] The last portion of CSS GC work: fixing fast/dom/StyleSheet/gc-stylesheet-wrapper.html

[anton muhin]

Created attachment 76691 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 76691 [details]
Patch

WebCore uses 4 space indent.  I'm not sure how this passed the style queue.

[Eric Seidel (no email)]

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.

[anton muhin]

Created attachment 76701 [details]
Patch

[anton muhin]

(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

[anton muhin]

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.

[anton muhin]

Created attachment 76705 [details]
Patch

[David Levin]

(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).)

[Eric Seidel (no email)]

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.

[Jeremy Orlow]

Mads, can you do an unofficial review of this please?

[Mads Ager]

This looks good to me.

[anton muhin]

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.

[WebKit Commit Bot]

Comment on attachment 76705 [details]
Patch

Clearing flags on attachment: 76705

Committed r74260: <http://trac.webkit.org/changeset/74260>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.