Propagate inert state to OOPIFs when a modal dialog is active

Propagate inert state to OOPIFs when a modal dialog is active

When showModal is called on a <dialog> element, the rest of the
Document becomes inert which prevents it from receiving events or taking
focus. When the Document contains an out-of-process iframe, however, its
contents are not aware of the modal dialog in a remote ancestor.

This CL caches the current inert state on each LocalFrame, which is
changed when a modal dialog because active or inactive. The bit is
plumbed to remote frame children so that OOPIFs will respect inertness.

Also it prevents having to search up the entire frame tree when an
element checks whether it is inert.

BUG=719788
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2883033003
Cr-Commit-Position: refs/heads/master@{#481761}
Committed: https://chromium.googlesource.com/chromium/src/+/04323789a189306e523b5982a9cd49dcebdc88f5

[kenrb, 2017-05-15 20:46:59]

Description was changed from

==========
Propagate inert state to OOPIFs when a modal dialog is active

When showModal is called on a <dialog> element, the rest of the
Document becomes inert which prevents it from receiving events or taking
focus. When the Document contains an out-of-process iframe, however, its
contents are not aware of the modal dialog in a remote ancestor.

This CL caches the current inert state on each LocalFrame, which is
changed when a modal dialog because active or inactive. The bit is
plumbed to remote frame children so that OOPIFs will respect inertness.

Also it prevents having to search up the entire frame tree when an
element checks whether it is inert.

BUG=719788
==========

to

==========
Propagate inert state to OOPIFs when a modal dialog is active

When showModal is called on a <dialog> element, the rest of the
Document becomes inert which prevents it from receiving events or taking
focus. When the Document contains an out-of-process iframe, however, its
contents are not aware of the modal dialog in a remote ancestor.

This CL caches the current inert state on each LocalFrame, which is
changed when a modal dialog because active or inactive. The bit is
plumbed to remote frame children so that OOPIFs will respect inertness.

Also it prevents having to search up the entire frame tree when an
element checks whether it is inert.

BUG=719788
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[kenrb, 2017-05-15 20:47:09]

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

[commit-bot: I haz the power, 2017-05-15 20:47:49]

Dry run: CQ is trying da patch.

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

[kenrb, 2017-05-15 20:51:54]

https://codereview.chromium.org/2883033003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/HTMLDialogElement.cpp (right):

https://codereview.chromium.org/2883033003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/HTMLDialogElement.cpp:192: // FIXME: We
should call inertSubtreesChanged() here.
Does this become a problem? With an inert bit being cached on Frames, I am
concerned this might create situations where Frames are stuck being inert.

[commit-bot: I haz the power, 2017-05-15 20:53:13]

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

[commit-bot: I haz the power, 2017-05-15 20:53:14]

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...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[kenrb, 2017-05-16 15:10:23]

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

[commit-bot: I haz the power, 2017-05-16 15:10:52]

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-16 15:14:37]

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

[commit-bot: I haz the power, 2017-05-16 15:14:38]

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

[kenrb, 2017-05-16 15:39:41]

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

[commit-bot: I haz the power, 2017-05-16 15:40: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-16 15:44:16]

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

[commit-bot: I haz the power, 2017-05-16 15:44:18]

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

[kenrb, 2017-05-16 16:12:27]

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

[commit-bot: I haz the power, 2017-05-16 16:13:17]

Dry run: CQ is trying da patch.

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

[kenrb, 2017-05-16 17:02:32]

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

[kenrb, 2017-05-16 17:02:33]

aboxhall, dmazzoni: Can one or both of you PTAL?

This is an attempt to implement comment #4 by dmazzoni on
https://crbug.com/719788, which caches the inert state on the LocalFrame object.
When a RemoteFrame becomes inert, the flag is propagated across processes.

Is this about what you had in mind?

Never mind the question I put on PS1. I answered it myself (the FIXME in
HTMLDialogElement::RemovedFrom does indeed need to be addressed).

[commit-bot: I haz the power, 2017-05-16 18:06:57]

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

[commit-bot: I haz the power, 2017-05-16 18:06:58]

Dry run: This issue passed the CQ dry run.

[aboxhall, 2017-05-18 04:47:27]

https://codereview.chromium.org/2883033003/diff/60002/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLDialogElement.cpp (right):

https://codereview.chromium.org/2883033003/diff/60002/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLDialogElement.cpp:89:
document.GetFrame()->SetIsInert(false);
When does SetIsInert(true) get called?

Also, you might want to add a case to
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/html/dial...
to ensure that frames *outside* the dialog (incl. OOPIFs) are correctly made
inert, while frames *inside* the dialog are *not* made inert.

[kenrb, 2017-05-18 12:21:49]

Thanks for the feedback.

https://codereview.chromium.org/2883033003/diff/60002/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLDialogElement.cpp (right):

https://codereview.chromium.org/2883033003/diff/60002/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLDialogElement.cpp:89:
document.GetFrame()->SetIsInert(false);
On 2017/05/18 04:47:27, aboxhall wrote:
> When does SetIsInert(true) get called?
> 

See LocalFrame::SetIsInert(). I should probably add some comments on that
method, but it works such that if you call it with true then it propagates the
inert flag down through all of its descendant frames, and if you call it with
false then it has each of its descendant frames check for inertness via
Node::IsInert().

> Also, you might want to add a case to
>
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/html/dial...
> to ensure that frames *outside* the dialog (incl. OOPIFs) are correctly made
> inert, while frames *inside* the dialog are *not* made inert.

I can do that. Unfortunately testing OOPIFs in layout tests is a bit more
complicated, since they have to be in LayoutTests/http/tests/ in order to load
frames in different origins. I'll see if I can get something working for that.

[dmazzoni, 2017-05-18 15:47:06]

Design question: what happens if a child frame calls showModal,
does that affect the parent frame? Probably not, that seems like
it would be a good way to hijack!

However, child frames of the frame with the modal dialog may
need to become inert, unless they're within the active dialog
element.

So the part I don't see is that when showModal is called, we
need to iterate over the child frames.

For testing accessibility, we already have some mechanisms
for out-of-process iframes in content/test/data/accessibility,
and we also already have tests for dialogs in iframes, but
both of those tests were disabled for being flaky. I can try
to help fix the flakiness in those dialog iframe tests and then
we can add two new variants of those for the oopif case.

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

https://codereview.chromium.org/2883033003/diff/60002/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Node.cpp:854: return GetDocument().GetFrame()
&& GetDocument().GetFrame()->IsInert();
I don't think this changes the logic, but I'm just checking - when would
GetFrame() return nullptr, and if so what should IsInert return?

https://codereview.chromium.org/2883033003/diff/60002/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLDialogElement.cpp (right):

https://codereview.chromium.org/2883033003/diff/60002/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLDialogElement.cpp:85: // and element
that is in a frame that is already inert? If not, we
I don't see how we can know that. A page can always try to call "open" on a
dialog, right?

I think we need to handle that case.

https://codereview.chromium.org/2883033003/diff/60002/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLDialogElement.cpp:183:
InertSubtreesChanged(GetDocument());
I think more logic is needed here.

Previously IsInert was computed just by checking if there's an
active modal dialog in the current frame, so all this method needed
to do was clear some state.

Now, calling showModal needs to actually trigger making other
frames inert. I don't see where this is done...

[kenrb, 2017-05-19 17:31:39]

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

[commit-bot: I haz the power, 2017-05-19 17:32:39]

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-19 17:36:34]

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

[commit-bot: I haz the power, 2017-05-19 17:36:35]

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

[kenrb, 2017-05-19 17:46:28]

