Bug 51426

Summary: CharacterData needs cleanup
Product: WebKit Reporter: Ryosuke Niwa <rniwa>
Component: DOMAssignee: Ryosuke Niwa <rniwa>
Status: RESOLVED FIXED    
Severity: Enhancement CC: ap, darin, eric
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 51389    
Attachments:
Description Flags
cleanup
none
cleanup darin: review+

Description Ryosuke Niwa 2010-12-21 14:58:53 PST
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.
Comment 1 Eric Seidel (no email) 2010-12-21 15:09:22 PST
Bleh, the whole manual re-attach is lame anyway.
Comment 2 Ryosuke Niwa 2010-12-21 15:18:09 PST
Created attachment 77155 [details]
cleanup
Comment 3 Darin Adler 2010-12-21 15:29:41 PST
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.
Comment 4 Ryosuke Niwa 2010-12-21 15:37:33 PST
(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?
Comment 5 Ryosuke Niwa 2010-12-21 15:48:34 PST
(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.
Comment 6 Darin Adler 2010-12-21 15:48:58 PST
(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”?
Comment 7 Darin Adler 2010-12-21 15:50:46 PST
(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.
Comment 8 Ryosuke Niwa 2010-12-21 15:58:23 PST
(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.
Comment 9 Ryosuke Niwa 2010-12-21 16:32:41 PST
Created attachment 77164 [details]
cleanup
Comment 10 Ryosuke Niwa 2010-12-21 16:34:05 PST
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.
Comment 11 Ryosuke Niwa 2010-12-21 17:37:22 PST
Thanks for the review, Darin.  Landing now.
Comment 12 Ryosuke Niwa 2010-12-21 17:59:24 PST
Committed r74444: <http://trac.webkit.org/changeset/74444>