Implement the inert attribute

Implement the inert attribute: https://github.com/WICG/inert#tldr

https://discourse.wicg.io/t/inert-attribute/1838/

BUG=692360
Review-Url: https://codereview.chromium.org/2088453002
Cr-Original-Commit-Position: refs/heads/master@{#474153}
Committed: https://chromium.googlesource.com/chromium/src/+/cca67d31f4a0b4ec2c20090a8bfc38c464274ab2
Review-Url: https://codereview.chromium.org/2088453002
Cr-Commit-Position: refs/heads/master@{#474598}
Committed: https://chromium.googlesource.com/chromium/src/+/58a3e83e1325c8a366941564ca8ce096bb1345df

[esprehn, 2016-06-27 22:27:14]

esprehn@chromium.org changed reviewers:
+ esprehn@chromium.org

[esprehn, 2016-06-27 22:27:16]

https://codereview.chromium.org/2088453002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Node.cpp (right):

https://codereview.chromium.org/2088453002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Node.cpp:775: const Node* n = this;
node, don't abbreviate

https://codereview.chromium.org/2088453002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Node.cpp:777: if (n->isElementNode() &&
toElement(n)->hasAttribute(HTMLNames::inertAttr))
I think you want instead use node.parentOrShadowHostElement() ?

That'll loop over all of the ancestor elements

https://codereview.chromium.org/2088453002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLElement.cpp (right):

https://codereview.chromium.org/2088453002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLElement.cpp:623: bool
HTMLElement::inert() const
This can just use Reflect in the idl I think, you don't need to manually write
it.

[Reflect,RuntimeEnabled=inertAttribute] attribute boolean inert;

https://codereview.chromium.org/2088453002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLElement.cpp:628: void
HTMLElement::setInert(const bool enabled, ExceptionState& exceptionState)
ditto

https://codereview.chromium.org/2088453002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLElement.h (right):

https://codereview.chromium.org/2088453002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLElement.h:52: void setInert(const bool
enabled, ExceptionState&);
no const on input primitive types

[aboxhall, 2016-06-27 23:59:22]

One question while addressing other comments.

https://codereview.chromium.org/2088453002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Node.cpp (right):

https://codereview.chromium.org/2088453002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Node.cpp:777: if (n->isElementNode() &&
toElement(n)->hasAttribute(HTMLNames::inertAttr))
On 2016/06/27 22:27:15, esprehn wrote:
> I think you want instead use node.parentOrShadowHostElement() ?
> 
> That'll loop over all of the ancestor elements

Where do I want to do that, sorry?

[aboxhall, 2016-06-28 00:01:48]

Description was changed from

==========
Implement the inert attribute

BUG=
==========

to

==========
Implement the inert attribute

BUG=618746
==========

[esprehn, 2016-06-28 18:55:07]

https://codereview.chromium.org/2088453002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Node.cpp (right):

https://codereview.chromium.org/2088453002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Node.cpp:777: if (n->isElementNode() &&
toElement(n)->hasAttribute(HTMLNames::inertAttr))
On 2016/06/27 at 23:59:22, aboxhall wrote:
> On 2016/06/27 22:27:15, esprehn wrote:
> > I think you want instead use node.parentOrShadowHostElement() ?
> > 
> > That'll loop over all of the ancestor elements
> 
> Where do I want to do that, sorry?

Your loop should be over parentOrShadowHostElement() instead of
parentOrShadowHostNode().

[aboxhall, 2016-06-29 00:13:12]

Had to update the tests to use testharness; other comments addressed as well.

https://codereview.chromium.org/2088453002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Node.cpp (right):

https://codereview.chromium.org/2088453002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Node.cpp:775: const Node* n = this;
On 2016/06/27 22:27:15, esprehn wrote:
> node, don't abbreviate

Done.

https://codereview.chromium.org/2088453002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Node.cpp:777: if (n->isElementNode() &&
toElement(n)->hasAttribute(HTMLNames::inertAttr))
On 2016/06/28 18:55:07, esprehn wrote:
> On 2016/06/27 at 23:59:22, aboxhall wrote:
> > On 2016/06/27 22:27:15, esprehn wrote:
> > > I think you want instead use node.parentOrShadowHostElement() ?
> > > 
> > > That'll loop over all of the ancestor elements
> > 
> > Where do I want to do that, sorry?
> 
> Your loop should be over parentOrShadowHostElement() instead of
> parentOrShadowHostNode().
> 

Done.

https://codereview.chromium.org/2088453002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLElement.cpp (right):

https://codereview.chromium.org/2088453002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLElement.cpp:623: bool
HTMLElement::inert() const
On 2016/06/27 22:27:15, esprehn wrote:
> This can just use Reflect in the idl I think, you don't need to manually write
> it.
> 
> [Reflect,RuntimeEnabled=inertAttribute] attribute boolean inert;

Done.

https://codereview.chromium.org/2088453002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLElement.cpp:628: void
HTMLElement::setInert(const bool enabled, ExceptionState& exceptionState)
On 2016/06/27 22:27:15, esprehn wrote:
> ditto

Done.

https://codereview.chromium.org/2088453002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLElement.h (right):

https://codereview.chromium.org/2088453002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLElement.h:52: void setInert(const bool
enabled, ExceptionState&);
On 2016/06/27 22:27:16, esprehn wrote:
> no const on input primitive types

n/a

[aboxhall, 2016-06-29 00:13:13]

Had to update the tests to use testharness; other comments addressed as well.

https://codereview.chromium.org/2088453002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLElement.cpp (right):

https://codereview.chromium.org/2088453002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLElement.cpp:623: bool
HTMLElement::inert() const
On 2016/06/27 22:27:15, esprehn wrote:
> This can just use Reflect in the idl I think, you don't need to manually write
> it.
> 
> [Reflect,RuntimeEnabled=inertAttribute] attribute boolean inert;

Done.

https://codereview.chromium.org/2088453002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLElement.cpp:628: void
HTMLElement::setInert(const bool enabled, ExceptionState& exceptionState)
On 2016/06/27 22:27:15, esprehn wrote:
> ditto

Done.

https://codereview.chromium.org/2088453002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLElement.h (right):

https://codereview.chromium.org/2088453002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLElement.h:52: void setInert(const bool
enabled, ExceptionState&);
On 2016/06/27 22:27:16, esprehn wrote:
> no const on input primitive types

n/a

[aboxhall, 2016-06-29 00:13:55]

aboxhall@chromium.org changed reviewers:
+ dmazzoni@chromium.org

[aboxhall, 2016-06-29 00:13:55]

aboxhall@chromium.org changed reviewers:
+ dmazzoni@chromium.org

[aboxhall, 2016-06-29 00:13:55]

+dmazzoni@ for /modules/accessibility

[aboxhall, 2016-06-29 00:13:56]

+dmazzoni@ for /modules/accessibility

[aboxhall, 2016-06-29 00:13:57]

+dmazzoni@ for /modules/accessibility

[dmazzoni, 2016-06-29 17:00:56]

https://codereview.chromium.org/2088453002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/accessibility/AXObject.cpp (right):

https://codereview.chromium.org/2088453002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/accessibility/AXObject.cpp:641: for (const
AXObject* object = this; object; object = object->parentObject()) {
Since "inert" is a property of the DOM, and aria-owns can rearrange the
accessibility tree, this should probably walk the DOM parent chain looking for
"inert", rather than walking the accessibility parent chain.

https://codereview.chromium.org/2088453002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/accessibility/AXObject.h (right):

https://codereview.chromium.org/2088453002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/accessibility/AXObject.h:362: AXInertRoot,
I wonder if AXInInertSubtree would be more clear (and similarly for
AXAriaHiddenRoot)?

After not looking at this code for a while, I remembered the difference between
AXAriaHidden and AXAriaHiddenRoot but I couldn't recall which one *is* the root
and which one is hidden *because* of a root.

[aboxhall, 2017-01-12 05:38:01]

The CQ bit was checked by aboxhall@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-12 05:38:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-12 05:40:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-12 05:40:37]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[aboxhall, 2017-01-13 01:47:05]

The CQ bit was checked by aboxhall@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-13 01:47:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-13 04:16:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-13 04:16:37]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[aboxhall, 2017-01-16 01:04:19]

The CQ bit was checked by aboxhall@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-16 01:04:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-16 02:40:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-16 02:40:34]

Dry run: This issue passed the CQ dry run.

[aboxhall, 2017-01-17 11:27:58]

Ready for another look; will send out intent once you're both satisfied with the
code.

[dmazzoni, 2017-01-18 16:38:55]

https://codereview.chromium.org/2088453002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/accessibility/AXObject.cpp (right):