Thanks. I have added some comments in the most recent patchset, and have
modified the test as suggested by aboxhall.

Unfortunately there isn't an OOPIF-test yet. I can have a look at the flaky
OOPIF tests for accessibility also. I know how tricky these can be to write.

On 2017/05/18 15:47:06, dmazzoni wrote:
> Design question: what happens if a child frame calls showModal,
> does that affect the parent frame? Probably not, that seems like
> it would be a good way to hijack!
> 
> However, child frames of the frame with the modal dialog may
> need to become inert, unless they're within the active dialog
> element.
> 
> So the part I don't see is that when showModal is called, we
> need to iterate over the child frames.
> 

That's in LocalFrame::SetIsInert(). When showModal() is called, it calls
SetIsInert on the frame that contains the dialog, and that recurses down through
all subframes.

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

https://codereview.chromium.org/2883033003/diff/60002/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Node.cpp:854: return GetDocument().GetFrame()
&& GetDocument().GetFrame()->IsInert();
On 2017/05/18 15:47:05, dmazzoni wrote:
> I don't think this changes the logic, but I'm just checking - when would
> GetFrame() return nullptr, and if so what should IsInert return?

It should be fine to remove that condition, on the basis that this code
shouldn't be reachable for a Document with a null frame_ pointer. If any tests
prove me wrong I can change it back.

https://codereview.chromium.org/2883033003/diff/60002/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLDialogElement.cpp (right):

https://codereview.chromium.org/2883033003/diff/60002/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLDialogElement.cpp:85: // and element
that is in a frame that is already inert? If not, we
On 2017/05/18 15:47:06, dmazzoni wrote:
> I don't see how we can know that. A page can always try to call "open" on a
> dialog, right?
> 
> I think we need to handle that case.

Done.

[kenrb, 2017-05-19 18:15:27]

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

[commit-bot: I haz the power, 2017-05-19 18:16:27]

Dry run: CQ is trying da patch.

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

[dmazzoni, 2017-05-19 18:58:19]

lgtm, I understand how it all works now,
just some suggestions!

https://codereview.chromium.org/2883033003/diff/110001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/2883033003/diff/110001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/LocalFrame.cpp:574: void
LocalFrame::SetIsInert(bool inert) {
Do you think it would be more clear to separate this
into two methods, SetIsInert() and
PropagateInertToChildFrames()?

https://codereview.chromium.org/2883033003/diff/110001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLDialogElement.cpp (right):

https://codereview.chromium.org/2883033003/diff/110001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLDialogElement.cpp:85: if
(document.GetFrame()) {
Perhaps this deserves a comment saying that this is what
propagates the state to all child frames?

[commit-bot: I haz the power, 2017-05-19 19:42:51]

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

[commit-bot: I haz the power, 2017-05-19 19:42:52]

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

[dmazzoni, 2017-05-22 21:53:00]

On Fri, May 19, 2017 at 10:46 AM <kenrb@chromium.org> wrote:

> Thanks. I have added some comments in the most recent patchset, and have
> modified the test as suggested by aboxhall.
>
> Unfortunately there isn't an OOPIF-test yet. I can have a look at the flaky
> OOPIF tests for accessibility also. I know how tricky these can be to
> write.
>

I re-enabled the dialog in iframe accessibility tests. It should be trivial
to add oopif variants of the same tests, just copy what we do
in iframe-cross-process.html and the rest should be automatic.

A follow-up change is fine.

-- 
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-22 21:53:00]

On Fri, May 19, 2017 at 10:46 AM <kenrb@chromium.org> wrote:

> Thanks. I have added some comments in the most recent patchset, and have
> modified the test as suggested by aboxhall.
>
> Unfortunately there isn't an OOPIF-test yet. I can have a look at the flaky
> OOPIF tests for accessibility also. I know how tricky these can be to
> write.
>

I re-enabled the dialog in iframe accessibility tests. It should be trivial
to add oopif variants of the same tests, just copy what we do
in iframe-cross-process.html and the rest should be automatic.

A follow-up change is fine.

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

[kenrb, 2017-05-23 14:28:55]

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

[commit-bot: I haz the power, 2017-05-23 14:29:09]

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-23 16:20:26]

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

[commit-bot: I haz the power, 2017-05-23 16:20:29]

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

[kenrb, 2017-05-30 15:12:14]

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

[commit-bot: I haz the power, 2017-05-30 15:12: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-05-30 15:14:36]

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

[commit-bot: I haz the power, 2017-05-30 15:14: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-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[kenrb, 2017-05-30 15:44:59]

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

[commit-bot: I haz the power, 2017-05-30 15:45:26]

Dry run: CQ is trying da patch.

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

[kenrb, 2017-05-31 18:32:01]

PTAL again?

I have added a browsertest, and also had to make some changes to account for
aboxhall@ having landed the Inert attribute last week. To summarize, we now
check whether we need to update the inert bit on a Frame when one of these
things happens:
- the Frame's parent has its inert bit changed (i.e., SetIsInert recurses
downward)
- the Frame is created, if it is attached to an HTMLFrameOwnerElement
- an element in the Frame has its inert attribute changed
- a dialog element in the Frame becomes modal, or is cancelled

