Bug 78069

Created attachment 126455 [details] W.I.P.

Created attachment 126457 [details] W.I.P.

Created attachment 126462 [details] Patch

Comment on attachment 126462 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=126462&action=review > Source/WebCore/ChangeLog:12 > + which are a previous element and a next element respectively. Could you elaborate why we need dually-linked list instead of single? When prev() is going to be used for example? > Source/WebCore/dom/Element.cpp:64 > +#include "ShadowRootList.h" You don't need this? > Source/WebCore/dom/Element.cpp:1160 > + return list->hasShadowRoot(); > + return false; Is this false case possible? Is it feasible to prevent this from happening? > Source/WebCore/dom/Element.cpp:1173 > + ElementRareData* data = ensureRareData(); Why not ElementRareData::ensureShadowList() ? In general, the list related method should be on ElementRareData because it owns the list. Also, Is it really worth doing to allocating an extra heap chunk only for implementing dually-linked list? > Source/WebCore/dom/Element.cpp:1187 > +void Element::setShadowRoot(PassRefPtr<ShadowRoot> prpShadowRoot, ExceptionCode& ec) Please don't use abbreviation. > Source/WebCore/dom/ElementRareData.h:74 > + ShadowRootList* m_shadowRootList; You can use OwnPtr. > Source/WebCore/dom/ShadowRoot.h:42 > + friend class WTF::DoublyLinkedListNode<ShadowRoot>; Is it possible to assert some invariant on the ShadowRoot destructor? For example, Is it possible to say that the instance is already unchained on destruction? > Source/WebCore/dom/ShadowRootList.cpp:65 > +void ShadowRootList::addShadowRoot(PassRefPtr<ShadowRoot> prpShadowRoot, ExceptionCode& ec) Please don't use abbreviation. > Source/WebCore/dom/ShadowRootList.cpp:67 > + // FIXME: We don't support multiple shadow root subtrees yet. Pelase note bug#. > Source/WebCore/dom/ShadowRootList.cpp:71 > + if (!validateShadowRoot(host()->document(), shadowRoot.get(), ec)) I hope this check also happens before ShadowRootList instance is allocated. > Source/WebCore/dom/ShadowRootList.cpp:83 > + } I feel that this kind of per-ShadowRoot cleanup doesn't fit into a method of the list. > Source/WebCore/dom/ShadowRootList.cpp:91 > + if (RefPtr<ShadowRoot> oldRoot = m_shadowRoots.removeHead()) { while()? Or let's have an assertion to make it clear that we have only one list item. > Source/WebCore/dom/ShadowRootList.cpp:93 > + Same feeling with addShadowRoot() vs ShadowRoot. > Source/WebCore/dom/ShadowRootList.cpp:94 > + // Remove from rendering tree We don't need this comment. It's obvious. > Source/WebCore/dom/ShadowRootList.h:55 > + Element* m_host; Do we need this?

Created attachment 126479 [details] Patch

Comment on attachment 126462 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=126462&action=review >> Source/WebCore/ChangeLog:12 >> + which are a previous element and a next element respectively. > > Could you elaborate why we need dually-linked list instead of single? > When prev() is going to be used for example? prev() will be used by a visual tree traversal. I will mention it in the ChangeLog. >> Source/WebCore/dom/Element.cpp:1160 >> + return false; > > Is this false case possible? > Is it feasible to prevent this from happening? Element won't have rare data every time. So I don't think we can remove this. >> Source/WebCore/dom/Element.cpp:1173 >> + ElementRareData* data = ensureRareData(); > > Why not ElementRareData::ensureShadowList() ? > In general, the list related method should be on ElementRareData because it owns the list. > > Also, Is it really worth doing to allocating an extra heap chunk only for implementing dually-linked list? Let's change not to allocate on the heap. >> Source/WebCore/dom/Element.cpp:1187 >> +void Element::setShadowRoot(PassRefPtr<ShadowRoot> prpShadowRoot, ExceptionCode& ec) > > Please don't use abbreviation. Done. >> Source/WebCore/dom/ElementRareData.h:74 >> + ShadowRootList* m_shadowRootList; > > You can use OwnPtr. Let's not allocate another heap chunk. >> Source/WebCore/dom/ShadowRoot.h:42 >> + friend class WTF::DoublyLinkedListNode<ShadowRoot>; > > Is it possible to assert some invariant on the ShadowRoot destructor? > For example, Is it possible to say that the instance is already unchained on destruction? I've added an assertion to ensure unchained. >> Source/WebCore/dom/ShadowRootList.cpp:65 >> +void ShadowRootList::addShadowRoot(PassRefPtr<ShadowRoot> prpShadowRoot, ExceptionCode& ec) > > Please don't use abbreviation. Done. >> Source/WebCore/dom/ShadowRootList.cpp:67 >> + // FIXME: We don't support multiple shadow root subtrees yet. > > Pelase note bug#. Done. >> Source/WebCore/dom/ShadowRootList.cpp:71 >> + if (!validateShadowRoot(host()->document(), shadowRoot.get(), ec)) > > I hope this check also happens before ShadowRootList instance is allocated. Done. >> Source/WebCore/dom/ShadowRootList.cpp:83 >> + } > > I feel that this kind of per-ShadowRoot cleanup doesn't fit into a method of the list. Done. >> Source/WebCore/dom/ShadowRootList.cpp:91 >> + if (RefPtr<ShadowRoot> oldRoot = m_shadowRoots.removeHead()) { > > while()? Or let's have an assertion to make it clear that we have only one list item. Done. >> Source/WebCore/dom/ShadowRootList.cpp:93 >> + > > Same feeling with addShadowRoot() vs ShadowRoot. Done. >> Source/WebCore/dom/ShadowRootList.cpp:94 >> + // Remove from rendering tree > > We don't need this comment. It's obvious. Done.

Created attachment 126492 [details] Test

Created attachment 126496 [details] Test

Created attachment 126498 [details] Patch

Comment on attachment 126498 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=126498&action=review > Source/WebCore/dom/ShadowRootList.h:38 > +class ShadowRootList { Let's make this uncopyable.

Created attachment 126695 [details] Patch

(In reply to comment #10) > (From update of attachment 126498 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=126498&action=review > > > Source/WebCore/dom/ShadowRootList.h:38 > > +class ShadowRootList { > > Let's make this uncopyable. Done. Thank for mentioning this!

Comment on attachment 126695 [details] Patch Clearing flags on attachment: 126695 Committed r107525: <http://trac.webkit.org/changeset/107525>

All reviewed patches have been landed. Closing bug.