Bug 51426 – CharacterData needs cleanup

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Ryosuke Niwa]

CharacterData.cpp duplicates the following code over and over:

    if ((!renderer() || !rendererIsNeeded(renderer()->style())) && attached()) {
        detach();
        attach();
    } else if (renderer())
        toRenderText(renderer())->setTextWithOffset(m_data, x, y);

        dispatchModifiedEvent(oldStr.get());

where x and y are some variable / constant.  We should extract it into a method.

[Eric Seidel (no email)]

Bleh, the whole manual re-attach is lame anyway.

[Ryosuke Niwa]

Created attachment 77155 [details]
cleanup

[Darin Adler]

Comment on attachment 77155 [details]
cleanup

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.

[Ryosuke Niwa]

(In reply to comment #3)
> 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.

How about updateRendererAndDispatchModifiedEventIfPossible?  Or simply notifyModified?

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

They're offset & length of old data, which is quite misleading.  I'm not happy about the argument names but I couldn't come up with anything better.

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

Will do.

> > WebCore/dom/CharacterData.cpp:43
> > +    modified(0, oldLength, oldStr);
> 
> Might use oldStr.release() instead of oldStr for slightly better performance. Same below.

Will do.

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

Ah! that's much nicer will do.

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

Oops. That's just my being careless.  Will fix.

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

How about modifiedOffset and modifiedLength?

[Ryosuke Niwa]

(In reply to comment #4)
> > 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.
> 
> Ah! that's much nicer will do.

Except that we can't do it because append()'s return type is void.

[Darin Adler]

(In reply to comment #4)
> (In reply to comment #3)
> > 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.
> 
> How about updateRendererAndDispatchModifiedEventIfPossible?  Or simply notifyModified?

Both good tries, but neither rings the “perfect name” bell. Maybe respondToModification?

> > 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.
> 
> How about modifiedOffset and modifiedLength?

Well, it’s not a “modified offset” so I don’t think that is quite right. Maybe “modificationOffset” and “modificationLength”, with a comment somewhere that says that the offsets are the location within the old string that was modified.

But I think “modificationLength” might need a different name, because it’s the length of the old text, not the length of new existing text. Maybe “replacedCharactersLength”?

[Darin Adler]

(In reply to comment #5)
> (In reply to comment #4)
> > > 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.
> > 
> > Ah! that's much nicer will do.
> 
> Except that we can't do it because append()'s return type is void.

OK, then it can still be reordered at least, like this:

    RefPtr<StringImpl> oldData = m_data;

    String combinedData(oldData.get());
    combinedData.append(data);
    m_data = combinedData.impl();

It’s clearer if saving the old data is done first.

The reason there is no good API for append is that String::append is super-slow and deprecated. Otherwise, I’d suggest we add an append to StringImpl.

[Ryosuke Niwa]

(In reply to comment #7)
> OK, then it can still be reordered at least, like this:
> 
>     RefPtr<StringImpl> oldData = m_data;
> 
>     String combinedData(oldData.get());
>     combinedData.append(data);
>     m_data = combinedData.impl();
> 
> It’s clearer if saving the old data is done first.
> 
> The reason there is no good API for append is that String::append is super-slow and deprecated. Otherwise, I’d suggest we add an append to StringImpl.

I just realized that we can put all of this into modified if we passed newStr instead of oldStr.  I'll submit a new patch in half an hour.

[Ryosuke Niwa]

Created attachment 77164 [details]
cleanup

[Ryosuke Niwa]

Comment on attachment 77164 [details]
cleanup

View in context: https://bugs.webkit.org/attachment.cgi?id=77164&action=review

> WebCore/dom/CharacterData.cpp:41
> +    setDataAndUpdate(dataImpl, 0, oldLength);

I'm not happy about the fact we have setData and setDataAndUpdate but I couldn't come up with a better name.  I also thought of updateData and setDataAndNotify but neither sounded right.

[Ryosuke Niwa]

Thanks for the review, Darin.  Landing now.

[Ryosuke Niwa]

Committed r74444: <http://trac.webkit.org/changeset/74444>