https://codereview.chromium.org/2088453002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/accessibility/AXObject.cpp:604: for (Node*
node = object->getNode(); node; node = node->parentNode()) {
I'm assuming this should also be parentOrShadowHostElement()?

We should probably change this throughout modules/accessibility/
but for sure do it in this change to be consistent.

https://codereview.chromium.org/2088453002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/accessibility/AXObject.cpp:607: if
(toElement(node)->hasAttribute(inertAttr))
Need to check RuntimeEnabledFeatures here too

https://codereview.chromium.org/2088453002/diff/180001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (left):

https://codereview.chromium.org/2088453002/diff/180001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:81041: -<enum
name="ContentSuggestionsNotificationsAction" type="int">
I see a lot of unrelated changes to histograms.xml, could you rebase?

[aboxhall, 2017-01-19 02:44:26]

https://codereview.chromium.org/2088453002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/accessibility/AXObject.cpp (right):

https://codereview.chromium.org/2088453002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/accessibility/AXObject.cpp:604: for (Node*
node = object->getNode(); node; node = node->parentNode()) {
On 2017/01/18 16:38:54, dmazzoni wrote:
> I'm assuming this should also be parentOrShadowHostElement()?
> 
> We should probably change this throughout modules/accessibility/
> but for sure do it in this change to be consistent.

Ah yep, good catch, thanks.

https://codereview.chromium.org/2088453002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/accessibility/AXObject.cpp:607: if
(toElement(node)->hasAttribute(inertAttr))
On 2017/01/18 16:38:54, dmazzoni wrote:
> Need to check RuntimeEnabledFeatures here too

Hm - probably doesn't matter in practice, since this is only called if
node->isInert() returns true, but definitely best to be safe.

[aboxhall, 2017-01-19 02:44:33]

The CQ bit was checked by aboxhall@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-19 02:44:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-19 04:54:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-19 04:54:20]

Dry run: This issue passed the CQ dry run.

[dmazzoni, 2017-01-19 05:23:18]

lgtm

[aboxhall, 2017-01-20 03:56:36]

esprehn: any thoughts?

[esprehn, 2017-02-07 23:50:59]

You need some Shadow DOM tests using <slot> and <content> probably.

https://codereview.chromium.org/2088453002/diff/220001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/dom/inert/inert-does-not-match-disabled-selector.html
(right):

https://codereview.chromium.org/2088453002/diff/220001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/dom/inert/inert-does-not-match-disabled-selector.html:22:
color = document.defaultView.getComputedStyle(button).getPropertyValue('color');
just getComputedStyle, no need for document.defaultView. ALso just do .color no
need to use getPropertyValue

https://codereview.chromium.org/2088453002/diff/220001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/fast/dom/inert/inert-focus-in-frames.html
(right):

https://codereview.chromium.org/2088453002/diff/220001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/dom/inert/inert-focus-in-frames.html:10:
<iframe height=400 width=600 id="main-iframe">
iframe doesn't accept children, I'm not sure I understand this test.

https://codereview.chromium.org/2088453002/diff/220001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/dom/inert/inert-focus-in-frames.html:11:
<frameset rows="*" cols="50,50">
frameset isn't valid here, what are you testing?

https://codereview.chromium.org/2088453002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Node.cpp (right):

https://codereview.chromium.org/2088453002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Node.cpp:786: if (node->isElementNode() &&
I think this probably wants to walk the composed tree using
FlatTreeTraversal::parentElement? Otherwise you can end up with non-inert stuff
inside inert stuff using <slot>.

https://codereview.chromium.org/2088453002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/accessibility/AXObject.cpp (right):

https://codereview.chromium.org/2088453002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/accessibility/AXObject.cpp:536:
ignoredReasons->append(
Why rename this back?

https://codereview.chromium.org/2088453002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/accessibility/AXObject.cpp:607: for (Node*
node = object->getNode(); node;
ditto, this wants the composed/flat tree.

[aboxhall, 2017-02-08 05:26:02]

https://codereview.chromium.org/2088453002/diff/220001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/fast/dom/inert/inert-focus-in-frames.html
(right):

https://codereview.chromium.org/2088453002/diff/220001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/dom/inert/inert-focus-in-frames.html:10:
<iframe height=400 width=600 id="main-iframe">
On 2017/02/07 23:50:59, esprehn wrote:
> iframe doesn't accept children, I'm not sure I understand this test.

Copied from the more or less identical dialog test:
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/html/dial...

What happens is that the script below takes the descendant content and writes it
into the iframe's contentDocument (line 47).

https://codereview.chromium.org/2088453002/diff/220001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/dom/inert/inert-focus-in-frames.html:11:
<frameset rows="*" cols="50,50">
On 2017/02/07 23:50:59, esprehn wrote:
> frameset isn't valid here, what are you testing?

See previous comment.

https://codereview.chromium.org/2088453002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Node.cpp (right):

https://codereview.chromium.org/2088453002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Node.cpp:786: if (node->isElementNode() &&
On 2017/02/07 23:50:59, esprehn wrote:
> I think this probably wants to walk the composed tree using
> FlatTreeTraversal::parentElement? Otherwise you can end up with non-inert
stuff
> inside inert stuff using <slot>.

Done.

https://codereview.chromium.org/2088453002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/accessibility/AXObject.cpp (right):

https://codereview.chromium.org/2088453002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/accessibility/AXObject.cpp:536:
ignoredReasons->append(
On 2017/02/07 23:50:59, esprehn wrote:
> Why rename this back?

bad merge :(

https://codereview.chromium.org/2088453002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/accessibility/AXObject.cpp:607: for (Node*
node = object->getNode(); node;
On 2017/02/07 23:50:59, esprehn wrote:
> ditto, this wants the composed/flat tree.

Done.

[aboxhall, 2017-02-08 22:36:00]

The CQ bit was checked by aboxhall@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-08 22:36:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-08 22:39:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-08 22:39:29]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[aboxhall, 2017-02-10 02:41:26]

The CQ bit was checked by aboxhall@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-10 02:41:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-10 04:26:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-10 04:26:23]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[aboxhall, 2017-02-14 23:53:24]

The CQ bit was checked by aboxhall@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-14 23:54:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[esprehn, 2017-02-15 01:40:47]

https://codereview.chromium.org/2088453002/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/UseCounter.h (right):

https://codereview.chromium.org/2088453002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/UseCounter.h:167:
HTMLObjectElementLegacyCall = 153,
looks like bad merge on UseCounter

https://codereview.chromium.org/2088453002/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/accessibility/AXObject.cpp (right):

https://codereview.chromium.org/2088453002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/accessibility/AXObject.cpp:541: const
AXObject* inertRootEl = inertRoot();
don't abbreviate, intertRoot = this->inertRoot()

https://codereview.chromium.org/2088453002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/accessibility/AXObject.cpp:605: return 0;
nullptr

https://codereview.chromium.org/2088453002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/accessibility/AXObject.cpp:611: if
(!node->isElementNode())
This is definitely weird since it's doing parentElement, but I guess getNode
isn't specific enough

https://codereview.chromium.org/2088453002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/accessibility/AXObject.cpp:617: return 0;
ditto

[aboxhall, 2017-02-15 02:06:07]

The CQ bit was checked by aboxhall@chromium.org to run a CQ dry run

[aboxhall, 2017-02-15 02:06:16]

https://codereview.chromium.org/2088453002/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/UseCounter.h (right):

https://codereview.chromium.org/2088453002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/UseCounter.h:167:
HTMLObjectElementLegacyCall = 153,
On 2017/02/15 01:40:46, esprehn wrote:
> looks like bad merge on UseCounter

Done.

https://codereview.chromium.org/2088453002/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/accessibility/AXObject.cpp (right):

https://codereview.chromium.org/2088453002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/accessibility/AXObject.cpp:541: const
AXObject* inertRootEl = inertRoot();
On 2017/02/15 01:40:47, esprehn wrote:
> don't abbreviate, intertRoot = this->inertRoot()

Done.

https://codereview.chromium.org/2088453002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/accessibility/AXObject.cpp:605: return 0;
On 2017/02/15 01:40:47, esprehn wrote:
> nullptr

Done.

https://codereview.chromium.org/2088453002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/accessibility/AXObject.cpp:611: if
(!node->isElementNode())
On 2017/02/15 01:40:47, esprehn wrote:
> This is definitely weird since it's doing parentElement, but I guess getNode
> isn't specific enough

Acknowledged.

https://codereview.chromium.org/2088453002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/accessibility/AXObject.cpp:617: return 0;
On 2017/02/15 01:40:47, esprehn wrote:
> ditto

Done.

[commit-bot: I haz the power, 2017-02-15 02:07:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[esprehn, 2017-02-15 02:15:01]

Can you link to the WICG in the change description and briefly explain what this
feature does?

https://codereview.chromium.org/2088453002/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Node.cpp (right):

https://codereview.chromium.org/2088453002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Node.cpp:787:
toElement(node)->hasAttribute(HTMLNames::inertAttr))
we often do using HTMLNames, that's actually at the top of this file, so you can
remove the HTMLNames:: here

https://codereview.chromium.org/2088453002/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/accessibility/AXObject.h (right):

https://codereview.chromium.org/2088453002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/accessibility/AXObject.h:399:
AXAriaHiddenSubtree,
I assume reordering this enum is safe?

[esprehn, 2017-02-15 02:15:10]

lgtm

[aboxhall, 2017-02-15 03:01:59]

Description was changed from

==========
Implement the inert attribute

BUG=618746
==========

to

==========
Implement the inert attribute: https://github.com/WICG/inert#tldr

BUG=618746
==========

[aboxhall, 2017-02-15 03:02:35]

Description was changed from

==========
Implement the inert attribute: https://github.com/WICG/inert#tldr

BUG=618746
==========

to

==========
Implement the inert attribute: https://github.com/WICG/inert#tldr

https://discourse.wicg.io/t/inert-attribute/1838/

BUG=618746
==========

[aboxhall, 2017-02-15 03:18:21]

The CQ bit was checked by aboxhall@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-15 03:19:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[aboxhall, 2017-02-15 04:01:32]

Added links as requested.

https://codereview.chromium.org/2088453002/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Node.cpp (right):

https://codereview.chromium.org/2088453002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Node.cpp:787:
toElement(node)->hasAttribute(HTMLNames::inertAttr))
On 2017/02/15 02:15:00, esprehn wrote:
> we often do using HTMLNames, that's actually at the top of this file, so you
can
> remove the HTMLNames:: here

