Bug 78455 – Use youngestShadowRoot and oldestShadowRoot instead of Element::shadowRoot()

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

[Shinya Kawanaka]

Element::shadowRoot() now becomes ambiguous, because in some cases, we want to get the youngest shadow root, in the other cases, we want to get the oldest shadow root.
It's time to convert them so that the code becomes more clear.

[Shinya Kawanaka]

Created attachment 126726 [details]
Test

[Philippe Normand]

Comment on attachment 126726 [details]
Test

Attachment 126726 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11509654

[WebKit Review Bot]

Comment on attachment 126726 [details]
Test

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

New failing tests:
fast/forms/access-key-for-all-elements.html
fast/forms/focus-selection-input.html
fast/forms/legend-access-key.html
fast/forms/focus-selection-textarea.html
fast/forms/access-key.html
fast/dom/shadow/access-key.html
fast/speech/speech-input-scripting.html
fast/forms/input-radio-checked-tab.html
fast/forms/ValidityState-valueMissing-002.html
fast/dom/access-key-iframe.html

[WebKit Review Bot]

Comment on attachment 126726 [details]
Test

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

New failing tests:
fast/forms/access-key-for-all-elements.html
fast/forms/focus-selection-input.html
fast/forms/legend-access-key.html
fast/forms/focus-selection-textarea.html
fast/forms/access-key.html
fast/dom/shadow/access-key.html
fast/speech/speech-input-scripting.html
fast/forms/input-radio-checked-tab.html
fast/forms/ValidityState-valueMissing-002.html
fast/dom/access-key-iframe.html

[Shinya Kawanaka]

Created attachment 126884 [details]
Test

[Philippe Normand]

Comment on attachment 126884 [details]
Test

Attachment 126884 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11523018

[WebKit Review Bot]

Comment on attachment 126884 [details]
Test

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

New failing tests:
fast/speech/speech-input-scripting.html

[Shinya Kawanaka]

Created attachment 126896 [details]
Patch

[Shinya Kawanaka]

Removes Element::shadowRoot(), and converted it to either:
(1) hasShadowRoot()
  to check the existence of shadow root,
(2) shadowRootList()->youngestShadowRoot()
  to render shadow root tree,
(3) shadowRootList()->oldestShadowRoot().
  to modify user agent shadow root.

[Hajime Morrita]

Comment on attachment 126896 [details]
Patch

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

> Source/WebCore/dom/Element.cpp:1181
> +#endif

We can remove this.

> Source/WebCore/dom/ShadowRootList.cpp:93
> +{

Is it safe to attach like this?

> Source/WebCore/html/InputType.cpp:387
> +            root->removeChild(root->firstChild());

Why not removeAllChildren()?

[Shinya Kawanaka]

(In reply to comment #10)
> (From update of attachment 126896 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=126896&action=review
> 
> > Source/WebCore/dom/Element.cpp:1181
> > +#endif
> 
> We can remove this.

Nice catch...

> 
> > Source/WebCore/dom/ShadowRootList.cpp:93
> > +{
> 
> Is it safe to attach like this?

Currently, yes, because all shadow roots are detached when calling this.

> 
> > Source/WebCore/html/InputType.cpp:387
> > +            root->removeChild(root->firstChild());
> 
> Why not removeAllChildren()?

Why not....? yeah... it should be...

[Shinya Kawanaka]

Created attachment 126921 [details]
Patch

[Shinya Kawanaka]

Created attachment 126926 [details]
Patch

[Hayato Ito]

Comment on attachment 126926 [details]
Patch

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

> Source/WebCore/dom/Element.cpp:876
> +    if (ShadowRootList* shadowList = shadowRootList())

Could you name 'shadowRootList' instead of 'shadowList' in this patch?
Since we now have ShadowElement, a naming of 'shadow' should be avoided and it might be better to use 'shadowRoot' or 'shadowElement' explicitly.

[Shinya Kawanaka]

(In reply to comment #14)
> (From update of attachment 126926 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=126926&action=review
> 
> > Source/WebCore/dom/Element.cpp:876
> > +    if (ShadowRootList* shadowList = shadowRootList())
> 
> Could you name 'shadowRootList' instead of 'shadowList' in this patch?
> Since we now have ShadowElement, a naming of 'shadow' should be avoided and it might be better to use 'shadowRoot' or 'shadowElement' explicitly.

Sounds reasonable.

[Shinya Kawanaka]

Created attachment 126929 [details]
Patch

[Shinya Kawanaka]

Created attachment 126932 [details]
Patch

[WebKit Review Bot]

The commit-queue encountered the following flaky tests while processing attachment 126932 [details]:

perf/object-keys.html bug 63769 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Review Bot]

Comment on attachment 126932 [details]
Patch

Clearing flags on attachment: 126932

Committed r107706: <http://trac.webkit.org/changeset/107706>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.