Fix in-document navigations breaking icons from Web Manifests

Fix in-document navigations breaking icons from Web Manifests

Content notifies with DidUpdateWebManifestURL() in page loads *unless*
its a navigation within the same document. This requires handling those
cases differently and hence not clearing the manifest URL, because
otherwise the page is believed to contain no manifest.

BUG=724832
Review-Url: https://codereview.chromium.org/2950563002
Cr-Commit-Position: refs/heads/master@{#483072}
Committed: https://chromium.googlesource.com/chromium/src/+/406899d78bedbdb4d09f65f65991126e61a8e758

[mastiz, 2017-06-19 18:22:59]

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

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

Dry run: CQ is trying da patch.

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

[mastiz, 2017-06-19 18:34:00]

mastiz@chromium.org changed reviewers:
+ pkotwicz@chromium.org

[commit-bot: I haz the power, 2017-06-19 20:02:51]

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

[commit-bot: I haz the power, 2017-06-19 20:02:52]

Dry run: This issue passed the CQ dry run.

[pkotwicz, 2017-06-20 17:46:57]

Description was changed from

==========
Fix in-document navigations breaking icons from Web Manifests

Content notifies with DidUpdateWebManifestURL() in page loads *unless*
its a navigation within the same document. This requires handling those
cases differently and hence not clearing the manifest URL, because
otherwise the page is believed to contain no manifest.

BUG=724832
==========

to

==========
Fix in-document navigations breaking icons from Web Manifests

Content notifies with DidUpdateWebManifestURL() in page loads *unless*
its a navigation within the same document. This requires handling those
cases differently and hence not clearing the manifest URL, because
otherwise the page is believed to contain no manifest.

BUG=724832
==========

[pkotwicz, 2017-06-20 17:46:57]

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

[pkotwicz, 2017-06-20 17:53:28]

Adding creis@ as a reviewer.

creis@: Are there scenarios where content::NavigationHandle::IsSameDocument() is
true but the navigation did not occur as a result of
FrameLoader::LoadInSameDocument()
(or conversely the navigation occured as a result of
FrameLoader::LoadInSameDocument() but
content::NavigationHandle::IsSameDocument() is false)

In particular, I am wondering whether the new if() in content_favicon_driver.cc
matches the early return in ManifestManager::DidCommitProvisionalLoad()

[Charlie Reis, 2017-06-20 18:15:10]

creis@chromium.org changed reviewers:
+ arthursonzogni@chromium.org, japhet@chromium.org

[Charlie Reis, 2017-06-20 18:15:10]

On 2017/06/20 17:53:28, pkotwicz wrote:
> Adding creis@ as a reviewer.
> 
> creis@: Are there scenarios where content::NavigationHandle::IsSameDocument()
is
> true but the navigation did not occur as a result of
> FrameLoader::LoadInSameDocument()
> (or conversely the navigation occured as a result of
> FrameLoader::LoadInSameDocument() but
> content::NavigationHandle::IsSameDocument() is false)
> 
> In particular, I am wondering whether the new if() in
content_favicon_driver.cc
> matches the early return in ManifestManager::DidCommitProvisionalLoad()

I think those should be equivalent.  NavigationHandle gets its value from
params.was_within_same_document, which I think only gets set via
FrameLoader::LoadInSameDocument.  japhet@, do you know of any cases that's not
true?

I guess there's also a PlzNavigate case in
NavigationRequest::CreateNavigationHandle which uses
common_params_.navigation_type.  Arthur, does this assumption about
LoadInSameDocument sound right?

[arthursonzogni, 2017-06-21 09:20:21]

On 2017/06/20 18:15:10, Charlie Reis (slow) wrote:
> On 2017/06/20 17:53:28, pkotwicz wrote:
> > Adding creis@ as a reviewer.
> > 
> > creis@: Are there scenarios where
content::NavigationHandle::IsSameDocument()
> is
> > true but the navigation did not occur as a result of
> > FrameLoader::LoadInSameDocument()
> > (or conversely the navigation occured as a result of
> > FrameLoader::LoadInSameDocument() but
> > content::NavigationHandle::IsSameDocument() is false)
> > 
> > In particular, I am wondering whether the new if() in
> content_favicon_driver.cc
> > matches the early return in ManifestManager::DidCommitProvisionalLoad()
> 
> I think those should be equivalent.  NavigationHandle gets its value from
> params.was_within_same_document, which I think only gets set via
> FrameLoader::LoadInSameDocument.  japhet@, do you know of any cases that's not
> true?
> 
> I guess there's also a PlzNavigate case in
> NavigationRequest::CreateNavigationHandle which uses
> common_params_.navigation_type.  Arthur, does this assumption about
> LoadInSameDocument sound right?

Let me summarize what I think happens.

I think there is two cases:
1) Navigation is initiated by the renderer.
  * [R] FrameLoader::LoadInSameDocument is called.
  * [B] A new NavigationHandle is created in