Done.

https://codereview.chromium.org/2088453002/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/accessibility/AXObject.h (right):

https://codereview.chromium.org/2088453002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/accessibility/AXObject.h:399:
AXAriaHiddenSubtree,
On 2017/02/15 02:15:01, esprehn wrote:
> I assume reordering this enum is safe?

Yes - only used by devtools, serialized to a string (not order dependent).

[commit-bot: I haz the power, 2017-02-15 04:58:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-15 04:58:38]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[aboxhall, 2017-02-15 05:28:26]

Hm, lots of failures like this: 

[18363:775:0214/203121.205468:1069997921293:FATAL:FlatTreeTraversal.h(127)]
Check failed: !node.needsDistributionRecalc(). 
0   Content Shell Framework             0x000000010792428c
base::debug::StackTrace::StackTrace(unsigned long) + 28
1   Content Shell Framework             0x0000000107948ea3
logging::LogMessage::~LogMessage() + 67
2   Content Shell Framework             0x000000010784e73a
blink::FlatTreeTraversal::assertPrecondition(blink::Node const&) + 90
3   Content Shell Framework             0x000000010a908c96
blink::Node::isInert() const + 422

I can't reproduce this locally, but looks like a real issue. Any suggestions on
where to start looking for how to fix this?

[esprehn, 2017-02-15 06:20:07]

On 2017/02/15 at 05:28:26, aboxhall wrote:
> Hm, lots of failures like this: 
> 
> [18363:775:0214/203121.205468:1069997921293:FATAL:FlatTreeTraversal.h(127)]
Check failed: !node.needsDistributionRecalc(). 
> 0   Content Shell Framework             0x000000010792428c
base::debug::StackTrace::StackTrace(unsigned long) + 28
> 1   Content Shell Framework             0x0000000107948ea3
logging::LogMessage::~LogMessage() + 67
> 2   Content Shell Framework             0x000000010784e73a
blink::FlatTreeTraversal::assertPrecondition(blink::Node const&) + 90
> 3   Content Shell Framework             0x000000010a908c96
blink::Node::isInert() const + 422
> 
> I can't reproduce this locally, but looks like a real issue. Any suggestions
on where to start looking for how to fix this?

You need to call updateDistribution before using the FlatTreeTraversal. Same
idea as using style.

[aboxhall, 2017-02-15 22:29:43]

The CQ bit was checked by aboxhall@chromium.org to run a CQ dry run

[aboxhall, 2017-02-15 22:29:59]

Patchset #19 (id:360001) has been deleted

[commit-bot: I haz the power, 2017-02-15 22:31:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[aboxhall, 2017-02-15 23:10:30]

Description was changed from

==========
Implement the inert attribute: https://github.com/WICG/inert#tldr

https://discourse.wicg.io/t/inert-attribute/1838/

BUG=618746
==========

to

==========
Implement the inert attribute: https://github.com/WICG/inert#tldr

https://discourse.wicg.io/t/inert-attribute/1838/

BUG=692360
==========

[aboxhall, 2017-02-15 23:42:53]

The CQ bit was checked by aboxhall@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-15 23:43:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-16 00:32:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-16 00:32:48]

Dry run: Try jobs failed on following builders:
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[aboxhall, 2017-02-16 00:34:35]

The CQ bit was checked by aboxhall@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-16 00:36:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-16 01:06:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-16 01:06:04]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[aboxhall, 2017-02-16 01:40:27]

The CQ bit was checked by aboxhall@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-16 01:41:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[aboxhall, 2017-02-16 01:56:36]

aboxhall@chromium.org changed reviewers:
+ hayato@chromium.org

[aboxhall, 2017-02-16 01:56:37]

+hayato: could you please take a look at Node.cpp (anything else is up to you,
but that's the actual interesting part of the implementation) and see what you
think?

[commit-bot: I haz the power, 2017-02-16 03:43:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-16 03:43:56]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)

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

https://codereview.chromium.org/2088453002/diff/420001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Node.cpp (right):

https://codereview.chromium.org/2088453002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Node.cpp:783: 
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.

https://codereview.chromium.org/2088453002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Node.cpp:797: element =
FlatTreeTraversal::parentElement(*element);
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:13]

https://codereview.chromium.org/2088453002/diff/420001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Node.cpp (right):

https://codereview.chromium.org/2088453002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Node.cpp:783: 
On 2017/02/16 at 05:52:54, hayato wrote:
> 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.

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.

https://codereview.chromium.org/2088453002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Node.cpp:797: element =
FlatTreeTraversal::parentElement(*element);
On 2017/02/16 at 05:52:54, hayato wrote:
> 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.

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.

> 
> 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.

What behavior do you mean?

> 
> 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.

Yeah we need to update that.

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

https://codereview.chromium.org/2088453002/diff/420001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Node.cpp (right):

https://codereview.chromium.org/2088453002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Node.cpp:783: 
On 2017/02/16 at 06:08:12, esprehn wrote:
> On 2017/02/16 at 05:52:54, hayato wrote:
> > 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.
> 
> 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.

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....

https://codereview.chromium.org/2088453002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Node.cpp:797: element =
FlatTreeTraversal::parentElement(*element);
On 2017/02/16 at 06:08:12, esprehn wrote:
> On 2017/02/16 at 05:52:54, hayato wrote:
> > 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.
> 
> 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.
> 
> > 
> > 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.
> 
> What behavior do you mean?

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.

[esprehn, 2017-02-16 06:28:12]

https://codereview.chromium.org/2088453002/diff/420001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/accessibility/AXObject.cpp (right):

https://codereview.chromium.org/2088453002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/accessibility/AXObject.cpp:609:
node->updateDistribution();
updateDistribution can actually cause a detach(), which can then destroy and
recrete the AXObject tree, I think you want to call this higher up in the
system?

Actually since this is inside the AXObject tree, and that depends on style, this
should only be used after we called updateStyle() which updates distribution, so
can distribution be dirty here?

[aboxhall, 2017-02-16 06:40:11]

On 2017/02/16 06:28:12, esprehn wrote:
>
https://codereview.chromium.org/2088453002/diff/420001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/modules/accessibility/AXObject.cpp (right):
> 
>
https://codereview.chromium.org/2088453002/diff/420001/third_party/WebKit/Sou...
> third_party/WebKit/Source/modules/accessibility/AXObject.cpp:609:
> node->updateDistribution();
> updateDistribution can actually cause a detach(), which can then destroy and
> recrete the AXObject tree, I think you want to call this higher up in the
> system?
> 
> Actually since this is inside the AXObject tree, and that depends on style,
this
> should only be used after we called updateStyle() which updates distribution,
so
> can distribution be dirty here?

Looks like it can:
https://storage.googleapis.com/chromium-layout-test-archives/win_chromium_rel...

[aboxhall, 2017-02-16 06:41:56]

