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

[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 50693: Patch v2
https://bugs.webkit.org/attachment.cgi?id=50693&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +    void makeLower() { if (m_impl) m_impl = m_impl->lower(); }
> +    void makeUpper() { if (m_impl) m_impl = m_impl->upper(); }
> +    void makeSecure(UChar aChar) { if (m_impl) m_impl =
m_impl->secure(aChar); }
> +    void makeCapitalized(UChar previousCharacter) { if (m_impl) m_impl =
m_impl->capitalize(previousCharacter); }

When it's unclear whether the function name is a verb or some other part of
speech, I think it's good to choose something that is clearer. In the case of
"capitalize" I don't think "makeCapitalized" is any better.

But lets go with these as-is. Should be easy to rename them later.

> +    bool charactersAreAllASCII() const { return
WebCore::charactersAreAllASCII(characters(), length()); }

For boolean members we try to name them so they complete a phrase "string
<xxx>". Given that, the function name should be something more like
containsOnlyASCIICharacters or containsOnlyASCII.

> -UChar32 StringImpl::characterStartingAt(unsigned i)
> +UChar32 StringImpl::characterStartingAt(unsigned i) const

> -    UChar operator[](unsigned i) { ASSERT(i < m_length); return m_data[i]; }

> -    UChar32 characterStartingAt(unsigned);
> +    UChar operator[](unsigned i) const { ASSERT(i < m_length); return
m_data[i]; }
> +    UChar32 characterStartingAt(unsigned) const;

StringImpl member functions should not be marked const. Because all StringImpl
are immutable, all functions could be marked const. Any that change the string
return a new StringImpl*. Our design choice for StringImpl was to never use
const StringImpl* at all and to leave them all non-const. So it's strange that
you decided to make these const.

> +	   const StringImpl& text = *m_text.impl();

> +    const StringImpl& text = *m_text.impl();

> +    const StringImpl& text = *m_text.impl();

There's no reason for the const here. StringImpl objects are all immutable. If
we do want to use const with StringImpl then we need to make all the member
functions const, since none of them change the state.

Patch is OK as-is, but it would be better not to tamper with the const on
StringImpl.

r=me