Bug 23741

Summary: StyleRareNonInheritedData::operator==() should not compare ContentData objects by pointer
Product: WebKit Reporter: David Kilzer (:ddkilzer) <ddkilzer>
Component: CSSAssignee: David Kilzer (:ddkilzer) <ddkilzer>
Status: RESOLVED FIXED    
Severity: Normal CC: hyatt
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Mac   
OS: OS X 10.5   
URL: http://trac.webkit.org/browser/trunk/WebCore/rendering/style/StyleRareNonInheritedData.cpp#L97
Attachments:
Description Flags
Patch v1
darin: review-
Patch v2 darin: review+

Description David Kilzer (:ddkilzer) 2009-02-04 14:08:46 PST
* SUMMARY
Using the power of code inspection, I discovered that StyleRareNonInheritedData::operator==() was comparing ContentData objects by pointer instead of using RenderStyle::contentDataEquivalent().

The RenderStyle::contentDataEquivalent() method needs to move to StyleRareNonInheritedData:: contentDataEquivalent() and StyleRareNonInheritedData::operator==() should start using it.
Comment 1 David Kilzer (:ddkilzer) 2009-02-04 18:37:49 PST
Created attachment 27339 [details]
Patch v1

Proposed fix.

NOTE: There may be too much const-ness in "bool contentDataEquivalent(const RenderStyle* otherStyle) const" now for the const_cast<>() constructs that must be used.
Comment 2 David Kilzer (:ddkilzer) 2009-02-04 18:39:42 PST
(In reply to comment #1)
> Created an attachment (id=27339) [review]
> Patch v1
> 
> Proposed fix.
> 
> NOTE: There may be too much const-ness in "bool contentDataEquivalent(const
> RenderStyle* otherStyle) const" now for the const_cast<>() constructs that must
> be used.

I should also note the only reason to keep around RenderStyle:: contentDataEquivalent() is that it's used by Node::diff().


Comment 3 Darin Adler 2009-02-05 15:29:33 PST
Comment on attachment 27339 [details]
Patch v1

> -    bool contentDataEquivalent(const RenderStyle* otherStyle) const;
> +    bool contentDataEquivalent(const RenderStyle* otherStyle) const { return const_cast<RenderStyle*>(this)->rareNonInheritedData.access()->contentDataEquivalent(*(const_cast<RenderStyle*>(otherStyle)->rareNonInheritedData.access())); }

You don't want to call access() here because it's going to force the non-inherited data to be copied. Instead you can just use the -> operator and * operators directly without the call to access().

review- because this needs to be changed to avoid causing memory bloat due to lost style sharing
Comment 4 David Kilzer (:ddkilzer) 2009-02-06 13:11:17 PST
Created attachment 27411 [details]
Patch v2

Addresses issue in Comment #3 by removing use of the DataRef::access() method.
Comment 5 Darin Adler 2009-02-06 13:41:57 PST
Comment on attachment 27411 [details]
Patch v2

> -    bool contentDataEquivalent(const RenderStyle* otherStyle) const;
> +    bool contentDataEquivalent(const RenderStyle* otherStyle) const { return const_cast<RenderStyle*>(this)->rareNonInheritedData->contentDataEquivalent(*(const_cast<RenderStyle*>(otherStyle)->rareNonInheritedData)); }

I think this would be slightly more readable for me without the extra set of parentheses after the "*".

r=me
Comment 6 David Kilzer (:ddkilzer) 2009-02-06 14:49:54 PST
$ git svn dcommit
Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/rendering/style/RenderStyle.cpp
	M	WebCore/rendering/style/RenderStyle.h
	M	WebCore/rendering/style/StyleRareNonInheritedData.cpp
	M	WebCore/rendering/style/StyleRareNonInheritedData.h
Committed r40734

http://trac.webkit.org/changeset/40734

Removed extra parens as requested in Comment #5.
Comment 7 David Kilzer (:ddkilzer) 2009-05-10 17:07:37 PDT
And a layout test:

$ git svn dcommit
Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	LayoutTests/ChangeLog
	A	LayoutTests/fast/css/compare-content-style.html
	A	LayoutTests/platform/mac/fast/css/compare-content-style-expected.txt
Committed r43474

http://trac.webkit.org/changeset/43474