Remove deprecated navigation callbacks on WebContentsObserver that are now unused.

Remove deprecated navigation callbacks on WebContentsObserver that are now unused.

BUG=682002
Review-Url: https://codereview.chromium.org/2684143002
Cr-Commit-Position: refs/heads/master@{#449974}
Committed: https://chromium.googlesource.com/chromium/src/+/5f358e52c9000b100f9b4e3964dd044425ad9b8c

[jam, 2017-02-08 23:52:04]

Description was changed from

==========
Remove deprecated navigation callbacks on WebContentsObserver that are now
unused.

BUG=682002
==========

to

==========
Remove deprecated navigation callbacks on WebContentsObserver that are now
unused.

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

[jam, 2017-02-10 08:11:21]

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

[jam, 2017-02-10 08:11:32]

Description was changed from

==========
Remove deprecated navigation callbacks on WebContentsObserver that are now
unused.

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

to

==========
Remove deprecated navigation callbacks on WebContentsObserver that are now
unused.

BUG=682002
==========

[commit-bot: I haz the power, 2017-02-10 08:11:44]

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 08:21:09]

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

[commit-bot: I haz the power, 2017-02-10 08:21:13]

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

[jam, 2017-02-10 16:00:38]

Description was changed from

==========
Remove deprecated navigation callbacks on WebContentsObserver that are now
unused.

BUG=682002
==========

to

==========
Remove deprecated navigation callbacks on WebContentsObserver that are now
unused.

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

[jam, 2017-02-10 16:01:35]

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

[jam, 2017-02-10 16:01:46]

Patchset #1 (id:1) has been deleted

[jam, 2017-02-10 16:01:56]

Description was changed from

==========
Remove deprecated navigation callbacks on WebContentsObserver that are now
unused.

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

to

==========
Remove deprecated navigation callbacks on WebContentsObserver that are now
unused.

BUG=682002
==========

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

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

[jam, 2017-02-10 16:12:26]

Description was changed from

==========
Remove deprecated navigation callbacks on WebContentsObserver that are now
unused.

BUG=682002
==========

to

==========
Remove deprecated navigation callbacks on WebContentsObserver that are now
unused.

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

[jam, 2017-02-10 16:23:01]

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

[commit-bot: I haz the power, 2017-02-10 16:23:30]

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

[jam, 2017-02-10 16:23:45]

Patchset #2 (id:40001) has been deleted

[jam, 2017-02-10 16:23:51]

Patchset #1 (id:20001) has been deleted

[jam, 2017-02-10 16:24:03]

Description was changed from

==========
Remove deprecated navigation callbacks on WebContentsObserver that are now
unused.

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

to

==========
Remove deprecated navigation callbacks on WebContentsObserver that are now
unused.

BUG=682002
==========

[commit-bot: I haz the power, 2017-02-10 16:43:08]

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

[commit-bot: I haz the power, 2017-02-10 16:43:09]

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

[jam, 2017-02-10 17:33:24]

Description was changed from

==========
Remove deprecated navigation callbacks on WebContentsObserver that are now
unused.

BUG=682002
==========

to

==========
Remove deprecated navigation callbacks on WebContentsObserver that are now
unused.

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

[jam, 2017-02-10 17:43:55]

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

[jam, 2017-02-10 17:44:00]

Description was changed from

==========
Remove deprecated navigation callbacks on WebContentsObserver that are now
unused.

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

to

==========
Remove deprecated navigation callbacks on WebContentsObserver that are now
unused.

BUG=682002
==========

[commit-bot: I haz the power, 2017-02-10 17:44:31]

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

[jam, 2017-02-10 17:52:34]

Patchset #1 (id:60001) has been deleted

[jam, 2017-02-10 18:40:53]

jam@chromium.org changed reviewers:
+ nasko@chromium.org

[commit-bot: I haz the power, 2017-02-10 18:48:32]

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

[commit-bot: I haz the power, 2017-02-10 18:48:32]

Dry run: This issue passed the CQ dry run.

[nasko, 2017-02-10 19:44:58]

LGTM with a couple of notes.

https://codereview.chromium.org/2684143002/diff/80001/content/browser/frame_h...
File content/browser/frame_host/navigator_impl.cc (left):