On 2017/02/16 06:40:11, aboxhall wrote:
> On 2017/02/16 06:28:12, esprehn wrote:
> >
>
https://codereview.chromium.org/2088453002/diff/420001/third_party/WebKit/Sou...
> > File third_party/WebKit/Source/modules/accessibility/AXObject.cpp (right):
> > 
> >
>
https://codereview.chromium.org/2088453002/diff/420001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/modules/accessibility/AXObject.cpp:609:
> > node->updateDistribution();
> > updateDistribution can actually cause a detach(), which can then destroy and
> > recrete the AXObject tree, I think you want to call this higher up in the
> > system?
> > 
> > Actually since this is inside the AXObject tree, and that depends on style,
> this
> > should only be used after we called updateStyle() which updates
distribution,
> so
> > can distribution be dirty here?
> 
> Looks like it can:
>
https://storage.googleapis.com/chromium-layout-test-archives/win_chromium_rel...

Wait, sorry - the call isn't coming from AXObject there.

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

https://codereview.chromium.org/2088453002/diff/420001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Node.cpp (right):

https://codereview.chromium.org/2088453002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Node.cpp:783: 
On 2017/02/16 at 06:25:55, hayato wrote:
> On 2017/02/16 at 06:08:12, esprehn wrote:
> > On 2017/02/16 at 05:52:54, hayato wrote:
> > > ...
> 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.
> 

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

[aboxhall, 2017-02-17 00:10:21]

The CQ bit was checked by aboxhall@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-17 00:11:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-17 01:35:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-17 01:35:20]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)

[aboxhall, 2017-02-17 02:36:21]

The CQ bit was checked by aboxhall@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-17 02:38:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-17 03:04:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-17 03:04:45]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[aboxhall, 2017-02-17 03:27:49]

The CQ bit was checked by aboxhall@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-17 03:28:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-17 05:24:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-17 05:24:24]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[aboxhall, 2017-02-19 23:19:56]

The CQ bit was checked by aboxhall@chromium.org

[aboxhall, 2017-02-19 23:19:57]

The patchset sent to the CQ was uploaded after l-g-t-m from
dmazzoni@chromium.org, esprehn@chromium.org
Link to the patchset: https://codereview.chromium.org/2088453002/#ps500001
(title: "Rebase")

[commit-bot: I haz the power, 2017-02-19 23:20:12]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[aboxhall, 2017-02-19 23:21:54]

The CQ bit was unchecked by aboxhall@chromium.org

[aboxhall, 2017-02-19 23:22:06]

The CQ bit was checked by aboxhall@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-19 23:22:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-20 00:19:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-20 00:19:54]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[aboxhall, 2017-02-27 19:15:41]

The CQ bit was checked by aboxhall@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-27 19:16:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

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

https://codereview.chromium.org/2088453002/diff/420001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Node.cpp (right):

https://codereview.chromium.org/2088453002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Node.cpp:783: 
On 2017/02/16 06:46:54, esprehn wrote:
> On 2017/02/16 at 06:25:55, hayato wrote:
> > On 2017/02/16 at 06:08:12, esprehn wrote:
> > > On 2017/02/16 at 05:52:54, hayato wrote:
> > > > ...
> > 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.
> > 
> 
> isInert() itself walks up the tree, so doing it inside updateDistribution() is
> making it O(2N) in the depth.

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

[commit-bot: I haz the power, 2017-02-27 21:42:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-27 21:42:45]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[hayato, 2017-03-01 07:41:27]

Could you summarize what are the consumers of isInert()? I do not think we have
to update distribution at every places.
In any use cases, I think we have to update distribution at most once, I guess.

[aboxhall, 2017-03-21 00:46:23]

On 2017/03/01 07:41:27, hayato wrote:
> Could you summarize what are the consumers of isInert()? I do not think we
have
> to update distribution at every places.
> In any use cases, I think we have to update distribution at most once, I
guess.

It's called in two main places (other than other implementions of isInert()):

Element::isFocusable() (via a number of callsites)
AXObject::computeIsInertOrAriaHidden() (result is cached and reused)

[hayato, 2017-03-21 02:20:19]

I see. If isFocusable() is called via a number of callsites and we can not
assume that distribution is dirty or not at each callsite,
we have to call updateDistribution() each time, either in isInert(),
isFocusable(), or in each callsite, conservatively, so that we do not return the
dirty wrong result.

Calling it in isInert() is most easiest, but that might have the potential
performance impact, as we have discussed. Thus, we can not do that unless we can
assume that isFocusable() is not called many times in one unit of *task*.

If we find such a case, isFocusable() is potentially called many time in one
task, we should consider:

1. Call updateDistribution() at the beginning of the task, instead of inside of
isFocusable() (or isInnert())
2. Then, call isFocusableWithCleanDistribution(), where we do not call
updateDistribution().

[aboxhall, 2017-03-21 04:23:39]

The CQ bit was checked by aboxhall@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-21 04:23:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[aboxhall, 2017-03-21 04:25:18]

The CQ bit was unchecked by aboxhall@chromium.org

[aboxhall, 2017-03-24 04:34:08]

The CQ bit was checked by aboxhall@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-24 04:34:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-24 04:36:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-24 04:36:51]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[aboxhall, 2017-03-24 04:51:51]

The CQ bit was checked by aboxhall@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-24 04:52:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-24 05:49:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-24 05:49:28]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[esprehn, 2017-03-28 21:44:21]

Calling updateDistribution here is fine, something outside the tree is never
focusable, and updateDistribution for things inside the tree is cheap.

[aboxhall, 2017-04-24 07:09:08]

The CQ bit was checked by aboxhall@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-24 07:09:27]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-24 07:19:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-24 07:19:56]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[aboxhall, 2017-04-27 03:05:57]

The CQ bit was checked by aboxhall@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-27 03:06:40]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-27 03:19:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-27 03:19:53]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[aboxhall, 2017-04-28 00:44:05]

Patchset #32 (id:640001) has been deleted

[aboxhall, 2017-04-28 00:44:28]

Patchset #32 (id:640001) has been deleted

[aboxhall, 2017-04-28 00:45:01]

Patchset #32 (id:660001) has been deleted

[aboxhall, 2017-04-28 00:45:35]

Patchset #32 (id:680001) has been deleted

[aboxhall, 2017-04-28 00:47:37]

Patchset #32 (id:700001) has been deleted

[aboxhall, 2017-04-28 02:29:49]

The CQ bit was checked by aboxhall@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-28 02:30:35]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[aboxhall, 2017-05-02 06:43:59]

The CQ bit was checked by aboxhall@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-02 06:44:15]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[aboxhall, 2017-05-02 07:01:37]

On 2017/03/28 21:44:21, esprehn wrote:
> Calling updateDistribution here is fine, something outside the tree is never
> focusable, and updateDistribution for things inside the tree is cheap.

I fixed the crash in AX code, albeit potentially with too large a hammer, PTAL.

Re isFocusable, I see it already states that callers should ensure the layout
tree is up to date, and UpdateStyleAndLayoutTree calls UpdateDistribution, so it
seems like we're ok there.

Dominic: could you take a look at the changes in AXNode and let me know what you
think?

[commit-bot: I haz the power, 2017-05-02 10:44:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-02 10:44:01]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[aboxhall, 2017-05-02 21:08:02]

On 2017/05/02 07:01:37, aboxhall wrote:
> On 2017/03/28 21:44:21, esprehn wrote:
> > Calling updateDistribution here is fine, something outside the tree is never
> > focusable, and updateDistribution for things inside the tree is cheap.
> 
> I fixed the crash in AX code, albeit potentially with too large a hammer,
PTAL.
> 
> Re isFocusable, I see it already states that callers should ensure the layout
> tree is up to date, and UpdateStyleAndLayoutTree calls UpdateDistribution, so
it
> seems like we're ok there.
> 
> Dominic: could you take a look at the changes in AXNode and let me know what
you
> think?

[Ping for Sydney morning/California afternoon]

[dmazzoni, 2017-05-03 15:48:17]

still lgtm with one question

https://codereview.chromium.org/2088453002/diff/850001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/accessibility/AXObject.cpp (right):

