Bug 76693 – ShadowRoot should inherit from DocumentFragment.

• Component: DOM
• Status: RESOLVED FIXED

[Hajime Morrita]

Current implementation is derived from Node. But it should inherit DocumentFragment.
For that, we need to add some missing APIs.

[Roland Steiner]

Hmmm - I wonder if it wouldn't be useful to have a ContainerNode IDL that both DocumentFragment and ShadowRoot (and Element, etc.) could derive from.

[Hajime Morrita]

> Hmmm - I wonder if it wouldn't be useful to have a ContainerNode IDL that both DocumentFragment and ShadowRoot (and Element, etc.) could derive from.
I don't expect we can do it because declaring IDL makes it visible from JS. 
Fortunately, there are few  API in DocumentFragment itself.

One specific thing in my mind here is nodeType() value.

[Hayato Ito]

Created attachment 123909 [details]
Make ShadowRoot inherit DocumentFragment

[Hayato Ito]

I added 76354 as dependency.
In theory, this issue doesn't depends on 76354. But work-in-progress patch depends on 76354.

[Hayato Ito]

Created attachment 123910 [details]
update

[Hayato Ito]

Ops. I missed this spec. Let me implement this further.

> The nodeType attribute of a ShadowRoot instance must return DOCUMENT_FRAGMENT_NODE. Accordingly, the nodeName attribute of a ShadowRoot instance must return "#document-fragment".

[Hajime Morrita]

(In reply to comment #6)
> Ops. I missed this spec. Let me implement this further.
> 
> > The nodeType attribute of a ShadowRoot instance must return DOCUMENT_FRAGMENT_NODE. Accordingly, the nodeName attribute of a ShadowRoot instance must return "#document-fragment".
Yes, please ;-)
Actually, this can be a bit tricky because we use it to check if the node is ShadowRoot. 
We need some other way to do such a check.

[Hayato Ito]

