Bug 97372 – Remove bogus assertions from ChildListMutationScope

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Adam Klein]

Remove bogus assertions from ChildListMutationScope

[Adam Klein]

Created attachment 165215 [details]
Patch

[Adam Klein]

This patch is against 97352, so it's not ready for EWS yet.

[Ryosuke Niwa]

Comment on attachment 165215 [details]
Patch

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

> Source/WebCore/ChangeLog:11
> +        Some asserts (and their accompanying comment) were trying to enforce
> +        proper usage of ChildListMutationScope from WebCore, but in the
> +        presence of MutationEvents they could fail due to arbitrary script
> +        execution.

Does this invalidate the assumption stated above the declaration of MutationAccumulator?
(I suppose that's the case given you're removing the comment).
In the presence of mutation events, can we end up delivering mutation records before the outmost scope closes?

If so, are we violating the specification in this regard?

[Adam Klein]

Comment on attachment 165215 [details]
Patch

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

>> Source/WebCore/ChangeLog:11
>> +        execution.
> 
> Does this invalidate the assumption stated above the declaration of MutationAccumulator?
> (I suppose that's the case given you're removing the comment).
> In the presence of mutation events, can we end up delivering mutation records before the outmost scope closes?
> 
> If so, are we violating the specification in this regard?

We don't deliver early, just enqueue early. Since the spec doesn't mention the existence of MutationEvents, it's hard to say whether we're violating the specification. But we're doing something sane (MutationObservers still report everything that happens), as documented in these test cases.

[Ryosuke Niwa]

(In reply to comment #4)
>
> We don't deliver early, just enqueue early. Since the spec doesn't mention the existence of MutationEvents, it's hard to say whether we're violating the specification. But we're doing something sane (MutationObservers still report everything that happens), as documented in these test cases.

Okay. Does our behavior match that of Gecko? I think it'll be nice to at least touch-base with Jonas/Olli to make sure they're okay with our behavior. Given that there are quite few js libraries still relying on mutation events (notably rich text editors), we need to make sure the way our behavior doesn't adversely affect websites that include those editors.

[Adam Klein]

(In reply to comment #5)
> (In reply to comment #4)
> >
> > We don't deliver early, just enqueue early. Since the spec doesn't mention the existence of MutationEvents, it's hard to say whether we're violating the specification. But we're doing something sane (MutationObservers still report everything that happens), as documented in these test cases.
> 
> Okay. Does our behavior match that of Gecko? I think it'll be nice to at least touch-base with Jonas/Olli to make sure they're okay with our behavior. Given that there are quite few js libraries still relying on mutation events (notably rich text editors), we need to make sure the way our behavior doesn't adversely affect websites that include those editors.

Not sure what you'd want to ask. The particular records emitted seem not terribly important in this case, as long as they're an accurate portrayal of the current state of the DOM. Without a spec (which we're not going to get, because MutationEvents will not be specced in DOM4), I don't think exact interoperability is something we should strive for (since it wouldn't be written down anywhere other than in an email). It's just an implementation detail.

[Ryosuke Niwa]

I just chatted with Jonas and confirmed that he doesn't really care about this case.

[Adam Klein]

Created attachment 165228 [details]
Patch

[WebKit Review Bot]

Comment on attachment 165228 [details]
Patch

Clearing flags on attachment: 165228

Committed r129288: <http://trac.webkit.org/changeset/129288>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.