Bug 87223 – [Shadow DOM] Node distribution should be orthogonal from node attachment

• Component: DOM
• Status: RESOLVED FIXED

[Hajime Morrita]

We are computing node distribution during node attachment, which is horrible.
It should be orthogonal concept and should be able to happen independently.

[Hajime Morrita]

Created attachment 144000 [details]
WIP: passes tests

[Hajime Morrita]

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

[Early Warning System Bot]

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

[Early Warning System Bot]

Comment on attachment 144000 [details]
WIP: passes tests

Attachment 144000 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12808301

[Gyuyoung Kim]

Comment on attachment 144000 [details]
WIP: passes tests

Attachment 144000 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12807374

[WebKit Review Bot]

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

[Build Bot]

Comment on attachment 144000 [details]
WIP: passes tests

Attachment 144000 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12793407

[Build Bot]

Comment on attachment 144000 [details]
WIP: passes tests

Attachment 144000 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12791459

[Dimitri Glazkov (Google)]

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?

[Hajime Morrita]

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.

[Hajime Morrita]

Created attachment 144327 [details]
Patch

[Build Bot]

Comment on attachment 144327 [details]
Patch

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

[Build Bot]

Comment on attachment 144327 [details]
Patch

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

[WebKit Review Bot]

Comment on attachment 144327 [details]
Patch

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

[Early Warning System Bot]

Comment on attachment 144327 [details]
Patch

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

[Early Warning System Bot]

Comment on attachment 144327 [details]
Patch

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

[Gyuyoung Kim]

Comment on attachment 144327 [details]
Patch

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

[Hajime Morrita]

Created attachment 144415 [details]
Patch

[Build Bot]

Comment on attachment 144415 [details]
Patch

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

[Hajime Morrita]

Created attachment 144436 [details]
Fixing win build

[Dimitri Glazkov (Google)]

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"

[Hajime Morrita]

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

[Hajime Morrita]

Created attachment 144655 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 144655 [details]
Patch for landing

Clearing flags on attachment: 144655

Committed r118886: <http://trac.webkit.org/changeset/118886>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.