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

Description Shinya Kawanaka 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.
Comment 1 Shinya Kawanaka 2012-02-07 01:04:57 PST
Created attachment 125779 [details]
Patch
Comment 2 Dominic Cooney 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
}

?
Comment 3 Dominic Cooney 2012-02-07 22:39:45 PST
Related Chromium bug: <http://code.google.com/p/chromium/issues/detail?id=113168>
Comment 4 Dimitri Glazkov (Google) 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.
Comment 5 Dominic Cooney 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.
Comment 6 Shinya Kawanaka 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.
Comment 7 Shinya Kawanaka 2012-02-08 22:15:55 PST
Created attachment 126242 [details]
Patch
Comment 8 Kent Tamura 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);
Comment 9 Shinya Kawanaka 2012-02-08 23:14:32 PST
Created attachment 126245 [details]
Patch
Comment 10 Kent Tamura 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.
Comment 11 Shinya Kawanaka 2012-02-09 01:17:02 PST
Created attachment 126257 [details]
Patch for landing
Comment 12 Shinya Kawanaka 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.
Comment 13 WebKit Review Bot 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>
Comment 14 WebKit Review Bot 2012-02-09 01:54:24 PST
All reviewed patches have been landed.  Closing bug.
Comment 15 Abhishek Arya 2012-02-21 15:33:15 PST
*** Bug 78138 has been marked as a duplicate of this bug. ***
Comment 16 David Kilzer (:ddkilzer) 2012-03-04 10:24:59 PST
<rdar://problem/10830087>