Bug 76693

Summary: ShadowRoot should inherit from DocumentFragment.
Product: WebKit Reporter: Hajime Morrita <morrita>
Component: DOMAssignee: Hayato Ito <hayato>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, dglazkov, hayato, ojan, rolandsteiner, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 76354    
Bug Blocks: 63601, 77511    
Attachments:
Description Flags
Make ShadowRoot inherit DocumentFragment
none
update
none
Inherits DocumentFragment
none
WIP. Make ShadowRoot inherit DocumentFragment. Some tests should be rebaselined.
none
Make ShadowRoot inherit DocumentFragment. TreeScope is now stand-alone class
none
Rebased
none
Another iteration.
none
Another iteration.
none
another iteration
darin: review+
let me retry for mac-ews none

Description Hajime Morrita 2012-01-20 03:20:36 PST
Current implementation is derived from Node. But it should inherit DocumentFragment.
For that, we need to add some missing APIs.
Comment 1 Roland Steiner 2012-01-23 01:01:27 PST
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.
Comment 2 Hajime Morrita 2012-01-23 11:53:51 PST
> 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.
Comment 3 Hayato Ito 2012-01-25 03:01:38 PST
Created attachment 123909 [details]
Make ShadowRoot inherit DocumentFragment
Comment 4 Hayato Ito 2012-01-25 03:10:16 PST
I added 76354 as dependency.
In theory, this issue doesn't depends on 76354. But work-in-progress patch depends on 76354.
Comment 5 Hayato Ito 2012-01-25 03:13:21 PST
Created attachment 123910 [details]
update
Comment 6 Hayato Ito 2012-01-25 03:23:04 PST
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".
Comment 7 Hajime Morrita 2012-01-25 13:29:25 PST
(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.
Comment 8 Hayato Ito 2012-01-25 23:14:32 PST
(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?
Comment 9 Hayato Ito 2012-01-25 23:19:45 PST
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.
Comment 10 Hayato Ito 2012-01-26 00:11:11 PST
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.
Comment 11 Hayato Ito 2012-01-26 01:16:23 PST
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.
Comment 12 Hayato Ito 2012-01-26 01:39:48 PST
Created attachment 124079 [details]
Inherits DocumentFragment
Comment 13 Hayato Ito 2012-01-26 01:42:15 PST
This is the first patch, simply making ShadowRoot inherit DocumentFragment in IDL and add some tests.
Comment 14 Adam Barth 2012-01-26 01:46:06 PST
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 ?
Comment 15 Hayato Ito 2012-01-26 02:34:33 PST
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.
Comment 16 WebKit Review Bot 2012-01-26 04:41:14 PST
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
Comment 17 Dimitri Glazkov (Google) 2012-01-26 09:16:15 PST
(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?
Comment 18 Adam Barth 2012-01-26 10:05:19 PST
Yeah, you can get a bad cast if the two are out of sync.
Comment 19 Hajime Morrita 2012-01-26 15:29:52 PST
(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.
Comment 20 Roland Steiner 2012-01-26 18:31:12 PST
(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.
Comment 21 Roland Steiner 2012-01-26 18:43:17 PST
(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.
Comment 22 Hayato Ito 2012-01-26 21:08:07 PST
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.
Comment 23 Hayato Ito 2012-01-26 23:03:16 PST
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?
Comment 24 Hayato Ito 2012-01-27 00:30:38 PST
Created attachment 124268 [details]
WIP. Make ShadowRoot inherit DocumentFragment. Some tests should be rebaselined.
Comment 25 Hayato Ito 2012-01-27 00:34:13 PST
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.
Comment 26 Roland Steiner 2012-01-27 01:06:00 PST
(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.
Comment 27 Roland Steiner 2012-01-27 01:07:53 PST
(In reply to comment #26)
> ...easily distinguish between a DocumentFragment ShadowRoot node 

This should be: "...between a DocumentFragment AND a ShadowRoot node..."
Comment 28 Hayato Ito 2012-01-27 01:29:59 PST
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?
Comment 29 Roland Steiner 2012-01-27 02:00:57 PST
(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).
Comment 30 Hajime Morrita 2012-01-27 13:39:45 PST
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.
Comment 31 Hayato Ito 2012-01-30 04:58:45 PST
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.
Comment 32 Dimitri Glazkov (Google) 2012-01-30 09:29:34 PST
(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.
Comment 33 Hayato Ito 2012-01-31 05:04:42 PST
Created attachment 124710 [details]
Make ShadowRoot inherit DocumentFragment. TreeScope is now stand-alone class
Comment 34 WebKit Review Bot 2012-01-31 05:06:52 PST
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.
Comment 35 Hayato Ito 2012-01-31 05:29:35 PST
Created attachment 124712 [details]
Rebased
Comment 36 Hayato Ito 2012-01-31 05:31:28 PST
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.
Comment 37 WebKit Review Bot 2012-01-31 05:32:18 PST
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.
Comment 38 Hayato Ito 2012-01-31 05:34:37 PST
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.
Comment 39 Dimitri Glazkov (Google) 2012-01-31 09:56:14 PST
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;
Comment 40 Hayato Ito 2012-01-31 21:31:49 PST
Created attachment 124879 [details]
Another iteration.
Comment 41 Hayato Ito 2012-01-31 21:36:10 PST
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.
Comment 42 Roland Steiner 2012-01-31 21:51:57 PST
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()".
Comment 43 Hayato Ito 2012-01-31 22:26:07 PST
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()".
Comment 44 Hayato Ito 2012-01-31 22:57:30 PST
Created attachment 124887 [details]
Another iteration.
Comment 45 Roland Steiner 2012-01-31 23:42:52 PST
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.
Comment 46 Hayato Ito 2012-01-31 23:57:25 PST
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.
Comment 47 Hayato Ito 2012-02-01 01:15:01 PST
Created attachment 124903 [details]
another iteration
Comment 48 Dimitri Glazkov (Google) 2012-02-01 09:10:49 PST
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.
Comment 49 Darin Adler 2012-02-01 11:48:40 PST
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.
Comment 50 Hayato Ito 2012-02-01 19:23:22 PST
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.
Comment 51 Hayato Ito 2012-02-01 19:26:08 PST
Created attachment 125065 [details]
let me retry for mac-ews
Comment 52 Hayato Ito 2012-02-01 19:40:59 PST
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.
Comment 53 Hayato Ito 2012-02-01 19:55:42 PST
Mac ews became green. Let me land this patch manually.
Comment 54 Hayato Ito 2012-02-01 20:21:57 PST
Committed r106530: <http://trac.webkit.org/changeset/106530>