https://codereview.chromium.org/2088453002/diff/850001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/accessibility/AXObject.cpp:538: while
(document) {
Calling UpdateDistribution seems okay.

Accessibility trees only span one frame, so do we really need
to keep walking up documents?

The only exception is that an accessibility tree spans
a few simple pop-ups like <select> or <input type=date>
popups - but still, I don't see the need to call
UpdateDistribution recursively.

[aboxhall, 2017-05-03 21:25:53]

On 2017/05/03 15:48:17, dmazzoni wrote:
> still lgtm with one question
> 
>
https://codereview.chromium.org/2088453002/diff/850001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/modules/accessibility/AXObject.cpp (right):
> 
>
https://codereview.chromium.org/2088453002/diff/850001/third_party/WebKit/Sou...
> third_party/WebKit/Source/modules/accessibility/AXObject.cpp:538: while
> (document) {
> Calling UpdateDistribution seems okay.
> 
> Accessibility trees only span one frame, so do we really need
> to keep walking up documents?
> 
> The only exception is that an accessibility tree spans
> a few simple pop-ups like <select> or <input type=date>
> popups - but still, I don't see the need to call
> UpdateDistribution recursively.

I was having issues with <object>s -
https://storage.googleapis.com/chromium-layout-test-archives/linux_chromium_r...
for this test
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/http/test...

Any thoughts on better ways to handle that case? It would be great not to
recursively update all the ancestor documents unnecessarily.

[dmazzoni, 2017-05-03 23:14:25]

I'm not sure why that case wouldn't work. I'm not even sure how <object> is
implemented, if it doesn't just turn into a frame behind the scenes.

On Wed, May 3, 2017 at 2:25 PM <aboxhall@chromium.org> wrote:

> On 2017/05/03 15:48:17, dmazzoni wrote:
> > still lgtm with one question
> >
> >
>
>
https://codereview.chromium.org/2088453002/diff/850001/third_party/WebKit/Sou...
> > File third_party/WebKit/Source/modules/accessibility/AXObject.cpp
> (right):
> >
> >
>
>
https://codereview.chromium.org/2088453002/diff/850001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/modules/accessibility/AXObject.cpp:538: while
> > (document) {
> > Calling UpdateDistribution seems okay.
> >
> > Accessibility trees only span one frame, so do we really need
> > to keep walking up documents?
> >
> > The only exception is that an accessibility tree spans
> > a few simple pop-ups like <select> or <input type=date>
> > popups - but still, I don't see the need to call
> > UpdateDistribution recursively.
>
> I was having issues with <object>s -
>
>
https://storage.googleapis.com/chromium-layout-test-archives/linux_chromium_r...
> for this test
>
>
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/http/test...
>
> Any thoughts on better ways to handle that case? It would be great not to
> recursively update all the ancestor documents unnecessarily.
>
> https://codereview.chromium.org/2088453002/
>
> --
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to chromium-reviews+unsubscribe@chromium.org.
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[dmazzoni, 2017-05-03 23:14:25]

I'm not sure why that case wouldn't work. I'm not even sure how <object> is
implemented, if it doesn't just turn into a frame behind the scenes.

On Wed, May 3, 2017 at 2:25 PM <aboxhall@chromium.org> wrote:

> On 2017/05/03 15:48:17, dmazzoni wrote:
> > still lgtm with one question
> >
> >
>
>
https://codereview.chromium.org/2088453002/diff/850001/third_party/WebKit/Sou...
> > File third_party/WebKit/Source/modules/accessibility/AXObject.cpp
> (right):
> >
> >
>
>
https://codereview.chromium.org/2088453002/diff/850001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/modules/accessibility/AXObject.cpp:538: while
> > (document) {
> > Calling UpdateDistribution seems okay.
> >
> > Accessibility trees only span one frame, so do we really need
> > to keep walking up documents?
> >
> > The only exception is that an accessibility tree spans
> > a few simple pop-ups like <select> or <input type=date>
> > popups - but still, I don't see the need to call
> > UpdateDistribution recursively.
>
> I was having issues with <object>s -
>
>
https://storage.googleapis.com/chromium-layout-test-archives/linux_chromium_r...
> for this test
>
>
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/http/test...
>
> Any thoughts on better ways to handle that case? It would be great not to
> recursively update all the ancestor documents unnecessarily.
>
> https://codereview.chromium.org/2088453002/
>
> --
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to chromium-reviews+unsubscribe@chromium.org.
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[aboxhall, 2017-05-03 23:41:31]

On 2017/05/03 23:14:25, dmazzoni wrote:
> I'm not sure why that case wouldn't work. I'm not even sure how <object> is
> implemented, if it doesn't just turn into a frame behind the scenes.

Hm, not sure, but it was crashing and the change to walk up the document tree
fixed it.


> On Wed, May 3, 2017 at 2:25 PM <mailto:aboxhall@chromium.org> wrote:
> 
> > On 2017/05/03 15:48:17, dmazzoni wrote:
> > > still lgtm with one question
> > >
> > >
> >
> >
>
https://codereview.chromium.org/2088453002/diff/850001/third_party/WebKit/Sou...
> > > File third_party/WebKit/Source/modules/accessibility/AXObject.cpp
> > (right):
> > >
> > >
> >
> >
>
https://codereview.chromium.org/2088453002/diff/850001/third_party/WebKit/Sou...
> > > third_party/WebKit/Source/modules/accessibility/AXObject.cpp:538: while
> > > (document) {
> > > Calling UpdateDistribution seems okay.
> > >
> > > Accessibility trees only span one frame, so do we really need
> > > to keep walking up documents?
> > >
> > > The only exception is that an accessibility tree spans
> > > a few simple pop-ups like <select> or <input type=date>
> > > popups - but still, I don't see the need to call
> > > UpdateDistribution recursively.
> >
> > I was having issues with <object>s -
> >
> >
>
https://storage.googleapis.com/chromium-layout-test-archives/linux_chromium_r...
> > for this test
> >
> >
>
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/http/test...
> >
> > Any thoughts on better ways to handle that case? It would be great not to
> > recursively update all the ancestor documents unnecessarily.
> >
> > https://codereview.chromium.org/2088453002/
> >
> > --
> > You received this message because you are subscribed to the Google Groups
> > "Chromium-reviews" group.
> > To unsubscribe from this group and stop receiving emails from it, send an
> > email to mailto:chromium-reviews+unsubscribe@chromium.org.
> >
> 
> -- 
> You received this message because you are subscribed to the Google Groups
"Blink
> Reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:blink-reviews+unsubscribe@chromium.org.

[aboxhall, 2017-05-05 07:18:07]

The CQ bit was checked by aboxhall@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-05 07:18:22]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-05 08:09:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-05 08:09:33]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[dmazzoni, 2017-05-08 06:06:09]

Looks like the issue is that Node::IsInert walks up the
frame tree, so that's why you need to call UpdateDistribution
on everything.

https://codereview.chromium.org/2088453002/diff/890001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Node.cpp (right):

https://codereview.chromium.org/2088453002/diff/890001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Node.cpp:852: return
GetDocument().LocalOwner() && GetDocument().LocalOwner()->IsInert();
I just realized this only walks local frames, so it won't work with
out-of-process iframes.

I think there's going to have to be a frame flag indicating whether
or not an entire frame / document is inert.

[aboxhall, 2017-05-09 06:24:14]

The CQ bit was checked by aboxhall@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-09 06:24:46]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-09 08:31:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-09 08:31:23]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[aboxhall, 2017-05-10 05:09:18]

The CQ bit was checked by aboxhall@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-10 05:10:00]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-10 06:28:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-10 06:28:50]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[aboxhall, 2017-05-11 02:45:15]

The CQ bit was checked by aboxhall@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-11 02:46:10]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-11 04:26:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-11 04:26:05]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[aboxhall, 2017-05-11 05:16:43]

The CQ bit was checked by aboxhall@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-11 05:18:00]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-11 07:36:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-11 07:36:19]

Dry run: This issue passed the CQ dry run.

[esprehn, 2017-05-12 21:16:51]

esprehn@chromium.org changed reviewers:
+ dcheng@chromium.org

[esprehn, 2017-05-12 21:16:54]

This doesn't work with OOPIF, although neither does the other isInert() code
which probably means <dialog> is broken in OOPIF? That seems bad and should
probably be fixed.

https://codereview.chromium.org/2088453002/diff/950001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Node.cpp (right):

https://codereview.chromium.org/2088453002/diff/950001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Node.cpp:838:
DCHECK(!ChildNeedsDistributionRecalc());
This assert should be inside FlatTreeTraversal probably, but I guess it's okay
here too.

https://codereview.chromium.org/2088453002/diff/950001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Node.cpp:856: return
GetDocument().LocalOwner() && GetDocument().LocalOwner()->IsInert();
This won't work in OOPIF, it means out of process iframes don't respect inert.

https://codereview.chromium.org/2088453002/diff/950001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/shadow/FlatTreeTraversal.h (right):

https://codereview.chromium.org/2088453002/diff/950001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/shadow/FlatTreeTraversal.h:129: LOG(INFO) <<
node << " needs recalc";
This is called all over the place, you're making the release binary slower by
adding info level logging. This needs to be removed before landing.

