RESOLVED FIXED 99556
Clean up ContentData operator overloads 
https://bugs.webkit.org/show_bug.cgi?id=99556
Summary Clean up ContentData operator overloads
Elliott Sprehn
Reported 2012-10-17 00:35:51 PDT
Clean up ContentData operator overloads
Attachments
Patch (5.56 KB, patch)
2012-10-17 00:40 PDT, Elliott Sprehn 		no flags
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Elliott Sprehn
Comment 1 2012-10-17 00:40:48 PDT
Created attachment 169109 [details] Patch
Eric Seidel (no email)
Comment 2 2012-10-17 00:48:55 PDT
Comment on attachment 169109 [details] Patch Seems OK... I worry if this will slow down canShareStyleWithElement (which can be really hot when adding siblings).
Eric Seidel (no email)
Comment 3 2012-10-17 00:49:36 PDT
I think that if style sharing is already disabled for elements with content: then this should have no perf impact.
Elliott Sprehn
Comment 4 2012-10-17 00:50:49 PDT
(In reply to comment #2) > (From update of attachment 169109 [details]) > Seems OK... I worry if this will slow down canShareStyleWithElement (which can be really hot when adding siblings). It shouldn't, type() is virtual and the old operator== called it 3 times and then did a switch. With this patch we only make 2 virtual calls and doesn't do a jump for the switch. If anything I feel like this should be (marginally) faster.
Eric Seidel (no email)
Comment 5 2012-10-17 00:53:47 PDT
It looks like styles with content actually can be shared in many cases: http://trac.webkit.org/browser/trunk/Source/WebCore/css/StyleResolver.cpp#L2925 Note that only one case sets unique. That said, I'm still not sure when this comparison is used, so this may still be fine?
Eric Seidel (no email)
Comment 6 2012-10-17 00:56:34 PDT
Comment on attachment 169109 [details] Patch OK. I'll buy that. Thanks.
Elliott Sprehn
Comment 7 2012-10-17 01:06:40 PDT
(In reply to comment #5) > It looks like styles with content actually can be shared in many cases: > http://trac.webkit.org/browser/trunk/Source/WebCore/css/StyleResolver.cpp#L2925 > > Note that only one case sets unique. > > That said, I'm still not sure when this comparison is used, so this may still be fine? It gets used in Node::diff through StyleRareNonInheritedData::contentDataEquivalent. Your comment made me think to look at where this is used and then I realized our usage is broken: https://bugs.webkit.org/show_bug.cgi?id=99560 :(
WebKit Review Bot
Comment 8 2012-10-17 01:18:10 PDT
Comment on attachment 169109 [details] Patch Clearing flags on attachment: 169109 Committed r131565: <http://trac.webkit.org/changeset/131565>
WebKit Review Bot
Comment 9 2012-10-17 01:18:14 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.