Bug 63596 – Extract setShadowRoot function, harden it in preparation to be exposed to script.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dimitri Glazkov (Google)]

Extract setShadowRoot function, harden it in preparation to be exposed to script.

[Dimitri Glazkov (Google)]

Created attachment 99026 [details]
Patch

[Adam Barth]

Comment on attachment 99026 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=99026&action=review

> Source/WebCore/dom/Element.cpp:1214
> +    if (!shadowRoot)
> +        return true;
> +
> +    if (shadowRoot->shadowHost()) {
> +        ec = HIERARCHY_REQUEST_ERR;
> +        return false;
> +    }
> +
> +    if (shadowRoot->document() != document) {
> +        ec = WRONG_DOCUMENT_ERR;
> +        return false;
> +    }
> +
> +    return true;

I would add this function in a separate patch once this function is exposed to JS so you can write tests for these branches.

> Source/WebCore/dom/Element.cpp:1239
> +    ExceptionCode ec = 0;
> +    setShadowRoot(ShadowRoot::create(document()), ec);

We just drop the ec on the floor.  Typically we ASSERT that it is zero.

[Dimitri Glazkov (Google)]

Comment on attachment 99026 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=99026&action=review

>> Source/WebCore/dom/Element.cpp:1214
>> +    return true;
> 
> I would add this function in a separate patch once this function is exposed to JS so you can write tests for these branches.

That is a very good point.

[Dimitri Glazkov (Google)]

Created attachment 99038 [details]
Patch for landing

[Dimitri Glazkov (Google)]

Comment on attachment 99038 [details]
Patch for landing

whoops, boneheaded mistake.

[Adam Barth]

Comment on attachment 99038 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=99038&action=review

> Source/WebCore/dom/Element.cpp:1220
> +    ASSERT(ec);

ASSERT(!ec);

Do I win the prize?

[Dimitri Glazkov (Google)]

(In reply to comment #6)
> (From update of attachment 99038 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=99038&action=review
> 
> > Source/WebCore/dom/Element.cpp:1220
> > +    ASSERT(ec);
> 
> ASSERT(!ec);
> 
> Do I win the prize?

TOTALLY. I was half-asleep. Also, I think I'll bring back the hardening function and file a bug to write a test for it. I am trying to avoid the problem where enabling APIs is a huge atomic commit.

[Dimitri Glazkov (Google)]

Created attachment 99109 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 99109 [details]
Patch for landing

Clearing flags on attachment: 99109

Committed r90026: <http://trac.webkit.org/changeset/90026>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.