RenderFrameHostImpl::TakeNavigationHandleForCommit()
  * [B] params.was_within_same_document = true =>
NavigationHandle::IsSameDocument() = true.
  * [B] WebContentObserver::DidStartNavigation() is called.

2) Navigation is initiated by the browser.
  * [B] NavigatorImpl::RequestNavigation is called.
  * [B] The navigation is classified to SAME_DOCUMENT or HISTORY_SAME_DOCUMENT
in GetNavigationType
  * [B] A new NavigationHandle is created (NavigationHandle::IsSameDocument() =
true).
  * [B] WebContentObserver::DidStartNavigation() is called.
  * [R] RenderFrameImpl::OnCommitNavigation is called
  * [R] FrameLoader::LoadInSameDocument is expected to be called. (See *) 

(*) If LoadInSameDocument is not called, the navigation is dropped. This could
happens because of race conditions. (when the browser initiates a same document
navigation and the renderer navigates to about:blank before being asked to
navigate by the browser)

What matter to you is that NavigationHandle::IsSameDocument() can be true and
FrameLoader::LoadInSameDocument() not being called yet (but expected to be
called, not called => the navigation is dropped)

Note:
You can easily try browser-initiated same-document navigation by modifying the
url part after the # in the omnibox.
The flag --enable-browser-side-navigation enables PlzNavigate.

[pkotwicz, 2017-06-21 18:48:34]

Thank you creis@ and arthursonzogni@ for the detailed explanation! I really
appreciate it

Based on the explanation from creis@ and arthursonzogni@ your CL looks good.
Just a comment about the tests

https://codereview.chromium.org/2950563002/diff/20001/chrome/browser/favicon/...
File chrome/browser/favicon/content_favicon_driver_browsertest.cc (right):

https://codereview.chromium.org/2950563002/diff/20001/chrome/browser/favicon/...
chrome/browser/favicon/content_favicon_driver_browsertest.cc:348:
ui_test_utils::BROWSER_TEST_NONE);
You should probably wait till the JavaScript has executed. I know of two ways of
doing this (there are probably others)

Option #1: Make the JavaScript change the title of the page when it is done and
wait till the page title was changed. You could use content::TitleWatcher to
wait till the title is set

Option #2: Execute JavaScript via C++ via
RenderFrameHost::ExecuteJavaScriptForTests() and use TestNavigationObserver

[mastiz, 2017-06-23 09:18:08]

On 2017/06/21 18:48:34, pkotwicz wrote:
> Thank you creis@ and arthursonzogni@ for the detailed explanation! I really
> appreciate it
> 
> Based on the explanation from creis@ and arthursonzogni@ your CL looks good.
> Just a comment about the tests
> 
>
https://codereview.chromium.org/2950563002/diff/20001/chrome/browser/favicon/...
> File chrome/browser/favicon/content_favicon_driver_browsertest.cc (right):
> 
>
https://codereview.chromium.org/2950563002/diff/20001/chrome/browser/favicon/...
> chrome/browser/favicon/content_favicon_driver_browsertest.cc:348:
> ui_test_utils::BROWSER_TEST_NONE);
> You should probably wait till the JavaScript has executed. I know of two ways
of
> doing this (there are probably others)
> 
> Option #1: Make the JavaScript change the title of the page when it is done
and
> wait till the page title was changed. You could use content::TitleWatcher to
> wait till the title is set
> 
> Option #2: Execute JavaScript via C++ via
> RenderFrameHost::ExecuteJavaScriptForTests() and use TestNavigationObserver

