Bug 74067

Created attachment 118533 [details] Patch

Hi Dimitri, could you take a look at this? This and blocking Bug 59816 pushes forward to make TreeScope first class citizen.

Comment on attachment 118533 [details] Patch Attachment 118533 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10831400

The redness on cr-linux is apparently due to tree breakage at that time.

Created attachment 118948 [details] Patch

Comment on attachment 118948 [details] Patch Attachment 118948 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/10847234

Comment on attachment 118948 [details] Patch Attachment 118948 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/10846218

Comment on attachment 118948 [details] Patch Attachment 118948 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/10845219

Comment on attachment 118948 [details] Patch Attachment 118948 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10845218

Created attachment 119137 [details] Patch

Comment on attachment 119137 [details] Patch Attachment 119137 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10874080

Created attachment 120304 [details] Patch

Comment on attachment 120304 [details] Patch Attachment 120304 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/10998413

Comment on attachment 120304 [details] Patch Attachment 120304 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/10981480

Comment on attachment 120304 [details] Patch Attachment 120304 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/10998418

Comment on attachment 120304 [details] Patch Attachment 120304 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10998416

Comment on attachment 120304 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=120304&action=review The EWS bot is failing on the patch, so I think you’ll need a new version. The idea seems fine, but it can be done in a more-lightweight fashion. > Source/WebCore/dom/Node.h:693 > + virtual void didMoveToNewOwnerDocument(const NodeOwnerChange&); It seems to me we should just pass the old document. The new document is easy to get, just call document(), so there is no need to pass it. Having a named structure just to pass two arguments also seems unnecessarily heavyweight. > Source/WebCore/html/HTMLFormControlElement.h:123 > + virtual void didMoveToNewOwnerDocument(const NodeOwnerChange&); Please use OVERRIDE in all the didMoveToNewOwnerDocument function declarations. This will catch if we get the argument list wrong in any of them.

Created attachment 120517 [details] Patch

Hi Darin, thanks you for taking look! I updated the patch. Could you take another look? > It seems to me we should just pass the old document. The new document is easy to get, just call document(), so there is no need to pass it. Having a named structure just to pass two arguments also seems unnecessarily heavyweight. Right. The updated patch addresses this. > > > Source/WebCore/html/HTMLFormControlElement.h:123 > > + virtual void didMoveToNewOwnerDocument(const NodeOwnerChange&); > > Please use OVERRIDE in all the didMoveToNewOwnerDocument function declarations. This will catch if we get the argument list wrong in any of them. Fixed.

Comment on attachment 120517 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=120517&action=review This is looking great. review- because of the HTMLInputElement being a bit too messy. Also, one last idea. I think didMoveToNewDocument is a fine name for this. I don’t think we need to emphasize “owner document” just because the DOM does; the old functions had awkward names, but no reason that we should keep those awkward names. > Source/WebCore/html/FormAssociatedElement.cpp:59 > + if (element->fastHasAttribute(formAttr) && oldDocument) > + oldDocument->unregisterFormElementWithFormAttribute(this); Probably should check oldDocument first for better performance in the new-element case. > Source/WebCore/html/FormAssociatedElement.h:67 > + void didMoveToNewOwnerDocument(Document* oldDocument) OVERRIDE; This is not an override of a virtual function; I would not expect this to compile at all! > Source/WebCore/html/HTMLInputElement.cpp:1510 > + if (needsSuspensionCallback()) Since this function always calls needsSuspensionCallback at least once, it would be better if it called it only once and stored it in a local variable. > Source/WebCore/html/HTMLMediaElement.cpp:3401 > +void HTMLMediaElement::setShouldDelayLoadEvent(Document* doc, bool shouldDelay) Please don’t use the abbreviation “doc” in an argument name. The name document would be better. This new function is messy. It’s not good for the media element itself to change its m_shouldDelayLoadEvent state when moving from one document to another. Instead, only the count should be updated when the element moves from document to the other and this function shouldn’t be called at all. I don’t even think didMoveToNewOwnerDocument should look at m_readyState. It should only look at m_shouldDelayLoadEvent.

Created attachment 120524 [details] Patch

Comment on attachment 120524 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=120524&action=review Please fix the one thing I mentioned; otherwise this is ready to go. > Source/WebCore/html/HTMLMediaElement.cpp:288 > + if (m_readyState < HAVE_CURRENT_DATA && m_shouldDelayLoadEvent) { We do not want to check m_readyState < HAVE_CURRENT_DATA here. If this somehow gets out of sync with m_shouldDelayLoadEvent it could result in us leaving a document with the wrong load event delay count. This logic should solely be based on the value of m_shouldDelayLoadEvent. The old code needed to check m_readyState because of a flaw in how the old code worked that is now fixed by your new function.

Hi Darin, thanks for lighting review! I'll land this after addressing your point. (In reply to comment #22) > (From update of attachment 120524 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=120524&action=review > > Please fix the one thing I mentioned; otherwise this is ready to go. > > > Source/WebCore/html/HTMLMediaElement.cpp:288 > > + if (m_readyState < HAVE_CURRENT_DATA && m_shouldDelayLoadEvent) { > > We do not want to check m_readyState < HAVE_CURRENT_DATA here. If this somehow gets out of sync with m_shouldDelayLoadEvent it could result in us leaving a document with the wrong load event delay count. This logic should solely be based on the value of m_shouldDelayLoadEvent. The old code needed to check m_readyState because of a flaw in how the old code worked that is now fixed by your new function.

Created attachment 120526 [details] Patch for landing

Comment on attachment 120526 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=120526&action=review > Source/WebCore/dom/Node.cpp:2511 > +#else > + UNUSED_PARAM(oldDocument); Hmm, I just noticed that oldDocument is never used on either side of the if statement. That means that you should leave out the argument name altogether. Without that I think we’ll fail to compile when MUTATION_OBSERVERS is enabled. > Source/WebCore/dom/StyledElement.cpp:185 > + // Normally we would be concerned about reseting the parent of those declarations in StyledElement::didMoveToNewDocument(). I think it’s spelled resetting rather than reseting.

Comment on attachment 120526 [details] Patch for landing Clearing flags on attachment: 120526 Committed r103679: <http://trac.webkit.org/changeset/103679>

All reviewed patches have been landed. Closing bug.