(In reply to comment #7)
> (In reply to comment #6)
> > Ops. I missed this spec. Let me implement this further.
> > 
> > > The nodeType attribute of a ShadowRoot instance must return DOCUMENT_FRAGMENT_NODE. Accordingly, the nodeName attribute of a ShadowRoot instance must return "#document-fragment".
> Yes, please ;-)
> Actually, this can be a bit tricky because we use it to check if the node is ShadowRoot. 
> We need some other way to do such a check.

Yeah, it seems that we have to have CustomGetter which returns "#document-fragment" if we are not touching existing ShadowRoot::nodeName() and ShadowRoot::nodeType().

Is there any way to achieve it?

[Hayato Ito]

I am wondering a ShadowRoot is the only exceptional Node in the meaning that internal nodeName() and exposed nodeName() used in binding are different.
Let me know if you have some other examples, which helped us to achieve this.

[Hayato Ito]

I am wondering whether there is some extended attribute like:

  readonly attribute [CustomGetter CustomGetterImplementation=nodeNameForBinding] DOMString nodeName

so that we don't have to write custom code manually.

[Hayato Ito]

I am now implementing CustomGetter for nodeName() and nodeType(). But such changes will be big since we should add files to V8 and JSC and update many build files.

So I'd like to separate the change into two patches:

1. Make ShadowRoot's IDL inherits DocumentFragment and implements querySelector and querySelectorAll, which doesn't require updating build files.
2. Implement custom nodeName() and nodeType().

(In reply to comment #10)
> I am wondering whether there is some extended attribute like:
> 
>   readonly attribute [CustomGetter CustomGetterImplementation=nodeNameForBinding] DOMString nodeName
> 
> so that we don't have to write custom code manually.

[Hayato Ito]

Created attachment 124079 [details]
Inherits DocumentFragment

[Hayato Ito]

This is the first patch, simply making ShadowRoot inherit DocumentFragment in IDL and add some tests.

[Adam Barth]

Comment on attachment 124079 [details]
Inherits DocumentFragment

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

> Source/WebCore/dom/ShadowRoot.idl:32
> -    ] ShadowRoot : Node {
> +    ] ShadowRoot : DocumentFragment {

I take it the WebCore version already inherits from DocumentFragment ?

[Hayato Ito]

Hi Adam,

(In reply to comment #14)
> (From update of attachment 124079 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=124079&action=review
> 
> > Source/WebCore/dom/ShadowRoot.idl:32
> > -    ] ShadowRoot : Node {
> > +    ] ShadowRoot : DocumentFragment {
> 
> I take it the WebCore version already inherits from DocumentFragment ?

No. ShadowRoot's WebCore doesn't inherit DocumentFragment yet.
That's my biggest concern. Could you tell me that we should sync hierarchy of WebCore and IDL?
Although I couldn't find any LayoutTest failures on this patch, I am afraid there is a potential issue.

[WebKit Review Bot]

Comment on attachment 124079 [details]
Inherits DocumentFragment

Attachment 124079 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11349304

New failing tests:
fast/dom/shadow/shadow-root-js-api.html

[Dimitri Glazkov (Google)]

(In reply to comment #15)
> Hi Adam,
> 
> (In reply to comment #14)
> > (From update of attachment 124079 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=124079&action=review
> > 
> > > Source/WebCore/dom/ShadowRoot.idl:32
> > > -    ] ShadowRoot : Node {
> > > +    ] ShadowRoot : DocumentFragment {
> > 
> > I take it the WebCore version already inherits from DocumentFragment ?
> 
> No. ShadowRoot's WebCore doesn't inherit DocumentFragment yet.
> That's my biggest concern. Could you tell me that we should sync hierarchy of WebCore and IDL?
> Although I couldn't find any LayoutTest failures on this patch, I am afraid there is a potential issue.

I don't think it's helpful to have IDL and WebCore hierarchies to be out of sync. Why aren't we starting with making ShadowRoot be a subclass of DocumentFragment?

[Adam Barth]

Yeah, you can get a bad cast if the two are out of sync.

[Hajime Morrita]

(In reply to comment #18)
> Yeah, you can get a bad cast if the two are out of sync.

Whoa, So this is going to be a big deal than I originally expected. 
Since ShadowRoot is a subclass of TreeScope for its own sake, 
Changing that hierarchy will need certain effort. 

Roland may have some opinion here.

[Roland Steiner]

(In reply to comment #6)
> The nodeType attribute of a ShadowRoot instance must return DOCUMENT_FRAGMENT_NODE. Accordingly, the nodeName attribute of a ShadowRoot instance must return "#document-fragment".

So I take adding a new SHADOW_ROOT_NODE node type has be vetoed (?).

(In reply to comment #19)
> (In reply to comment #18)
> > Yeah, you can get a bad cast if the two are out of sync.
> 
> Whoa, So this is going to be a big deal than I originally expected. 
> Since ShadowRoot is a subclass of TreeScope for its own sake, 
> Changing that hierarchy will need certain effort. 
> 
> Roland may have some opinion here.

Hm, ad hoc I would say the best approach I can think of would be making DocumentFragment a TreeScope as well - i.e.:

Node
  ContainerNode
    Element
    TreeScope
      Document
      DocumentFragment
        ShadowRoot

IMHO it would be only natural to have getElementById, getElementsByTagName, et al available on DocumentFragment. However, this is certainly not a small change, and means that moving Nodes into/out of a DocumentFragment would require TreeScope fiddling (but then we have to traverse the tree anyway to set/clear the inDocument flag).

Alternatively we could make TreeScope a member rather than a base class, but I think that approach is less elegant.

[Roland Steiner]

(In reply to comment #8)
> Yeah, it seems that we have to have CustomGetter which returns "#document-fragment" if we are not touching existing ShadowRoot::nodeName() and ShadowRoot::nodeType().
> 
> Is there any way to achieve it?

Whoa, why can't we simply change nodeType() and nodeName() to return DOCUMENT_FRAGMENT_NODE and "#document-fragment", respectively? Internally, we can always simply use isShadowRoot() to discern between ShadowRoot and DocumentFragment. 

Granted, it sucks to have to remove all instances of SHADOW_ROOT_NODE again, but except for a few cases (e.g., Range::insertNode, Document::importNode), SHADOW_ROOT_NODE mostly follows the implementation cases of DOCUMENT_FRAGMENT_NODE anyway.

[Hayato Ito]

Okay. I realized that we must make ShadowRoot inherit DocumentFragment in WebCore at first.

Roland's suggestion (DocumentFragment inherits TreeScope) looks goods to me.

(In reply to comment #21)
> (In reply to comment #8)
> > Yeah, it seems that we have to have CustomGetter which returns "#document-fragment" if we are not touching existing ShadowRoot::nodeName() and ShadowRoot::nodeType().
> > 
> > Is there any way to achieve it?
> 
> Whoa, why can't we simply change nodeType() and nodeName() to return DOCUMENT_FRAGMENT_NODE and "#document-fragment", respectively? Internally, we can always simply use isShadowRoot() to discern between ShadowRoot and DocumentFragment. 

We can. Although we have to update case/switch statements which uses SHADOW_ROOT_NODE in many places, we can do it.

> 
> Granted, it sucks to have to remove all instances of SHADOW_ROOT_NODE again, but except for a few cases (e.g., Range::insertNode, Document::importNode), SHADOW_ROOT_NODE mostly follows the implementation cases of DOCUMENT_FRAGMENT_NODE anyway.

[Hayato Ito]

I have a couple of questions before I start actual works:

1. DocumentFragment should be 'lightweight' according to the spec: http://www.w3.org/TR/DOM-Level-3-Core/core.html#ID-B63ED1A3
  If we make DocumentFragment inherit TreeScope, will DocmentFragment be heavyweight? Since TreeScope has some non-lightweight member variables, I am afraid the impact of performance. We must be careful to handle this issue.

2. Are all operations that are permitted on DocumentFragment also applicable to ShadowRoot?
  I think this assumption is implicitly declared once the spec says ShadowRoot's nodeType is DOCUMENT_FRAGMENT_NODE.

The spec says:
> When a DocumentFragment is inserted into a Document (or indeed any other Node that may take children) the children of the DocumentFragment and not the DocumentFragment itself are inserted into the Node. This makes the DocumentFragment very useful when the user wishes to create nodes that are siblings; the DocumentFragment acts as the parent of these nodes so that the user can use the standard methods from the Node interface, such as Node.insertBefore and Node.appendChild.

  As well as we can insert DocumentFragment into arbitrary Node, we can insert ShadowRoot into arbitrary Node, cannot we?

[Hayato Ito]

Created attachment 124268 [details]
WIP. Make ShadowRoot inherit DocumentFragment. Some tests should be rebaselined.

[Hayato Ito]

I've uploaded the work-in-progress patch of the initial attempt. Most layout tests seems passes. Some tests needs rebaseline.
Please enumerate what should be done further and what issues remain.

[Roland Steiner]

(In reply to comment #22)
> Roland's suggestion (DocumentFragment inherits TreeScope) looks goods to me.

Thanks, but I think this is a non-trivial change on which definitely should get broader consensus on before proceeding!

As a first step, we'd need to make DocumentFragment inherit from TreeScope. As you write, this makes DocumentFragment "heavier", and probably is a larger refactoring of its own. For one, DocumentFragment would then implicitly track element IDs, which it currently doesn't. I don't see this as a mistake, but I can also easily imagine people getting their torches and pitchforks out of the closet over it... How much it becoming "heavier" is an actual problem otherwise I can't say.

Second, we'd need to make ShadowRoot inherit from DocumentFragment. Once the first step is done, this should be rather simple.

However, as you also note, ShadowRoot does behave differently from DocumentFragment in some regards: I personally cannot see a ShadowRoot being moved around willy-nilly. IMHO attempting to call insertBefore() or appendChild() with a ShadowRoot should throw (and indicate that something's seriously wrong). OTOH, the JavaScript user cannot even easily distinguish between a DocumentFragment ShadowRoot node so he managed to get his grubby hands on one somehow: AFAICT you'd need hacks like 'if (node.nodeType() == DOCUMENT_FRAGMENT_NODE && node.element)'. Altogether reasons why I personally would have preferred to keep the SHADOW_ROOT_NODE node type.

> (In reply to comment #21)
> We can. Although we have to update case/switch statements which uses SHADOW_ROOT_NODE in many places, we can do it.

I would make this a separate bug - this should be quite orthogonal to the actual inheritance question.

[Roland Steiner]

(In reply to comment #26)
> ...easily distinguish between a DocumentFragment ShadowRoot node 

This should be: "...between a DocumentFragment AND a ShadowRoot node..."

[Hayato Ito]

This is completely off-topic, but I came up a crazy idea while I was working on this issue.
ShadowRoot and DocumentFragment is very similar and looks exchangeable for me.

So instead of current ShadowRoot API:

  new ShadowRoot(element)

How about this? :)

interface Element {
  DocumentFragment addShadowRoot(in DocumentFragment shadowRoot) raises(DOMException);
  void removeShadowRoot(in DocumentFragment shadowRoot) raises(DOMException);
}

Usage:
  var fragment = document.createDocumentFragment()
  element.addShadowRoot(fragment);

This sounds crazy?

[Roland Steiner]

(In reply to comment #28)
A shadow root needs to be a TreeScope, in order to be able to track IDs separately from the document. If DocumentFragment becomes a TreeScope, this would be handled implicitly. 
We'd also need to find a way for the shadow root to refer back to the host element (currently 'element' in the ShadowRoot IDL).

[Hajime Morrita]

If we need to talk about changing the spec, especially for one not small, let's talk at somewhere else.
Bugzilla isn't good for such purpose.

[Hayato Ito]

I have yet another idea: TreeScope should be separated from the class hierarchy.
Document and DocumentFragment should inherit it using multiple-inheritance.

Node
  ContainerNode
    Element
      Document (also inherits TreeScope)
      DocumentFragment
        ShadowRoot (also inherits TreeScope)

Is there strong restriction that TreeScope should inherit Element?
If there is nothing, it's time to use multiple-inheritance, isn't it?

I know multiple inheritance is discouraged in general. I hate it, but it seems natural to use it.

[Dimitri Glazkov (Google)]

(In reply to comment #31)
> I have yet another idea: TreeScope should be separated from the class hierarchy.
> Document and DocumentFragment should inherit it using multiple-inheritance.
> 
> Node
>   ContainerNode
>     Element
>       Document (also inherits TreeScope)
>       DocumentFragment
>         ShadowRoot (also inherits TreeScope)
> 
> Is there strong restriction that TreeScope should inherit Element?
> If there is nothing, it's time to use multiple-inheritance, isn't it?
> 
> I know multiple inheritance is discouraged in general. I hate it, but it seems natural to use it.

As long as we we don't have virtual methods on TreeScope, I think this is fine.

[Hayato Ito]

Created attachment 124710 [details]
Make ShadowRoot inherit DocumentFragment. TreeScope is now stand-alone class

[WebKit Review Bot]

Attachment 124710 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9

Updating OpenSource
First, rewinding head to replay your work on top of it...
Applying: Fix compilation errors on build-webkit --debug --no-workers on mac.
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging LayoutTests/ChangeLog
CONFLICT (content): Merge conflict in LayoutTests/ChangeLog
Auto-merging LayoutTests/platform/qt/Skipped
CONFLICT (content): Merge conflict in LayoutTests/platform/qt/Skipped
Auto-merging Source/WebCore/ChangeLog
CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog
Failed to merge in the changes.
Patch failed at 0001 Fix compilation errors on build-webkit --debug --no-workers on mac.

When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".

rebase refs/remotes/origin/master: command returned error: 1

Died at Tools/Scripts/update-webkit line 164.


If any of these errors are false positives, please file a bug against check-webkit-style.

[Hayato Ito]

Created attachment 124712 [details]
Rebased

[Hayato Ito]

Thank you. I've uploaded the patch for the review.
Although I might polish further, it'd be nice to have early feedbacks.

(In reply to comment #32)
> (In reply to comment #31)
> > I have yet another idea: TreeScope should be separated from the class hierarchy.
> > Document and DocumentFragment should inherit it using multiple-inheritance.
> > 
> > Node
> >   ContainerNode
> >     Element
> >       Document (also inherits TreeScope)
> >       DocumentFragment
> >         ShadowRoot (also inherits TreeScope)
> > 
> > Is there strong restriction that TreeScope should inherit Element?
> > If there is nothing, it's time to use multiple-inheritance, isn't it?
> > 
> > I know multiple inheritance is discouraged in general. I hate it, but it seems natural to use it.
> 
> As long as we we don't have virtual methods on TreeScope, I think this is fine.

[WebKit Review Bot]

Attachment 124712 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9

Updating OpenSource
First, rewinding head to replay your work on top of it...
Applying: Fix compilation errors on build-webkit --debug --no-workers on mac.
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging LayoutTests/ChangeLog
CONFLICT (content): Merge conflict in LayoutTests/ChangeLog
Auto-merging LayoutTests/platform/qt/Skipped
CONFLICT (content): Merge conflict in LayoutTests/platform/qt/Skipped
Auto-merging Source/WebCore/ChangeLog
CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog
Failed to merge in the changes.
Patch failed at 0001 Fix compilation errors on build-webkit --debug --no-workers on mac.

When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".

rebase refs/remotes/origin/master: command returned error: 1

Died at Tools/Scripts/update-webkit line 164.


If any of these errors are false positives, please file a bug against check-webkit-style.

[Hayato Ito]

It seems there is something wrong in style-queue bot. This patch doesn't contain that files...

> CONFLICT (content): Merge conflict in LayoutTests/platform/qt/Skipped

(In reply to comment #37)
> Attachment 124712 [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9
> 
> Updating OpenSource
> First, rewinding head to replay your work on top of it...
> Applying: Fix compilation errors on build-webkit --debug --no-workers on mac.
> Using index info to reconstruct a base tree...
> Falling back to patching base and 3-way merge...
> Auto-merging LayoutTests/ChangeLog
> CONFLICT (content): Merge conflict in LayoutTests/ChangeLog
> Auto-merging LayoutTests/platform/qt/Skipped
> CONFLICT (content): Merge conflict in LayoutTests/platform/qt/Skipped
> Auto-merging Source/WebCore/ChangeLog
> CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog
> Failed to merge in the changes.
> Patch failed at 0001 Fix compilation errors on build-webkit --debug --no-workers on mac.
> 
> When you have resolved this problem run "git rebase --continue".
> If you would prefer to skip this patch, instead run "git rebase --skip".
> To restore the original branch and stop rebasing run "git rebase --abort".
> 
> rebase refs/remotes/origin/master: command returned error: 1
> 
> Died at Tools/Scripts/update-webkit line 164.
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

[Dimitri Glazkov (Google)]

Comment on attachment 124712 [details]
Rebased

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

Looks like a nice improvement. It's along the lines of ScriptExecutionContext and other similar abstractions we've done in WebKit. A couple of nagging questions though:

> Source/WebCore/dom/Document.cpp:376
> +    : ContainerNode(0), TreeScope(this)

TreeScope goes on new line.

> Source/WebCore/dom/Document.cpp:647
> +    Node* node = root ? root->attachedNode() : 0;

Can root ever be 0? If not, we should just assert here.

> Source/WebCore/dom/TreeScope.cpp:-50
> -    if (hasRareData())
> -        clearRareData();

Oops, where does this go now?

> Source/WebCore/dom/TreeScope.h:72
> +    Node* attachedNode() const { return m_attachedNode; };

I think you can just call it node(). "attach" implies some sort of attachment/detachment machinery.

> Source/WebCore/dom/TreeScope.h:81
> +    Node* m_attachedNode;

m_node;

[Hayato Ito]

Created attachment 124879 [details]
Another iteration.

[Hayato Ito]

Comment on attachment 124712 [details]
Rebased

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

>> Source/WebCore/dom/Document.cpp:376
>> +    : ContainerNode(0), TreeScope(this)
> 
> TreeScope goes on new line.

Done.

>> Source/WebCore/dom/Document.cpp:647
>> +    Node* node = root ? root->attachedNode() : 0;
> 
> Can root ever be 0? If not, we should just assert here.

root can be 0 in the previous patch.
But I changed caller side so that root never can be 0. ASSERT is added.

>> Source/WebCore/dom/TreeScope.cpp:-50
>> -        clearRareData();
> 
> Oops, where does this go now?

Ops. I wrongly assumed we don't need this anymore because Node's destructor does it.
It seems we need some tweaks in TreeScope's destructor also. I recovered it.

>> Source/WebCore/dom/TreeScope.h:72
>> +    Node* attachedNode() const { return m_attachedNode; };
> 
> I think you can just call it node(). "attach" implies some sort of attachment/detachment machinery.

Done. I renamed it.

>> Source/WebCore/dom/TreeScope.h:81
>> +    Node* m_attachedNode;
> 
> m_node;

Done.

[Roland Steiner]

Comment on attachment 124879 [details]
Another iteration.

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

> Source/WebCore/dom/TreeScope.h:72
> +    Node* node() const { return m_node; };

I think this should be using and returning a ContainerNode* - I don't think a TreeScope that consists of a single Node would ever make sense. Likewise, since this must be the root node of a tree, I would suggest naming the function root() or rootNode(): "treeScope->rootNode()" is IMHO clearer than "treeScope->node()".

[Hayato Ito]

I agree. rootNode() sounds good name for me than node(). Let me update the patch.

(In reply to comment #42)
> (From update of attachment 124879 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=124879&action=review
> 
> > Source/WebCore/dom/TreeScope.h:72
> > +    Node* node() const { return m_node; };
> 
> I think this should be using and returning a ContainerNode* - I don't think a TreeScope that consists of a single Node would ever make sense. Likewise, since this must be the root node of a tree, I would suggest naming the function root() or rootNode(): "treeScope->rootNode()" is IMHO clearer than "treeScope->node()".

[Hayato Ito]

Created attachment 124887 [details]
Another iteration.

[Roland Steiner]

Comment on attachment 124887 [details]
Another iteration.

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

(Sorry for the piece-meal review! ^_^;)

> Source/WebCore/dom/Document.cpp:646
>  void Document::buildAccessKeyMap(TreeScope* root)

Small nit: IMHO the parameter should be renamed 'scope', as it's no longer really a node.

> Source/WebCore/dom/TreeScope.cpp:54
> +        rootNode()->clearRareData();

Looking closer at this, this looks fragile. (Refer to http://trac.webkit.org/changeset/90378 for the original reason of the code). Since the patch now uses multiple inheritance, this now relies on the TreeScope destructor to run before the Node destructor. In turn, this relies on the written order of the base classes. So if anyone ever happens to change that, things will break again.
Although duplicating code, moving this to the destructors of the derived classes (i.e., Document and ShadowRoot), with an appropriate comment, would be safer.

[Hayato Ito]

Thank you for the review.

(In reply to comment #45)
> (From update of attachment 124887 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=124887&action=review
> 
> (Sorry for the piece-meal review! ^_^;)
> 
> > Source/WebCore/dom/Document.cpp:646
> >  void Document::buildAccessKeyMap(TreeScope* root)
> 
> Small nit: IMHO the parameter should be renamed 'scope', as it's no longer really a node.
> 

Okay. Let me rename.

> > Source/WebCore/dom/TreeScope.cpp:54
> > +        rootNode()->clearRareData();
> 
> Looking closer at this, this looks fragile. (Refer to http://trac.webkit.org/changeset/90378 for the original reason of the code). Since the patch now uses multiple inheritance, this now relies on the TreeScope destructor to run before the Node destructor. In turn, this relies on the written order of the base classes. So if anyone ever happens to change that, things will break again.
> Although duplicating code, moving this to the destructors of the derived classes (i.e., Document and ShadowRoot), with an appropriate comment, would be safer.


Yeah, I thought it at first and was aware such issues. but I chose avoiding duplications for laziness.
Okay. Let me rewrite it as you suggested.

[Hayato Ito]

Created attachment 124903 [details]
another iteration

[Dimitri Glazkov (Google)]

Comment on attachment 124903 [details]
another iteration

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

Looks good, but for some reason there seems to be a build error on mac EWS?

> Source/WebCore/dom/TreeScope.cpp:34
> +#include "Node.h"

ContainerNode includes Node, so you probably don't need this include.

[Darin Adler]

Comment on attachment 124903 [details]
another iteration

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

> Source/WebCore/dom/Document.cpp:577
> +    if (hasRareData())
> +        clearRareData();

This very mysterious bit of code needs a “why” comment. Since this is called in Node, there must be a very good reason why it’s critical to call it earlier here, and the comment should state that reason. It’s sort of obvious to me why, but I don’t think it would be obvious to most programmers.

> Source/WebCore/dom/Document.cpp:661
> +        if (element->shadowRoot())
> +            buildAccessKeyMap(element->shadowRoot());

Should put this in a local variable so we don’t do it twice.

> Source/WebCore/dom/ShadowRoot.cpp:56
> +    if (hasRareData())
> +        clearRareData();

Same comment about this as in Document.

> Source/WebCore/dom/TreeScope.h:41
> +// A class which inherits both Node and TreeScope must call clearRareData() in its destructor.
> +// See http://trac.webkit.org/changeset/90378 for the reason.

This comment explains the calls to clearRareData, but it seems likely that people reading the destructors won’t see the comment. I also think it’s unnecessarily indirect to give a trac link instead of having a comment somewhere in the code.

[Hayato Ito]

Thank you for the review. If mac EWS build won't complain for the next patch, I'll land this.

(In reply to comment #48)
> (From update of attachment 124903 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=124903&action=review
> 
> Looks good, but for some reason there seems to be a build error on mac EWS?

Hmm. I am not sure why that happened. I could not read a reason clearly from the build output.
Let me retry mac EWS later.

> 
> > Source/WebCore/dom/TreeScope.cpp:34
> > +#include "Node.h"
> 
> ContainerNode includes Node, so you probably don't need this include.

Done.

(In reply to comment #49)
> (From update of attachment 124903 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=124903&action=review
> 
> > Source/WebCore/dom/Document.cpp:577
> > +    if (hasRareData())
> > +        clearRareData();
> 
> This very mysterious bit of code needs a “why” comment. Since this is called in Node, there must be a very good reason why it’s critical to call it earlier here, and the comment should state that reason. It’s sort of obvious to me why, but I don’t think it would be obvious to most programmers.

Done. I've added the comment here as well as the comment on TreeScope.h.

> 
> > Source/WebCore/dom/Document.cpp:661
> > +        if (element->shadowRoot())
> > +            buildAccessKeyMap(element->shadowRoot());
> 
> Should put this in a local variable so we don’t do it twice.

Done.

> 
> > Source/WebCore/dom/ShadowRoot.cpp:56
> > +    if (hasRareData())
> > +        clearRareData();
> 
> Same comment about this as in Document.

Done.

> 
> > Source/WebCore/dom/TreeScope.h:41
> > +// A class which inherits both Node and TreeScope must call clearRareData() in its destructor.
> > +// See http://trac.webkit.org/changeset/90378 for the reason.
> 
> This comment explains the calls to clearRareData, but it seems likely that people reading the destructors won’t see the comment. I also think it’s unnecessarily indirect to give a trac link instead of having a comment somewhere in the code.

Done. I've removed the link and put a comment here.

[Hayato Ito]

Created attachment 125065 [details]
let me retry for mac-ews

[Hayato Ito]

I am sure that mac ews issue was caused by unnecessary [OldStyleObjC] in ShadowRoot.idl.
That was removed in  http://trac.webkit.org/changeset/106449.

So I am sure it's safe to land this patch now. But let me watch mac ews again for safety.

[Hayato Ito]

Mac ews became green. Let me land this patch manually.

[Hayato Ito]

Committed r106530: <http://trac.webkit.org/changeset/106530>