Summary: | Use youngestShadowRoot and oldestShadowRoot instead of Element::shadowRoot() | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Shinya Kawanaka <shinyak> | ||||||||||||||||
Component: | DOM | Assignee: | 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
Shinya Kawanaka
2012-02-12 20:54:29 PST
Created attachment 126726 [details]
Test
Comment on attachment 126726 [details] Test Attachment 126726 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/11509654 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 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 Created attachment 126884 [details]
Test
Comment on attachment 126884 [details] Test Attachment 126884 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/11523018 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 Created attachment 126896 [details]
Patch
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 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()? (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... Created attachment 126921 [details]
Patch
Created attachment 126926 [details]
Patch
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. (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. Created attachment 126929 [details]
Patch
Created attachment 126932 [details]
Patch
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 on attachment 126932 [details] Patch Clearing flags on attachment: 126932 Committed r107706: <http://trac.webkit.org/changeset/107706> All reviewed patches have been landed. Closing bug. |