Bug 56117 - [v8] Change the way group id for CSS objects is calculated
Summary: [v8] Change the way group id for CSS objects is calculated
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: anton muhin
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-03-10 10:10 PST by anton muhin
Modified: 2011-03-11 04:11 PST (History)
4 users (show)

See Also:


Attachments
Patch (3.41 KB, patch)
2011-03-10 10:23 PST, anton muhin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description anton muhin 2011-03-10 10:10:48 PST
[v8] Change the way group id for CSS objects is calculated
Comment 1 anton muhin 2011-03-10 10:18:42 PST
Currently we simply traverse parents of StyleBase objects.  However, in contrast to DOM proper objects, those C++ pointers do not correspond to JS references.  For example, there is apparently no way to reach CSSStyleDeclaration if not from immediate CSSRule parent.

We hit exactly this problem: for inline styles, WebKit places CSSStyleDeclarations immediately under CSSStyleSheet (note there is no CSSRule in between).  Previous algorithm grouped all those inline styles under this style sheet (which is per document), now we don't group those objects.

See also https://bugs.webkit.org/show_bug.cgi?id=55899 and http://code.google.com/p/chromium/issues/detail?id=75118
Comment 2 anton muhin 2011-03-10 10:23:36 PST
Created attachment 85346 [details]
Patch
Comment 3 Adam Barth 2011-03-10 13:57:15 PST
Comment on attachment 85346 [details]
Patch

Ok.  It's too bad it's hard to test for the absence of these memory leaks.  This code is complex and subtle.  I don't fully understand the traversal you're doing here.  It seems to rely on the details of the CSS object model, which I don't know how stable they are.  I'm marking this r+ for now, but that's mostly because I don't know who else would be better to review it...
Comment 4 anton muhin 2011-03-11 03:12:28 PST
Thanks a lot, Adam.

Yes, it's indeed scary and I am not an expert in CSS OM model either.

(In reply to comment #3)
> (From update of attachment 85346 [details])
> Ok.  It's too bad it's hard to test for the absence of these memory leaks.  This code is complex and subtle.  I don't fully understand the traversal you're doing here.  It seems to rely on the details of the CSS object model, which I don't know how stable they are.  I'm marking this r+ for now, but that's mostly because I don't know who else would be better to review it...
Comment 5 WebKit Commit Bot 2011-03-11 04:11:03 PST
Comment on attachment 85346 [details]
Patch

Clearing flags on attachment: 85346

Committed r80842: <http://trac.webkit.org/changeset/80842>
Comment 6 WebKit Commit Bot 2011-03-11 04:11:07 PST
All reviewed patches have been landed.  Closing bug.