Done, PTAL. I've achieved the same by waiting for a specific URL. Title-based
waiting presented some additional issues.

[mastiz, 2017-06-23 09:22:33]

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

[commit-bot: I haz the power, 2017-06-23 09:22: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-06-23 11:28:18]

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

[commit-bot: I haz the power, 2017-06-23 11:28:18]

Dry run: This issue passed the CQ dry run.

[pkotwicz, 2017-06-23 17:52:20]

https://codereview.chromium.org/2950563002/diff/40001/chrome/browser/favicon/...
File chrome/browser/favicon/content_favicon_driver_browsertest.cc (right):

https://codereview.chromium.org/2950563002/diff/40001/chrome/browser/favicon/...
chrome/browser/favicon/content_favicon_driver_browsertest.cc:110: required_url_
!= web_contents()->GetVisibleURL()) {
Should this be GetLastCommittedURL() instead?

https://codereview.chromium.org/2950563002/diff/40001/chrome/browser/favicon/...
chrome/browser/favicon/content_favicon_driver_browsertest.cc:132:
CHECK(!quit_closure_.is_null());
Why are you making this a CHECK()?

It is possible that the page finishes loading between PendingTaskWaiter() being
created and Wait() being called

https://codereview.chromium.org/2950563002/diff/40001/chrome/browser/favicon/...
chrome/browser/favicon/content_favicon_driver_browsertest.cc:308: #if
defined(OS_ANDROID) || defined(OS_IOS)
Shouldn't this test be Android only given that iOS does not use
content_favicon_driver.cc ?

[mastiz, 2017-06-27 08:57:14]

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

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

Dry run: CQ is trying da patch.

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

[mastiz, 2017-06-27 08:57:36]

PTAL! Sorry for the slow responses due to team conference.

https://codereview.chromium.org/2950563002/diff/40001/chrome/browser/favicon/...
File chrome/browser/favicon/content_favicon_driver_browsertest.cc (right):

https://codereview.chromium.org/2950563002/diff/40001/chrome/browser/favicon/...
chrome/browser/favicon/content_favicon_driver_browsertest.cc:110: required_url_
!= web_contents()->GetVisibleURL()) {
On 2017/06/23 17:52:20, pkotwicz wrote:
> Should this be GetLastCommittedURL() instead?

I think both should be equally fine, and in seems to work as expected, i.e. it
does wait until the second page gets loaded. Why do you think otherwise?

https://codereview.chromium.org/2950563002/diff/40001/chrome/browser/favicon/...
chrome/browser/favicon/content_favicon_driver_browsertest.cc:132:
CHECK(!quit_closure_.is_null());
On 2017/06/23 17:52:20, pkotwicz wrote:
> Why are you making this a CHECK()?
> 
> It is possible that the page finishes loading between PendingTaskWaiter()
being
> created and Wait() being called

Reverted. I however suspect this cannot happen in practice, because there's no
blocking code between the two (if I understand BROWSER_TEST_NONE well).

https://codereview.chromium.org/2950563002/diff/40001/chrome/browser/favicon/...
chrome/browser/favicon/content_favicon_driver_browsertest.cc:308: #if
defined(OS_ANDROID) || defined(OS_IOS)
On 2017/06/23 17:52:20, pkotwicz wrote:
> Shouldn't this test be Android only given that iOS does not use
> content_favicon_driver.cc ?

Done.

[commit-bot: I haz the power, 2017-06-27 10:49:30]

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

[commit-bot: I haz the power, 2017-06-27 10:49:31]

Dry run: This issue passed the CQ dry run.

[pkotwicz, 2017-06-27 15:21:13]

https://codereview.chromium.org/2950563002/diff/40001/chrome/browser/favicon/...
File chrome/browser/favicon/content_favicon_driver_browsertest.cc (right):

https://codereview.chromium.org/2950563002/diff/40001/chrome/browser/favicon/...
chrome/browser/favicon/content_favicon_driver_browsertest.cc:110: required_url_
!= web_contents()->GetVisibleURL()) {
GetLastCommittedURL() is the typical function to use. I was wondering why you
chose the less common GetVisibleURL().

GetLastCommittedURL() and GetVisibleURL() are not the interchangeable as
described in the comments of web_contents.h

https://codereview.chromium.org/2950563002/diff/40001/chrome/browser/favicon/...
chrome/browser/favicon/content_favicon_driver_browsertest.cc:132:
CHECK(!quit_closure_.is_null());
Aside: ui_test_utils::NavigateToURLWithDisposition() is a blocking function

[mastiz, 2017-06-28 09:01:58]

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

[commit-bot: I haz the power, 2017-06-28 09:02:06]

Dry run: CQ is trying da patch.

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

[mastiz, 2017-06-28 09:02:14]

PTAL.

https://codereview.chromium.org/2950563002/diff/40001/chrome/browser/favicon/...
File chrome/browser/favicon/content_favicon_driver_browsertest.cc (right):

https://codereview.chromium.org/2950563002/diff/40001/chrome/browser/favicon/...
chrome/browser/favicon/content_favicon_driver_browsertest.cc:110: required_url_
!= web_contents()->GetVisibleURL()) {
On 2017/06/27 15:21:13, pkotwicz wrote:
> GetLastCommittedURL() is the typical function to use. I was wondering why you
> chose the less common GetVisibleURL().
> 
> GetLastCommittedURL() and GetVisibleURL() are not the interchangeable as
> described in the comments of web_contents.h
> 

Thanks for clarifying, I was not aware of that and assumed GetVisibleURL() would
involve less complexity. Done, replaced.

https://codereview.chromium.org/2950563002/diff/40001/chrome/browser/favicon/...
chrome/browser/favicon/content_favicon_driver_browsertest.cc:132:
CHECK(!quit_closure_.is_null());
On 2017/06/27 15:21:13, pkotwicz wrote:
> Aside: ui_test_utils::NavigateToURLWithDisposition() is a blocking function

I don't think it is blocking with BROWSER_TEST_NONE, but nevertheless it'd be
subtle to rely on this.

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

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

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

Dry run: This issue passed the CQ dry run.

[pkotwicz, 2017-06-28 18:22:27]

LGTM. Thank you for bearing with me

[mastiz, 2017-06-28 18:24:21]

The CQ bit was checked by mastiz@chromium.org

[commit-bot: I haz the power, 2017-06-28 18:25:00]

CQ is trying da patch.

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

[commit-bot: I haz the power, 2017-06-28 18:29:08]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1498674261886500,
"parent_rev": "c08a4dc09efc285bbb12c8dd62480b73332c56b6", "commit_rev":
"406899d78bedbdb4d09f65f65991126e61a8e758"}

[commit-bot: I haz the power, 2017-06-28 18:29:29]

Description was changed from

==========
Fix in-document navigations breaking icons from Web Manifests

Content notifies with DidUpdateWebManifestURL() in page loads *unless*
its a navigation within the same document. This requires handling those
cases differently and hence not clearing the manifest URL, because
otherwise the page is believed to contain no manifest.

BUG=724832
==========

to

==========
Fix in-document navigations breaking icons from Web Manifests

Content notifies with DidUpdateWebManifestURL() in page loads *unless*
its a navigation within the same document. This requires handling those
cases differently and hence not clearing the manifest URL, because
otherwise the page is believed to contain no manifest.

BUG=724832

Review-Url: https://codereview.chromium.org/2950563002
Cr-Commit-Position: refs/heads/master@{#483072}
Committed:
https://chromium.googlesource.com/chromium/src/+/406899d78bedbdb4d09f65f65991...
==========

[commit-bot: I haz the power, 2017-06-28 18:29:31]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/406899d78bedbdb4d09f65f65991...