Bug 87223

Created attachment 144000 [details] WIP: passes tests

I'm not ultra confident whether this lazy approach is better. Maybe it's better to keep the structure fresh.

Comment on attachment 144000 [details] WIP: passes tests Attachment 144000 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/12801394

Comment on attachment 144000 [details] WIP: passes tests Attachment 144000 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/12808301

Comment on attachment 144000 [details] WIP: passes tests Attachment 144000 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/12807374

Comment on attachment 144000 [details] WIP: passes tests Attachment 144000 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12803404

Comment on attachment 144000 [details] WIP: passes tests Attachment 144000 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12793407

Comment on attachment 144000 [details] WIP: passes tests Attachment 144000 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12791459

Comment on attachment 144000 [details] WIP: passes tests View in context: https://bugs.webkit.org/attachment.cgi?id=144000&action=review I will need to re-read this patch several times to really understand all the stuff that's happening. I like the direction and encourage you to continue your work. > Source/WebCore/dom/Element.cpp:136 > + elementShadow->removeAllShadowRoots(); Curious why you decided to split this off from destructor. > Source/WebCore/html/HTMLFormControlElement.cpp:-75 > - m_validationMessage = nullptr; Can this change be split off into a separate patch? > Source/WebCore/html/shadow/ContentDistributor.h:54 > + Dated = 1, > + Spoiling = 2, What do these two mean?

Hi Dimitri, Thanks for taking a look! I'll update the patch with more detailed explanation on its ChangeLog entry. (In reply to comment #9) > (From update of attachment 144000 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=144000&action=review > > I will need to re-read this patch several times to really understand all the stuff that's happening. I like the direction and encourage you to continue your work. > > > Source/WebCore/dom/Element.cpp:136 > > + elementShadow->removeAllShadowRoots(); > > Curious why you decided to split this off from destructor. That's because InsertionPoint::detach(), which is called during this, now touches its ascendants. But the shadow root is already disconnected from the host at the point of the destructor. We need to give a change to insertion point to notify its removal to the host, to invalidate the distribution. > > > Source/WebCore/html/HTMLFormControlElement.cpp:-75 > > - m_validationMessage = nullptr; > > Can this change be split off into a separate patch? Sure. I have no intention to land this patch at once. > > > Source/WebCore/html/shadow/ContentDistributor.h:54 > > + Dated = 1, > > + Spoiling = 2, > > What do these two mean? Wel... Dated should be Invalid, and Spoiling could be Invalidating.... We need to distinguish these two because a tree-detach after invalidation can trigger ensureDistribution(). which we need to prevent. "Spoiling" (should be Invalidating) state lets the caller know the distribution is under invalidation and it shouldn't re-distribute there.

Created attachment 144327 [details] Patch

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

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

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

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

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

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

Created attachment 144415 [details] Patch

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

Created attachment 144436 [details] Fixing win build

Comment on attachment 144436 [details] Fixing win build View in context: https://bugs.webkit.org/attachment.cgi?id=144436&action=review This is a much cleaner design! I looked over the code a few times, and it seems to be workable. Let's give it a spin. > Source/WebCore/ChangeLog:14 > + This change extracts these code to a set of ContentDistributor methods, which are "these code" is awkward. Maybe "these bits of code"? > Source/WebCore/ChangeLog:30 > + Here is such DOM mutations: "Here are"

Thanks for reviewing this big chunk! I'll land this after recovering from the syntax error ;-)

Created attachment 144655 [details] Patch for landing

Comment on attachment 144655 [details] Patch for landing Clearing flags on attachment: 144655 Committed r118886: <http://trac.webkit.org/changeset/118886>

All reviewed patches have been landed. Closing bug.