Bug 77935

Summary: Disable adding a shadow root to elements having a dynamic built-in shadow root.
Product: WebKit Reporter: Shinya Kawanaka <shinyak>
Component: DOMAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: dglazkov, dominicc, eric, tkent, tsepez, webkit.review.bot
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 77503, 77931    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
morrita: commit-queue-
Patch for landing none

Shinya Kawanaka
Reported 2012-02-06 22:17:15 PST
Supporting multiple shadow subtrees for element having a dynamic built-in shadow root is difficult. It doesn't work well in current implementation. So we have do refactoring them to fully support multiple shadow subtrees. However, let's disable adding user-generated shadow root for them for a while. If their refactoring ends, let turn it on again.
Attachments
Patch (11.97 KB, patch)
2012-02-07 01:04 PST, Shinya Kawanaka 		no flags
DetailsFormatted DiffDiff
Patch (17.14 KB, patch)
2012-02-08 22:15 PST, Shinya Kawanaka 		no flags
DetailsFormatted DiffDiff
Patch (17.37 KB, patch)
2012-02-08 23:14 PST, Shinya Kawanaka 		morrita: commit-queue-
DetailsFormatted DiffDiff
Patch for landing (17.25 KB, patch)
2012-02-09 01:17 PST, Shinya Kawanaka 		no flags
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Shinya Kawanaka
Comment 1 2012-02-07 01:04:57 PST
Created attachment 125779 [details] Patch
Dominic Cooney
Comment 2 2012-02-07 19:02:30 PST
Comment on attachment 125779 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=125779&action=review Temporarily turning off author shadow roots for elements that use UA shadow roots sounds like a good idea. > Source/WebCore/dom/ShadowRoot.h:49 > + enum CheckUserShadowRootAllowance { It might be better to rename this to reflect the caller intent, eg enum (mumble) { CreatingUserAgentShadowRoot, CreatingAuthorShadowRoot } ?
Dominic Cooney
Comment 3 2012-02-07 22:39:45 PST
Related Chromium bug: <http://code.google.com/p/chromium/issues/detail?id=113168>
Dimitri Glazkov (Google)
Comment 4 2012-02-08 09:19:46 PST
Comment on attachment 125779 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=125779&action=review >> Source/WebCore/dom/ShadowRoot.h:49 >> + enum CheckUserShadowRootAllowance { > > It might be better to rename this to reflect the caller intent, eg > > enum (mumble) { > CreatingUserAgentShadowRoot, > CreatingAuthorShadowRoot > } > > ? I like Dominic's idea here. Other than that, the patch looks fine.
Dominic Cooney
Comment 5 2012-02-08 17:51:30 PST
Related to <http://code.google.com/p/chromium/issues/detail?id=113168> and <http://code.google.com/p/chromium/issues/detail?id=113174> downstream in Chromium.
Shinya Kawanaka
Comment 6 2012-02-08 21:35:44 PST
Since I want to add new flag which will introduced in Bug 77929, I'll update this after Bug 77929 is landed.
Shinya Kawanaka
Comment 7 2012-02-08 22:15:55 PST
Created attachment 126242 [details] Patch
Kent Tamura
Comment 8 2012-02-08 22:24:36 PST
Comment on attachment 126242 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=126242&action=review > Source/WebCore/dom/ShadowRoot.cpp:85 > + return false; > + maybe TEXTAREA, too. > Source/WebCore/dom/ShadowRoot.h:53 > + static PassRefPtr<ShadowRoot> create(Element*, ShadowRootCreationPurpose, ExceptionCode&); We had better add a default argument to ExceptionCode&. ..., ExceptionCode& = ASSERT_NO_EXCEPTION);
Shinya Kawanaka
Comment 9 2012-02-08 23:14:32 PST
Created attachment 126245 [details] Patch
Kent Tamura
Comment 10 2012-02-09 00:22:30 PST
Comment on attachment 126245 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=126245&action=review > Source/WebCore/dom/Element.cpp:1202 > + return ShadowRoot::create(this, ShadowRoot::CreatingUserAgentShadowRoot, ASSERT_NO_EXCEPTION).get(); You can remove ASSERT_NO_EXCEPTION argument. > Source/WebCore/html/HTMLDetailsElement.cpp:113 > + RefPtr<ShadowRoot> root = ShadowRoot::create(this, ShadowRoot::CreatingUserAgentShadowRoot, ASSERT_NO_EXCEPTION); ditto. > Source/WebCore/html/HTMLKeygenElement.cpp:89 > + RefPtr<ShadowRoot> root = ShadowRoot::create(this, ShadowRoot::CreatingUserAgentShadowRoot, ASSERT_NO_EXCEPTION); ditto. > Source/WebCore/html/HTMLMeterElement.cpp:244 > + RefPtr<ShadowRoot> root = ShadowRoot::create(this, ShadowRoot::CreatingUserAgentShadowRoot, ASSERT_NO_EXCEPTION); ditto. > Source/WebCore/html/HTMLSummaryElement.cpp:77 > + RefPtr<ShadowRoot> root = ShadowRoot::create(this, ShadowRoot::CreatingUserAgentShadowRoot, ASSERT_NO_EXCEPTION); ditto. > Source/WebCore/html/HTMLTextAreaElement.cpp:88 > + RefPtr<ShadowRoot> root = ShadowRoot::create(this, ShadowRoot::CreatingUserAgentShadowRoot, ASSERT_NO_EXCEPTION); ditto.
Shinya Kawanaka
Comment 11 2012-02-09 01:17:02 PST
Created attachment 126257 [details] Patch for landing
Shinya Kawanaka
Comment 12 2012-02-09 01:17:47 PST
(In reply to comment #10) > (From update of attachment 126245 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=126245&action=review > > > Source/WebCore/dom/Element.cpp:1202 > > + return ShadowRoot::create(this, ShadowRoot::CreatingUserAgentShadowRoot, ASSERT_NO_EXCEPTION).get(); > > You can remove ASSERT_NO_EXCEPTION argument. > > > Source/WebCore/html/HTMLDetailsElement.cpp:113 > > + RefPtr<ShadowRoot> root = ShadowRoot::create(this, ShadowRoot::CreatingUserAgentShadowRoot, ASSERT_NO_EXCEPTION); > > ditto. > > > Source/WebCore/html/HTMLKeygenElement.cpp:89 > > + RefPtr<ShadowRoot> root = ShadowRoot::create(this, ShadowRoot::CreatingUserAgentShadowRoot, ASSERT_NO_EXCEPTION); > > ditto. > > > Source/WebCore/html/HTMLMeterElement.cpp:244 > > + RefPtr<ShadowRoot> root = ShadowRoot::create(this, ShadowRoot::CreatingUserAgentShadowRoot, ASSERT_NO_EXCEPTION); > > ditto. > > > Source/WebCore/html/HTMLSummaryElement.cpp:77 > > + RefPtr<ShadowRoot> root = ShadowRoot::create(this, ShadowRoot::CreatingUserAgentShadowRoot, ASSERT_NO_EXCEPTION); > > ditto. > > > Source/WebCore/html/HTMLTextAreaElement.cpp:88 > > + RefPtr<ShadowRoot> root = ShadowRoot::create(this, ShadowRoot::CreatingUserAgentShadowRoot, ASSERT_NO_EXCEPTION); > > ditto. Done.
WebKit Review Bot
Comment 13 2012-02-09 01:54:18 PST
Comment on attachment 126257 [details] Patch for landing Clearing flags on attachment: 126257 Committed r107202: <http://trac.webkit.org/changeset/107202>
WebKit Review Bot
Comment 14 2012-02-09 01:54:24 PST
All reviewed patches have been landed. Closing bug.
Abhishek Arya
Comment 15 2012-02-21 15:33:15 PST
*** Bug 78138 has been marked as a duplicate of this bug. ***
David Kilzer (:ddkilzer)
Comment 16 2012-03-04 10:24:59 PST
<rdar://problem/10830087>
Note You need to log in before you can comment on or make changes to this bug.