Bug 77935 – Disable adding a shadow root to elements having a dynamic built-in shadow root.

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Shinya Kawanaka]

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.

[Shinya Kawanaka]

Created attachment 125779 [details]
Patch

[Dominic Cooney]

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]

Related Chromium bug: <http://code.google.com/p/chromium/issues/detail?id=113168>

[Dimitri Glazkov (Google)]

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]

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]

Since I want to add new flag which will introduced in Bug 77929, I'll update this after Bug 77929 is landed.

[Shinya Kawanaka]

Created attachment 126242 [details]
Patch

[Kent Tamura]

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]

Created attachment 126245 [details]
Patch

[Kent Tamura]

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]

Created attachment 126257 [details]
Patch for landing

[Shinya Kawanaka]

(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 on attachment 126257 [details]
Patch for landing

Clearing flags on attachment: 126257

Committed r107202: <http://trac.webkit.org/changeset/107202>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Abhishek Arya]

*** Bug 78138 has been marked as a duplicate of this bug. ***

[David Kilzer (:ddkilzer)]

<rdar://problem/10830087>