https://codereview.chromium.org/2883033003/diff/110001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/2883033003/diff/110001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/LocalFrame.cpp:574: void
LocalFrame::SetIsInert(bool inert) {
On 2017/05/19 18:58:19, dmazzoni wrote:
> Do you think it would be more clear to separate this
> into two methods, SetIsInert() and
> PropagateInertToChildFrames()?

Done.

https://codereview.chromium.org/2883033003/diff/110001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLDialogElement.cpp (right):

https://codereview.chromium.org/2883033003/diff/110001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLDialogElement.cpp:85: if
(document.GetFrame()) {
On 2017/05/19 18:58:19, dmazzoni wrote:
> Perhaps this deserves a comment saying that this is what
> propagates the state to all child frames?

Done.

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

https://codereview.chromium.org/2883033003/diff/130001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Node.cpp:854: return GetDocument().GetFrame()
&& GetDocument().GetFrame()->IsInert();
I had to keep the GetDocument().GetFrame() check, since removing it causes a
layout test to crash.

[kenrb, 2017-06-02 21:20:52]

Friendly ping.

dmazzoni or aboxhall, do either of you have any more thoughts before I start
asking OWNERS for review?

[dmazzoni, 2017-06-05 17:23:33]

Taking another look now

[dmazzoni, 2017-06-05 17:32:43]

https://codereview.chromium.org/2883033003/diff/190001/content/browser/site_p...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/2883033003/diff/190001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:10272: // The setTimeout ensures
that the inert bit can propagate before the
Usually a setTimeout of anything other than 0 in a test is a red flag, it's
likely to
lead to occasional flakiness.

Could you instead have the injected script just try to repeatedly focus the
element
until it fails? That way either it eventually fails to get focus and the test
succeeds, or the test
times out.

[kenrb, 2017-06-05 18:59:19]

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

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

Dry run: CQ is trying da patch.

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

[kenrb, 2017-06-05 19:22:27]

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

[commit-bot: I haz the power, 2017-06-05 19:22:49]

Dry run: CQ is trying da patch.

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

[kenrb, 2017-06-05 19:25:50]

https://codereview.chromium.org/2883033003/diff/190001/content/browser/site_p...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/2883033003/diff/190001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:10272: // The setTimeout ensures
that the inert bit can propagate before the
On 2017/06/05 17:32:42, dmazzoni wrote:
> Usually a setTimeout of anything other than 0 in a test is a red flag, it's
> likely to
> lead to occasional flakiness.
> 
> Could you instead have the injected script just try to repeatedly focus the
> element
> until it fails? That way either it eventually fails to get focus and the test
> succeeds, or the test
> times out.

I've tried something a bit different in patchset 13. The timeout value is now 0,
and I've added code that should ensure the SetIsInert update arrives at the
renderer process before the JavaScript. In that case I think the setTimeout is
sufficient to guarantee that the inert state is all updated, and it appears to
be reliable in local testing.

[commit-bot: I haz the power, 2017-06-05 20:38:06]

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

[commit-bot: I haz the power, 2017-06-05 20:38:07]

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

[kenrb, 2017-06-05 21:36:38]

It looks like there are bugs with the changes that I made to support the Inert
attribute. The triggers to test for a Frame's inert state might need to happen
at different times.

[kenrb, 2017-06-06 16:19:03]

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

[commit-bot: I haz the power, 2017-06-06 16:19: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-06-06 17:32:29]

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

[commit-bot: I haz the power, 2017-06-06 17:32:30]

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

[kenrb, 2017-06-06 21:37:38]

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

[commit-bot: I haz the power, 2017-06-06 21:38: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-06-06 23:57:57]

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

[commit-bot: I haz the power, 2017-06-06 23:57:58]

Dry run: This issue passed the CQ dry run.

[kenrb, 2017-06-07 01:41:12]

I have PS#14 up now, which passes all tests, but I am not certain this is the
best approach. This now does an inertness check for the current frame in
Document::UpdateStyle, and only propagates down the frame tree if it would
change the child frame's inert bit. Previous approaches were wrong because it
would sometimes try to check inertness on Nodes with dirty Redistribution bits,
which would trigger a DCHECK.

[dmazzoni, 2017-06-07 16:39:04]

On 2017/06/07 01:41:12, kenrb wrote:
> I have PS#14 up now, which passes all tests, but I am not certain this is the
> best approach. This now does an inertness check for the current frame in
> Document::UpdateStyle,

Do you also need to set style dirty when updating a frame's inertness?

Probably time for a Blink core owner to take a look. Perhaps dcheng@ or
tkent@?

[kenrb, 2017-06-08 20:20:50]

kenrb@chromium.org changed reviewers:
+ tkent@chromium.org

[kenrb, 2017-06-08 20:20:51]

tkent@: PTAL WebKit/?

[tkent, 2017-06-08 23:51:55]

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

[tkent, 2017-06-08 23:53:49]

https://codereview.chromium.org/2883033003/diff/250001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/2883033003/diff/250001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Document.cpp:2271:
GetFrame()->SetIsInert(owner->IsInert());
This was added to fix "!node.NeedsDistributionRecalc()" dcheck failures, right?
I'm not sure if this is a right fix.
hayato@, do you have any advice?

https://codereview.chromium.org/2883033003/diff/250001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp (right):

https://codereview.chromium.org/2883033003/diff/250001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp:776:
DCHECK(local_root_->Parent() && local_root_->Parent()->IsWebRemoteFrame());
nit: Split the DCHECK into two so that we can know which one failed.

DCHECK(local_root_->Parent());
DCHECK(local_root_->Parent()->IsWebRemoteFrame());

[kenrb, 2017-06-09 15:15:48]

https://codereview.chromium.org/2883033003/diff/250001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/2883033003/diff/250001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Document.cpp:2271:
GetFrame()->SetIsInert(owner->IsInert());
On 2017/06/08 23:53:48, tkent wrote:
> This was added to fix "!node.NeedsDistributionRecalc()" dcheck failures,
right?
> I'm not sure if this is a right fix.

Correct, and I'm not sure it is the right fix either. My earlier attempts were
to update the cached inert values on the Frames whenever an inert attribute
changed, or else when a Frame was attached to its parent element. There were
DCHECKs hit because I was breaking assumptions about elements being style clean.
Re-checking a Frame's inertness during style update, which I added here, seems
to work, but I would welcome better suggestions on how to do it.

[kenrb, 2017-06-09 19:25:35]

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

[commit-bot: I haz the power, 2017-06-09 19:26:04]

Dry run: CQ is trying da patch.

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

[kenrb, 2017-06-09 19:26:31]

https://codereview.chromium.org/2883033003/diff/250001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp (right):

https://codereview.chromium.org/2883033003/diff/250001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp:776:
DCHECK(local_root_->Parent() && local_root_->Parent()->IsWebRemoteFrame());
On 2017/06/08 23:53:49, tkent wrote:
> nit: Split the DCHECK into two so that we can know which one failed.
> 
> DCHECK(local_root_->Parent());
> DCHECK(local_root_->Parent()->IsWebRemoteFrame());

Done.

[commit-bot: I haz the power, 2017-06-09 19:29:07]

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

[commit-bot: I haz the power, 2017-06-09 19:29:08]

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

[kenrb, 2017-06-12 16:54:07]

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

[commit-bot: I haz the power, 2017-06-12 16:54: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-06-12 18:57:35]

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

[commit-bot: I haz the power, 2017-06-12 18:57:36]

Dry run: This issue passed the CQ dry run.

[kenrb, 2017-06-12 20:23:58]

hayato@: PTAL? See comment by tkent@ above. Thanks!

[hayato, 2017-06-13 09:48:00]

https://codereview.chromium.org/2883033003/diff/250001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/2883033003/diff/250001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Document.cpp:2271:
GetFrame()->SetIsInert(owner->IsInert());
On 2017/06/09 at 15:15:48, kenrb wrote:
> On 2017/06/08 23:53:48, tkent wrote:
> > This was added to fix "!node.NeedsDistributionRecalc()" dcheck failures,
right?
> > I'm not sure if this is a right fix.
> 
> Correct, and I'm not sure it is the right fix either. My earlier attempts were
to update the cached inert values on the Frames whenever an inert attribute
changed, or else when a Frame was attached to its parent element. There were
DCHECKs hit because I was breaking assumptions about elements being style clean.
Re-checking a Frame's inertness during style update, which I added here, seems
to work, but I would welcome better suggestions on how to do it.

Using UpdateStyleAndLayout() as a hook for SetIsInert() doesn't look good to me.
What we have to do is calling UpdateDistribution() somewhere so that it doesn't
hit DCHCEK() before calling SetIsInert(), I think.

[kenrb, 2017-06-13 19:36:15]

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

[commit-bot: I haz the power, 2017-06-13 19:37:21]

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-06-13 22:07:59]

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

[commit-bot: I haz the power, 2017-06-13 22:08:01]

Dry run: This issue passed the CQ dry run.

[kenrb, 2017-06-14 01:40:37]

Okay, I have gone back to the first way of doing this, and added calls to
UpdateDistribution() where necessary. Is this reasonable?

[hayato, 2017-06-14 01:55:14]

On 2017/06/14 at 01:40:37, kenrb wrote:
> Okay, I have gone back to the first way of doing this, and added calls to
UpdateDistribution() where necessary. Is this reasonable?

Yeah, sounds reasonable.

[kenrb, 2017-06-14 14:21:29]

On 2017/06/14 01:55:14, hayato wrote:
> On 2017/06/14 at 01:40:37, kenrb wrote:
> > Okay, I have gone back to the first way of doing this, and added calls to
> UpdateDistribution() where necessary. Is this reasonable?
> 
> Yeah, sounds reasonable.

Thanks.

tkent@: Can you review the Blink side of this?

[tkent, 2017-06-15 07:17:49]

> tkent@: Can you review the Blink side of this?

lgtm

[kenrb, 2017-06-15 14:16:12]

kenrb@chromium.org changed reviewers:
+ alexmos@chromium.org

[kenrb, 2017-06-15 14:16:13]

alexmos@: PTAL content/?

[alexmos, 2017-06-16 02:17:58]

Thanks Ken!  A few comments/questions below.

https://codereview.chromium.org/2883033003/diff/310001/content/browser/frame_...
File content/browser/frame_host/render_widget_host_view_child_frame.cc (right):

https://codereview.chromium.org/2883033003/diff/310001/content/browser/frame_...
content/browser/frame_host/render_widget_host_view_child_frame.cc:388:
host_->Send(new ViewMsg_SetIsInert(host_->GetRoutingID(), inert));
To me it seemed a little confusing to have the inert state come in with a frame
message, but then forward it with a view message, especially given that the
inert state is stored per-frame.  Wouldn't it work to just handle
FrameHostMsg_SetIsInert in RenderFrameProxyHost and send it to
frame_tree_node_->current_frame_host()?

https://codereview.chromium.org/2883033003/diff/310001/content/browser/site_p...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/2883033003/diff/310001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:10222: void Reset() {
Is this needed?  It doesn't appear to be used in the test.

https://codereview.chromium.org/2883033003/diff/310001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:10270:
root->current_frame_host()->GetProcess()->AddFilter(filter.get());
Perhaps mention in a comment somewhere that the inert message will be sent by
the proxy for b.com in a.com's process, and hence the filter is installed for
a.com.

https://codereview.chromium.org/2883033003/diff/310001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:10302: EXPECT_EQ("",
focused_element);
I was also curious about scenarios like the page dynamically adding a subframe
while its top frame is showing a modal dialog, and then navigating the subframe
cross-site (and maybe remote-to-local after that).  I *think* those should work
correctly (e.g., the swaps should preserve the bit), but just wanted to see if
you've thought about them.  Don't know if it's worth adding test coverage for
that, as it's unlikely to happen in practice, but might be worth it if it's
easy.

