|
|
DescriptionFix 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
Patch Set 1 #Patch Set 2 : Simplify test. #
Total comments: 1
Patch Set 3 : Wait for JS. #
Total comments: 10
Patch Set 4 : Addressed comment. #Patch Set 5 : Adopted GetLastCommittedURL(). #
Messages
Total messages: 36 (23 generated)
The CQ bit was checked by mastiz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
mastiz@chromium.org changed reviewers: + pkotwicz@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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@chromium.org changed reviewers: + creis@chromium.org
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()
creis@chromium.org changed reviewers: + arthursonzogni@chromium.org, japhet@chromium.org
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?
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.
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
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.
The CQ bit was checked by mastiz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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 ?
The CQ bit was checked by mastiz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
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.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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
The CQ bit was checked by mastiz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
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.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM. Thank you for bearing with me
The CQ bit was checked by mastiz@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 80001, "attempt_start_ts": 1498674261886500, "parent_rev": "c08a4dc09efc285bbb12c8dd62480b73332c56b6", "commit_rev": "406899d78bedbdb4d09f65f65991126e61a8e758"}
Message was sent while issue was closed.
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... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/406899d78bedbdb4d09f65f65991... |