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

Shinya Kawanaka
Reported 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.
Attachments
Test (42.38 KB, patch)
2012-02-12 23:51 PST, Shinya Kawanaka 		no flags
DetailsFormatted DiffDiff
Test (45.16 KB, patch)
2012-02-13 18:43 PST, Shinya Kawanaka 		no flags
DetailsFormatted DiffDiff
Patch (46.91 KB, patch)
2012-02-13 20:07 PST, Shinya Kawanaka 		no flags
DetailsFormatted DiffDiff
Patch (48.37 KB, patch)
2012-02-14 00:09 PST, Shinya Kawanaka 		no flags
DetailsFormatted DiffDiff
Patch (48.88 KB, patch)
2012-02-14 00:50 PST, Shinya Kawanaka 		no flags
DetailsFormatted DiffDiff
Patch (48.92 KB, patch)
2012-02-14 01:12 PST, Shinya Kawanaka 		no flags
DetailsFormatted DiffDiff
Patch (48.92 KB, patch)
2012-02-14 01:34 PST, Shinya Kawanaka 		no flags
DetailsFormatted DiffDiff
Show Obsolete (6) View All Add attachment proposed patch, testcase, etc.
Shinya Kawanaka
Comment 1 2012-02-12 23:51:36 PST
Created attachment 126726 [details] Test
Philippe Normand
Comment 2 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
WebKit Review Bot
Comment 3 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
WebKit Review Bot
Comment 4 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
Shinya Kawanaka
Comment 5 2012-02-13 18:43:27 PST
Created attachment 126884 [details] Test
Philippe Normand
Comment 6 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
WebKit Review Bot
Comment 7 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
Shinya Kawanaka
Comment 8 2012-02-13 20:07:46 PST
Created attachment 126896 [details] Patch
Shinya Kawanaka
Comment 9 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.
Hajime Morrita
Comment 10 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()?
Shinya Kawanaka
Comment 11 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...
Shinya Kawanaka
Comment 12 2012-02-14 00:09:12 PST
Created attachment 126921 [details] Patch
Shinya Kawanaka
Comment 13 2012-02-14 00:50:39 PST
Created attachment 126926 [details] Patch
Hayato Ito
Comment 14 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.
Shinya Kawanaka
Comment 15 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.
Shinya Kawanaka
Comment 16 2012-02-14 01:12:29 PST
Created attachment 126929 [details] Patch
Shinya Kawanaka
Comment 17 2012-02-14 01:34:00 PST
Created attachment 126932 [details] Patch
WebKit Review Bot
Comment 18 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.
WebKit Review Bot
Comment 19 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>
WebKit Review Bot
Comment 20 2012-02-14 04:48:48 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.