Bug 48697

Summary: Provide means to store shadow DOM nodes on Element
Product: WebKit Reporter: Dimitri Glazkov (Google) <dglazkov>
Component: HTML DOMAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, arv, commit-queue, darin, eric.carlson, eric, hyatt, jschuh, mjs, morrita, sam, tkent
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Mac OS X 10.5   
Bug Depends on:    
Bug Blocks: 44907, 48980    
Attachments:
Description Flags
Patch
none
Patch none

Description Dimitri Glazkov (Google) 2010-10-29 16:27:42 PDT
We need a way to associate shadow DOM subtrees with an element. Currently, it's done by hard-coding a RefPtr to a node into a custom-built RenderObject. Instead, we should move it to the Element, probably to ElementRareData?
Comment 1 Alexey Proskuryakov 2010-11-01 00:27:47 PDT
Hmm, but isn't shadow tree more of a rendering concept than a DOM one? I don't know much about how it works and why, but from your description, it sounds like the current design is more logical.
Comment 2 Dimitri Glazkov (Google) 2010-11-01 09:12:11 PDT
(In reply to comment #1)
> Hmm, but isn't shadow tree more of a rendering concept than a DOM one? I don't know much about how it works and why, but from your description, it sounds like the current design is more logical.

Sorry, I abbreviated description too much :)

Shadow DOM is just DOM. In no situation having DOM nodes' lifetime being managed by a RenderObject is a good thing.

Moving shadow DOM nodes to live in DOM would allow a much more natural and less hand-coded way to build and maintain elements that need shadow DOM.
Comment 3 Alexey Proskuryakov 2010-11-01 09:50:23 PDT
I'm not sure if I'm convinced. Shadow DOM is not just DOM, it's an implementation detail of element's renderer. If anything, we should be further isolating it from regular DOM.

