Bug 78455

Summary: Use youngestShadowRoot and oldestShadowRoot instead of Element::shadowRoot()
Product: WebKit Reporter: Shinya Kawanaka <shinyak>
Component: DOMAssignee: Shinya Kawanaka <shinyak>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, dglazkov, dominicc, gustavo, hayato, japhet, morrita, pnormand, rolandsteiner, shinyak, tkent, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 78313    
Attachments:
Description Flags
Test
none
Test
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Shinya Kawanaka 2012-02-12 20:54:29 PST
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.
Comment 1 Shinya Kawanaka 2012-02-12 23:51:36 PST
Created attachment 126726 [details]
Test
Comment 2 Philippe Normand 2012-02-13 00:12:22 PST
Comment on attachment 126726 [details]
Test

Attachment 126726 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11509654
Comment 3 WebKit Review Bot 2012-02-13 01:01:11 PST
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
Comment 4 WebKit Review Bot 2012-02-13 01:58:45 PST
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
Comment 5 Shinya Kawanaka 2012-02-13 18:43:27 PST
Created attachment 126884 [details]
Test
Comment 6 Philippe Normand 2012-02-13 18:59:41 PST
Comment on attachment 126884 [details]
Test

Attachment 126884 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11523018
Comment 7 WebKit Review Bot 2012-02-13 20:04:28 PST
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
Comment 8 Shinya Kawanaka 2012-02-13 20:07:46 PST
Created attachment 126896 [details]
Patch
Comment 9 Shinya Kawanaka 2012-02-13 20:36:26 PST
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.
Comment 10 Hajime Morrita 2012-02-13 22:33:58 PST
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()?
Comment 11 Shinya Kawanaka 2012-02-14 00:01:13 PST
(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...
Comment 12 Shinya Kawanaka 2012-02-14 00:09:12 PST
Created attachment 126921 [details]
Patch
Comment 13 Shinya Kawanaka 2012-02-14 00:50:39 PST
Created attachment 126926 [details]
Patch
Comment 14 Hayato Ito 2012-02-14 01:02:51 PST
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.
Comment 15 Shinya Kawanaka 2012-02-14 01:11:04 PST
(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.
Comment 16 Shinya Kawanaka 2012-02-14 01:12:29 PST
Created attachment 126929 [details]
Patch
Comment 17 Shinya Kawanaka 2012-02-14 01:34:00 PST
Created attachment 126932 [details]
Patch
Comment 18 WebKit Review Bot 2012-02-14 04:44:04 PST
The commit-queue encountered the following flaky tests while processing attachment 126932 [details]:

perf/object-keys.html bug 63769 (author: ojan@chromium.org)
The commit-queue is continuing to process your patch.
Comment 19 WebKit Review Bot 2012-02-14 04:48:41 PST
Comment on attachment 126932 [details]
Patch

Clearing flags on attachment: 126932

Committed r107706: <http://trac.webkit.org/changeset/107706>
Comment 20 WebKit Review Bot 2012-02-14 04:48:48 PST
All reviewed patches have been landed.  Closing bug.