https://codereview.chromium.org/2883033003/diff/310001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/Frame.cpp (right):

https://codereview.chromium.org/2883033003/diff/310001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/Frame.cpp:201: void
Frame::UpdateInertIfPossible() {
Is the "IfPossible" conveying that we only need to do this for subframes? 
Perhaps UpdateInertIfNeeded or UpdateInertIfNecessary to be more consistent with
similar cases?

https://codereview.chromium.org/2883033003/diff/310001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/2883033003/diff/310001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/LocalFrame.cpp:596:
PropagateInertToChildFrames();
Should this also only be done if |is_inert_| was different from |inert|?

https://codereview.chromium.org/2883033003/diff/310001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/LocalFrame.cpp:601: child =
child->Tree().NextSibling()) {
How does this walk work for nested RemoteFrames?  I.e., for a(b(c)), seems like
we'll only call SetIsInert on b but not on c.  Or do you rely on b.com
LocalFrame sending the message once it receives it?  If the latter, could we
just walk the whole hierarchy here and send the message to all remote frame
descendants, to avoid extra roundtrips?

https://codereview.chromium.org/2883033003/diff/310001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/LocalFrame.cpp:602: if (child->Owner()) {
Is there any case where this could be null, given that these are child frames?

https://codereview.chromium.org/2883033003/diff/310001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/LocalFrame.cpp:604:
ToHTMLFrameOwnerElement(child->Owner())->IsInert());
When would is_inert_ be different from
ToHTMLFrameOwnerElement(child->Owner())->IsInert()?

https://codereview.chromium.org/2883033003/diff/310001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/LocalFrame.cpp:954:
UpdateInertIfPossible();
Perhaps a comment about why this is not needed for local roots?

https://codereview.chromium.org/2883033003/diff/310001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/LocalFrame.h (right):

https://codereview.chromium.org/2883033003/diff/310001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/LocalFrame.h:258: bool IsInert() const {
return is_inert_; }
Is this used anywhere?  I might've missed it, but I only see IsInert called on
HTMLFrameOwnerElements.

https://codereview.chromium.org/2883033003/diff/310001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/RemoteFrame.h (right):

https://codereview.chromium.org/2883033003/diff/310001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/RemoteFrame.h:68: bool last_inert_state_;
Since both local and remote frames need this bool, would it make sense to just
keep it in Frame?

https://codereview.chromium.org/2883033003/diff/310001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLDialogElement.cpp (right):

https://codereview.chromium.org/2883033003/diff/310001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLDialogElement.cpp:89:
document.GetFrame()->SetIsInert(document.LocalOwner() &&
How does this work when showModal is called in a main frame where owner is null?
 Won't we incorrectly pass the bit as false then, and set is_inert_ as false on
the main LocalFrame?  (Looks like the remote frames would still propagate the
right value, as they consult is_inert_ ||
ToHTMLFrameOwnerElement(child->Owner())->IsInert() and the latter would be
true.)

[kenrb, 2017-06-19 18:29:59]

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

[commit-bot: I haz the power, 2017-06-19 18:30:39]

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-06-19 18:55:41]

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

[commit-bot: I haz the power, 2017-06-19 18:55:43]

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

[kenrb, 2017-06-19 19:26:23]

Thanks for the review. Responses below, and I've tried to address your
suggestions in PS#17.

https://codereview.chromium.org/2883033003/diff/310001/content/browser/frame_...
File content/browser/frame_host/render_widget_host_view_child_frame.cc (right):

https://codereview.chromium.org/2883033003/diff/310001/content/browser/frame_...
content/browser/frame_host/render_widget_host_view_child_frame.cc:388:
host_->Send(new ViewMsg_SetIsInert(host_->GetRoutingID(), inert));
On 2017/06/16 02:17:57, alexmos wrote:
> To me it seemed a little confusing to have the inert state come in with a
frame
> message, but then forward it with a view message, especially given that the
> inert state is stored per-frame.  Wouldn't it work to just handle
> FrameHostMsg_SetIsInert in RenderFrameProxyHost and send it to
> frame_tree_node_->current_frame_host()? 

Originally I did it this way to be consistent with how parent-child state
updates are generally done, which lets us keep handlers on RenderWidget for
messages that only go to local frame roots.

More concretely, your point elsewhere about frame navigations is potentially a
problem, and suggests that this is better as a ViewMsg. If the child frame does
a cross-process navigation, the inert bit needs to be re-sent, along with other
widget-level state. It would be difficult to do that with a FrameMsg, rather
than keeping the state in CPFC and sending it with other state from the
RenderWidgetHostView. The current patchset incorporates the latter approach.

https://codereview.chromium.org/2883033003/diff/310001/content/browser/site_p...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/2883033003/diff/310001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:10222: void Reset() {
On 2017/06/16 02:17:57, alexmos wrote:
> Is this needed?  It doesn't appear to be used in the test.

Removed.

https://codereview.chromium.org/2883033003/diff/310001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:10270:
root->current_frame_host()->GetProcess()->AddFilter(filter.get());
On 2017/06/16 02:17:57, alexmos wrote:
> Perhaps mention in a comment somewhere that the inert message will be sent by
> the proxy for b.com in a.com's process, and hence the filter is installed for
> a.com.

Done.

https://codereview.chromium.org/2883033003/diff/310001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:10302: EXPECT_EQ("",
focused_element);
On 2017/06/16 02:17:57, alexmos wrote:
> I was also curious about scenarios like the page dynamically adding a subframe
> while its top frame is showing a modal dialog, and then navigating the
subframe
> cross-site (and maybe remote-to-local after that).  I *think* those should
work
> correctly (e.g., the swaps should preserve the bit), but just wanted to see if
> you've thought about them.  Don't know if it's worth adding test coverage for
> that, as it's unlikely to happen in practice, but might be worth it if it's
> easy.

