[webkit-reviews] review granted: [Bug 36010] RenderText::m_text should be a String, not RefPtr<StringImpl> : [Attachment 50486] Patch v1

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Shinichiro Hamaji
<hamaji at chromium.org>'s request for review:
Bug 36010: RenderText::m_text should be a String, not RefPtr<StringImpl>
https://bugs.webkit.org/show_bug.cgi?id=36010

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

------- Additional Comments from Darin Adler <darin at apple.com>
The main thing we need to do here is do performance testing to be sure we
didn't slow anything down.

> +bool charactersAreAllASCII(const UChar*, size_t);

I think we should move all the UChar* operations to the top of the file here
instead of moving up just this one. If we can come up with a good file name we
also might want to move them into a separate header.

> -	       char c = (*m_text)[i];
> +	       char c = m_text[i];

It's kind of inefficient to go through the String -> StringImpl -> characters
thing each time through the loop. It would be better to get a pointer to the
characters outside the loop. But only if this has a measurable performance
effect.

>	       case CAPITALIZE: {
> -		   m_text = m_text->capitalize(previousCharacter());
> +		   m_text = m_text.impl()->capitalize(previousCharacter());
>		   break;
>	       }

No need for the braces here. Do we have a guarantee that m_text.impl() is not
0? We should add this function to PlatformString.h so we don't have to go down
to the StringImpl level.

>	       case UPPERCASE:
> -		   m_text = m_text->upper();
> +		   m_text = m_text.impl()->upper();
>		   break;
>	       case LOWERCASE:
> -		   m_text = m_text->lower();
> +		   m_text = m_text.impl()->lower();
>		   break;

>	       case TSCIRCLE:
> -		   m_text = m_text->secure(whiteBullet);
> +		   m_text = m_text.impl()->secure(whiteBullet);
>		   break;
>	       case TSDISC:
> -		   m_text = m_text->secure(bullet);
> +		   m_text = m_text.impl()->secure(bullet);
>		   break;
>	       case TSSQUARE:
> -		   m_text = m_text->secure(blackSquare);
> +		   m_text = m_text.impl()->secure(blackSquare);

Do we have a guarantee that m_text.impl() is not 0? We should add these
functions to PlatformString.h so we don't have to go down to the StringImpl
level.

> -    StringImpl* text() const { return m_text.get(); }
> +    StringImpl* text() const { return m_text.impl(); }

I think we should change this method to return a const String&, but in a later
patch.

> -    const UChar* characters() const { return m_text->characters(); }
> -    unsigned textLength() const { return m_text->length(); } // non virtual
implementation of length()
> +    const UChar* characters() const { return m_text.characters(); }
> +    unsigned textLength() const { return m_text.length(); } // non virtual
implementation of length()

I think we should consider removing one or both of these since we already have
text(), but in a separate patch.

I'm going to say review+ but the "impl non-0" concern and the performance
concern are both things that need to be checked somehow before landing