Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2548903002:
[OfflinePages] Classifies PRERENDERING_FAILED cases whether to TryNext (Closed)
|
Created:
4 years ago by dougarnett Modified:
4 years ago CC:
chromium-reviews, cbentzel+watch_chromium.org, dewittj+watch_chromium.org, tburkard+watch_chromium.org, fgorski+watch_chromium.org, romax+watch_chromium.org, petewil+watch_chromium.org, gavinp+prer_chromium.org, asvitkine+watch_chromium.org, chili+watch_chromium.org, dimich+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[OfflinePages] Classifies PRERENDERING_FAILED cases whether to TryNext
Defines new PRERENDERING_FAILED_NO_NEXT for failure types where the
Coordinator should not continue process the next request and now will
process the next request for PRERENDERING_FAILED failures.
I took a code inspection pass on the prerender code and identified
most of the FinalStatus codes we see in UMA into whether they are
FAILED, FAILED_NO_RETRY, or FAILED_NO_NEXT. There we a couple I
wasn't confident about that I left to default as FAILED.
BUG=670529
Committed: https://crrev.com/f9a803de56a74d1f92a07d2ee0934ecd0ca8ace4
Cr-Commit-Position: refs/heads/master@{#436461}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Added a PrerenderingLoader unittest #
Messages
Total messages: 35 (17 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: + chili@chromium.org, petewil@chromium.org
mostly good, one question. https://codereview.chromium.org/2548903002/diff/1/chrome/browser/android/offl... File chrome/browser/android/offline_pages/prerendering_loader.cc (right): https://codereview.chromium.org/2548903002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/prerendering_loader.cc:65: return Offliner::RequestStatus::PRERENDERING_FAILED_NO_NEXT; Should we add a test in prerendering loader unittest for this new return code?
https://codereview.chromium.org/2548903002/diff/1/components/offline_pages/ba... File components/offline_pages/background/offliner.h (right): https://codereview.chromium.org/2548903002/diff/1/components/offline_pages/ba... components/offline_pages/background/offliner.h:49: PRERENDERING_FAILED_NO_NEXT = 11, Just a thought as I'm working on the background loader... Should we rename this to "FAILED_NO_NEXT" to make it less prerendering-specific?
forgot to LGTM :)
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
https://codereview.chromium.org/2548903002/diff/1/chrome/browser/android/offl... File chrome/browser/android/offline_pages/prerendering_loader.cc (right): https://codereview.chromium.org/2548903002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/prerendering_loader.cc:65: return Offliner::RequestStatus::PRERENDERING_FAILED_NO_NEXT; On 2016/12/02 23:06:09, Pete Williamson wrote: > Should we add a test in prerendering loader unittest for this new return code? Yes, thanks https://codereview.chromium.org/2548903002/diff/1/components/offline_pages/ba... File components/offline_pages/background/offliner.h (right): https://codereview.chromium.org/2548903002/diff/1/components/offline_pages/ba... components/offline_pages/background/offliner.h:49: PRERENDERING_FAILED_NO_NEXT = 11, On 2016/12/02 23:17:43, chili wrote: > Just a thought as I'm working on the background loader... > > Should we rename this to "FAILED_NO_NEXT" to make it less prerendering-specific? Or LOADING_FAILED_NO_NEXT? Pete, WDYT - should we rename the Prerendering terminology here to Loading as part of this CL?
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/12/02 23:50:08, dougarnett wrote: > https://codereview.chromium.org/2548903002/diff/1/chrome/browser/android/offl... > File chrome/browser/android/offline_pages/prerendering_loader.cc (right): > > https://codereview.chromium.org/2548903002/diff/1/chrome/browser/android/offl... > chrome/browser/android/offline_pages/prerendering_loader.cc:65: return > Offliner::RequestStatus::PRERENDERING_FAILED_NO_NEXT; > On 2016/12/02 23:06:09, Pete Williamson wrote: > > Should we add a test in prerendering loader unittest for this new return code? > > Yes, thanks > > https://codereview.chromium.org/2548903002/diff/1/components/offline_pages/ba... > File components/offline_pages/background/offliner.h (right): > > https://codereview.chromium.org/2548903002/diff/1/components/offline_pages/ba... > components/offline_pages/background/offliner.h:49: PRERENDERING_FAILED_NO_NEXT = > 11, > On 2016/12/02 23:17:43, chili wrote: > > Just a thought as I'm working on the background loader... > > > > Should we rename this to "FAILED_NO_NEXT" to make it less > prerendering-specific? > > Or LOADING_FAILED_NO_NEXT? > > Pete, WDYT - should we rename the Prerendering terminology here to Loading as > part of this CL? I like Chili's idea, but let's do it in a different changelist by itself, I suspect it will touch a lot of places, and having a changelist just for that would be good.
lgtm
The CQ bit was unchecked by dougarnett@chromium.org
The CQ bit was checked by dougarnett@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from chili@chromium.org Link to the patchset: https://codereview.chromium.org/2548903002/#ps20001 (title: "Added a PrerenderingLoader unittest")
Still LGTM On 2016/12/02 23:50:08, dougarnett wrote: > https://codereview.chromium.org/2548903002/diff/1/chrome/browser/android/offl... > File chrome/browser/android/offline_pages/prerendering_loader.cc (right): > > https://codereview.chromium.org/2548903002/diff/1/chrome/browser/android/offl... > chrome/browser/android/offline_pages/prerendering_loader.cc:65: return > Offliner::RequestStatus::PRERENDERING_FAILED_NO_NEXT; > On 2016/12/02 23:06:09, Pete Williamson wrote: > > Should we add a test in prerendering loader unittest for this new return code? > > Yes, thanks > > https://codereview.chromium.org/2548903002/diff/1/components/offline_pages/ba... > File components/offline_pages/background/offliner.h (right): > > https://codereview.chromium.org/2548903002/diff/1/components/offline_pages/ba... > components/offline_pages/background/offliner.h:49: PRERENDERING_FAILED_NO_NEXT = > 11, > On 2016/12/02 23:17:43, chili wrote: > > Just a thought as I'm working on the background loader... > > > > Should we rename this to "FAILED_NO_NEXT" to make it less > prerendering-specific? > > Or LOADING_FAILED_NO_NEXT? > > Pete, WDYT - should we rename the Prerendering terminology here to Loading as > part of this CL? +1 LOADING. Alternatively we can wait for my changes to go in. I don't think some of these prerender failures apply to the new loader - like the new window, unsupported http method, etc? Interesting question though - with the new loader, redirects will happen automatically i believe, whereas the prerender will fail. i.e. if it was an ad click, it will cycle through all the redirects before going to landing page. I am assuming this will be ok, but maybe something to consider
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/12/03 00:01:17, chili wrote: > Still LGTM > > On 2016/12/02 23:50:08, dougarnett wrote: > > > https://codereview.chromium.org/2548903002/diff/1/chrome/browser/android/offl... > > File chrome/browser/android/offline_pages/prerendering_loader.cc (right): > > > > > https://codereview.chromium.org/2548903002/diff/1/chrome/browser/android/offl... > > chrome/browser/android/offline_pages/prerendering_loader.cc:65: return > > Offliner::RequestStatus::PRERENDERING_FAILED_NO_NEXT; > > On 2016/12/02 23:06:09, Pete Williamson wrote: > > > Should we add a test in prerendering loader unittest for this new return > code? > > > > Yes, thanks > > > > > https://codereview.chromium.org/2548903002/diff/1/components/offline_pages/ba... > > File components/offline_pages/background/offliner.h (right): > > > > > https://codereview.chromium.org/2548903002/diff/1/components/offline_pages/ba... > > components/offline_pages/background/offliner.h:49: PRERENDERING_FAILED_NO_NEXT > = > > 11, > > On 2016/12/02 23:17:43, chili wrote: > > > Just a thought as I'm working on the background loader... > > > > > > Should we rename this to "FAILED_NO_NEXT" to make it less > > prerendering-specific? > > > > Or LOADING_FAILED_NO_NEXT? > > > > Pete, WDYT - should we rename the Prerendering terminology here to Loading as > > part of this CL? > > +1 LOADING. Alternatively we can wait for my changes to go in. I don't think > some of these prerender failures apply to the new loader - like the new window, > unsupported http method, etc? > > Interesting question though - with the new loader, redirects will happen > automatically i believe, whereas the prerender will fail. i.e. if it was an ad > click, it will cycle through all the redirects before going to landing page. I > am assuming this will be ok, but maybe something to consider Definitely you will not need some of the specific error types that prerenderer does. I wasn't thinking we would want to share the ClassifyFinalStatus statement between impls. Seems better to define your own for just errors you detect. I guess we don't know yet if you will need all the flavors of the Offliner:RequestStatus enum but seems reasonably likely. I don't understand the redirect point (prerenderer does follow some redirects). Maybe we can discuss next week when I'm there.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
dougarnett@chromium.org changed reviewers: + holte@chromium.org
Steven, can you review added enum value to histograms.xml?
lgtm
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...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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": 20001, "attempt_start_ts": 1480980545520110,
"parent_rev": "d9f9601c2760250e7cd9d5bfe9628e04a0888c14", "commit_rev":
"075afa67609ab62aa773ef3743147db8d503dde1"}
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== [OfflinePages] Classifies PRERENDERING_FAILED cases whether to TryNext Defines new PRERENDERING_FAILED_NO_NEXT for failure types where the Coordinator should not continue process the next request and now will process the next request for PRERENDERING_FAILED failures. I took a code inspection pass on the prerender code and identified most of the FinalStatus codes we see in UMA into whether they are FAILED, FAILED_NO_RETRY, or FAILED_NO_NEXT. There we a couple I wasn't confident about that I left to default as FAILED. BUG=670529 ========== to ========== [OfflinePages] Classifies PRERENDERING_FAILED cases whether to TryNext Defines new PRERENDERING_FAILED_NO_NEXT for failure types where the Coordinator should not continue process the next request and now will process the next request for PRERENDERING_FAILED failures. I took a code inspection pass on the prerender code and identified most of the FinalStatus codes we see in UMA into whether they are FAILED, FAILED_NO_RETRY, or FAILED_NO_NEXT. There we a couple I wasn't confident about that I left to default as FAILED. BUG=670529 Committed: https://crrev.com/f9a803de56a74d1f92a07d2ee0934ecd0ca8ace4 Cr-Commit-Position: refs/heads/master@{#436461} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/f9a803de56a74d1f92a07d2ee0934ecd0ca8ace4 Cr-Commit-Position: refs/heads/master@{#436461} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
