Bug 23741 – StyleRareNonInheritedData::operator==() should not compare ContentData objects by pointer

• Component: CSS
• Status: RESOLVED FIXED

[David Kilzer (:ddkilzer)]

* 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.

[David Kilzer (:ddkilzer)]

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.

[David Kilzer (:ddkilzer)]

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

[Darin Adler]

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

[David Kilzer (:ddkilzer)]

Created attachment 27411 [details]
Patch v2

Addresses issue in Comment #3 by removing use of the DataRef::access() method.

[Darin Adler]

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

[David Kilzer (:ddkilzer)]

$ 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.

[David Kilzer (:ddkilzer)]

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