The bit needs to be sent again after a swap from one process to another, which I
have added to the next PS. Also, I have added a navigation to this test in order
to verify the bit is preserved.

https://codereview.chromium.org/2883033003/diff/310001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/Frame.cpp (right):

https://codereview.chromium.org/2883033003/diff/310001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/Frame.cpp:201: void
Frame::UpdateInertIfPossible() {
On 2017/06/16 02:17:57, alexmos wrote:
> Is the "IfPossible" conveying that we only need to do this for subframes? 
> Perhaps UpdateInertIfNeeded or UpdateInertIfNecessary to be more consistent
with
> similar cases?

Not exactly, the 'IfPossible' is because the Frame might not be connected to its
owner yet, in which case the check will be done later.

https://codereview.chromium.org/2883033003/diff/310001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/2883033003/diff/310001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/LocalFrame.cpp:596:
PropagateInertToChildFrames();
On 2017/06/16 02:17:57, alexmos wrote:
> Should this also only be done if |is_inert_| was different from |inert|?

No, because sometimes the propagation to child frames is needed even when the
current frame's bit doesn't change; i.e. because an element in the current frame
changed. In a previous iteration I had checked the child's bit from
PropagateInertToChildFrame() and only propagated if it would change, but removed
it because it required an extra method on Frame.h and it seemed okay to be
slightly conservative.

https://codereview.chromium.org/2883033003/diff/310001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/LocalFrame.cpp:601: child =
child->Tree().NextSibling()) {
On 2017/06/16 02:17:57, alexmos wrote:
> How does this walk work for nested RemoteFrames?  I.e., for a(b(c)), seems
like
> we'll only call SetIsInert on b but not on c.  Or do you rely on b.com
> LocalFrame sending the message once it receives it?  If the latter, could we
> just walk the whole hierarchy here and send the message to all remote frame
> descendants, to avoid extra roundtrips?

From within Blink, we don't know if nested OOPIFs exist. If (c) is same-process
under (b), then the update will happen properly within the child frame's
renderer. If (c) is remote from (b), then there will be an extra round trip, as
you note. If we want to optimize that then it would have to be within the
browser process, but would add complexity (because (b) would need to know to
*not* update (c)) and I'm not sure the trade-off is worth it.

https://codereview.chromium.org/2883033003/diff/310001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/LocalFrame.cpp:602: if (child->Owner()) {
On 2017/06/16 02:17:57, alexmos wrote:
> Is there any case where this could be null, given that these are child frames?

Maybe not. I've removed it and layout tests seem to pass.

https://codereview.chromium.org/2883033003/diff/310001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/LocalFrame.cpp:604:
ToHTMLFrameOwnerElement(child->Owner())->IsInert());
On 2017/06/16 02:17:57, alexmos wrote:
> When would is_inert_ be different from
> ToHTMLFrameOwnerElement(child->Owner())->IsInert()?

If child->Owner()->IsInert() returns true, it means there is an element in this
Frame that is causing other elements to become inert, including the child Frame
element. is_inert_ being true means that we are inheriting inertness from a
parent frame's context.

https://codereview.chromium.org/2883033003/diff/310001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/LocalFrame.cpp:954:
UpdateInertIfPossible();
On 2017/06/16 02:17:57, alexmos wrote:
> Perhaps a comment about why this is not needed for local roots?

Done.

https://codereview.chromium.org/2883033003/diff/310001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/LocalFrame.h (right):

https://codereview.chromium.org/2883033003/diff/310001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/LocalFrame.h:258: bool IsInert() const {
return is_inert_; }
On 2017/06/16 02:17:58, alexmos wrote:
> Is this used anywhere?  I might've missed it, but I only see IsInert called on
> HTMLFrameOwnerElements.

Yes, in Node::IsInert().

https://codereview.chromium.org/2883033003/diff/310001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/RemoteFrame.h (right):

https://codereview.chromium.org/2883033003/diff/310001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/RemoteFrame.h:68: bool last_inert_state_;
On 2017/06/16 02:17:58, alexmos wrote:
> Since both local and remote frames need this bool, would it make sense to just
> keep it in Frame?

Done.

https://codereview.chromium.org/2883033003/diff/310001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLDialogElement.cpp (right):

https://codereview.chromium.org/2883033003/diff/310001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLDialogElement.cpp:89:
document.GetFrame()->SetIsInert(document.LocalOwner() &&
On 2017/06/16 02:17:58, alexmos wrote:
> How does this work when showModal is called in a main frame where owner is
null?
>  Won't we incorrectly pass the bit as false then, and set is_inert_ as false
on
> the main LocalFrame?  (Looks like the remote frames would still propagate the
> right value, as they consult is_inert_ ||
> ToHTMLFrameOwnerElement(child->Owner())->IsInert() and the latter would be
> true.)

For the main frame, is_inert_ is correctly always false, since that strictly
represents whether the owner element is inert in a parent frame. I have added a
comment to Frame.h to make that meaning clearer.

[alexmos, 2017-06-20 18:46:10]

Thanks Ken for all the explanations!  One more question about a potentially
problematic case below.

https://codereview.chromium.org/2883033003/diff/310001/content/browser/frame_...
File content/browser/frame_host/render_widget_host_view_child_frame.cc (right):

https://codereview.chromium.org/2883033003/diff/310001/content/browser/frame_...
content/browser/frame_host/render_widget_host_view_child_frame.cc:388:
host_->Send(new ViewMsg_SetIsInert(host_->GetRoutingID(), inert));
On 2017/06/19 19:26:22, kenrb wrote:
> On 2017/06/16 02:17:57, alexmos wrote:
> > To me it seemed a little confusing to have the inert state come in with a
> frame
> > message, but then forward it with a view message, especially given that the
> > inert state is stored per-frame.  Wouldn't it work to just handle
> > FrameHostMsg_SetIsInert in RenderFrameProxyHost and send it to
> > frame_tree_node_->current_frame_host()? 
> 
> Originally I did it this way to be consistent with how parent-child state
> updates are generally done, which lets us keep handlers on RenderWidget for
> messages that only go to local frame roots.
> 
> More concretely, your point elsewhere about frame navigations is potentially a
> problem, and suggests that this is better as a ViewMsg. If the child frame
does
> a cross-process navigation, the inert bit needs to be re-sent, along with
other
> widget-level state. It would be difficult to do that with a FrameMsg, rather
> than keeping the state in CPFC and sending it with other state from the
> RenderWidgetHostView. The current patchset incorporates the latter approach.

Would there still be a problem if we have a(b(b)), and the bottom child
navigates to c?  The IPC would only be sent to the middle child and cached on
its CPFC, and then when the bottom child navigates, how will it pull it in?

Given that we do need to cache this in the browser process, I'm also thinking
why we couldn't use FrameReplicationState, which also stores the replicated bits
on FTN so we can send them in cases like this.  I guess the differences here are
(1) this is about input events, which, as you note, as generally handled on a
per-widget level, and (2) proxies don't need to know inertness -- right?

https://codereview.chromium.org/2883033003/diff/310001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/Frame.cpp (right):

https://codereview.chromium.org/2883033003/diff/310001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/Frame.cpp:201: void
Frame::UpdateInertIfPossible() {
On 2017/06/19 19:26:23, kenrb wrote:
> On 2017/06/16 02:17:57, alexmos wrote:
> > Is the "IfPossible" conveying that we only need to do this for subframes? 
> > Perhaps UpdateInertIfNeeded or UpdateInertIfNecessary to be more consistent
> with
> > similar cases?
> 
> Not exactly, the 'IfPossible' is because the Frame might not be connected to
its
> owner yet, in which case the check will be done later.

Acknowledged.

https://codereview.chromium.org/2883033003/diff/310001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/2883033003/diff/310001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/LocalFrame.cpp:596:
PropagateInertToChildFrames();
On 2017/06/19 19:26:23, kenrb wrote:
> On 2017/06/16 02:17:57, alexmos wrote:
> > Should this also only be done if |is_inert_| was different from |inert|?
> 
> No, because sometimes the propagation to child frames is needed even when the
> current frame's bit doesn't change; i.e. because an element in the current
frame
> changed. In a previous iteration I had checked the child's bit from
> PropagateInertToChildFrame() and only propagated if it would change, but
removed
> it because it required an extra method on Frame.h and it seemed okay to be
> slightly conservative.

Acknowledged.

https://codereview.chromium.org/2883033003/diff/310001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/LocalFrame.cpp:601: child =
child->Tree().NextSibling()) {
On 2017/06/19 19:26:23, kenrb wrote:
> On 2017/06/16 02:17:57, alexmos wrote:
> > How does this walk work for nested RemoteFrames?  I.e., for a(b(c)), seems
> like
> > we'll only call SetIsInert on b but not on c.  Or do you rely on b.com
> > LocalFrame sending the message once it receives it?  If the latter, could we
> > just walk the whole hierarchy here and send the message to all remote frame
> > descendants, to avoid extra roundtrips?
> 
> From within Blink, we don't know if nested OOPIFs exist. If (c) is
same-process
> under (b), then the update will happen properly within the child frame's
> renderer. If (c) is remote from (b), then there will be an extra round trip,
as
> you note. If we want to optimize that then it would have to be within the
> browser process, but would add complexity (because (b) would need to know to
> *not* update (c)) and I'm not sure the trade-off is worth it.

I was thinking that if we just send the IPC to all remote frames, and each
RenderFrameProxyHost forwards it to whatever process is hosting that actual
frame, then we don't really need any extra knowledge, right?  I.e., for a(b(c)),
we could send the IPC here to both (b) and (c) proxies, which would forward them
to b.com and c.com frames.  For a case like a(b(b(b))), we'd send three IPCs
instead of one with your approach, so it's a tradeoff.  I'm fine with your
approach though - I wouldn't expect inertness to be used often enough for us to
care about # of IPCs or roundtrips involved.

https://codereview.chromium.org/2883033003/diff/310001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/LocalFrame.cpp:604:
ToHTMLFrameOwnerElement(child->Owner())->IsInert());
On 2017/06/19 19:26:23, kenrb wrote:
> On 2017/06/16 02:17:57, alexmos wrote:
> > When would is_inert_ be different from
> > ToHTMLFrameOwnerElement(child->Owner())->IsInert()?
> 
> If child->Owner()->IsInert() returns true, it means there is an element in
this
> Frame that is causing other elements to become inert, including the child
Frame
> element. is_inert_ being true means that we are inheriting inertness from a
> parent frame's context.

Thanks for clearing up my confusion. :)  It might be helpful to add a short
comment along those lines.

