Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2656763002:
[Offline pages] Add navigation error handling to background loader. (Closed)
|
Created:
3 years, 11 months ago by chili Modified:
3 years, 10 months ago CC:
chromium-reviews, romax+watch_chromium.org, fgorski+watch_chromium.org, dewittj+watch_chromium.org, petewil+watch_chromium.org, chili+watch_chromium.org, dimich+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Offline pages] Add navigation error handling to background loader.
This ensures that we don't try to download any net error pages.
BUG=691141
Review-Url: https://codereview.chromium.org/2656763002
Cr-Commit-Position: refs/heads/master@{#450146}
Committed: https://chromium.googlesource.com/chromium/src/+/91941581b49f242755c8246fb788ec2ceeee65bd
Patch Set 1 #
Total comments: 6
Patch Set 2 : code review update #
Total comments: 2
Patch Set 3 : compile fixes #Patch Set 4 : move some errors to non-catch, so we can notify user #
Total comments: 6
Patch Set 5 : cl comments and format #
Messages
Total messages: 37 (18 generated)
The CQ bit was checked by chili@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...
chili@chromium.org changed reviewers: + fgorski@chromium.org, petewil@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm Please fix nit and add bug to the CL description. https://codereview.chromium.org/2656763002/diff/1/chrome/browser/android/offl... File chrome/browser/android/offline_pages/background_loader_offliner.cc (right): https://codereview.chromium.org/2656763002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/background_loader_offliner.cc:192: if (navigation_handle->IsErrorPage()) { nit: {} not needed
Mostly looks good, one question. https://codereview.chromium.org/2656763002/diff/1/chrome/browser/android/offl... File chrome/browser/android/offline_pages/background_loader_offliner.cc (right): https://codereview.chromium.org/2656763002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/background_loader_offliner.cc:137: Offliner::RequestStatus::LOADING_FAILED_NO_RETRY); Why NO_RETRY? It might be a malformed URL, in which case NO_RETRY is right, but maybe the net being down also results in an error, in which case retrying is appropriate.
dimich@chromium.org changed reviewers: + dimich@chromium.org
drive-by: https://codereview.chromium.org/2656763002/diff/1/chrome/browser/android/offl... File chrome/browser/android/offline_pages/background_loader_offliner.h (right): https://codereview.chromium.org/2656763002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/background_loader_offliner.h:81: bool is_error_; naming nit: perhaps it can be more explicit if it was is_error_page_ or did_navigate_to_error_page_
Updated CL with a list of non-retriable net errors. Please take a look and determine if you agree and/or see that I missed something :) thanks! https://codereview.chromium.org/2656763002/diff/1/chrome/browser/android/offl... File chrome/browser/android/offline_pages/background_loader_offliner.cc (right): https://codereview.chromium.org/2656763002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/background_loader_offliner.cc:137: Offliner::RequestStatus::LOADING_FAILED_NO_RETRY); On 2017/01/25 18:34:26, Pete Williamson wrote: > Why NO_RETRY? It might be a malformed URL, in which case NO_RETRY is right, but > maybe the net being down also results in an error, in which case retrying is > appropriate. This is a good point. While my gut feeling says there are more NO_RETRY cases than RETRY cases, it'll probably be more detrimental user experience if we mis-mark a retriable case as no_retry than the other way around. I added a case statement for most popular/common non-retry cases. Please review :) https://codereview.chromium.org/2656763002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/background_loader_offliner.cc:192: if (navigation_handle->IsErrorPage()) { On 2017/01/25 17:05:14, fgorski wrote: > nit: {} not needed Added a switch case. Keeping the {} :) https://codereview.chromium.org/2656763002/diff/1/chrome/browser/android/offl... File chrome/browser/android/offline_pages/background_loader_offliner.h (right): https://codereview.chromium.org/2656763002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/background_loader_offliner.h:81: bool is_error_; On 2017/01/25 19:36:55, Dmitry Titov wrote: > naming nit: perhaps it can be more explicit if it was is_error_page_ or > did_navigate_to_error_page_ renamed to page_loading_state_ with enum because that seems better to also convey whether it's simply loading failed vs no retry
https://codereview.chromium.org/2656763002/diff/20001/chrome/browser/android/... File chrome/browser/android/offline_pages/background_loader_offliner.cc (right): https://codereview.chromium.org/2656763002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/background_loader_offliner.cc:227: page_load_state_ = LOADING_FAILED; RETRIABLE?
https://codereview.chromium.org/2656763002/diff/20001/chrome/browser/android/... File chrome/browser/android/offline_pages/background_loader_offliner.cc (right): https://codereview.chromium.org/2656763002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/background_loader_offliner.cc:227: page_load_state_ = LOADING_FAILED; On 2017/01/31 23:19:49, Pete Williamson wrote: > RETRIABLE? Yes... The one time i didn't wait for compile to finish before sending it -__-'
lgtm
From lunch conversation, we've decided that it's probably useful to show the user some error pages rather than simply telling them that loading failed. These are pages for invalid scheme (they typed htp rather than http), name resolution failure (they misspelled something), SSL error, etc. I've moved some of them over in the big switch statement. Please take a look for completeness. The full list of net errors is located at https://cs.chromium.org/chromium/src/net/base/net_errors.h?type=cs&q=net::err...
The CQ bit was checked by chili@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from fgorski@chromium.org, petewil@chromium.org Link to the patchset: https://codereview.chromium.org/2656763002/#ps60001 (title: "move some errors to non-catch, so we can notify user")
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_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
https://codereview.chromium.org/2656763002/diff/60001/chrome/browser/android/... File chrome/browser/android/offline_pages/background_loader_offliner.cc (right): https://codereview.chromium.org/2656763002/diff/60001/chrome/browser/android/... chrome/browser/android/offline_pages/background_loader_offliner.cc:137: if (page_load_state_ == RETRIABLE) can you inline with ?: https://codereview.chromium.org/2656763002/diff/60001/chrome/browser/android/... chrome/browser/android/offline_pages/background_loader_offliner.cc:139: completion_callback_.Run( nit: one line https://codereview.chromium.org/2656763002/diff/60001/chrome/browser/android/... chrome/browser/android/offline_pages/background_loader_offliner.cc:216: // For these errors, we want to show user the error page so we will end with success? Do you want to snapshot error page here?
still LGTM
Offline discussion capture: It is a probably a good thing to let user know the Download failed due to [some reason]. However the specific UI for that should be mocked up/proposed by UI folks. Capturing an error page is less useful, it's very technical. We should avoid capturing the error page for now, and keep the "Download Failed" notifications w/o error page captured/saved in Download Home. At the same time, ask Tal/Rachel (via a bug?) to come up with the way to tell users what exactly happened, if needed. ---- I think as a result, this patch should be updated to always avoid capturing an error page for now, and only determine if it should be retried or not.
The CQ bit was checked by chili@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...
please add a bug number to this change.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2656763002/diff/60001/chrome/browser/android/... File chrome/browser/android/offline_pages/background_loader_offliner.cc (right): https://codereview.chromium.org/2656763002/diff/60001/chrome/browser/android/... chrome/browser/android/offline_pages/background_loader_offliner.cc:137: if (page_load_state_ == RETRIABLE) On 2017/02/07 21:42:23, fgorski wrote: > can you inline with ?: Done. https://codereview.chromium.org/2656763002/diff/60001/chrome/browser/android/... chrome/browser/android/offline_pages/background_loader_offliner.cc:139: completion_callback_.Run( On 2017/02/07 21:42:23, fgorski wrote: > nit: one line Done. https://codereview.chromium.org/2656763002/diff/60001/chrome/browser/android/... chrome/browser/android/offline_pages/background_loader_offliner.cc:216: // For these errors, we want to show user the error page On 2017/02/07 21:42:23, fgorski wrote: > so we will end with success? > Do you want to snapshot error page here? Before: yes. Per offline discussion and dmitry's comment, we will just fail for now and ask Tal to think of better way to surface these items
Description was changed from ========== [Offline pages] Add navigation error handling to background loader. This ensures that we don't try to download any net error pages. BUG= ========== to ========== [Offline pages] Add navigation error handling to background loader. This ensures that we don't try to download any net error pages. BUG=691141 ==========
On 2017/02/10 23:36:30, fgorski wrote: > please add a bug number to this change. done
The CQ bit was checked by chili@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from petewil@chromium.org, fgorski@chromium.org Link to the patchset: https://codereview.chromium.org/2656763002/#ps80001 (title: "cl comments and format")
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": 1487024468753800,
"parent_rev": "6da27d806a2386ab7baacf5692727ffa0a7706ca", "commit_rev":
"91941581b49f242755c8246fb788ec2ceeee65bd"}
Message was sent while issue was closed.
Description was changed from ========== [Offline pages] Add navigation error handling to background loader. This ensures that we don't try to download any net error pages. BUG=691141 ========== to ========== [Offline pages] Add navigation error handling to background loader. This ensures that we don't try to download any net error pages. BUG=691141 Review-Url: https://codereview.chromium.org/2656763002 Cr-Commit-Position: refs/heads/master@{#450146} Committed: https://chromium.googlesource.com/chromium/src/+/91941581b49f242755c8246fb788... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/91941581b49f242755c8246fb788... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
