Bug 122143 – Document::dispatchFullScreenChangeOrErrorEvent repeats the same loop twice

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

[Ryosuke Niwa]

Cleanup Document::dispatchFullScreenChangeOrErrorEvent

[Ryosuke Niwa]

Created attachment 213063 [details]
Cleanup

[Darin Adler]

Comment on attachment 213063 [details]
Cleanup

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

> Source/WebCore/dom/Document.cpp:5405
> -        if (!contains(node.get()) && !node->inDocument())
> +        if (!node->inDocument() && &node->document() == this)

This new code is very different from the old. I don’t understand this change at all.

[Ryosuke Niwa]

(In reply to comment #2)
> (From update of attachment 213063 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=213063&action=review
> 
> > Source/WebCore/dom/Document.cpp:5405
> > -        if (!contains(node.get()) && !node->inDocument())
> > +        if (!node->inDocument() && &node->document() == this)
> 
> This new code is very different from the old. I don’t understand this change at all.

Not really. We're looking for a node that is detached from a document that hasn't been inserted into any other document yet.

[Andrei Bucur]

Comment on attachment 213063 [details]
Cleanup

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

>>> Source/WebCore/dom/Document.cpp:5405
>>> +        if (!node->inDocument() && &node->document() == this)
>> 
>> This new code is very different from the old. I don’t understand this change at all.
> 
> Not really. We're looking for a node that is detached from a document that hasn't been inserted into any other document yet.

Not convinced it's the same. Correct me if I'm wrong (I'm not familiar with the spec):
Before the change, the code was taking a snapshot of the change queue, a snapshot of the error queue and then it dispatched the events to the change queue and then to the error queue.
After the change, you snapshot the change queue, dispatch the events, snapshot the error queue and dispatch the events. Is it possible to change the error queue while dispatching the change events?
If true, with the initial code this was invisible to the error queue because it was copied before any dispatch took place.

[Ryosuke Niwa]

(In reply to comment #4)
> Not convinced it's the same. Correct me if I'm wrong (I'm not familiar with the spec):
> Before the change, the code was taking a snapshot of the change queue, a snapshot of the error queue and then it dispatched the events to the change queue and then to the error queue.
> After the change, you snapshot the change queue, dispatch the events, snapshot the error queue and dispatch the events. Is it possible to change the error queue while dispatching the change events?
> If true, with the initial code this was invisible to the error queue because it was copied before any dispatch took place.

That's a separate concern than the one Darin pointed out but it's a valid one.  Will fix.

[Ryosuke Niwa]

Created attachment 213103 [details]
Updated per comments

[WebKit Commit Bot]

Comment on attachment 213103 [details]
Updated per comments

Clearing flags on attachment: 213103

Committed r156733: <http://trac.webkit.org/changeset/156733>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.