[Webkit-unassigned] [Bug 36010] RenderText::m_text should be a String, not RefPtr<StringImpl>

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=36010


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #50486|review?                     |review+
               Flag|                            |




--- Comment #2 from Darin Adler <darin at apple.com>  2010-03-12 09:58:18 PST ---
(From update of attachment 50486)
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

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.