https://codereview.chromium.org/2684143002/diff/80001/content/browser/frame_h...
content/browser/frame_host/navigator_impl.cc:709: params.transition |
ui::PAGE_TRANSITION_FORWARD_BACK);
Hmm, I wonder if we have a discrepancy between the transition type on the
NavigationHandle and this one. Do you think temporarily leaving this and having
a DCHECK that the types here and in the NavigationHandle match?

https://codereview.chromium.org/2684143002/diff/80001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/2684143002/diff/80001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:3312: // Notify accessibility
if this is a reload.
This used to be at navigation start. This makes more sense to me, however, it
will be useful to have dmazzoni@ or someone else from a11y look it over.

[jam, 2017-02-10 20:00:34]

https://codereview.chromium.org/2684143002/diff/80001/content/browser/frame_h...
File content/browser/frame_host/navigator_impl.cc (left):

https://codereview.chromium.org/2684143002/diff/80001/content/browser/frame_h...
content/browser/frame_host/navigator_impl.cc:709: params.transition |
ui::PAGE_TRANSITION_FORWARD_BACK);
On 2017/02/10 19:44:58, nasko wrote:
> Hmm, I wonder if we have a discrepancy between the transition type on the
> NavigationHandle and this one. Do you think temporarily leaving this and
having
> a DCHECK that the types here and in the NavigationHandle match?

Given that no tests failed, I'm not inclined to have code to check the old and
new code paths.

https://codereview.chromium.org/2684143002/diff/80001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/2684143002/diff/80001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:3312: // Notify accessibility
if this is a reload.
On 2017/02/10 19:44:58, nasko wrote:
> This used to be at navigation start. This makes more sense to me, however, it
> will be useful to have dmazzoni@ or someone else from a11y look it over.

I moved it because of the GetRenderFrameHost call, which isn't available
earlier.

I'll ping Dominic

[dmazzoni, 2017-02-10 20:47:44]

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

[dmazzoni, 2017-02-10 20:47:45]

https://codereview.chromium.org/2684143002/diff/80001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/2684143002/diff/80001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:3312: // Notify accessibility
if this is a reload.
BrowserAccessibilityManager::UserIsReloading() is supposed to be called on the
BAM associated with the OLD RFHI.

If that's still the case, we're good.

What it does it (1) notify AT that the reload is happening (a nice-to-have, but
not critical), and
(2) ignores further accessibility notifications on the OLD accessibility tree,
since it's no longer valid. This prevents race conditions where the new page has
started to load but we still have some accessibility IPC trickling in from the
old page.

This really needs some test coverage. Please assign something to me. If you can
find a good test in content_browsertests that covers reloading now that I could
build on / fork, that would be helpful.

[jam, 2017-02-10 20:52:40]

https://codereview.chromium.org/2684143002/diff/80001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/2684143002/diff/80001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:3312: // Notify accessibility
if this is a reload.
On 2017/02/10 20:47:45, dmazzoni wrote:
> BrowserAccessibilityManager::UserIsReloading() is supposed to be called on the
> BAM associated with the OLD RFHI.
> 
> If that's still the case, we're good.
> 
> What it does it (1) notify AT that the reload is happening (a nice-to-have,
but
> not critical), and
> (2) ignores further accessibility notifications on the OLD accessibility tree,
> since it's no longer valid. This prevents race conditions where the new page
has
> started to load but we still have some accessibility IPC trickling in from the
> old page.
> 
> This really needs some test coverage. Please assign something to me. If you
can
> find a good test in content_browsertests that covers reloading now that I
could
> build on / fork, that would be helpful.

With a reload, we wouldn't be changing the RFH so this should be fine.

Nasko:?

[nasko, 2017-02-10 21:28:39]

https://codereview.chromium.org/2684143002/diff/80001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/2684143002/diff/80001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:3312: // Notify accessibility
if this is a reload.
On 2017/02/10 20:52:40, jam wrote:
> On 2017/02/10 20:47:45, dmazzoni wrote:
> > BrowserAccessibilityManager::UserIsReloading() is supposed to be called on
the
> > BAM associated with the OLD RFHI.
> > 
> > If that's still the case, we're good.
> > 
> > What it does it (1) notify AT that the reload is happening (a nice-to-have,
> but
> > not critical), and
> > (2) ignores further accessibility notifications on the OLD accessibility
tree,
> > since it's no longer valid. This prevents race conditions where the new page
> has
> > started to load but we still have some accessibility IPC trickling in from
the
> > old page.
> > 
> > This really needs some test coverage. Please assign something to me. If you
> can
> > find a good test in content_browsertests that covers reloading now that I
> could
> > build on / fork, that would be helpful.
> 
> With a reload, we wouldn't be changing the RFH so this should be fine.
> 
> Nasko:?

