Convert ContentFaviconDriver to use the new navigation callbacks.

Convert ContentFaviconDriver to use the new navigation callbacks.

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

Review-Url: https://codereview.chromium.org/2638423008
Cr-Commit-Position: refs/heads/master@{#445514}
Committed: https://chromium.googlesource.com/chromium/src/+/2ec369bd8d2542a0e163c2b5efe647a54ef69ae6

[jam, 2017-01-21 07:16:40]

Description was changed from

==========
Convert ContentFaviconDriver to use the new navigation callbacks.

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

to

==========
Convert ContentFaviconDriver to use the new navigation callbacks.

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

[jam, 2017-01-21 07:17:10]

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

[commit-bot: I haz the power, 2017-01-21 07:17:19]

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

[commit-bot: I haz the power, 2017-01-21 07:57:42]

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

[commit-bot: I haz the power, 2017-01-21 07:57:43]

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

[jam, 2017-01-21 08:58:44]

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

[commit-bot: I haz the power, 2017-01-21 08:58:54]

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

[commit-bot: I haz the power, 2017-01-21 10:26:10]

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

[commit-bot: I haz the power, 2017-01-21 10:26:11]

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

[jam, 2017-01-21 22:48:26]

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

[commit-bot: I haz the power, 2017-01-21 22:48:33]

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

[commit-bot: I haz the power, 2017-01-22 00:40:35]

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

[commit-bot: I haz the power, 2017-01-22 00:40:36]

Dry run: This issue passed the CQ dry run.

[jam, 2017-01-23 03:32:11]

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

[clamy, 2017-01-23 13:02:21]

Description was changed from

==========
Convert ContentFaviconDriver to use the new navigation callbacks.

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

to

==========
Convert ContentFaviconDriver to use the new navigation callbacks.

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

[clamy, 2017-01-23 13:02:21]

clamy@chromium.org changed reviewers:
+ creis@chromium.org

[clamy, 2017-01-23 13:02:44]

+creis to check for correctness of the reload type setting.

[jam, 2017-01-23 15:50:26]

On 2017/01/23 13:02:44, clamy wrote:
> +creis to check for correctness of the reload type setting.

Is there a specific part you thought might have issues? It seemed
straightforward since in all the code paths that create a NavigationHandle and
the navigation entry ID is passed in, it's either an existing navigation entry
(i.e. GetEntryWithUniqueID retrieves it) or it's a pending one
(GetPendingEntry).

[clamy, 2017-01-23 15:55:46]

I think it is correct, but I'd prefer creis double check. I'm not fully familiar
with the lifetime of the NavigationEntry, so I want to ensure that at the time
we set up the NavigationHandle the reload type has not changed compared to the
start of the navigation.

[jam, 2017-01-23 16:49:12]

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

[commit-bot: I haz the power, 2017-01-23 16:49:29]

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

[commit-bot: I haz the power, 2017-01-23 18:02:56]

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

[commit-bot: I haz the power, 2017-01-23 18:02:57]

Dry run: This issue passed the CQ dry run.

[Charlie Reis, 2017-01-23 18:30:07]

creis@chromium.org changed reviewers:
+ pkotwicz@chromium.org, sdefresne@chromium.org

[Charlie Reis, 2017-01-23 18:30:08]

[+sdefresne, pkotwicz for favicon question]

This is probably ok, though I have some small concerns about it below.  What do
you think?

https://codereview.chromium.org/2638423008/diff/30001/components/favicon/cont...
File components/favicon/content/content_favicon_driver.cc (right):

https://codereview.chromium.org/2638423008/diff/30001/components/favicon/cont...
components/favicon/content/content_favicon_driver.cc:166: void
ContentFaviconDriver::DidStartNavigation(
Note: This will get called more often than before, such as for
renderer-initiated navigations (e.g., link clicks, script navigations, etc).

sdefresne@ or pkotwicz@, can you confirm that's ok for the
SetFaviconOutOfDateForPage logic?  I would expect so, but if not, we can do an
early return if !navigation_handle->IsRendererInitiated().

https://codereview.chromium.org/2638423008/diff/30001/content/browser/frame_h...
File content/browser/frame_host/navigation_handle_impl.cc (right):

https://codereview.chromium.org/2638423008/diff/30001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl.cc:114:
NavigationControllerImpl* nav_controller =
Hmm.  This looks like it will be correct if we find the entry (since it's based
on pending_nav_entry_id_) but missing in some cases.  For example, I'm guessing
we won't have a pending_nav_entry_id_ for renderer-initiated reloads
(location.reload()).  That may be ok for the favicon case, but it's kind of
subtle for a public API.

I think it's also possible in some cases for the pending entry to be gone by the
time of DidStartProvisionalLoad when the handle is constructed.  That's more of
a uncommon race between navigations, I think, and we wouldn't be able to recover
the reload type anyway if it happened.

In general, I was kind of hoping to avoid a dependency on NavigationController
in NavigationHandle, but I suppose it's not a big problem.  On one hand, I was
toying with the idea of getting rid of pending NavigationEntries entirely if
NavigationHandle could be sufficient, but on the other hand, we already have
pending_nav_entry_id_ here to solve another problem.

I guess I'm ok with it, but let's mention some of the downsides (e.g., the
renderer-initiated reload case) in a comment.

https://codereview.chromium.org/2638423008/diff/30001/content/public/browser/...
File content/public/browser/navigation_handle.h (right):

https://codereview.chromium.org/2638423008/diff/30001/content/public/browser/...
content/public/browser/navigation_handle.h:102: virtual ReloadType
GetReloadType() = 0;
Let's add a comment mentioning this is only present for browser-initiated
navigations (e.g., not location.reload()) if we keep this.

[jam, 2017-01-23 19:03:39]

https://codereview.chromium.org/2638423008/diff/30001/content/browser/frame_h...
File content/browser/frame_host/navigation_handle_impl.cc (right):

https://codereview.chromium.org/2638423008/diff/30001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl.cc:114:
NavigationControllerImpl* nav_controller =
On 2017/01/23 18:30:08, Charlie Reis wrote:
> Hmm.  This looks like it will be correct if we find the entry (since it's
based
> on pending_nav_entry_id_) but missing in some cases.  For example, I'm
guessing
> we won't have a pending_nav_entry_id_ for renderer-initiated reloads
> (location.reload()).  

I did find a NavEntry for renderer-initiated reloads, although it doesn't have a
ReloadType set. This is the same as the old behavior, as
WebContentsObserver::DidStartNavigationToPendingEntry also has ReloadType::NONE
in this case.

> That may be ok for the favicon case, but it's kind of
> subtle for a public API.
> 
> I think it's also possible in some cases for the pending entry to be gone by
the
> time of DidStartProvisionalLoad when the handle is constructed.  That's more
of
> a uncommon race between navigations, I think, and we wouldn't be able to
recover
> the reload type anyway if it happened.
> 
> In general, I was kind of hoping to avoid a dependency on NavigationController
> in NavigationHandle, but I suppose it's not a big problem.  On one hand, I was
> toying with the idea of getting rid of pending NavigationEntries entirely if
> NavigationHandle could be sufficient, but on the other hand, we already have
> pending_nav_entry_id_ here to solve another problem.
> 
> I guess I'm ok with it, but let's mention some of the downsides (e.g., the
> renderer-initiated reload case) in a comment.

[jam, 2017-01-23 19:05:02]

https://codereview.chromium.org/2638423008/diff/30001/components/favicon/cont...
File components/favicon/content/content_favicon_driver.cc (right):

https://codereview.chromium.org/2638423008/diff/30001/components/favicon/cont...
components/favicon/content/content_favicon_driver.cc:166: void
ContentFaviconDriver::DidStartNavigation(
On 2017/01/23 18:30:08, Charlie Reis wrote:
> Note: This will get called more often than before, such as for
> renderer-initiated navigations (e.g., link clicks, script navigations, etc).
> 
> sdefresne@ or pkotwicz@, can you confirm that's ok for the
> SetFaviconOutOfDateForPage logic?  I would expect so, but if not, we can do an
> early return if !navigation_handle->IsRendererInitiated().

This shouldn't make a difference because the function early returns for
non-reloads, so the cases you mention wouldn't have any effect right?

[Charlie Reis, 2017-01-23 19:53:30]

I suppose we can get away with it, despite the complexity underneath.  The main
remaining risk is that pending NavEntry might be gone in a few rare cases that
we would have had a ReloadType (if the entry gets discarded), but it's probably
too rare to matter.

LGTM, but I think it's worth adding a comment to navigation_handle.h about this
only being set for browser-initiated reloads.

https://codereview.chromium.org/2638423008/diff/30001/components/favicon/cont...
File components/favicon/content/content_favicon_driver.cc (right):

https://codereview.chromium.org/2638423008/diff/30001/components/favicon/cont...
components/favicon/content/content_favicon_driver.cc:166: void
ContentFaviconDriver::DidStartNavigation(
On 2017/01/23 19:05:02, jam wrote:
> On 2017/01/23 18:30:08, Charlie Reis wrote:
> > Note: This will get called more often than before, such as for
> > renderer-initiated navigations (e.g., link clicks, script navigations, etc).
> > 
> > sdefresne@ or pkotwicz@, can you confirm that's ok for the
> > SetFaviconOutOfDateForPage logic?  I would expect so, but if not, we can do
an
> > early return if !navigation_handle->IsRendererInitiated().
> 
> This shouldn't make a difference because the function early returns for
> non-reloads, so the cases you mention wouldn't have any effect right?

Yes, looks like you're right.

https://codereview.chromium.org/2638423008/diff/30001/content/browser/frame_h...
File content/browser/frame_host/navigation_handle_impl.cc (right):

https://codereview.chromium.org/2638423008/diff/30001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl.cc:114:
NavigationControllerImpl* nav_controller =
On 2017/01/23 19:03:39, jam wrote:
> On 2017/01/23 18:30:08, Charlie Reis wrote:
> > Hmm.  This looks like it will be correct if we find the entry (since it's
> based
> > on pending_nav_entry_id_) but missing in some cases.  For example, I'm
> guessing
> > we won't have a pending_nav_entry_id_ for renderer-initiated reloads
> > (location.reload()).  
> 
> I did find a NavEntry for renderer-initiated reloads, although it doesn't have
a
> ReloadType set. This is the same as the old behavior, as
> WebContentsObserver::DidStartNavigationToPendingEntry also has
ReloadType::NONE
> in this case.

Ah, that's from NavigatorImpl::DidStartMainFrameNavigation.  That's conditional,
though-- it won't be there if there's already a browser-initiated pending entry
(e.g., a slow cross-process navigation getting interrupted by a
location.reload).

Sounds like the outcome may be the same overall, though (i.e., leaving it as
ReloadType::NONE).

> 
> > That may be ok for the favicon case, but it's kind of
> > subtle for a public API.
> > 
> > I think it's also possible in some cases for the pending entry to be gone by
> the
> > time of DidStartProvisionalLoad when the handle is constructed.  That's more
> of
> > a uncommon race between navigations, I think, and we wouldn't be able to
> recover
> > the reload type anyway if it happened.
> > 
> > In general, I was kind of hoping to avoid a dependency on
NavigationController
> > in NavigationHandle, but I suppose it's not a big problem.  On one hand, I
was
> > toying with the idea of getting rid of pending NavigationEntries entirely if
> > NavigationHandle could be sufficient, but on the other hand, we already have
> > pending_nav_entry_id_ here to solve another problem.
> > 
> > I guess I'm ok with it, but let's mention some of the downsides (e.g., the
> > renderer-initiated reload case) in a comment.
>

[jam, 2017-01-23 20:37:06]

jam@chromium.org changed reviewers:
- pkotwicz@chromium.org, sdefresne@chromium.org

[jam, 2017-01-23 20:39:32]

The CQ bit was checked by jam@chromium.org

[jam, 2017-01-23 20:39:32]

The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org
Link to the patchset: https://codereview.chromium.org/2638423008/#ps50001
(title: "add comment")

[commit-bot: I haz the power, 2017-01-23 20:40:04]

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

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

CQ is committing da patch.

Bot data: {"patchset_id": 50001, "attempt_start_ts": 1485203972228770,
"parent_rev": "1b5a833bca72a9f89e8c6e8c592037540eaf1411", "commit_rev":
"2ec369bd8d2542a0e163c2b5efe647a54ef69ae6"}

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

Description was changed from

==========
Convert ContentFaviconDriver to use the new navigation callbacks.

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

to

==========
Convert ContentFaviconDriver to use the new navigation callbacks.

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

Review-Url: https://codereview.chromium.org/2638423008
Cr-Commit-Position: refs/heads/master@{#445514}
Committed:
https://chromium.googlesource.com/chromium/src/+/2ec369bd8d2542a0e163c2b5efe6...
==========

[commit-bot: I haz the power, 2017-01-23 22:23:56]

Committed patchset #4 (id:50001) as
https://chromium.googlesource.com/chromium/src/+/2ec369bd8d2542a0e163c2b5efe6...