https://codereview.chromium.org/2088453002/diff/950001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/shadow/FlatTreeTraversal.h:130:
DCHECK(!node.NeedsDistributionRecalc());
Revert this change to just DCHECK.

https://codereview.chromium.org/2088453002/diff/950001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/accessibility/AXObject.cpp (right):

https://codereview.chromium.org/2088453002/diff/950001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/accessibility/AXObject.cpp:552:
document->LocalOwner()->UpdateDistribution();
Why are you updating the distribution of the parent iframe? This doesn't go all
the way up, so I suspect it still doesn't work if you're multiple levels down in
distributed frames.

https://codereview.chromium.org/2088453002/diff/950001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/accessibility/AXObject.cpp:708: :
FlatTreeTraversal::ParentElement(*node);
You could add FlatTreeTraversal::ParentElementOrSelf(node) and use it here and
elsewhere.

[dcheng, 2017-05-12 21:18:46]

On 2017/05/12 21:16:54, esprehn wrote:
> This doesn't work with OOPIF, although neither does the other isInert() code
> which probably means <dialog> is broken in OOPIF? That seems bad and should
> probably be fixed.
> 
>
https://codereview.chromium.org/2088453002/diff/950001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/dom/Node.cpp (right):
> 
>
https://codereview.chromium.org/2088453002/diff/950001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/dom/Node.cpp:838:
> DCHECK(!ChildNeedsDistributionRecalc());
> This assert should be inside FlatTreeTraversal probably, but I guess it's okay
> here too.
> 
>
https://codereview.chromium.org/2088453002/diff/950001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/dom/Node.cpp:856: return
> GetDocument().LocalOwner() && GetDocument().LocalOwner()->IsInert();
> This won't work in OOPIF, it means out of process iframes don't respect inert.
> 
>
https://codereview.chromium.org/2088453002/diff/950001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/dom/shadow/FlatTreeTraversal.h (right):
> 
>
https://codereview.chromium.org/2088453002/diff/950001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/dom/shadow/FlatTreeTraversal.h:129: LOG(INFO)
<<
> node << " needs recalc";
> This is called all over the place, you're making the release binary slower by
> adding info level logging. This needs to be removed before landing.
> 
>
https://codereview.chromium.org/2088453002/diff/950001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/dom/shadow/FlatTreeTraversal.h:130:
> DCHECK(!node.NeedsDistributionRecalc());
> Revert this change to just DCHECK.
> 
>
https://codereview.chromium.org/2088453002/diff/950001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/modules/accessibility/AXObject.cpp (right):
> 
>
https://codereview.chromium.org/2088453002/diff/950001/third_party/WebKit/Sou...
> third_party/WebKit/Source/modules/accessibility/AXObject.cpp:552:
> document->LocalOwner()->UpdateDistribution();
> Why are you updating the distribution of the parent iframe? This doesn't go
all
> the way up, so I suspect it still doesn't work if you're multiple levels down
in
> distributed frames.
> 
>
https://codereview.chromium.org/2088453002/diff/950001/third_party/WebKit/Sou...
> third_party/WebKit/Source/modules/accessibility/AXObject.cpp:708: :
> FlatTreeTraversal::ParentElement(*node);
> You could add FlatTreeTraversal::ParentElementOrSelf(node) and use it here and
> elsewhere.

Fixing inert for OOPIFs is being tracked in
https://bugs.chromium.org/p/chromium/issues/detail?id=719788.

[aboxhall, 2017-05-17 01:35:12]

The CQ bit was checked by aboxhall@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-17 01:37:03]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-17 01:43:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-17 01:43:45]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[aboxhall, 2017-05-18 01:03:33]

The CQ bit was checked by aboxhall@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-18 01:07:04]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[aboxhall, 2017-05-18 01:30:49]

https://codereview.chromium.org/2088453002/diff/950001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Node.cpp (right):

https://codereview.chromium.org/2088453002/diff/950001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Node.cpp:838:
DCHECK(!ChildNeedsDistributionRecalc());
On 2017/05/12 21:16:54, esprehn wrote:
> This assert should be inside FlatTreeTraversal probably, but I guess it's okay
> here too.

Acknowledged.

https://codereview.chromium.org/2088453002/diff/950001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Node.cpp:856: return
GetDocument().LocalOwner() && GetDocument().LocalOwner()->IsInert();
On 2017/05/12 21:16:54, esprehn wrote:
> This won't work in OOPIF, it means out of process iframes don't respect inert.

Acknowledged.

https://codereview.chromium.org/2088453002/diff/950001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/shadow/FlatTreeTraversal.h (right):

https://codereview.chromium.org/2088453002/diff/950001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/shadow/FlatTreeTraversal.h:129: LOG(INFO) <<
node << " needs recalc";
On 2017/05/12 21:16:54, esprehn wrote:
> This is called all over the place, you're making the release binary slower by
> adding info level logging. This needs to be removed before landing.

Done.

https://codereview.chromium.org/2088453002/diff/950001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/shadow/FlatTreeTraversal.h:130:
DCHECK(!node.NeedsDistributionRecalc());
On 2017/05/12 21:16:54, esprehn wrote:
> Revert this change to just DCHECK.

Done.

https://codereview.chromium.org/2088453002/diff/950001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/accessibility/AXObject.cpp (right):

https://codereview.chromium.org/2088453002/diff/950001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/accessibility/AXObject.cpp:552:
document->LocalOwner()->UpdateDistribution();
On 2017/05/12 21:16:54, esprehn wrote:
> Why are you updating the distribution of the parent iframe? This doesn't go
all
> the way up, so I suspect it still doesn't work if you're multiple levels down
in
> distributed frames.

It crashes (DCHECK fails) without this in certain situations. I should probably
walk all the way up in case there are crashes we're missing in tests.

https://codereview.chromium.org/2088453002/diff/950001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/accessibility/AXObject.cpp:708: :
FlatTreeTraversal::ParentElement(*node);
On 2017/05/12 21:16:54, esprehn wrote:
> You could add FlatTreeTraversal::ParentElementOrSelf(node) and use it here and
> elsewhere.

This actually doesn't seem to be a super common case, so I'll leave it as-is.

[commit-bot: I haz the power, 2017-05-18 02:59:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-18 02:59:09]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[aboxhall, 2017-05-18 03:19:08]

The CQ bit was checked by aboxhall@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-18 03:21:20]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-18 06:00:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-18 06:00:45]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[aboxhall, 2017-05-22 00:18:46]

The CQ bit was checked by aboxhall@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-22 00:18:55]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-22 00:21:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-22 00:21:26]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[aboxhall, 2017-05-22 01:21:57]

aboxhall@chromium.org changed reviewers:
+ kochi@chromium.org
- esprehn@chromium.org

[aboxhall, 2017-05-22 01:21:59]

aboxhall@chromium.org changed reviewers:
+ kochi@chromium.org
- esprehn@chromium.org

[aboxhall, 2017-05-22 01:25:00]

kochi@ - PTAL?

[hayato, 2017-05-22 04:33:28]

hayato@chromium.org changed reviewers:
+ esprehn@chromium.org

[hayato, 2017-05-22 04:33:30]

LGTM

[aboxhall, 2017-05-22 04:49:27]

The CQ bit was checked by aboxhall@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-22 04:49:37]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-22 06:58:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-22 06:58:50]

Dry run: This issue passed the CQ dry run.

[kochi, 2017-05-22 07:31:16]

Thanks for including me for reviews.

Could you remove the following file that seems to be
included accidentally?
third_party/WebKit/LayoutTests/inspectorprotocol/accessibility/#%2Aediff-merge%2A#7168wah#

One naive question: if an existing web site uses "inert"
attribute for their use, which is different from what is
implemented here, this change may break those sites -
have you done any research on how "inert" is already used
in the wild?

https://codereview.chromium.org/2088453002/diff/1030001/third_party/WebKit/So...
File third_party/WebKit/Source/core/dom/Node.cpp (right):

