[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





--- Comment #5 from Shinichiro Hamaji <hamaji at chromium.org>  2010-03-15 02:05:02 PST ---
Thanks for the review! Could you take a look again?

I think style checker will complain about this patch, but I think they would be
OK. This is another instance of Bug 33814.

> 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.

I see. I found there are a small difference between StringImpl::operator[] and
String::operator[]. The former ASSERTs the specified index is smaller than the
length of string, and the latter just returns zero. I think the former would be
more informative when someone actually meets the overflow. I rewrote all
String::operator[] calls in loops with StringImpl::operator[] calls. So, there
should be no performance issue anymore (but it's very nice if you told me the
good or standard way to measure the performances for this kind of changes in
this chance).

> 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.

Yes, there is ASSERT(m_text) in the beginning of this function, but I agree we
want these functions in PlatformString.h. I've added
make(Lower|Upper|Secure|Capitalized) as lower and upper have already been
defined and have different semantics. I'm not sure if they are standard WebKit
naming. Please let me know if you know better names.

> I think we should change this method to return a const String&, but in a later
> patch.
>
> I think we should consider removing one or both of these since we already have
> text(), but in a separate patch.

I agree.

I found many functions in StringImpl should be const functions. I'd like to do
this refactoring in a later patch.

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