Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2506293002:
Trigger redirect for offline pages (Closed)
|
Created:
4 years, 1 month ago by jianli Modified:
4 years, 1 month ago CC:
chili+watch_chromium.org, chromium-reviews, dewittj+watch_chromium.org, dimich+watch_chromium.org, fgorski+watch_chromium.org, petewil+watch_chromium.org, romax+watch_chromium.org, vitaliii Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionTrigger redirect for offline pages
If original request URL is found, we will use it to trigger redirect
such that offline page for final redirected URL could be served.
BUG=618716
Committed: https://crrev.com/55994375eb39e2965999474fb3612737626bed13
Cr-Commit-Position: refs/heads/master@{#433318}
Patch Set 1 #Patch Set 2 : Add test #
Total comments: 18
Patch Set 3 : Address comments #Patch Set 4 : Rebase #Patch Set 5 : Fix typo #Patch Set 6 : Fix trybot #
Messages
Total messages: 27 (16 generated)
jianli@chromium.org changed reviewers: + fgorski@chromium.org, mmenke@chromium.org
lgtm with nits and comments. https://codereview.chromium.org/2506293002/diff/20001/chrome/browser/android/... File chrome/browser/android/offline_pages/offline_page_request_job.cc (right): https://codereview.chromium.org/2506293002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_request_job.cc:581: if (!fake_headers_for_redirect_.get()) { this should be enough: if (!fake_headers_for_redirect_) { https://codereview.chromium.org/2506293002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_request_job.cc:605: if (!fake_headers_for_redirect_.get()) ditto https://codereview.chromium.org/2506293002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_request_job.cc:651: DCHECK(fake_headers_for_redirect_->IsRedirect(NULL)); nit: would nullptr work here? https://codereview.chromium.org/2506293002/diff/20001/chrome/browser/android/... File chrome/browser/android/offline_pages/offline_page_request_job_unittest.cc (right): https://codereview.chromium.org/2506293002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_request_job_unittest.cc:243: const GURL& original_url, nit: This is quirky (you know me), but could GURLs stick together?
mmenke@chromium.org changed reviewers: + mkwst@chromium.org
This LGTM. [+mkwst]: Mind doing a sanity check for me, in case I'm missing something? Basically if a page is saved for offline mode, the offline code remembers the redirect chain and the final destination, and on any future navigations to the page while offline, uses a single bogus redirect to go directly to the final destination (Using the real URL). This CL uses a 302 redirect to clear the body and a header to clear the referrer. Both of these could break things if we end up going to the real live website (Either because we go online in the middle of the redirect, or if we reload the page once we're online), but I don't think it leaks any private information in any case? https://codereview.chromium.org/2506293002/diff/20001/chrome/browser/android/... File chrome/browser/android/offline_pages/offline_page_request_job.cc (right): https://codereview.chromium.org/2506293002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_request_job.cc:578: } If you're relying on the default implementation, don't need this method. https://codereview.chromium.org/2506293002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_request_job.cc:645: net::URLRequestRedirectJob::REDIRECT_302_FOUND, Comment that a 302 is used to remove response bodies, to prevent leaking them in case this is what happens with the real sites? https://codereview.chromium.org/2506293002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_request_job.cc:646: redirected_url.spec().c_str()); Need to include gurl.h https://codereview.chromium.org/2506293002/diff/20001/chrome/browser/android/... File chrome/browser/android/offline_pages/offline_page_request_job.h (right): https://codereview.chromium.org/2506293002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_request_job.h:104: std::unique_ptr<Delegate> delegate_; include <memory> https://codereview.chromium.org/2506293002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_request_job.h:107: scoped_refptr<net::HttpResponseHeaders> fake_headers_for_redirect_; include ref_counted
https://codereview.chromium.org/2506293002/diff/20001/chrome/browser/android/... File chrome/browser/android/offline_pages/offline_page_request_job.cc (right): https://codereview.chromium.org/2506293002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_request_job.cc:578: } On 2016/11/17 18:42:26, mmenke wrote: > If you're relying on the default implementation, don't need this method. We need this because default implementation goes to URLRequestFileJob::IsRedirectResponse since OfflinePageRequestJob derives from URLRequestFileJob. Updated comment. https://codereview.chromium.org/2506293002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_request_job.cc:581: if (!fake_headers_for_redirect_.get()) { On 2016/11/17 17:37:48, fgorski wrote: > this should be enough: > > if (!fake_headers_for_redirect_) { Done. https://codereview.chromium.org/2506293002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_request_job.cc:605: if (!fake_headers_for_redirect_.get()) On 2016/11/17 17:37:48, fgorski wrote: > ditto Done. https://codereview.chromium.org/2506293002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_request_job.cc:645: net::URLRequestRedirectJob::REDIRECT_302_FOUND, On 2016/11/17 18:42:26, mmenke wrote: > Comment that a 302 is used to remove response bodies, to prevent leaking them in > case this is what happens with the real sites? Done. https://codereview.chromium.org/2506293002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_request_job.cc:646: redirected_url.spec().c_str()); On 2016/11/17 18:42:26, mmenke wrote: > Need to include gurl.h Done. https://codereview.chromium.org/2506293002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_request_job.cc:651: DCHECK(fake_headers_for_redirect_->IsRedirect(NULL)); On 2016/11/17 17:37:48, fgorski wrote: > nit: would nullptr work here? Done. https://codereview.chromium.org/2506293002/diff/20001/chrome/browser/android/... File chrome/browser/android/offline_pages/offline_page_request_job.h (right): https://codereview.chromium.org/2506293002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_request_job.h:104: std::unique_ptr<Delegate> delegate_; On 2016/11/17 18:42:26, mmenke wrote: > include <memory> Done. https://codereview.chromium.org/2506293002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_request_job.h:107: scoped_refptr<net::HttpResponseHeaders> fake_headers_for_redirect_; On 2016/11/17 18:42:26, mmenke wrote: > include ref_counted Done. https://codereview.chromium.org/2506293002/diff/20001/chrome/browser/android/... File chrome/browser/android/offline_pages/offline_page_request_job_unittest.cc (right): https://codereview.chromium.org/2506293002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_request_job_unittest.cc:243: const GURL& original_url, On 2016/11/17 17:37:48, fgorski wrote: > nit: This is quirky (you know me), but could GURLs stick together? I thought about this but decided to go with this since the order is consistent with the fields defined in SavePageParam.
The CQ bit was checked by jianli@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 checked by jianli@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org, fgorski@chromium.org Link to the patchset: https://codereview.chromium.org/2506293002/#ps80001 (title: "Fix typo")
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 jianli@chromium.org
Patchset #5 (id:80001) has been deleted
The CQ bit was checked by jianli@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org, fgorski@chromium.org Link to the patchset: https://codereview.chromium.org/2506293002/#ps100001 (title: "Fix typo")
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: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
Description was changed from ========== Trigger redirect for offline pages If original request URL is found, we will use it to trigger redirect such that offline page for final redirected URL could be served. BUG=618716 ========== to ========== Trigger redirect for offline pages If original request URL is found, we will use it to trigger redirect such that offline page for final redirected URL could be served. BUG=618716 ==========
mmenke@: I agree. The plan seems reasonable to me.
The CQ bit was checked by jianli@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org, fgorski@chromium.org Link to the patchset: https://codereview.chromium.org/2506293002/#ps120001 (title: "Fix trybot")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Trigger redirect for offline pages If original request URL is found, we will use it to trigger redirect such that offline page for final redirected URL could be served. BUG=618716 ========== to ========== Trigger redirect for offline pages If original request URL is found, we will use it to trigger redirect such that offline page for final redirected URL could be served. BUG=618716 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Trigger redirect for offline pages If original request URL is found, we will use it to trigger redirect such that offline page for final redirected URL could be served. BUG=618716 ========== to ========== Trigger redirect for offline pages If original request URL is found, we will use it to trigger redirect such that offline page for final redirected URL could be served. BUG=618716 Committed: https://crrev.com/55994375eb39e2965999474fb3612737626bed13 Cr-Commit-Position: refs/heads/master@{#433318} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/55994375eb39e2965999474fb3612737626bed13 Cr-Commit-Position: refs/heads/master@{#433318} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