https://codereview.chromium.org/2088453002/diff/1030001/third_party/WebKit/So...
third_party/WebKit/Source/core/dom/Node.cpp:849: bool Node::IsInert() const {
Could you also update the comment in Node.h above IsInert(),
if necessary?
I think including a link to the explainer or whatever
you think is the most relevant information would be good.

https://codereview.chromium.org/2088453002/diff/1030001/third_party/WebKit/So...
third_party/WebKit/Source/core/dom/Node.cpp:853: if (!isConnected())
style nit: you can combine line 850 and 853 into one condition - I prefer
that for readability, but this is very personal opinion and you can ignore
this comment.

https://codereview.chromium.org/2088453002/diff/1030001/third_party/WebKit/So...
File third_party/WebKit/Source/core/html/HTMLElement.idl (right):

https://codereview.chromium.org/2088453002/diff/1030001/third_party/WebKit/So...
third_party/WebKit/Source/core/html/HTMLElement.idl:35:
[RuntimeEnabled=InertAttribute, Reflect] attribute boolean inert;
Can you add "CEReactions" extended attribute here as well as other
attributes? It will make sure changing inert attribute will trigger
custom elements reactions (if any callback is registered for
"inert" change").

FYI
https://html.spec.whatwg.org/multipage/scripting.html#cereactions

[aboxhall, 2017-05-23 01:56:57]

The CQ bit was checked by aboxhall@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-23 01:57:17]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[aboxhall, 2017-05-23 01:57:54]

Patchset #49 (id:1050001) has been deleted

[commit-bot: I haz the power, 2017-05-23 01:59:42]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[aboxhall, 2017-05-23 02:00:15]

The CQ bit was checked by aboxhall@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-23 02:00:39]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[aboxhall, 2017-05-23 02:45:45]

https://codereview.chromium.org/2088453002/diff/1030001/third_party/WebKit/So...
File third_party/WebKit/Source/core/dom/Node.cpp (right):

https://codereview.chromium.org/2088453002/diff/1030001/third_party/WebKit/So...
third_party/WebKit/Source/core/dom/Node.cpp:849: bool Node::IsInert() const {
On 2017/05/22 07:31:15, kochi wrote:
> Could you also update the comment in Node.h above IsInert(),
> if necessary?
> I think including a link to the explainer or whatever
> you think is the most relevant information would be good.

Added some links in the .h

https://codereview.chromium.org/2088453002/diff/1030001/third_party/WebKit/So...
third_party/WebKit/Source/core/dom/Node.cpp:853: if (!isConnected())
On 2017/05/22 07:31:15, kochi wrote:
> style nit: you can combine line 850 and 853 into one condition - I prefer
> that for readability, but this is very personal opinion and you can ignore
> this comment.

Makes sense, done.

https://codereview.chromium.org/2088453002/diff/1030001/third_party/WebKit/So...
File third_party/WebKit/Source/core/html/HTMLElement.idl (right):

https://codereview.chromium.org/2088453002/diff/1030001/third_party/WebKit/So...
third_party/WebKit/Source/core/html/HTMLElement.idl:35:
[RuntimeEnabled=InertAttribute, Reflect] attribute boolean inert;
On 2017/05/22 07:31:15, kochi wrote:
> Can you add "CEReactions" extended attribute here as well as other
> attributes? It will make sure changing inert attribute will trigger
> custom elements reactions (if any callback is registered for
> "inert" change").
> 
> FYI
> https://html.spec.whatwg.org/multipage/scripting.html#cereactions

Done, thanks for the explanation!

[aboxhall, 2017-05-23 02:51:08]

On 2017/05/22 07:31:16, kochi wrote:
> Thanks for including me for reviews.
> 
> Could you remove the following file that seems to be
> included accidentally?
>
third_party/WebKit/LayoutTests/inspectorprotocol/accessibility/#%2Aediff-merge%2A#7168wah#

Done, not sure how that happened.

> One naive question: if an existing web site uses "inert"
> attribute for their use, which is different from what is
> implemented here, this change may break those sites -
> have you done any research on how "inert" is already used
> in the wild?

Hmmm this is tricky to research. BigQuery didn't find anything. I guess we could
land the UMA metric first and see what it tells us?

[commit-bot: I haz the power, 2017-05-23 04:13:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-23 04:13:40]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[aboxhall, 2017-05-23 06:54:10]

On 2017/05/23 02:51:08, aboxhall wrote:
> On 2017/05/22 07:31:16, kochi wrote:
> > Thanks for including me for reviews.
> > 
> > Could you remove the following file that seems to be
> > included accidentally?
> >
>
third_party/WebKit/LayoutTests/inspectorprotocol/accessibility/#%2Aediff-merge%2A#7168wah#
> 
> Done, not sure how that happened.
> 
> > One naive question: if an existing web site uses "inert"
> > attribute for their use, which is different from what is
> > implemented here, this change may break those sites -
> > have you done any research on how "inert" is already used
> > in the wild?
> 
> Hmmm this is tricky to research. BigQuery didn't find anything. I guess we
could
> land the UMA metric first and see what it tells us?

Actually - since this is behind a flag, it won't break anything immediately. We
can research after it goes in.

[aboxhall, 2017-05-23 07:16:57]

The CQ bit was checked by aboxhall@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-23 07:17:18]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[kochi, 2017-05-23 07:50:42]

On 2017/05/23 02:45:45, aboxhall wrote:
>
https://codereview.chromium.org/2088453002/diff/1030001/third_party/WebKit/So...
> File third_party/WebKit/Source/core/dom/Node.cpp (right):
> 
>
https://codereview.chromium.org/2088453002/diff/1030001/third_party/WebKit/So...
> third_party/WebKit/Source/core/dom/Node.cpp:849: bool Node::IsInert() const {
> On 2017/05/22 07:31:15, kochi wrote:
> > Could you also update the comment in Node.h above IsInert(),
> > if necessary?
> > I think including a link to the explainer or whatever
> > you think is the most relevant information would be good.
> 
> Added some links in the .h
> 
>
https://codereview.chromium.org/2088453002/diff/1030001/third_party/WebKit/So...
> third_party/WebKit/Source/core/dom/Node.cpp:853: if (!isConnected())
> On 2017/05/22 07:31:15, kochi wrote:
> > style nit: you can combine line 850 and 853 into one condition - I prefer
> > that for readability, but this is very personal opinion and you can ignore
> > this comment.
> 
> Makes sense, done.
> 
>
https://codereview.chromium.org/2088453002/diff/1030001/third_party/WebKit/So...
> File third_party/WebKit/Source/core/html/HTMLElement.idl (right):
> 
>
https://codereview.chromium.org/2088453002/diff/1030001/third_party/WebKit/So...
> third_party/WebKit/Source/core/html/HTMLElement.idl:35:
> [RuntimeEnabled=InertAttribute, Reflect] attribute boolean inert;
> On 2017/05/22 07:31:15, kochi wrote:
> > Can you add "CEReactions" extended attribute here as well as other
> > attributes? It will make sure changing inert attribute will trigger
> > custom elements reactions (if any callback is registered for
> > "inert" change").
> > 
> > FYI
> > https://html.spec.whatwg.org/multipage/scripting.html#cereactions
> 
> Done, thanks for the explanation!

LGTM

[kochi, 2017-05-23 07:54:31]

On 2017/05/23 06:54:10, aboxhall wrote:
> On 2017/05/23 02:51:08, aboxhall wrote:
> > On 2017/05/22 07:31:16, kochi wrote:
> > > Thanks for including me for reviews.
> > > 
> > > Could you remove the following file that seems to be
> > > included accidentally?
> > >
> >
>
third_party/WebKit/LayoutTests/inspectorprotocol/accessibility/#%2Aediff-merge%2A#7168wah#
> > 
> > Done, not sure how that happened.
> > 
> > > One naive question: if an existing web site uses "inert"
> > > attribute for their use, which is different from what is
> > > implemented here, this change may break those sites -
> > > have you done any research on how "inert" is already used
> > > in the wild?
> > 
> > Hmmm this is tricky to research. BigQuery didn't find anything. I guess we
> could
> > land the UMA metric first and see what it tells us?
> 
> Actually - since this is behind a flag, it won't break anything immediately.
We
> can research after it goes in.

Yeah, but one anecdote of Shadow DOM - it once tried to introduce Node.rootNode
attribute to return the root of its tree (i.e. document or shadow root) and
once shipped then reverted as some existing framework used that property on
Node and had conflict.  That resulted in renaming it to Node.getRootNode().

If you hide 'inert' behind the flag, it is hard to detect it - is there any
blocker for simply shipping?

[kochi, 2017-05-23 07:57:18]

On 2017/05/23 07:54:31, kochi wrote:
> On 2017/05/23 06:54:10, aboxhall wrote:
> > On 2017/05/23 02:51:08, aboxhall wrote:
> > > On 2017/05/22 07:31:16, kochi wrote:
> > > > Thanks for including me for reviews.
> > > > 
> > > > Could you remove the following file that seems to be
> > > > included accidentally?
> > > >
> > >
> >
>
third_party/WebKit/LayoutTests/inspectorprotocol/accessibility/#%2Aediff-merge%2A#7168wah#
> > > 
> > > Done, not sure how that happened.
> > > 
> > > > One naive question: if an existing web site uses "inert"
> > > > attribute for their use, which is different from what is
> > > > implemented here, this change may break those sites -
> > > > have you done any research on how "inert" is already used
> > > > in the wild?
> > > 
> > > Hmmm this is tricky to research. BigQuery didn't find anything. I guess we
> > could
> > > land the UMA metric first and see what it tells us?
> > 
> > Actually - since this is behind a flag, it won't break anything immediately.
> We
> > can research after it goes in.
> 
> Yeah, but one anecdote of Shadow DOM - it once tried to introduce
Node.rootNode
> attribute to return the root of its tree (i.e. document or shadow root) and
> once shipped then reverted as some existing framework used that property on
> Node and had conflict.  That resulted in renaming it to Node.getRootNode().

Just FYI, if you are interested, here is the history:
https://github.com/whatwg/dom/issues/241

> If you hide 'inert' behind the flag, it is hard to detect it - is there any
> blocker for simply shipping?

[commit-bot: I haz the power, 2017-05-23 08:53:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-23 08:53:12]

Dry run: This issue passed the CQ dry run.

[aboxhall, 2017-05-23 09:28:36]

The CQ bit was checked by aboxhall@chromium.org

[aboxhall, 2017-05-23 09:28:37]

The patchset sent to the CQ was uploaded after l-g-t-m from
esprehn@chromium.org, dmazzoni@chromium.org, hayato@chromium.org
Link to the patchset: https://codereview.chromium.org/2088453002/#ps1090001
(title: "rebase")

[commit-bot: I haz the power, 2017-05-23 09:28:54]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-23 09:41:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-23 09:42:02]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[aboxhall, 2017-05-23 10:50:45]

The CQ bit was checked by aboxhall@chromium.org

[commit-bot: I haz the power, 2017-05-23 10:51:00]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-23 11:03:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-23 11:04:01]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[aboxhall, 2017-05-23 12:49:17]

aboxhall@chromium.org changed reviewers:
+ dgozman@chromium.org

[dglazkov, 2017-05-23 15:48:04]

dglazkov@chromium.org changed reviewers:
+ dglazkov@chromium.org

[dglazkov, 2017-05-23 15:48:07]

lgtm

[dglazkov, 2017-05-23 15:48:12]

The CQ bit was checked by dglazkov@chromium.org

[commit-bot: I haz the power, 2017-05-23 15:48:53]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-23 15:57:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-23 15:57:52]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[aboxhall, 2017-05-23 22:40:24]