This isn't quite correct, there are cases where this can happen. An example is
installing a hosted app for the page you are currently on. Upon reload, we will
change the RFH, as it is no longer regular one, but an extension one.

Dominic: why do we need to send a signal to the old RFH? It will get destroyed
shortly anyway.

[jam, 2017-02-10 22:32:30]

On 2017/02/10 21:28:39, nasko wrote:
>
https://codereview.chromium.org/2684143002/diff/80001/content/browser/web_con...
> File content/browser/web_contents/web_contents_impl.cc (right):
> 
>
https://codereview.chromium.org/2684143002/diff/80001/content/browser/web_con...
> content/browser/web_contents/web_contents_impl.cc:3312: // Notify
accessibility
> if this is a reload.
> On 2017/02/10 20:52:40, jam wrote:
> > On 2017/02/10 20:47:45, dmazzoni wrote:
> > > BrowserAccessibilityManager::UserIsReloading() is supposed to be called on
> the
> > > BAM associated with the OLD RFHI.
> > > 
> > > If that's still the case, we're good.
> > > 
> > > What it does it (1) notify AT that the reload is happening (a
nice-to-have,
> > but
> > > not critical), and
> > > (2) ignores further accessibility notifications on the OLD accessibility
> tree,
> > > since it's no longer valid. This prevents race conditions where the new
page
> > has
> > > started to load but we still have some accessibility IPC trickling in from
> the
> > > old page.
> > > 
> > > This really needs some test coverage. Please assign something to me. If
you
> > can
> > > find a good test in content_browsertests that covers reloading now that I
> > could
> > > build on / fork, that would be helpful.
> > 
> > With a reload, we wouldn't be changing the RFH so this should be fine.
> > 
> > Nasko:?
> 
> This isn't quite correct, there are cases where this can happen. An example is
> installing a hosted app for the page you are currently on. Upon reload, we
will
> change the RFH, as it is no longer regular one, but an extension one.

That seems like a degenerate case :)

> 
> Dominic: why do we need to send a signal to the old RFH? It will get destroyed
> shortly anyway.

[jam, 2017-02-13 15:18:52]

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

[commit-bot: I haz the power, 2017-02-13 15:19:17]

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

[nasko, 2017-02-13 15:22:10]

On 2017/02/10 22:32:30, jam wrote:
> On 2017/02/10 21:28:39, nasko wrote:
> >
>
https://codereview.chromium.org/2684143002/diff/80001/content/browser/web_con...
> > File content/browser/web_contents/web_contents_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/2684143002/diff/80001/content/browser/web_con...
> > content/browser/web_contents/web_contents_impl.cc:3312: // Notify
> accessibility
> > if this is a reload.
> > On 2017/02/10 20:52:40, jam wrote:
> > > On 2017/02/10 20:47:45, dmazzoni wrote:
> > > > BrowserAccessibilityManager::UserIsReloading() is supposed to be called
on
> > the
> > > > BAM associated with the OLD RFHI.
> > > > 
> > > > If that's still the case, we're good.
> > > > 
> > > > What it does it (1) notify AT that the reload is happening (a
> nice-to-have,
> > > but
> > > > not critical), and
> > > > (2) ignores further accessibility notifications on the OLD accessibility
> > tree,
> > > > since it's no longer valid. This prevents race conditions where the new
> page
> > > has
> > > > started to load but we still have some accessibility IPC trickling in
from
> > the
> > > > old page.
> > > > 
> > > > This really needs some test coverage. Please assign something to me. If
> you
> > > can
> > > > find a good test in content_browsertests that covers reloading now that
I
> > > could
> > > > build on / fork, that would be helpful.
> > > 
> > > With a reload, we wouldn't be changing the RFH so this should be fine.
> > > 
> > > Nasko:?
> > 
> > This isn't quite correct, there are cases where this can happen. An example
is
> > installing a hosted app for the page you are currently on. Upon reload, we
> will
> > change the RFH, as it is no longer regular one, but an extension one.
> 
> That seems like a degenerate case :)

Yes, I'd agree it is less common.

> > Dominic: why do we need to send a signal to the old RFH? It will get
destroyed
> > shortly anyway.

Let's follow up on this part. It is listed in a previous comment as a "nice to
have", so LGTM for this CL.

