Bug 99556 - Clean up ContentData operator overloads
Summary: Clean up ContentData operator overloads
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Elliott Sprehn
URL:
Keywords:
Depends on:
Blocks: 99498
  Show dependency treegraph
 
Reported: 2012-10-17 00:35 PDT by Elliott Sprehn
Modified: 2012-10-17 01:18 PDT (History)
6 users (show)

See Also:


Attachments
Patch (5.56 KB, patch)
2012-10-17 00:40 PDT, Elliott Sprehn
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Elliott Sprehn 2012-10-17 00:35:51 PDT
Clean up ContentData operator overloads
Comment 1 Elliott Sprehn 2012-10-17 00:40:48 PDT
Created attachment 169109 [details]
Patch
Comment 2 Eric Seidel (no email) 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).
Comment 3 Eric Seidel (no email) 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.
Comment 4 Elliott Sprehn 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.
Comment 5 Eric Seidel (no email) 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?
Comment 6 Eric Seidel (no email) 2012-10-17 00:56:34 PDT
Comment on attachment 169109 [details]
Patch

OK.  I'll buy that.  Thanks.
Comment 7 Elliott Sprehn 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 :(
Comment 8 WebKit Review Bot 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>
Comment 9 WebKit Review Bot 2012-10-17 01:18:14 PDT
All reviewed patches have been landed.  Closing bug.