The CQ bit was checked by aboxhall@chromium.org

[aboxhall, 2017-05-23 22:40:25]

The patchset sent to the CQ was uploaded after l-g-t-m from hayato@chromium.org,
dglazkov@chromium.org, esprehn@chromium.org, dmazzoni@chromium.org,
kochi@chromium.org
Link to the patchset: https://codereview.chromium.org/2088453002/#ps1110001
(title: "rebase")

[commit-bot: I haz the power, 2017-05-23 22:41:43]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-24 03:47:24]

CQ is committing da patch.

Bot data: {"patchset_id": 1110001, "attempt_start_ts": 1495579224990990,
"parent_rev": "297cc934f9950cf3644d28f1aea182bea6a9e25e", "commit_rev":
"902968d6cae5c5c6fbe124abdcbb06fb053a6397"}

[commit-bot: I haz the power, 2017-05-24 03:47:57]

CQ is committing da patch.

Bot data: {"patchset_id": 1110001, "attempt_start_ts": 1495579224990990,
"parent_rev": "54bf385aa6becac4aeecd2bf79604bc9421b6306", "commit_rev":
"cca67d31f4a0b4ec2c20090a8bfc38c464274ab2"}

[commit-bot: I haz the power, 2017-05-24 03:48:13]

Description was changed from

==========
Implement the inert attribute: https://github.com/WICG/inert#tldr

https://discourse.wicg.io/t/inert-attribute/1838/

BUG=692360
==========

to

==========
Implement the inert attribute: https://github.com/WICG/inert#tldr

https://discourse.wicg.io/t/inert-attribute/1838/

BUG=692360

Review-Url: https://codereview.chromium.org/2088453002
Cr-Commit-Position: refs/heads/master@{#474153}
Committed:
https://chromium.googlesource.com/chromium/src/+/cca67d31f4a0b4ec2c20090a8bfc...
==========

[commit-bot: I haz the power, 2017-05-24 03:48:19]

Committed patchset #51 (id:1110001) as
https://chromium.googlesource.com/chromium/src/+/cca67d31f4a0b4ec2c20090a8bfc...

[vitaliii, 2017-05-24 10:56:58]

On 2017/05/24 03:48:19, commit-bot: I haz the power wrote:
> Committed patchset #51 (id:1110001) as
>
https://chromium.googlesource.com/chromium/src/+/cca67d31f4a0b4ec2c20090a8bfc...

This CL is being reverted in https://codereview.chromium.org/2902133002/.
Please see crbug.com/725815.

[aboxhall, 2017-05-25 04:47:59]

Description was changed from

==========
Implement the inert attribute: https://github.com/WICG/inert#tldr

https://discourse.wicg.io/t/inert-attribute/1838/

BUG=692360

Review-Url: https://codereview.chromium.org/2088453002
Cr-Commit-Position: refs/heads/master@{#474153}
Committed:
https://chromium.googlesource.com/chromium/src/+/cca67d31f4a0b4ec2c20090a8bfc...
==========

to

==========
Implement the inert attribute: https://github.com/WICG/inert#tldr

https://discourse.wicg.io/t/inert-attribute/1838/

BUG=692360

Review-Url: https://codereview.chromium.org/2088453002
Cr-Commit-Position: refs/heads/master@{#474153}
Committed:
https://chromium.googlesource.com/chromium/src/+/cca67d31f4a0b4ec2c20090a8bfc...
==========

[aboxhall, 2017-05-25 05:04:30]

The CQ bit was checked by aboxhall@chromium.org

[aboxhall, 2017-05-25 05:04:31]

The patchset sent to the CQ was uploaded after l-g-t-m from hayato@chromium.org,
dglazkov@chromium.org, esprehn@chromium.org, dmazzoni@chromium.org,
kochi@chromium.org
Link to the patchset: https://codereview.chromium.org/2088453002/#ps1130001
(title: "Add leak expectations for tests which reveal existing leaks")

[aboxhall, 2017-05-25 05:04:36]

The CQ bit was unchecked by aboxhall@chromium.org

[aboxhall, 2017-05-25 05:05:52]

The CQ bit was checked by aboxhall@chromium.org

[commit-bot: I haz the power, 2017-05-25 05:06:08]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-25 05:16:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-25 05:16:33]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[aboxhall, 2017-05-25 05:20:37]

The CQ bit was checked by aboxhall@chromium.org

[aboxhall, 2017-05-25 05:20:38]

The patchset sent to the CQ was uploaded after l-g-t-m from hayato@chromium.org,
dglazkov@chromium.org, esprehn@chromium.org, dmazzoni@chromium.org,
kochi@chromium.org
Link to the patchset: https://codereview.chromium.org/2088453002/#ps1150001
(title: "Revert histograms.xml")

[commit-bot: I haz the power, 2017-05-25 05:21:03]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-25 07:10:09]

CQ is committing da patch.

Bot data: {"patchset_id": 1150001, "attempt_start_ts": 1495689637383640,
"parent_rev": "933dc3199a9418e3c5b1d14d681d976867e8e4f9", "commit_rev":
"58a3e83e1325c8a366941564ca8ce096bb1345df"}

[commit-bot: I haz the power, 2017-05-25 07:10:31]

Description was changed from

==========
Implement the inert attribute: https://github.com/WICG/inert#tldr

https://discourse.wicg.io/t/inert-attribute/1838/

BUG=692360

Review-Url: https://codereview.chromium.org/2088453002
Cr-Commit-Position: refs/heads/master@{#474153}
Committed:
https://chromium.googlesource.com/chromium/src/+/cca67d31f4a0b4ec2c20090a8bfc...
==========

to

==========
Implement the inert attribute: https://github.com/WICG/inert#tldr

https://discourse.wicg.io/t/inert-attribute/1838/

BUG=692360

Review-Url: https://codereview.chromium.org/2088453002
Cr-Original-Commit-Position: refs/heads/master@{#474153}
Committed:
https://chromium.googlesource.com/chromium/src/+/cca67d31f4a0b4ec2c20090a8bfc...
Review-Url: https://codereview.chromium.org/2088453002
Cr-Commit-Position: refs/heads/master@{#474598}
Committed:
https://chromium.googlesource.com/chromium/src/+/58a3e83e1325c8a366941564ca8c...
==========

[commit-bot: I haz the power, 2017-05-25 07:10:35]

Committed patchset #53 (id:1150001) as
https://chromium.googlesource.com/chromium/src/+/58a3e83e1325c8a366941564ca8c...