Bug 48697

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.

(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.

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.

(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? :)

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.

(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.

Created attachment 74035 [details] Patch

Created attachment 74038 [details] Patch

(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 on attachment 74038 [details] Patch Looks good. Any objection to r+?

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().

(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.

(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.

(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 on attachment 74038 [details] Patch ok

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 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 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

More local commits trouble, see bug 49798.

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.

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.

(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?

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. :)

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 on attachment 74038 [details] Patch Clearing flags on attachment: 74038 Committed r73114: <http://trac.webkit.org/changeset/73114>

All reviewed patches have been landed. Closing bug.

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.