Bug 50971

Created attachment 76446 [details] Patch

Comment on attachment 76446 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=76446&action=review > WebCore/ChangeLog:5 > + Combine setShadowHost and clearShadowHost into a simpler API You mean setShadowRoot and clearShadowRoot, right? > WebCore/dom/Element.cpp:1093 > + } else if (!newRoot) I think this else is wrong. > WebCore/dom/Element.cpp:1097 > + newRoot->setShadowHost(this); If hasRareData was true and newRoot is a null pointer, then we’ll dereference the null pointer here. > WebCore/dom/Node.h:220 > + Element* shadowHost() const; > + void setShadowHost(Element*); Why are you making these member functions public? Seems unrelated to the rest of the patch?

Comment on attachment 76446 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=76446&action=review Thanks for the review! New patch coming up. >> WebCore/dom/Element.cpp:1093 >> + } else if (!newRoot) > > I think this else is wrong. You're right! I don't know what I've been drinking last night. >> WebCore/dom/Element.cpp:1097 >> + newRoot->setShadowHost(this); > > If hasRareData was true and newRoot is a null pointer, then we’ll dereference the null pointer here. Yup. My "else" above was enabling this to happen. >> WebCore/dom/Node.h:220 >> + void setShadowHost(Element*); > > Why are you making these member functions public? Seems unrelated to the rest of the patch? Because I need to use setShadowHost() from Element::setShadowRoot() and thought I might as well bring shadowHost() out with it.

Created attachment 76548 [details] Now with less confusion.

Comment on attachment 76548 [details] Now with less confusion. View in context: https://bugs.webkit.org/attachment.cgi?id=76548&action=review > WebCore/dom/Element.cpp:1093 > + if (RefPtr<Node> oldRoot = rareData()->m_shadowRoot.release()) { > + document()->removeFocusedNodeOfSubtree(oldRoot.get()); > + oldRoot->setShadowHost(0); > + if (oldRoot->inDocument()) > + oldRoot->removedFromDocument(); > + else > + oldRoot->removedFromTree(true); This feels like a helper method.

Created attachment 76776 [details] With more clarity in code.

Comment on attachment 76776 [details] With more clarity in code. View in context: https://bugs.webkit.org/attachment.cgi?id=76776&action=review Seems OK to me. > WebCore/dom/Element.cpp:1087 > + // FIXME: Because today this is never called from script directly, we don't have to worry > + // about compromising DOM tree integrity (eg. node being a parent of this). However, > + // once we implement XBL2, we will have to add integrity checks here. Can we ASSERT here instead of just a FIXME? I'm not sure what we'd ASSERT though... > WebCore/dom/Element.cpp:1089 > + RefPtr<Node> newRoot = node; I'm not sure why you use the newRoot here. We don't really need the extra ref. Maybe Darin asked you to use a local RefPtr? > WebCore/dom/Element.cpp:1093 > + ensureRareData()->m_shadowRoot = newRoot; You could just assign it here w/o the check, no? > WebCore/dom/Element.cpp:1094 > + newRoot->setShadowHost(this); Then you would check ensureRareData()->m_shadowRoot before calling ensureRareData()->m_shadowRoot->setShadowHost(this)? Donno. Your current setup is fine too.

Comment on attachment 76776 [details] With more clarity in code. View in context: https://bugs.webkit.org/attachment.cgi?id=76776&action=review Cq to the resQUE! :) >> WebCore/dom/Element.cpp:1089 >> + RefPtr<Node> newRoot = node; > > I'm not sure why you use the newRoot here. We don't really need the extra ref. Maybe Darin asked you to use a local RefPtr? Yeah, I could've danced around this with just using node.get() when assigning m_shadowRoot to avoid PassRefPtr derefing. But I generally don't like using PassRefPtrs for doing anything other than passing :) >> WebCore/dom/Element.cpp:1093 >> + ensureRareData()->m_shadowRoot = newRoot; > > You could just assign it here w/o the check, no? No, because ensureRareData always creates a new ElementRareData, which is not necessary if newRoot is 0.

The commit-queue encountered the following flaky tests while processing attachment 76776 [details]: fast/workers/storage/use-same-database-in-page-and-workers.html bug 50995 (author: dumi@chromium.org) The commit-queue is continuing to process your patch.

Comment on attachment 76776 [details] With more clarity in code. Clearing flags on attachment: 76776 Committed r74715: <http://trac.webkit.org/changeset/74715>

All reviewed patches have been landed. Closing bug.