https://codereview.chromium.org/2883033003/diff/310001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/LocalFrame.h (right):

https://codereview.chromium.org/2883033003/diff/310001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/LocalFrame.h:258: bool IsInert() const {
return is_inert_; }
On 2017/06/19 19:26:23, kenrb wrote:
> On 2017/06/16 02:17:58, alexmos wrote:
> > Is this used anywhere?  I might've missed it, but I only see IsInert called
on
> > HTMLFrameOwnerElements.
> 
> Yes, in Node::IsInert().

Acknowledged.

https://codereview.chromium.org/2883033003/diff/310001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLDialogElement.cpp (right):

https://codereview.chromium.org/2883033003/diff/310001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLDialogElement.cpp:89:
document.GetFrame()->SetIsInert(document.LocalOwner() &&
On 2017/06/19 19:26:23, kenrb wrote:
> On 2017/06/16 02:17:58, alexmos wrote:
> > How does this work when showModal is called in a main frame where owner is
> null?
> >  Won't we incorrectly pass the bit as false then, and set is_inert_ as false
> on
> > the main LocalFrame?  (Looks like the remote frames would still propagate
the
> > right value, as they consult is_inert_ ||
> > ToHTMLFrameOwnerElement(child->Owner())->IsInert() and the latter would be
> > true.)
> 
> For the main frame, is_inert_ is correctly always false, since that strictly
> represents whether the owner element is inert in a parent frame. I have added
a
> comment to Frame.h to make that meaning clearer.

Ah, I see - I missed this distinction that this means that the frame's owner is
inert in the parent.

https://codereview.chromium.org/2883033003/diff/330001/content/browser/frame_...
File content/browser/frame_host/render_widget_host_view_child_frame.cc (right):

https://codereview.chromium.org/2883033003/diff/330001/content/browser/frame_...
content/browser/frame_host/render_widget_host_view_child_frame.cc:632:
SetIsInert();
Assuming that this handles the new process case, it seems like this will send
the inert IPC more often than necessary (every time there's a screen rects
update) - or maybe there's another reason why that's needed here and not
somewhere like Init()?  See also my earlier question about whether this will
work for cases like a(b(b)) and the bottom child navigating to a new process.

https://codereview.chromium.org/2883033003/diff/330001/content/browser/site_p...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/2883033003/diff/330001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:10317: EXPECT_EQ("",
focused_element);
For completeness, and since it should be easy, could we also add coverage for
the remote-to-local case? E.g., navigate the frame back to b.com and re-check
that inertness was kept after the remote-to-local swap, and also start from
a(b,b) instead of a(b) so that b.com won't go away after the c.com navigation.

https://codereview.chromium.org/2883033003/diff/330001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/Frame.h (right):

https://codereview.chromium.org/2883033003/diff/330001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/Frame.h:177: // parent frame's document
becomes inert.
nit: Can we also explicitly say that it is always false for main frames?

[kenrb, 2017-06-22 15:33:35]

Thanks for all the good comments. New PS uploaded.

https://codereview.chromium.org/2883033003/diff/310001/content/browser/frame_...
File content/browser/frame_host/render_widget_host_view_child_frame.cc (right):

https://codereview.chromium.org/2883033003/diff/310001/content/browser/frame_...
content/browser/frame_host/render_widget_host_view_child_frame.cc:388:
host_->Send(new ViewMsg_SetIsInert(host_->GetRoutingID(), inert));
On 2017/06/20 18:46:09, alexmos wrote:
> On 2017/06/19 19:26:22, kenrb wrote:
> > On 2017/06/16 02:17:57, alexmos wrote:
> > > To me it seemed a little confusing to have the inert state come in with a
> > frame
> > > message, but then forward it with a view message, especially given that
the
> > > inert state is stored per-frame.  Wouldn't it work to just handle
> > > FrameHostMsg_SetIsInert in RenderFrameProxyHost and send it to
> > > frame_tree_node_->current_frame_host()? 
> > 
> > Originally I did it this way to be consistent with how parent-child state
> > updates are generally done, which lets us keep handlers on RenderWidget for
> > messages that only go to local frame roots.
> > 
> > More concretely, your point elsewhere about frame navigations is potentially
a
> > problem, and suggests that this is better as a ViewMsg. If the child frame
> does
> > a cross-process navigation, the inert bit needs to be re-sent, along with
> other
> > widget-level state. It would be difficult to do that with a FrameMsg, rather
> > than keeping the state in CPFC and sending it with other state from the
> > RenderWidgetHostView. The current patchset incorporates the latter approach.
> 
> Would there still be a problem if we have a(b(b)), and the bottom child
> navigates to c?  The IPC would only be sent to the middle child and cached on
> its CPFC, and then when the bottom child navigates, how will it pull it in?
> 

