Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2233443003:
Some initial hookup of offline page request interception (Closed)
|
Created:
4 years, 4 months ago by jianli Modified:
4 years, 4 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 Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSome initial hookup of offline page request interception
Add OfflinePageRequestHandler that services requests based on their
offline info. This patch contains only initial hookup. The detail
check and handling will be in next patch.
BUG=636072
Committed: https://crrev.com/aa2255830720b54f30fdbd7847035270cf85e2ce
Cr-Commit-Position: refs/heads/master@{#411406}
Patch Set 1 #
Total comments: 15
Patch Set 2 : Address feedback #
Total comments: 1
Patch Set 3 : use std::move #
Total comments: 8
Patch Set 4 : Address more feedback #
Messages
Total messages: 29 (15 generated)
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 unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
jianli@chromium.org changed reviewers: + dimich@chromium.org, fgorski@chromium.org, mmenke@chromium.org
dimich and fgorski for offline pages mmnenke for profile_io_data
A few comments. Also, is there a way to test that a request handler/interceptor was created and attached to the request? https://codereview.chromium.org/2233443003/diff/1/chrome/browser/android/offl... File chrome/browser/android/offline_pages/offline_page_request_handler.cc (right): https://codereview.chromium.org/2233443003/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/offline_page_request_handler.cc:16: int kUserDataKey; // Only address is used. Wow. https://codereview.chromium.org/2233443003/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/offline_page_request_handler.cc:57: // Ignore non-http/https requests. We don't need to handle file:// because we are switching to offline scheme? Please explain a bit. https://codereview.chromium.org/2233443003/diff/1/chrome/browser/android/offl... File chrome/browser/android/offline_pages/offline_page_request_handler.h (right): https://codereview.chromium.org/2233443003/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/offline_page_request_handler.h:22: // per URLRequest and attached to each request. Please add a little longer comment that explains the relationship between the interceptor and handler and what is the role each one of them plays in the offline request handling. https://codereview.chromium.org/2233443003/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/offline_page_request_handler.h:25: // Attaches a newly created handler if the given |request| needs to is there something missing in this sentence? If yes, please add, if not please put a "." at the end. https://codereview.chromium.org/2233443003/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/offline_page_request_handler.h:35: void* profile_id); please document profile_id and explain why it is void* I have hard time understanding this code.
https://codereview.chromium.org/2233443003/diff/1/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_io_data.cc (right): https://codereview.chromium.org/2233443003/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_io_data.cc:1172: #endif I don't think we want this in incognito mode. Move this to ProfileImplIOData::InitializeInternal?
https://codereview.chromium.org/2233443003/diff/1/chrome/browser/renderer_hos... File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc (right): https://codereview.chromium.org/2233443003/diff/1/chrome/browser/renderer_hos... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:479: request, resource_type); Also, skip in incognito mode?
https://codereview.chromium.org/2233443003/diff/1/chrome/browser/android/offl... File chrome/browser/android/offline_pages/offline_page_request_handler.cc (right): https://codereview.chromium.org/2233443003/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/offline_page_request_handler.cc:16: int kUserDataKey; // Only address is used. On 2016/08/10 16:26:09, fgorski wrote: > Wow. Acknowledged. https://codereview.chromium.org/2233443003/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/offline_page_request_handler.cc:57: // Ignore non-http/https requests. On 2016/08/10 16:26:09, fgorski wrote: > We don't need to handle file:// because we are switching to offline scheme? > Please explain a bit. We're not adding offline scheme. Instead the response for http/https URL might get replaced with offline content if needed. So we only need to hook up with http/https requests. https://codereview.chromium.org/2233443003/diff/1/chrome/browser/android/offl... File chrome/browser/android/offline_pages/offline_page_request_handler.h (right): https://codereview.chromium.org/2233443003/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/offline_page_request_handler.h:22: // per URLRequest and attached to each request. On 2016/08/10 16:26:10, fgorski wrote: > Please add a little longer comment that explains the relationship between the > interceptor and handler and what is the role each one of them plays in the > offline request handling. Done. https://codereview.chromium.org/2233443003/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/offline_page_request_handler.h:25: // Attaches a newly created handler if the given |request| needs to On 2016/08/10 16:26:10, fgorski wrote: > is there something missing in this sentence? > If yes, please add, if not please put a "." at the end. Done. https://codereview.chromium.org/2233443003/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/offline_page_request_handler.h:35: void* profile_id); On 2016/08/10 16:26:10, fgorski wrote: > please document profile_id and explain why it is void* > I have hard time understanding this code. Done. https://codereview.chromium.org/2233443003/diff/1/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_io_data.cc (right): https://codereview.chromium.org/2233443003/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_io_data.cc:1172: #endif On 2016/08/10 20:20:17, mmenke wrote: > I don't think we want this in incognito mode. Move this to > ProfileImplIOData::InitializeInternal? Done. https://codereview.chromium.org/2233443003/diff/1/chrome/browser/renderer_hos... File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc (right): https://codereview.chromium.org/2233443003/diff/1/chrome/browser/renderer_hos... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:479: request, resource_type); On 2016/08/10 20:27:51, mmenke wrote: > Also, skip in incognito mode? Done.
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...
chrome/browser/profiles, chrome/browser/renderer_host both LGTM. https://codereview.chromium.org/2233443003/diff/20001/chrome/browser/profiles... File chrome/browser/profiles/profile_impl_io_data.cc (right): https://codereview.chromium.org/2233443003/diff/20001/chrome/browser/profiles... chrome/browser/profiles/profile_impl_io_data.cc:541: request_interceptors.push_back(offline_page_interceptor.release()); Think you can use std::move(offline_page_interceptor) here
lgtm https://codereview.chromium.org/2233443003/diff/1/chrome/browser/android/offl... File chrome/browser/android/offline_pages/offline_page_request_handler.cc (right): https://codereview.chromium.org/2233443003/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/offline_page_request_handler.cc:57: // Ignore non-http/https requests. On 2016/08/10 21:37:25, jianli wrote: > On 2016/08/10 16:26:09, fgorski wrote: > > We don't need to handle file:// because we are switching to offline scheme? > > Please explain a bit. > > We're not adding offline scheme. Instead the response for http/https URL might > get replaced with offline content if needed. So we only need to hook up with > http/https requests. Acknowledged.
jianli@chromium.org changed reviewers: + thestig@chromium.org
thestig, can you review and approve changes to chrome/browser? thanks.
lgtm https://codereview.chromium.org/2233443003/diff/40001/chrome/browser/android/... File chrome/browser/android/offline_pages/offline_page_request_handler.cc (right): https://codereview.chromium.org/2233443003/diff/40001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_request_handler.cc:38: // The profile for processing offline pages. Should not be NULL. DCHECK(profile_id_) in the ctor, instead of writing "Should not be NULL." here? https://codereview.chromium.org/2233443003/diff/40001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_request_handler.cc:39: void* profile_id_; You can do: void* const profile_id_; // if you like. https://codereview.chromium.org/2233443003/diff/40001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_request_handler.cc:67: std::unique_ptr<OfflinePageRequestHandler> handler( Do you even need |handler|? Why not just SetUserData(&kUserDataKey, handler.release()); ? https://codereview.chromium.org/2233443003/diff/40001/chrome/browser/profiles... File chrome/browser/profiles/profile_impl_io_data.cc (right): https://codereview.chromium.org/2233443003/diff/40001/chrome/browser/profiles... chrome/browser/profiles/profile_impl_io_data.cc:540: if (offline_page_interceptor.get()) You can omit the ".get()"
The CQ bit was checked by jianli@chromium.org to run a CQ dry run
https://codereview.chromium.org/2233443003/diff/40001/chrome/browser/android/... File chrome/browser/android/offline_pages/offline_page_request_handler.cc (right): https://codereview.chromium.org/2233443003/diff/40001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_request_handler.cc:38: // The profile for processing offline pages. Should not be NULL. On 2016/08/10 22:40:00, Lei Zhang (Soon to be OOO) wrote: > DCHECK(profile_id_) in the ctor, instead of writing "Should not be NULL." here? Done. https://codereview.chromium.org/2233443003/diff/40001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_request_handler.cc:39: void* profile_id_; On 2016/08/10 22:40:00, Lei Zhang (Soon to be OOO) wrote: > You can do: void* const profile_id_; // if you like. Done. https://codereview.chromium.org/2233443003/diff/40001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_request_handler.cc:67: std::unique_ptr<OfflinePageRequestHandler> handler( On 2016/08/10 22:40:00, Lei Zhang (Soon to be OOO) wrote: > Do you even need |handler|? Why not just SetUserData(&kUserDataKey, > handler.release()); ? Done. https://codereview.chromium.org/2233443003/diff/40001/chrome/browser/profiles... File chrome/browser/profiles/profile_impl_io_data.cc (right): https://codereview.chromium.org/2233443003/diff/40001/chrome/browser/profiles... chrome/browser/profiles/profile_impl_io_data.cc:540: if (offline_page_interceptor.get()) On 2016/08/10 22:40:00, Lei Zhang (Soon to be OOO) wrote: > You can omit the ".get()" Done.
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.
It should not be under browser/android since it does not depend on Android specifically. Ideally it should be browser/offline_pages but I'm willing to address this later. lgtm
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, thestig@chromium.org Link to the patchset: https://codereview.chromium.org/2233443003/#ps60001 (title: "Address more feedback")
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.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Some initial hookup of offline page request interception Add OfflinePageRequestHandler that services requests based on their offline info. This patch contains only initial hookup. The detail check and handling will be in next patch. BUG=636072 ========== to ========== Some initial hookup of offline page request interception Add OfflinePageRequestHandler that services requests based on their offline info. This patch contains only initial hookup. The detail check and handling will be in next patch. BUG=636072 Committed: https://crrev.com/aa2255830720b54f30fdbd7847035270cf85e2ce Cr-Commit-Position: refs/heads/master@{#411406} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/aa2255830720b54f30fdbd7847035270cf85e2ce Cr-Commit-Position: refs/heads/master@{#411406} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
