Bug 70862 - [MutationObservers] Support characterDataOldValue for characterData mutations
Summary: [MutationObservers] Support characterDataOldValue for characterData mutations
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Adam Klein
URL:
Keywords:
Depends on:
Blocks: 68729
  Show dependency treegraph
 
Reported: 2011-10-25 16:32 PDT by Adam Klein
Modified: 2011-10-31 16:51 PDT (History)
5 users (show)

See Also:


Attachments
Patch (13.68 KB, patch)
2011-10-28 17:56 PDT, Adam Klein
no flags Details | Formatted Diff | Diff
Patch (13.66 KB, patch)
2011-10-31 13:11 PDT, Adam Klein
no flags Details | Formatted Diff | Diff
Patch for landing (13.65 KB, patch)
2011-10-31 16:28 PDT, Adam Klein
no flags Details | Formatted Diff | Diff
More const String&ification (13.66 KB, patch)
2011-10-31 16:33 PDT, Adam Klein
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Klein 2011-10-25 16:32:24 PDT
[MutationObservers] Support characterDataOldValue for characterData mutations
Comment 1 Adam Klein 2011-10-28 17:56:22 PDT
Created attachment 112953 [details]
Patch
Comment 2 Adam Klein 2011-10-31 13:11:36 PDT
Created attachment 113077 [details]
Patch
Comment 3 Adam Klein 2011-10-31 13:22:31 PDT
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.
Comment 4 Ojan Vafai 2011-10-31 16:10:48 PDT
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.
Comment 5 Adam Klein 2011-10-31 16:16:07 PDT
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.
Comment 6 Ryosuke Niwa 2011-10-31 16:24:04 PDT
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.
Comment 7 Ojan Vafai 2011-10-31 16:25:18 PDT
(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.
Comment 8 Adam Klein 2011-10-31 16:28:24 PDT
Created attachment 113104 [details]
Patch for landing
Comment 9 Adam Klein 2011-10-31 16:29:12 PDT
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&".
Comment 10 Ryosuke Niwa 2011-10-31 16:29:45 PDT
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.
Comment 11 Adam Klein 2011-10-31 16:31:21 PDT
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?
Comment 12 Adam Klein 2011-10-31 16:33:48 PDT
Created attachment 113105 [details]
More const String&ification
Comment 13 Ryosuke Niwa 2011-10-31 16:38:51 PDT
Comment on attachment 113105 [details]
More const String&ification

I think Ojan's r+ withstands here.
Comment 14 Adam Klein 2011-10-31 16:46:59 PDT
Committed r98910: <http://trac.webkit.org/changeset/98910>
Comment 15 Adam Klein 2011-10-31 16:47:43 PDT
(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).
Comment 16 Ryosuke Niwa 2011-10-31 16:51:38 PDT
(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.