When the bottom child swaps, the newly created RemoteFrame sends the inert state
to the renderer.

> Given that we do need to cache this in the browser process, I'm also thinking
> why we couldn't use FrameReplicationState, which also stores the replicated
bits
> on FTN so we can send them in cases like this.  I guess the differences here
are
> (1) this is about input events, which, as you note, as generally handled on a
> per-widget level, and (2) proxies don't need to know inertness -- right?

When I started working on this I had considered FrameReplicationState, but
thought this was a better approach for the reasons you mention. In general it
seems like the parent-child state message should be in this path, whereas
generic state replication should use FrameReplicationState.

https://codereview.chromium.org/2883033003/diff/310001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/2883033003/diff/310001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/LocalFrame.cpp:601: child =
child->Tree().NextSibling()) {
On 2017/06/20 18:46:10, alexmos wrote:
> On 2017/06/19 19:26:23, kenrb wrote:
> > On 2017/06/16 02:17:57, alexmos wrote:
> > > How does this walk work for nested RemoteFrames?  I.e., for a(b(c)), seems
> > like
> > > we'll only call SetIsInert on b but not on c.  Or do you rely on b.com
> > > LocalFrame sending the message once it receives it?  If the latter, could
we
> > > just walk the whole hierarchy here and send the message to all remote
frame
> > > descendants, to avoid extra roundtrips?
> > 
> > From within Blink, we don't know if nested OOPIFs exist. If (c) is
> same-process
> > under (b), then the update will happen properly within the child frame's
> > renderer. If (c) is remote from (b), then there will be an extra round trip,
> as
> > you note. If we want to optimize that then it would have to be within the
> > browser process, but would add complexity (because (b) would need to know to
> > *not* update (c)) and I'm not sure the trade-off is worth it.
> 
> I was thinking that if we just send the IPC to all remote frames, and each
> RenderFrameProxyHost forwards it to whatever process is hosting that actual
> frame, then we don't really need any extra knowledge, right?  I.e., for
a(b(c)),
> we could send the IPC here to both (b) and (c) proxies, which would forward
them
> to b.com and c.com frames.  For a case like a(b(b(b))), we'd send three IPCs
> instead of one with your approach, so it's a tradeoff.  I'm fine with your
> approach though - I wouldn't expect inertness to be used often enough for us
to
> care about # of IPCs or roundtrips involved.

There is another problem here. Think about a(b(c)) where a and b frames both
contain elements with the inert attribute. If a's element removes the inert
attribute then it sends messages to b and c to stop being inert. But b still
wants c to be inert, so it now has to send a message to c to become inert again,
which is racing with a's message. This is complicated to resolve.

https://codereview.chromium.org/2883033003/diff/310001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/LocalFrame.cpp:604:
ToHTMLFrameOwnerElement(child->Owner())->IsInert());
On 2017/06/20 18:46:10, alexmos wrote:
> On 2017/06/19 19:26:23, kenrb wrote:
> > On 2017/06/16 02:17:57, alexmos wrote:
> > > When would is_inert_ be different from
> > > ToHTMLFrameOwnerElement(child->Owner())->IsInert()?
> > 
> > If child->Owner()->IsInert() returns true, it means there is an element in
> this
> > Frame that is causing other elements to become inert, including the child
> Frame
> > element. is_inert_ being true means that we are inheriting inertness from a
> > parent frame's context.
> 
> Thanks for clearing up my confusion. :)  It might be helpful to add a short
> comment along those lines.

Done.

https://codereview.chromium.org/2883033003/diff/330001/content/browser/frame_...
File content/browser/frame_host/render_widget_host_view_child_frame.cc (right):

https://codereview.chromium.org/2883033003/diff/330001/content/browser/frame_...
content/browser/frame_host/render_widget_host_view_child_frame.cc:632:
SetIsInert();
On 2017/06/20 18:46:10, alexmos wrote:
> Assuming that this handles the new process case, it seems like this will send
> the inert IPC more often than necessary (every time there's a screen rects
> update) - or maybe there's another reason why that's needed here and not
> somewhere like Init()?  See also my earlier question about whether this will
> work for cases like a(b(b)) and the bottom child navigating to a new process.

I agree this doesn't feel like the greatest place, but I don't see a better one
for either SetIsInert or UpdateViewportIntersection, which need to trigger at a
certain point after cross-process navigations (this is covering a(b) -> a(c)
navigation scenario). RWHVCF::Init() is too early in terms of renderer state.
This will send the IPC unnecessarily, sometimes, but it doesn't get called very
often so it shouldn't be too bad. There are definitely some improvements that
can be made to RWHVCF initialization, so this might be re-thought when we get
around to dealing with subframe vs top-level frame resizing (bug 618353).

As mentioned in the other response, a(b(b)) -> a(b(c)) is covered by Blink
initializing the RemoteFrame.

https://codereview.chromium.org/2883033003/diff/330001/content/browser/site_p...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/2883033003/diff/330001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:10317: EXPECT_EQ("",
focused_element);
On 2017/06/20 18:46:10, alexmos wrote:
> For completeness, and since it should be easy, could we also add coverage for
> the remote-to-local case? E.g., navigate the frame back to b.com and re-check
> that inertness was kept after the remote-to-local swap, and also start from
> a(b,b) instead of a(b) so that b.com won't go away after the c.com navigation.

Done.

https://codereview.chromium.org/2883033003/diff/330001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/Frame.h (right):

https://codereview.chromium.org/2883033003/diff/330001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/Frame.h:177: // parent frame's document
becomes inert.
On 2017/06/20 18:46:10, alexmos wrote:
> nit: Can we also explicitly say that it is always false for main frames?

Done.

[kenrb, 2017-06-22 15:34:02]

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

[commit-bot: I haz the power, 2017-06-22 15:34:10]

Dry run: CQ is trying da patch.

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

[alexmos, 2017-06-22 17:09:28]

Thanks, LGTM!

https://codereview.chromium.org/2883033003/diff/310001/content/browser/frame_...
File content/browser/frame_host/render_widget_host_view_child_frame.cc (right):

https://codereview.chromium.org/2883033003/diff/310001/content/browser/frame_...
content/browser/frame_host/render_widget_host_view_child_frame.cc:388:
host_->Send(new ViewMsg_SetIsInert(host_->GetRoutingID(), inert));
On 2017/06/22 15:33:35, kenrb wrote:
> On 2017/06/20 18:46:09, alexmos wrote:
> > On 2017/06/19 19:26:22, kenrb wrote:
> > > On 2017/06/16 02:17:57, alexmos wrote:
> > > > To me it seemed a little confusing to have the inert state come in with
a
> > > frame
> > > > message, but then forward it with a view message, especially given that
> the
> > > > inert state is stored per-frame.  Wouldn't it work to just handle
> > > > FrameHostMsg_SetIsInert in RenderFrameProxyHost and send it to
> > > > frame_tree_node_->current_frame_host()? 
> > > 
> > > Originally I did it this way to be consistent with how parent-child state
> > > updates are generally done, which lets us keep handlers on RenderWidget
for
> > > messages that only go to local frame roots.
> > > 
> > > More concretely, your point elsewhere about frame navigations is
potentially
> a
> > > problem, and suggests that this is better as a ViewMsg. If the child frame
> > does
> > > a cross-process navigation, the inert bit needs to be re-sent, along with
> > other
> > > widget-level state. It would be difficult to do that with a FrameMsg,
rather
> > > than keeping the state in CPFC and sending it with other state from the
> > > RenderWidgetHostView. The current patchset incorporates the latter
approach.
> > 
> > Would there still be a problem if we have a(b(b)), and the bottom child
> > navigates to c?  The IPC would only be sent to the middle child and cached
on
> > its CPFC, and then when the bottom child navigates, how will it pull it in?
> > 
> 
> When the bottom child swaps, the newly created RemoteFrame sends the inert
state
> to the renderer.
> 
> > Given that we do need to cache this in the browser process, I'm also
thinking
> > why we couldn't use FrameReplicationState, which also stores the replicated
> bits
> > on FTN so we can send them in cases like this.  I guess the differences here
> are
> > (1) this is about input events, which, as you note, as generally handled on
a
> > per-widget level, and (2) proxies don't need to know inertness -- right?
> 
> When I started working on this I had considered FrameReplicationState, but
> thought this was a better approach for the reasons you mention. In general it
> seems like the parent-child state message should be in this path, whereas
> generic state replication should use FrameReplicationState.

