| Summary: | Document::dispatchFullScreenChangeOrErrorEvent repeats the same loop twice | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Ryosuke Niwa <rniwa> | ||||||
| Component: | DOM | Assignee: | Ryosuke Niwa <rniwa> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | benjamin, commit-queue, eric.carlson, esprehn+autocc, jer.noble, kangil.han, kling, koivisto, sam | ||||||
| Priority: | P2 | ||||||||
| Version: | 528+ (Nightly build) | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Bug Depends on: | |||||||||
| Bug Blocks: | 122144 | ||||||||
| Attachments: |
|
||||||||
|
Description
Ryosuke Niwa
2013-10-01 00:59:18 PDT
Created attachment 213063 [details]
Cleanup
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. (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. 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. (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. Created attachment 213103 [details]
Updated per comments
Comment on attachment 213103 [details] Updated per comments Clearing flags on attachment: 213103 Committed r156733: <http://trac.webkit.org/changeset/156733> All reviewed patches have been landed. Closing bug. |