[webkit-reviews] review granted: [Bug 24542] Improve ContentData encapsulation : [Attachment 28521] Patch v1

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted David Kilzer
(ddkilzer) <ddkilzer at webkit.org>'s request for review:
Bug 24542: Improve ContentData encapsulation
https://bugs.webkit.org/show_bug.cgi?id=24542

Attachment 28521: Patch v1
https://bugs.webkit.org/attachment.cgi?id=28521&action=review

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
> -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.