Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2656653002:
Suppress NEW_NAVIGATION_ENTRY prerenderer failures for ORIGIN_OFFLINE (Closed)
|
Created:
3 years, 10 months ago by dougarnett Modified:
3 years, 10 months ago CC:
chromium-reviews, tburkard+watch_chromium.org, gavinp+prer_chromium.org, cbentzel+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSuppress NEW_NAVIGATION_ENTRY prerenderer failures for ORIGIN_OFFLINE
We have noticed some FINAL_STATUS_NEW_NAVIGATION_ENTRY failures in UMA
for offline pages but didn't know how to trigger them. Bug 675767 has
identified a site (foxsports.com) with articles/links that do have this
issue. The issue is repeatable for those pages and they are never
succeeding. I end-to-end tested this change with logging to confirm
we do repeatably see entry count going to 2 and that with this change
the pages can background load successfully.
BUG=675767
Review-Url: https://codereview.chromium.org/2656653002
Cr-Commit-Position: refs/heads/master@{#446379}
Committed: https://chromium.googlesource.com/chromium/src/+/3ae6f734f3566484634a068cbe663bbccc24937c
Patch Set 1 #
Total comments: 2
Patch Set 2 : Revised to quick return for offliner origin as requested. #Patch Set 3 : Revised to quick return for offliner origin as requested #
Total comments: 2
Patch Set 4 : Fixes comparison and adds browser test. #
Total comments: 4
Patch Set 5 : Addresses pasko comments. #Patch Set 6 : Simple cancel any subresources with unsupported schemes and continue for Offline requests #
Messages
Total messages: 32 (19 generated)
The CQ bit was checked by dougarnett@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...
dougarnett@chromium.org changed reviewers: + pasko@chromium.org, petewil@chromium.org
Btw, looks like NEW_NAVIGATION_ENTRY failures are running around 2-2.5% of FinalStatus codes for Offline on Stable and Canary.
lgtm
Overall looks good. Please add a test that checks the condition of PrerenderBrowserTest.PrerenderNewNavigationEntry with ORIGIN_OFFLINE. https://codereview.chromium.org/2656653002/diff/1/chrome/browser/prerender/pr... File chrome/browser/prerender/prerender_contents.cc (right): https://codereview.chromium.org/2656653002/diff/1/chrome/browser/prerender/pr... chrome/browser/prerender/prerender_contents.cc:599: for (size_t i = 0; i < params.redirects.size(); i++) { sending these IPCs to the renderer is not needed for offline, so it would be cleaner to: if (origin() == ORIGIN_OFFLINE) return;
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by dougarnett@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...
Adding browser test looks like much plumbing so please advise if you see simpler approach than plumbing origin parm through PrerenderTestURL() => PrerenderTestURLImpl() => NavigateWithPrenders() => ExpectPrerenderContents()? Maybe replace the PrerenderContentsFactory on PrerenderManager and then put back the original one? I looked a bit at prerender_unittest instead and think I might need to mock a controller to do that. https://codereview.chromium.org/2656653002/diff/1/chrome/browser/prerender/pr... File chrome/browser/prerender/prerender_contents.cc (right): https://codereview.chromium.org/2656653002/diff/1/chrome/browser/prerender/pr... chrome/browser/prerender/prerender_contents.cc:599: for (size_t i = 0; i < params.redirects.size(); i++) { On 2017/01/24 19:43:38, pasko wrote: > sending these IPCs to the renderer is not needed for offline, so it would be > cleaner to: > > if (origin() == ORIGIN_OFFLINE) > return; Done.
The CQ bit was unchecked by commit-bot@chromium.org
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...)
On 2017/01/24 21:14:03, dougarnett wrote: > Adding browser test looks like much plumbing so please advise if you see simpler > approach than plumbing origin parm through PrerenderTestURL() => > PrerenderTestURLImpl() => NavigateWithPrenders() => ExpectPrerenderContents()? > Maybe replace the PrerenderContentsFactory on PrerenderManager and then put back > the original one? We do mock out PrerenderContentsFactory. see: https://codesearch.chromium.org/chromium/src/chrome/browser/prerender/prerend... Plumbing Origin through these seems like an overkill for this test. In such cases we used to inline the contents of these methods into the test and apply tweaks. Once you inline, you can use AddPrerenderForOffline(). See for example NoStatePrefetchBrowserTest.RendererCrash (even though it is nostateprefetch, it is using PrerenderManager mechanics to do prefetches). I am not 100% sure this path would work for you, let me know what the roadblocks are. > I looked a bit at prerender_unittest instead and think I might need to mock a > controller to do that. AFAIR prerender_unittests do not issue actual prerendering, so we would not be able to check anything that is sent from the renderer, hence no coverage for DidNavigateMainFrame(). https://codereview.chromium.org/2656653002/diff/40001/chrome/browser/prerende... File chrome/browser/prerender/prerender_contents.cc (right): https://codereview.chromium.org/2656653002/diff/40001/chrome/browser/prerende... chrome/browser/prerender/prerender_contents.cc:581: if (origin() != ORIGIN_OFFLINE) s/!=/==/ in case you wondered why a few tests failed ;)
The CQ bit was checked by dougarnett@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...
Thanks for the test pointer https://codereview.chromium.org/2656653002/diff/40001/chrome/browser/prerende... File chrome/browser/prerender/prerender_contents.cc (right): https://codereview.chromium.org/2656653002/diff/40001/chrome/browser/prerende... chrome/browser/prerender/prerender_contents.cc:581: if (origin() != ORIGIN_OFFLINE) On 2017/01/25 12:20:54, pasko wrote: > s/!=/==/ > > in case you wondered why a few tests failed ;) :-)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Btw, test now added so PTAL
lgtm with nits, thank you https://codereview.chromium.org/2656653002/diff/60001/chrome/browser/prerende... File chrome/browser/prerender/prerender_browsertest.cc (right): https://codereview.chromium.org/2656653002/diff/60001/chrome/browser/prerende... chrome/browser/prerender/prerender_browsertest.cc:3116: PrerenderNoNewNavigationEntryForOffline) { nit: I think this checks when there _is_ a new navigation entry on the page, so I'd name it with s/No//. https://codereview.chromium.org/2656653002/diff/60001/chrome/browser/prerende... File chrome/browser/prerender/prerender_contents.cc (right): https://codereview.chromium.org/2656653002/diff/60001/chrome/browser/prerende... chrome/browser/prerender/prerender_contents.cc:580: // Offline requests are not subject to navigation and use of history. I'd prefer something more verbose as a comment, like: // Prevent ORIGIN_OFFLINE prerenders from being destroyed on location.href // change, since the history is never merged for offline prerenders. Also // avoid adding aliases as they may potentially mark other valid requests to // offline as duplicate.
https://codereview.chromium.org/2656653002/diff/60001/chrome/browser/prerende... File chrome/browser/prerender/prerender_browsertest.cc (right): https://codereview.chromium.org/2656653002/diff/60001/chrome/browser/prerende... chrome/browser/prerender/prerender_browsertest.cc:3116: PrerenderNoNewNavigationEntryForOffline) { On 2017/01/26 12:09:39, pasko wrote: > nit: I think this checks when there _is_ a new navigation entry on the page, so > I'd name it with s/No//. Done. https://codereview.chromium.org/2656653002/diff/60001/chrome/browser/prerende... File chrome/browser/prerender/prerender_contents.cc (right): https://codereview.chromium.org/2656653002/diff/60001/chrome/browser/prerende... chrome/browser/prerender/prerender_contents.cc:580: // Offline requests are not subject to navigation and use of history. On 2017/01/26 12:09:39, pasko wrote: > I'd prefer something more verbose as a comment, like: > > // Prevent ORIGIN_OFFLINE prerenders from being destroyed on location.href > // change, since the history is never merged for offline prerenders. Also > // avoid adding aliases as they may potentially mark other valid requests to > // offline as duplicate. > Done.
The CQ bit was checked by dougarnett@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from petewil@chromium.org, pasko@chromium.org Link to the patchset: https://codereview.chromium.org/2656653002/#ps80001 (title: "Addresses pasko comments.")
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 dougarnett@chromium.org
The CQ bit was checked by dougarnett@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": 1485452552303440,
"parent_rev": "ce00d1fcc3a2779d4db9771ca35513a416de9f9d", "commit_rev":
"3ae6f734f3566484634a068cbe663bbccc24937c"}
Message was sent while issue was closed.
Description was changed from ========== Suppress NEW_NAVIGATION_ENTRY prerenderer failures for ORIGIN_OFFLINE We have noticed some FINAL_STATUS_NEW_NAVIGATION_ENTRY failures in UMA for offline pages but didn't know how to trigger them. Bug 675767 has identified a site (foxsports.com) with articles/links that do have this issue. The issue is repeatable for those pages and they are never succeeding. I end-to-end tested this change with logging to confirm we do repeatably see entry count going to 2 and that with this change the pages can background load successfully. BUG=675767 ========== to ========== Suppress NEW_NAVIGATION_ENTRY prerenderer failures for ORIGIN_OFFLINE We have noticed some FINAL_STATUS_NEW_NAVIGATION_ENTRY failures in UMA for offline pages but didn't know how to trigger them. Bug 675767 has identified a site (foxsports.com) with articles/links that do have this issue. The issue is repeatable for those pages and they are never succeeding. I end-to-end tested this change with logging to confirm we do repeatably see entry count going to 2 and that with this change the pages can background load successfully. BUG=675767 Review-Url: https://codereview.chromium.org/2656653002 Cr-Commit-Position: refs/heads/master@{#446379} Committed: https://chromium.googlesource.com/chromium/src/+/3ae6f734f3566484634a068cbe66... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/3ae6f734f3566484634a068cbe66... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
