Bug 63596

Summary: Extract setShadowRoot function, harden it in preparation to be exposed to script.
Product: WebKit Reporter: Dimitri Glazkov (Google) <dglazkov>
Component: New BugsAssignee: Dimitri Glazkov (Google) <dglazkov>
Status: RESOLVED FIXED    
Severity: Normal CC: morrita, tkent, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 63606    
Attachments:
Description Flags
Patch
none
Patch for landing
none
Patch for landing none

Dimitri Glazkov (Google)
Reported 2011-06-28 19:25:07 PDT
Extract setShadowRoot function, harden it in preparation to be exposed to script.
Attachments
Patch (3.14 KB, patch)
2011-06-28 19:29 PDT, Dimitri Glazkov (Google) 		no flags
DetailsFormatted DiffDiff
Patch for landing (2.70 KB, patch)
2011-06-28 22:04 PDT, Dimitri Glazkov (Google) 		no flags
DetailsFormatted DiffDiff
Patch for landing (3.16 KB, patch)
2011-06-29 10:04 PDT, Dimitri Glazkov (Google) 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Dimitri Glazkov (Google)
Comment 1 2011-06-28 19:29:23 PDT
Created attachment 99026 [details] Patch
Adam Barth
Comment 2 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.
Dimitri Glazkov (Google)
Comment 3 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.
Dimitri Glazkov (Google)
Comment 4 2011-06-28 22:04:46 PDT
Created attachment 99038 [details] Patch for landing
Dimitri Glazkov (Google)
Comment 5 2011-06-28 23:02:26 PDT
Comment on attachment 99038 [details] Patch for landing whoops, boneheaded mistake.
Adam Barth
Comment 6 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?
Dimitri Glazkov (Google)
Comment 7 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.
Dimitri Glazkov (Google)
Comment 8 2011-06-29 10:04:04 PDT
Created attachment 99109 [details] Patch for landing
WebKit Review Bot
Comment 9 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>
WebKit Review Bot
Comment 10 2011-06-29 10:47:15 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.