Bug 48697

Summary: Provide means to store shadow DOM nodes on Element
Product: WebKit Reporter: Dimitri Glazkov (Google) <dglazkov@chromium.org>
Component: HTML DOMAssignee: Nobody <webkit-unassigned@lists.webkit.org>
Status: RESOLVED FIXED    
Severity: Normal CC: ap@webkit.org, arv@chromium.org, commit-queue@webkit.org, darin@apple.com, eric.carlson@apple.com, eric@webkit.org, hyatt@apple.com, jschuh@chromium.org, mjs@apple.com, morrita@google.com, sam@webkit.org, tkent@chromium.org
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 From 2010-10-29 16:27:42 PST
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 From 2010-11-01 00:27:47 PST -------
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 From 2010-11-01 09:12:11 PST -------
(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 From 2010-11-01 09:50:23 PST -------
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 From 2010-11-01 10:02:12 PST -------
(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 From 2010-11-01 10:27:00 PST -------
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 From 2010-11-03 11:31:30 PST -------
(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 From 2010-11-16 13:40:55 PST -------
Created an attachment (id=74035) [details]
Patch
------- Comment #8 From 2010-11-16 13:50:18 PST -------
Created an attachment (id=74038) [details]
Patch
------- Comment #9 From 2010-11-16 19:38:09 PST -------
(In reply to comment #8)
> Created an attachment (id=74038) [details] [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 From 2010-11-29 16:07:22 PST -------
(From update of attachment 74038 [details])
Looks good.  Any objection to r+?
------- Comment #11 From 2010-11-29 17:48:58 PST -------
(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().
------- Comment #12 From 2010-11-29 19:04:46 PST -------
(In reply to comment #11)
> (From update of attachment 74038 [details] [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 From 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 From 2010-11-30 09:07:59 PST -------
(In reply to comment #10)
> (From update of attachment 74038 [details] [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 From 2010-11-30 17:59:37 PST -------
(From update of attachment 74038 [details])
ok
------- Comment #16 From 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 From 2010-12-01 05:51:59 PST -------
(From update of attachment 74038 [details])
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 From 2010-12-01 09:18:50 PST -------
(From update of attachment 74038 [details])
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 From 2010-12-01 10:23:33 PST -------
More local commits trouble, see bug 49798.
------- Comment #20 From 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 From 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 From 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] [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 From 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 From 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 From 2010-12-02 01:15:20 PST -------
(From update of attachment 74038 [details])
Clearing flags on attachment: 74038

Committed r73114: <http://trac.webkit.org/changeset/73114>
------- Comment #26 From 2010-12-02 01:15:28 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #27 From 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.