Acknowledged.

https://codereview.chromium.org/2883033003/diff/310001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/2883033003/diff/310001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/LocalFrame.cpp:601: child =
child->Tree().NextSibling()) {
On 2017/06/22 15:33:35, kenrb wrote:
> On 2017/06/20 18:46:10, alexmos wrote:
> > On 2017/06/19 19:26:23, kenrb wrote:
> > > On 2017/06/16 02:17:57, alexmos wrote:
> > > > How does this walk work for nested RemoteFrames?  I.e., for a(b(c)),
seems
> > > like
> > > > we'll only call SetIsInert on b but not on c.  Or do you rely on b.com
> > > > LocalFrame sending the message once it receives it?  If the latter,
could
> we
> > > > just walk the whole hierarchy here and send the message to all remote
> frame
> > > > descendants, to avoid extra roundtrips?
> > > 
> > > From within Blink, we don't know if nested OOPIFs exist. If (c) is
> > same-process
> > > under (b), then the update will happen properly within the child frame's
> > > renderer. If (c) is remote from (b), then there will be an extra round
trip,
> > as
> > > you note. If we want to optimize that then it would have to be within the
> > > browser process, but would add complexity (because (b) would need to know
to
> > > *not* update (c)) and I'm not sure the trade-off is worth it.
> > 
> > I was thinking that if we just send the IPC to all remote frames, and each
> > RenderFrameProxyHost forwards it to whatever process is hosting that actual
> > frame, then we don't really need any extra knowledge, right?  I.e., for
> a(b(c)),
> > we could send the IPC here to both (b) and (c) proxies, which would forward
> them
> > to b.com and c.com frames.  For a case like a(b(b(b))), we'd send three IPCs
> > instead of one with your approach, so it's a tradeoff.  I'm fine with your
> > approach though - I wouldn't expect inertness to be used often enough for us
> to
> > care about # of IPCs or roundtrips involved.
> 
> There is another problem here. Think about a(b(c)) where a and b frames both
> contain elements with the inert attribute. If a's element removes the inert
> attribute then it sends messages to b and c to stop being inert. But b still
> wants c to be inert, so it now has to send a message to c to become inert
again,
> which is racing with a's message. This is complicated to resolve.

Acknowledged - I agree that's a good reason to keep your current approach.

https://codereview.chromium.org/2883033003/diff/330001/content/browser/frame_...
File content/browser/frame_host/render_widget_host_view_child_frame.cc (right):

https://codereview.chromium.org/2883033003/diff/330001/content/browser/frame_...
content/browser/frame_host/render_widget_host_view_child_frame.cc:632:
SetIsInert();
On 2017/06/22 15:33:35, kenrb wrote:
> On 2017/06/20 18:46:10, alexmos wrote:
> > Assuming that this handles the new process case, it seems like this will
send
> > the inert IPC more often than necessary (every time there's a screen rects
> > update) - or maybe there's another reason why that's needed here and not
> > somewhere like Init()?  See also my earlier question about whether this will
> > work for cases like a(b(b)) and the bottom child navigating to a new
process.
> 
> I agree this doesn't feel like the greatest place, but I don't see a better
one
> for either SetIsInert or UpdateViewportIntersection, which need to trigger at
a
> certain point after cross-process navigations (this is covering a(b) -> a(c)
> navigation scenario). RWHVCF::Init() is too early in terms of renderer state.
> This will send the IPC unnecessarily, sometimes, but it doesn't get called
very
> often so it shouldn't be too bad. There are definitely some improvements that
> can be made to RWHVCF initialization, so this might be re-thought when we get
> around to dealing with subframe vs top-level frame resizing (bug 618353).
> 
> As mentioned in the other response, a(b(b)) -> a(b(c)) is covered by Blink
> initializing the RemoteFrame.

OK.  Perhaps add a TODO and describe some of those concerns here?

[commit-bot: I haz the power, 2017-06-22 17:38:58]

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

[commit-bot: I haz the power, 2017-06-22 17:38:59]

Dry run: This issue passed the CQ dry run.

[kenrb, 2017-06-22 20:31:14]

The CQ bit was checked by kenrb@chromium.org

[kenrb, 2017-06-22 20:31:15]

The patchset sent to the CQ was uploaded after l-g-t-m from
dmazzoni@chromium.org, tkent@chromium.org, alexmos@chromium.org
Link to the patchset: https://codereview.chromium.org/2883033003/#ps390001
(title: "nit addressed")

[commit-bot: I haz the power, 2017-06-22 20:31:56]

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-06-23 01:23:35]

CQ is committing da patch.

Bot data: {"patchset_id": 390001, "attempt_start_ts": 1498163474356110,
"parent_rev": "28bcb85f5c7f1e56f57a0b26f9a878196adedb3f", "commit_rev":
"04323789a189306e523b5982a9cd49dcebdc88f5"}

[commit-bot: I haz the power, 2017-06-23 01:23:53]

Description was changed from

==========
Propagate inert state to OOPIFs when a modal dialog is active

When showModal is called on a <dialog> element, the rest of the
Document becomes inert which prevents it from receiving events or taking
focus. When the Document contains an out-of-process iframe, however, its
contents are not aware of the modal dialog in a remote ancestor.

This CL caches the current inert state on each LocalFrame, which is
changed when a modal dialog because active or inactive. The bit is
plumbed to remote frame children so that OOPIFs will respect inertness.

Also it prevents having to search up the entire frame tree when an
element checks whether it is inert.

BUG=719788
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Propagate inert state to OOPIFs when a modal dialog is active

When showModal is called on a <dialog> element, the rest of the
Document becomes inert which prevents it from receiving events or taking
focus. When the Document contains an out-of-process iframe, however, its
contents are not aware of the modal dialog in a remote ancestor.

This CL caches the current inert state on each LocalFrame, which is
changed when a modal dialog because active or inactive. The bit is
plumbed to remote frame children so that OOPIFs will respect inertness.

Also it prevents having to search up the entire frame tree when an
element checks whether it is inert.

BUG=719788
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2883033003
Cr-Commit-Position: refs/heads/master@{#481761}
Committed:
https://chromium.googlesource.com/chromium/src/+/04323789a189306e523b5982a9cd...
==========

[commit-bot: I haz the power, 2017-06-23 01:23:55]

Committed patchset #21 (id:390001) as
https://chromium.googlesource.com/chromium/src/+/04323789a189306e523b5982a9cd...