Can you give examples of where it's being used wrong?

I'd rather not rename it. TreeScope behaves the same way as Node::document(). It returns a reference and every Node always has a Document even if isDocument() is false.

A Document is a TreeScope, I don't think renaming it something verbose is helping the code.

You can see the WIP patch in https://codereview.chromium.org/1885453002.

In some places, they use Node::treeScope() as the shortcut of getting the root node, without considering the node's root might not be Document nor Shadow Root.

e.g.

- ScopedStyleResolver. They use Node::treeScope() to get the *scope*, but the scope of stylesheets should be the root node, instead of the Document, if the node's root is not a Document.

- ContainerNode* FrameSelection::rootEditableElementOrTreeScopeRootNode()
  return node ? &node->treeScope().rootNode() : 0;



The problem is that it's difficult to tell, from the code which uses Node::treeScope(), whether they really want to get a Document even if the node's root is neither Document nor ShadowRoot, 

Thus, I would like make code more readable by explicitly stating that it is okay to get Document when the node's root is neither Document nor ShadowRoot.

I think we can replace the current usage of Node::TreeScope() by one of them:

1. TreeScope* Node::treeScopeIfInTreeScope() <- New
2. TreeScope& Node::treeScopeIfInTreeScopeOrDocument()  <- New
3. Node& Node::treeRoot()

so that we could be more confident to understand what the code really wants.

How about having these?

1. TreeScope* Node::rootAsTreeScope()
2. TreeScope& Node::rootAsTreeScopeOrDocument() (or continue to use Node::treeScope() for this meaning? However, I still think that explicit is better to avoid a wrong usage)
3. Node& Node::root()

These sound too verbose?

My guess is that we use treeScope() a lot (if not most places) where we know we are inDocument(). I'd rather see that we keep the treeScope() name with an assert added than the churn in the current CL where we would eventually go back to treeScope() in most cases.

Those names still don't make sense, a Document is a TreeScope, treeScopeOrDocument is like saying nodeOrElement, every Element is a Node, it doesn't mean anything.

I think most callers are actually correct here, ScopedStyleResolver is correct, you only use that when in the tree, so the root node is your scope. ScopedStyleResolver is also owned by a TreeScope, you never have a situation like:

<div>
   \
    <span> --> ScopedStyleResolver

the resolver is always connected directly to the Document or ShadowRoot.

If you think there's bugs lets just start by fixing those and work through some test cases. I don't really think most callers need fixing, and I don't really want the churn here renaming everything all over.

> My guess is that we use treeScope() a lot (if not most places) where we know we are inDocument().

Yeah, that's exactly what I am worrying about. That's wrong usage.
Node::treeScope() does not tell anything whether the node is inDocument or not.

I agree that treeScopeOrDocument() is not a good name.
Then, how about the Node::rootTreeScopeOrDocument()? Does it sound good?

Thus, we will have the followings:

1. TreeScope* Node::rootTreeScope()
   
   This returns nullptr if the root is neither Document nor ShadowRoot.

2. TreeScope& Node::rootTreeScopeOrDocument()
   
   This returns (rootTreeScope() ? *rootTreeScope() : document()).

   The current Node::treeScope() has this behavior.

Then, replace the current usage of Node::treeScope() with one of them, incrementally.

That still doesn't make sense, nothing that has the name treeScopeOrDocument makes sense, all documents are tree scopes. Putting an Or in the middle doesn't tell you anything.

Please post patches that fix bugs by adding inDocument() or inTreeScope() in places where you think it's missing with attached tests. Then we can consider if we need to rename things.

I got it. Yeah, it would be better to prove something before proceeding this.
Let me investigate further before renaming it to something.


My main concern is that a lot of features are not working well in Blink, historically, if a node's root is neither Document nor ShadowRoot, though most of DOM features should work regardless of its root.

I guess that one of the reasons is the current behavior of Node::treeScope() and its usage.

Yeah sorry for being hash there, we just have a lot of half done super verbose renames that didn't work out. I think treeScope() and document() actually make sense. You're saying a lot of features haven't worked well but I haven't seen that myself. Perhaps you can give me examples so I can see what the problem is. :)

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/97a40b7bad083e7f9382a92e00adf1bd650db4a5

commit 97a40b7bad083e7f9382a92e00adf1bd650db4a5
Author: hayato <hayato@chromium.org>
Date: Wed Jun 01 05:32:15 2016

Introduce Node::containingTreeScope(), which asserts that the node's root is a tree scope

The difference between Node::containingTreeScope() and Node::treeScope() is that
Node::containingTreeScope() asserts that the node is in a treeScope.
Node::treeScope() does not.

Replace some of the current usage of Node::treeScope() with
Node::containingTreeScope() if containingTreeScope() is better to express their
intention.

This CL also introduces Element::adjustedFocusedElementInTreeScope() to optimize
some code path where we can early exit if the element is not in a tree scope.

BUG= 602556 

Review-Url: https://codereview.chromium.org/1883083002
Cr-Commit-Position: refs/heads/master@{#397065}

[modify] https://crrev.com/97a40b7bad083e7f9382a92e00adf1bd650db4a5/third_party/WebKit/Source/core/css/ElementRuleCollector.cpp
[modify] https://crrev.com/97a40b7bad083e7f9382a92e00adf1bd650db4a5/third_party/WebKit/Source/core/css/resolver/StyleResolver.cpp
[modify] https://crrev.com/97a40b7bad083e7f9382a92e00adf1bd650db4a5/third_party/WebKit/Source/core/dom/ContainerNode.cpp
[modify] https://crrev.com/97a40b7bad083e7f9382a92e00adf1bd650db4a5/third_party/WebKit/Source/core/dom/Element.cpp
[modify] https://crrev.com/97a40b7bad083e7f9382a92e00adf1bd650db4a5/third_party/WebKit/Source/core/dom/Element.h
[modify] https://crrev.com/97a40b7bad083e7f9382a92e00adf1bd650db4a5/third_party/WebKit/Source/core/dom/LiveNodeListBase.cpp
[modify] https://crrev.com/97a40b7bad083e7f9382a92e00adf1bd650db4a5/third_party/WebKit/Source/core/dom/Node.cpp
[modify] https://crrev.com/97a40b7bad083e7f9382a92e00adf1bd650db4a5/third_party/WebKit/Source/core/dom/Node.h
[modify] https://crrev.com/97a40b7bad083e7f9382a92e00adf1bd650db4a5/third_party/WebKit/Source/core/dom/SelectorQuery.cpp
[modify] https://crrev.com/97a40b7bad083e7f9382a92e00adf1bd650db4a5/third_party/WebKit/Source/core/html/HTMLAnchorElement.cpp
[modify] https://crrev.com/97a40b7bad083e7f9382a92e00adf1bd650db4a5/third_party/WebKit/Source/core/html/HTMLFormControlElement.cpp
[modify] https://crrev.com/97a40b7bad083e7f9382a92e00adf1bd650db4a5/third_party/WebKit/Source/core/page/FocusController.cpp