[webkit-reviews] review granted: [Bug 51426] CharacterData needs cleanup : [Attachment 77155] cleanup

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Ryosuke Niwa <rniwa at webkit.org>'s
request for review:
Bug 51426: CharacterData needs cleanup
https://bugs.webkit.org/show_bug.cgi?id=51426

Attachment 77155: cleanup
https://bugs.webkit.org/attachment.cgi?id=77155&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=77155&action=review

> WebCore/ChangeLog:8
> +	   Extracted CharacterData::modified.

The new function is so abstract that we need to do our best to give it a good
name and good argument names. I think modified is a bit vague. Perhaps we can
rename the function after what it does rather than what happened? The two
things it does are to update the renderer and send DOM events. I guess you’re
going to change it to do more in the future.

It’s not clear what the offset and the length are. If you gave the arguments
good enough names then they might be clear. I think they are the offset and
length of new data within the character data object, but I’m not even sure!

> WebCore/dom/CharacterData.cpp:40
>      RefPtr<StringImpl> oldStr = m_data;

Given the other naming in this patch, it might read nicer if it was oldData
instead of oldStr. Same below.

> WebCore/dom/CharacterData.cpp:43
> +    modified(0, oldLength, oldStr);

Might use oldStr.release() instead of oldStr for slightly better performance.
Same below.

> WebCore/dom/CharacterData.cpp:78
> +    String newStr = m_data;
> +    newStr.append(data);
> +
> +    RefPtr<StringImpl> oldStr = m_data;
> +    m_data = newStr.impl();

There must be a clearer way to write this. How about:

    RefPtr<StringImpl> oldData = m_data;
    m_data = String(oldData.get()).append(data).impl();

The repeated use of the term “data” might be confusing, but I think that reads
much more clearly in that order and without the local variables.

> WebCore/dom/CharacterData.cpp:164
> +void CharacterData::modified(int offset, int len, PassRefPtr<StringImpl>
oldData)

I think unsigned might be better than int for these arguments. But maybe we
already use int for this. Too bad.

Could you use the word length instead of the abbreviation len? And maybe names
that make it clearer what the offset and length are the offset and length of.