Bug 63596 - Extract setShadowRoot function, harden it in preparation to be exposed to script.
Summary: Extract setShadowRoot function, harden it in preparation to be exposed to scr...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dimitri Glazkov (Google)
URL:
Keywords:
Depends on:
Blocks: 63606
  Show dependency treegraph
 
Reported: 2011-06-28 19:25 PDT by Dimitri Glazkov (Google)
Modified: 2011-06-29 10:47 PDT (History)
3 users (show)

See Also:


Attachments
Patch (3.14 KB, patch)
2011-06-28 19:29 PDT, Dimitri Glazkov (Google)
no flags Details | Formatted Diff | Diff
Patch for landing (2.70 KB, patch)
2011-06-28 22:04 PDT, Dimitri Glazkov (Google)
no flags Details | Formatted Diff | Diff
Patch for landing (3.16 KB, patch)
2011-06-29 10:04 PDT, Dimitri Glazkov (Google)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dimitri Glazkov (Google) 2011-06-28 19:25:07 PDT
Extract setShadowRoot function, harden it in preparation to be exposed to script.
Comment 1 Dimitri Glazkov (Google) 2011-06-28 19:29:23 PDT
Created attachment 99026 [details]
Patch
Comment 2 Adam Barth 2011-06-28 20:34:16 PDT
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.
Comment 3 Dimitri Glazkov (Google) 2011-06-28 22:02:15 PDT
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.
Comment 4 Dimitri Glazkov (Google) 2011-06-28 22:04:46 PDT
Created attachment 99038 [details]
Patch for landing
Comment 5 Dimitri Glazkov (Google) 2011-06-28 23:02:26 PDT
Comment on attachment 99038 [details]
Patch for landing

whoops, boneheaded mistake.
Comment 6 Adam Barth 2011-06-28 23:04:02 PDT
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?
Comment 7 Dimitri Glazkov (Google) 2011-06-29 09:19:52 PDT
(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.
Comment 8 Dimitri Glazkov (Google) 2011-06-29 10:04:04 PDT
Created attachment 99109 [details]
Patch for landing
Comment 9 WebKit Review Bot 2011-06-29 10:47:09 PDT
Comment on attachment 99109 [details]
Patch for landing

Clearing flags on attachment: 99109

Committed r90026: <http://trac.webkit.org/changeset/90026>
Comment 10 WebKit Review Bot 2011-06-29 10:47:15 PDT
All reviewed patches have been landed.  Closing bug.