Its lifetime is quite different, too, because shadow DOM nodes never have wrappers.
Comment 4 Dimitri Glazkov (Google) 2010-11-01 10:02:12 PDT
(In reply to comment #3)
> I'm not sure if I'm convinced. Shadow DOM is not just DOM, it's an implementation detail of element's renderer. If anything, we should be further isolating it from regular DOM.
> 
> Its lifetime is quite different, too, because shadow DOM nodes never have wrappers.

Whoa, I thought you were asking for clarification here, not actually arguing about validity of current design.

Shadow DOM is just DOM, period. There's nothing to argue here. The way it is implemented currently is backwards.

The absence of JS wrappers has little to do with the matter, since shadow DOM elements still participate in the rest of lifecycle, including events, attachment, detachment, tree and document insertion/removal. Getting it right in the current design is extremely difficult, tricky, and brittle. Arguing for this being hand-coded instead of using standard DOM machinery seems... Halloweeney? :)
Comment 5 Alexey Proskuryakov 2010-11-01 10:27:00 PDT
Hyatt says that shadow DOM will be just DOM as we move to XBL, so I recede. I still think that for its current use, existing design is appropriate.
Comment 6 Darin Adler 2010-11-03 11:31:30 PDT
(In reply to comment #0)
> We need a way to associate shadow DOM subtrees with an element. Currently, it's done by hard-coding a RefPtr to a node into a custom-built RenderObject. Instead, we should move it to the Element, probably to ElementRareData?

Given what Hyatt has said to me many times about this, I think this is OK.

Putting the root of the shadow DOM tree into ElementRareData seems OK.
Comment 7 Dimitri Glazkov (Google) 2010-11-16 13:40:55 PST
Created attachment 74035 [details]
Patch
Comment 8 Dimitri Glazkov (Google) 2010-11-16 13:50:18 PST
Created attachment 74038 [details]
Patch
Comment 9 Dimitri Glazkov (Google) 2010-11-16 19:38:09 PST
(In reply to comment #8)
> Created an attachment (id=74038) [details]
> Patch

This one does what the bug says, but I am pursuing a slightly different approach. If it doesn't pan out, I'll flip this patch to r?.
Comment 10 Kent Tamura 2010-11-29 16:07:22 PST
Comment on attachment 74038 [details]
Patch

Looks good.  Any objection to r+?
Comment 11 Hajime Morrita 2010-11-29 17:48:58 PST
Comment on attachment 74038 [details]
Patch

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

> WebCore/dom/Element.cpp:895
> +void Element::removedFromTree(bool deep)

Just for curious: is the order correct?
In a naive view, ContainerNode::removedFromTree() should be called afte shadow->removeFromTree() because
it should be reverse order of what insertedIntoTree() does.

> WebCore/dom/Element.cpp:@
>  void Element::detach()

Same question as one for removedFromTree().
Comment 12 Dimitri Glazkov (Google) 2010-11-29 19:04:46 PST
(In reply to comment #11)
> (From update of attachment 74038 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=74038&action=review
> 
> > WebCore/dom/Element.cpp:895
> > +void Element::removedFromTree(bool deep)
> 
> Just for curious: is the order correct?
> In a naive view, ContainerNode::removedFromTree() should be called afte shadow->removeFromTree() because
> it should be reverse order of what insertedIntoTree() does.
> 
> > WebCore/dom/Element.cpp:@
> >  void Element::detach()
> 
> Same question as one for removedFromTree().

Ah, good question. Think of shadowRoot as an extra child. The order in which the children are inserted/attached is the same (and not the reverse) as their removal/detachment.
Comment 13 Hajime Morrita 2010-11-29 21:43:02 PST
(In reply to comment #12)
> Ah, good question. Think of shadowRoot as an extra child. The order in which the children are inserted/attached is the same (and not the reverse) as their removal/detachment.
Makes sense. Thank you for the clarification.
Comment 14 Dimitri Glazkov (Google) 2010-11-30 09:07:59 PST
(In reply to comment #10)
> (From update of attachment 74038 [details])
> Looks good.  Any objection to r+?

Yup, this is ready for review. I thought I could make this depend on bug 50184, to make the operation of setting shadowHost/shadowRoot atomic, but turns out I can't twine them together until all current shadow DOM consumers are converted to the new model.
Comment 15 Kent Tamura 2010-11-30 17:59:37 PST
Comment on attachment 74038 [details]
Patch

ok
Comment 16 WebKit Commit Bot 2010-11-30 23:41:50 PST
The commit-queue encountered the following flaky tests while processing attachment 74038 [details]:

fast/loader/recursive-before-unload-crash.html
fast/images/load-img-with-empty-src.html

Please file bugs against the tests.  These tests were authored by beidson@apple.com, eric@webkit.org, and mitz@webkit.org.  The commit-queue is continuing to process your patch.
Comment 17 WebKit Commit Bot 2010-12-01 05:51:59 PST
Comment on attachment 74038 [details]
Patch

Rejecting patch 74038 from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=eseidel-cq-sl', 'build', '--no-clean', '--no-update', '--build-style=both']" exit_code: 1
ERROR: Working directory has local commits, pass --force-clean to continue.

Full output: http://queues.webkit.org/results/6572010
Comment 18 WebKit Commit Bot 2010-12-01 09:18:50 PST
Comment on attachment 74038 [details]
Patch

Rejecting patch 74038 from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=eseidel-cq-sl', 'build', '--no-clean', '--no-update', '--build-style=both']" exit_code: 2
Last 500 characters of output:
/WebKitBuild/WebCore.build/Debug/WebCore.build/Objects-normal/x86_64/DOMSVGAltGlyphElement.o /Projects/CommitQueue/WebKitBuild/Debug/DerivedSources/WebCore/DOMSVGAltGlyphElement.mm normal x86_64 objective-c++ com.apple.compilers.gcc.4_2
	CompileC /Projects/CommitQueue/WebKitBuild/WebCore.build/Debug/WebCore.build/Objects-normal/x86_64/DOMSVGAngle.o /Projects/CommitQueue/WebKitBuild/Debug/DerivedSources/WebCore/DOMSVGAngle.mm normal x86_64 objective-c++ com.apple.compilers.gcc.4_2
(19 failures)


Full output: http://queues.webkit.org/results/6671012
Comment 19 Eric Seidel 2010-12-01 10:23:33 PST
More local commits trouble, see bug 49798.
Comment 20 WebKit Commit Bot 2010-12-01 11:54:41 PST
The commit-queue encountered the following flaky tests while processing attachment 74038 [details]:

fast/images/load-img-with-empty-src.html
fast/preloader/script.html

Please file bugs against the tests.  These tests were authored by abarth@webkit.org and mitz@webkit.org.  The commit-queue is continuing to process your patch.
Comment 21 WebKit Commit Bot 2010-12-01 17:12:39 PST
The commit-queue encountered the following flaky tests while processing attachment 74038 [details]:

compositing/iframes/overlapped-nested-iframes.html
fast/dom/Geolocation/timestamp.html

Please file bugs against the tests.  These tests were authored by simon.fraser@apple.com.  The commit-queue is continuing to process your patch.
Comment 22 Dimitri Glazkov (Google) 2010-12-01 19:34:39 PST
(In reply to comment #21)
> The commit-queue encountered the following flaky tests while processing attachment 74038 [details]:
> 
> compositing/iframes/overlapped-nested-iframes.html
> fast/dom/Geolocation/timestamp.html
> 
> Please file bugs against the tests.  These tests were authored by simon.fraser@apple.com.  The commit-queue is continuing to process your patch.

I don't get it. Is commit queue choking on this patch? Does anybody know how to do a bot-Heimlich?
Comment 23 Eric Seidel 2010-12-01 20:10:52 PST
The tree has been perma-red, and there are at least 3 badly flaky tests at the moment.  Basically the cq is useless when the tree is this bad.  If someone fixes it, the queue and bots will start workign again. :)
Comment 24 WebKit Commit Bot 2010-12-01 22:29:29 PST
The commit-queue encountered the following flaky tests while processing attachment 74038 [details]:

fast/frames/flattening/iframe-flattening-fixed-height.html
canvas/philip/tests/2d.text.draw.align.center.html

Please file bugs against the tests.  These tests were authored by Chang.Shu@nokia.com, kenneth@webkit.org, and kling@webkit.org.  The commit-queue is continuing to process your patch.
Comment 25 WebKit Commit Bot 2010-12-02 01:15:20 PST
Comment on attachment 74038 [details]
Patch

Clearing flags on attachment: 74038

Committed r73114: <http://trac.webkit.org/changeset/73114>
Comment 26 WebKit Commit Bot 2010-12-02 01:15:28 PST
All reviewed patches have been landed.  Closing bug.
Comment 27 WebKit Commit Bot 2010-12-02 03:12:37 PST
The commit-queue encountered the following flaky tests while processing attachment 74038 [details]:

animations/combo-transform-translate+scale.html
fast/preloader/script.html

Please file bugs against the tests.  These tests were authored by abarth@webkit.org, cmarrin@apple.com, darin@apple.com, ojan@chromium.org, and pol@apple.com.  The commit-queue is continuing to process your patch.