Bug 24542 – Improve ContentData encapsulation

• Component: CSS
• Status: RESOLVED FIXED

[David Kilzer (:ddkilzer)]

* SUMMARY
While fixing Bug 23741, I noticed that the encapsulation of the ContentData class could be improved.

[David Kilzer (:ddkilzer)]

Created attachment 28521 [details]
Patch v1

Proposed patch.

[Simon Fraser (smfr)]

Comment on attachment 28521 [details]
Patch v1

> -void RenderStyle::setContent(StringImpl* s, bool add)
> +void RenderStyle::setContent(PassRefPtr<StringImpl> s, bool add)

Does this change reduce reference churn?

> diff --git a/WebCore/rendering/style/StyleRareNonInheritedData.cpp b/WebCore/rendering/style/StyleRareNonInheritedData.cpp
> index c337fa4..f7c41ca 100644
> --- a/WebCore/rendering/style/StyleRareNonInheritedData.cpp
> +++ b/WebCore/rendering/style/StyleRareNonInheritedData.cpp
> @@ -159,28 +159,28 @@ bool StyleRareNonInheritedData::contentDataEquivalent(const StyleRareNonInherite
>      ContentData* c2 = o.m_content.get();
>  
>      while (c1 && c2) {
> -        if (c1->m_type != c2->m_type)
> +        if (c1->type() != c2->type())
>              return false;
>  
> -        switch (c1->m_type) {
> +        switch (c1->type()) {
>              case CONTENT_NONE:
>                  break;
>              case CONTENT_TEXT:
> -                if (!equal(c1->m_content.m_text, c2->m_content.m_text))
> +                if (!equal(c1->text(), c2->text()))
>                      return false;
>                  break;
>              case CONTENT_OBJECT:
> -                if (!StyleImage::imagesEquivalent(c1->m_content.m_image, c2->m_content.m_image))
> +                if (!StyleImage::imagesEquivalent(c1->image(), c2->image()))
>                      return false;
>                  break;
>              case CONTENT_COUNTER:
> -                if (*c1->m_content.m_counter != *c2->m_content.m_counter)
> +                if (*c1->counter() != *c2->counter())
>                      return false;
>                  break;
>          }

I think it would be good to implement ContentData::dataEquivalent(const ContentData&),
which checks type and data. Then this code would be much simpler.

I'd also suggest bool ContentData::isText() const, isObject(), isCounter() to make the rest of the code
more readable.

r=me, with these suggestions if you like.

[David Kilzer (:ddkilzer)]

(In reply to comment #2)
> (From update of attachment 28521 [details] [review])
> > -void RenderStyle::setContent(StringImpl* s, bool add)
> > +void RenderStyle::setContent(PassRefPtr<StringImpl> s, bool add)
> 
> Does this change reduce reference churn?

That was not the goal I had in mind when I wrote it.  Instead I wanted to give the setter standard semantics for passing in a ref-counted object that will be owned by the container object, and I wanted to hide the releaseRef()/deref() ugliness inside the ContentData class (caused by the use of a union to store pointers to the various data types).

I'll make the other changes before I land the patch.  Thanks!

[David Kilzer (:ddkilzer)]

$ git svn dcommit
Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/rendering/RenderObject.cpp
	M	WebCore/rendering/RenderObjectChildList.cpp
	M	WebCore/rendering/style/ContentData.cpp
	M	WebCore/rendering/style/ContentData.h
	M	WebCore/rendering/style/CounterContent.h
	M	WebCore/rendering/style/RenderStyle.cpp
	M	WebCore/rendering/style/RenderStyle.h
	M	WebCore/rendering/style/StyleRareNonInheritedData.cpp
Committed r41725

https://trac.webkit.org/changeset/41725

[Simon Fraser (smfr)]

Build fix in r41726