[jam1, 2017-02-13 15:40:24]

jam@google.com changed reviewers:
+ jam@google.com

[jam1, 2017-02-13 15:40:25]

https://codereview.chromium.org/2684143002/diff/80001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/2684143002/diff/80001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:3312: // Notify accessibility
if this is a reload.
ok on second thought, I've maintainted the old behavior (calling on old RFHI) to
decrease the chance of side effects of this cl.

I filed https://bugs.chromium.org/p/chromium/issues/detail?id=691583 to clarify
this behavior as well.

[jam, 2017-02-13 15:40:27]

Description was changed from

==========
Remove deprecated navigation callbacks on WebContentsObserver that are now
unused.

BUG=682002
==========

to

==========
Remove deprecated navigation callbacks on WebContentsObserver that are now
unused.

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

[jam1, 2017-02-13 15:40:37]

The CQ bit was unchecked by jam@google.com

[jam1, 2017-02-13 15:40:49]

The CQ bit was checked by jam@google.com

[commit-bot: I haz the power, 2017-02-13 15:41:04]

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

[jam, 2017-02-13 16:31:11]

Description was changed from

==========
Remove deprecated navigation callbacks on WebContentsObserver that are now
unused.

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

to

==========
Remove deprecated navigation callbacks on WebContentsObserver that are now
unused.

BUG=682002
==========

[jam, 2017-02-13 16:31:17]

The CQ bit was unchecked by jam@chromium.org

[jam, 2017-02-13 16:31:22]

The CQ bit was checked by jam@chromium.org

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

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-13 16:37:10]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1487003482082880,
"parent_rev": "9c834ea3d82ffee215657c59d3a57e0de31ca59a", "commit_rev":
"5f358e52c9000b100f9b4e3964dd044425ad9b8c"}

[commit-bot: I haz the power, 2017-02-13 16:37:45]

Description was changed from

==========
Remove deprecated navigation callbacks on WebContentsObserver that are now
unused.

BUG=682002
==========

to

==========
Remove deprecated navigation callbacks on WebContentsObserver that are now
unused.

BUG=682002

Review-Url: https://codereview.chromium.org/2684143002
Cr-Commit-Position: refs/heads/master@{#449974}
Committed:
https://chromium.googlesource.com/chromium/src/+/5f358e52c9000b100f9b4e3964dd...
==========

[commit-bot: I haz the power, 2017-02-13 16:37:46]

Committed patchset #2 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/5f358e52c9000b100f9b4e3964dd...

[dmazzoni, 2017-02-13 19:05:46]

https://codereview.chromium.org/2684143002/diff/80001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/2684143002/diff/80001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:3312: // Notify accessibility
if this is a reload.
On 2017/02/10 21:28:39, nasko wrote:
> On 2017/02/10 20:52:40, jam wrote:
> > On 2017/02/10 20:47:45, dmazzoni wrote:
> > > BrowserAccessibilityManager::UserIsReloading() is supposed to be called on
> the
> > > BAM associated with the OLD RFHI.
> > > 
> > > If that's still the case, we're good.
> > > 
> > > What it does it (1) notify AT that the reload is happening (a
nice-to-have,
> > but
> > > not critical), and
> > > (2) ignores further accessibility notifications on the OLD accessibility
> tree,
> > > since it's no longer valid. This prevents race conditions where the new
page
> > has
> > > started to load but we still have some accessibility IPC trickling in from
> the
> > > old page.
> > > 
> > > This really needs some test coverage. Please assign something to me. If
you
> > can
> > > find a good test in content_browsertests that covers reloading now that I
> > could
> > > build on / fork, that would be helpful.
> > 
> > With a reload, we wouldn't be changing the RFH so this should be fine.
> > 
> > Nasko:?
> 
> This isn't quite correct, there are cases where this can happen. An example is
> installing a hosted app for the page you are currently on. Upon reload, we
will
> change the RFH, as it is no longer regular one, but an extension one.
> 
> Dominic: why do we need to send a signal to the old RFH? It will get destroyed
> shortly anyway.

Digging in some more, the call to UserIsNavigatingAway() is actually sufficient.
The issue is that there's a delay between when the new page starts loading and
when it becomes the default RFH for this frame node. During that time window,
we get accessibility events for both frames, so we want to ignore accessibility
events on the old frame as soon as we know it's going to be destroyed.

I'll add some test coverage and follow up if I have more questions.