[MutationObservers] Support characterDataOldValue for characterData mutations

• Component: DOM
• Status: RESOLVED FIXED

[Adam Klein]

[MutationObservers] Support characterDataOldValue for characterData mutations

[Adam Klein]

Created attachment 112953 [details]
Patch

[Adam Klein]

Created attachment 113077 [details]
Patch

[Adam Klein]

Now ready for review.  Note that there are FIXMEs in Element.cpp and CharacterData.cpp to factor out the mutationWithNullOldValue logic.  Those will be fixed after Rafael lands his attributeFilter patch, as we hope to clean up Node::getRegisteredMutationObserversOfType to return something smarter than a HashMap.

[Ojan Vafai]

Comment on attachment 113077 [details]
Patch

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

> Source/WebCore/dom/CharacterData.cpp:216
> +    RefPtr<MutationRecord> mutation = MutationRecord::createCharacterData(node, isOldValueRequested(observers) ? oldData : 0);

Can we create this lazily in the for loop below? That would obviate the need for the isOldValueRequested and would avoid doing a second loop over the observers.

I guess you're trying to have a single record for all observers. I think that's fine, but you could invert the way you create mutations...create the null old value record first and create the mutation record with the old value from the one without.

It's not a big deal, but it seems the code is less complicated (or am I missing something?) and it avoids another O(n) operation, not that I actually expect that to affect real-world performance.

[Adam Klein]

Comment on attachment 113077 [details]
Patch

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

>> Source/WebCore/dom/CharacterData.cpp:216
>> +    RefPtr<MutationRecord> mutation = MutationRecord::createCharacterData(node, isOldValueRequested(observers) ? oldData : 0);
> 
> Can we create this lazily in the for loop below? That would obviate the need for the isOldValueRequested and would avoid doing a second loop over the observers.
> 
> I guess you're trying to have a single record for all observers. I think that's fine, but you could invert the way you create mutations...create the null old value record first and create the mutation record with the old value from the one without.
> 
> It's not a big deal, but it seems the code is less complicated (or am I missing something?) and it avoids another O(n) operation, not that I actually expect that to affect real-world performance.

The code-as-is optimizes for creating fewer mutation records at the cost of a bit more work up front to figure out what actually needs to be created.  If all observers actually want old value, then we'd have two MutationRecords per mutation just to support them.  Given that the common case is likely a single observer, it seems wasteful to create two records where one would do.

I'm currently working on benchmarks for this stuff so we can get a better idea of what the right tradeoffs are, so I'd like to leave this for now as I find it "conceptually" simplest (minimizing the number of records).  And of course the duplication of this logic will go away in the next patch.

[Ryosuke Niwa]

Comment on attachment 113077 [details]
Patch

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

> Source/WebCore/dom/MutationRecord.cpp:98
> -    CharacterDataRecord(PassRefPtr<Node> target)
> +    CharacterDataRecord(PassRefPtr<Node> target, StringImpl* oldValue)

I don't think we want to pass StringImpl around like this.

[Ojan Vafai]

(In reply to comment #5)
> The code-as-is optimizes for creating fewer mutation records at the cost of a bit more work up front to figure out what actually needs to be created.  If all observers actually want old value, then we'd have two MutationRecords per mutation just to support them.  Given that the common case is likely a single observer, it seems wasteful to create two records where one would do.

That makes sense. I think that's the right tradeoff. I'd be very surprised if this for-loop showed up in a profile.

[Adam Klein]

Created attachment 113104 [details]
Patch for landing

[Adam Klein]

Comment on attachment 113077 [details]
Patch

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

>> Source/WebCore/dom/MutationRecord.cpp:98
>> +    CharacterDataRecord(PassRefPtr<Node> target, StringImpl* oldValue)
> 
> I don't think we want to pass StringImpl around like this.

Changed to "const String&".

[Ryosuke Niwa]

Comment on attachment 113104 [details]
Patch for landing

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

> Source/WebCore/dom/CharacterData.cpp:208
> +static void enqueueCharacterDataMutationRecord(Node* node, StringImpl* oldData)

Same comment about StringImpl here.

> Source/WebCore/dom/CharacterData.cpp:235
> +void CharacterData::dispatchModifiedEvent(StringImpl* oldData)

and here.

[Adam Klein]

Comment on attachment 113104 [details]
Patch for landing

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

>> Source/WebCore/dom/CharacterData.cpp:235
>> +void CharacterData::dispatchModifiedEvent(StringImpl* oldData)
> 
> and here.

I didn't write this code, do you want me to change this interface as part of the same patch?

[Adam Klein]

Created attachment 113105 [details]
More const String&ification

[Ryosuke Niwa]

Comment on attachment 113105 [details]
More const String&ification

I think Ojan's r+ withstands here.

[Adam Klein]

Committed r98910: <http://trac.webkit.org/changeset/98910>

[Adam Klein]

(In reply to comment #11)
> (From update of attachment 113104 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=113104&action=review
> 
> >> Source/WebCore/dom/CharacterData.cpp:235
> >> +void CharacterData::dispatchModifiedEvent(StringImpl* oldData)
> > 
> > and here.
> 
> I didn't write this code, do you want me to change this interface as part of the same patch?

Assuming no due to the r+, I'll write up a second patch to use String instead of StringImpl inside CharacterData (though I do wonder if there's some reason for this usage).

[Ryosuke Niwa]

(In reply to comment #15)
> (In reply to comment #11)
> > (From update of attachment 113104 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=113104&action=review
> > 
> > >> Source/WebCore/dom/CharacterData.cpp:235
> > >> +void CharacterData::dispatchModifiedEvent(StringImpl* oldData)
> > > 
> > > and here.
> > 
> > I didn't write this code, do you want me to change this interface as part of the same patch?
> 
> Assuming no due to the r+, I'll write up a second patch to use String instead of StringImpl inside CharacterData (though I do wonder if there's some reason for this usage).

r+ with comments mean that I still want the comments to be addressed although it can be landed either way.