Implement the inert attribute

third_party/WebKit/Source/core/dom/Node.cpp

Index: third_party/WebKit/Source/core/dom/Node.cpp
diff --git a/third_party/WebKit/Source/core/dom/Node.cpp b/third_party/WebKit/Source/core/dom/Node.cpp
index 7a5eca1ad8e04ddf27f1747d56fa9275aeb5f0f8..8914c2624ff508f3ab30e25e6d3f57c02852d931 100644
--- a/third_party/WebKit/Source/core/dom/Node.cpp
+++ b/third_party/WebKit/Source/core/dom/Node.cpp
@@ -774,11 +774,30 @@ bool Node::shouldHaveFocusAppearance() const {
 }
 
 bool Node::isInert() const {
+  if (!canParticipateInFlatTree())
+    return true;
+
+  // TODO(aboxhall): figure out whether to un-const all callers,
+  // or to call updateDistribution() from call sites.
+  const_cast<Node*>(this)->updateDistribution();
+

[hayato, 2017/02/16 05:52:54]

I guess Node::isInert() has a lot of (internal) callers.
We do not want to call updateDistribution always here to avoid a potential perf
regression.

We should call updateDistribution only when Node::isInert() is directly called
via JavaScript.
In this case, a distribution could be dirty. In other cases, I guess
distribution is already updated and do not have to update distribution, given
that the curretnNode::isInert() does not call it at all.

Thus, we might want to separate this function to two functions, such as:

- Node::isInertForBinding() { updateDistribution(); return isInert() }
- Node::isInert() { DCHECK(!node.needsDistributionRecalc(); ... }

That would make the contract clear.

[esprehn, 2017/02/16 06:08:12]

We should avoid adding more of these ForBindings calls, they're a burden on the
codebase and often hide bugs. There's no perf regression from calling
updateDistribution() unless distribution was actually dirty, it's a single
boolean check and an early return. If distribution was not dirty then we needed
to update it anyway.

If your code is so hot that the one branch in updateDistribution() is hurting
you then we can discuss other options.

Note that isInert is not exposed to script either, this is always an internal
caller.

[hayato, 2017/02/16 06:25:55]

Yeah, I am not a fan of adding "XXXBindng" in general, however,
updateDistribution() is not a single boolean check if the node is not connected.
If the node is not connected, we have to traverse up the tree until we reach the
shadow-including root.
I am not sure which code path is using isInert(). If isInert() is called a lot
from a node which is not connected, that might be worth adding a separate
function.

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Node....

[esprehn, 2017/02/16 06:46:54]

isInert() itself walks up the tree, so doing it inside updateDistribution() is
making it O(2N) in the depth.

[aboxhall, 2017/02/27 19:33:19]

I'm not sure where we ended up on this. Does this need a redesign?

   const HTMLDialogElement* dialog = document().activeModalDialog();
   if (dialog && this != document() &&
-      (!canParticipateInFlatTree() ||
-       !FlatTreeTraversal::containsIncludingPseudoElement(*dialog, *this)))
+      !FlatTreeTraversal::containsIncludingPseudoElement(*dialog, *this)) {
     return true;
+  }
+
+  if (RuntimeEnabledFeatures::inertAttributeEnabled()) {
+    const Element* element = this->isElementNode()
+                                 ? toElement(this)
+                                 : FlatTreeTraversal::parentElement(*this);
+    while (element) {
+      if (element->hasAttribute(inertAttr))
+        return true;
+      element = FlatTreeTraversal::parentElement(*element);

[hayato, 2017/02/16 05:52:54]

Now we are exposing Node::isInert to WebIDL. That means the current Blink's
implementation would become *spec*, regarding how inert attribute should work
for shadow trees.

I think we might rethink how inert should work for shadow trees before exposing
this behavior to the Web because the current behavior is not intended to be
exposed to the Web.

We might want to clarify the behavior in the spec before that.
The *monky patches* are here:
http://w3c.github.io/webcomponents/spec/shadow/#attributes

I guess `inert` should be treated in the same way.

[esprehn, 2017/02/16 06:08:12]

The thing exposed to idl is a boolean attribute, it's true if the attribute
exists, it has nothing to do with the isInert() internal function.
What behavior do you mean?
Yeah we need to update that.

[hayato, 2017/02/16 06:25:55]

Ah, I thought that node.inert is using this Node::isInert(), directly or
indirectly. If Node::isInert is only used internally, that should not be an
issue.

+    }
+  }
+
   return document().localOwner() && document().localOwner()->isInert();
 }