Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2245733004:
Serve offline page for online URL on disconnected or bad networks (Closed)
|
Created:
4 years, 4 months ago by jianli Modified:
4 years, 3 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, asvitkine+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. |
DescriptionServe offline page for online URL on disconnected or bad networks
This patch provides the implementation for OfflinePageRequestHandler
that may return OfflinePageRequestJob to replace the response with
offline content if needed.
The original request redirection logic in OfflinePageTabHelper is
removed.
BUG=636072
Committed: https://crrev.com/7b4c282100e0ff3ef68b7e95b520325c7e94d11b
Cr-Commit-Position: refs/heads/master@{#414577}
Patch Set 1 #Patch Set 2 : Update #
Total comments: 30
Patch Set 3 : Address feedback #
Total comments: 24
Patch Set 4 : Address more feedback #
Total comments: 23
Patch Set 5 : Address some more feedback #
Total comments: 14
Patch Set 6 : Add tests #Patch Set 7 : Address feedback from dimich #
Total comments: 2
Patch Set 8 : Rebase + add missing new test file #Patch Set 9 : Address more feedback #
Total comments: 4
Patch Set 10 : Fix NQE test cleanup #Patch Set 11 : Update unittest per feedback #Patch Set 12 : Add Java integration test #Patch Set 13 : Rebase #Patch Set 14 : Fix build #Patch Set 15 : Make opening offline download item work + fix build #Patch Set 16 : Fix junit test #
Total comments: 12
Patch Set 17 : Address feedback from fgorski + fix test #Patch Set 18 : Rebase #Patch Set 19 : Fix test #
Total comments: 3
Patch Set 20 : Update java test per final feedback #Patch Set 21 : Update comment in test #Messages
Total messages: 104 (60 generated)
jianli@chromium.org changed reviewers: + dimich@chromium.org, fgorski@chromium.org
The tests will be updated or added later.
Jian, This is a complex patch. I need you to walk me through it so that I get a better feel for what happens when chronologically. But overall I don't see big problems with it. https://codereview.chromium.org/2245733004/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java (right): https://codereview.chromium.org/2245733004/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java:2068: * Gets the URL of the web page in the tab. nit: sinle line: /** Gets the URL of the web page in the tab. */ https://codereview.chromium.org/2245733004/diff/40001/chrome/browser/android/... File chrome/browser/android/offline_pages/offline_page_request_handler.cc (right): https://codereview.chromium.org/2245733004/diff/40001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_request_handler.cc:98: NetworkState::FLAKY_NETWORK : NetworkState::SKIPPED_NETWORK_CHECK; would it make sense to continue network checks if != error instead of skipping? https://codereview.chromium.org/2245733004/diff/40001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_request_handler.cc:116: return effective_connection_type >= net::EFFECTIVE_CONNECTION_TYPE_OFFLINE && Is the effective connection type enum guaranteed to stay in order of network quality? https://codereview.chromium.org/2245733004/diff/40001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_request_handler.cc:122: void ReportRequestResult(RequestResult request_result, Consider splitting it in 2 methods, please. (But up to you) One returns AggregatedRequestResult from RequestResult and Network state. the other calls it and reports OfflinePageRequestHandler::ReportAggregatedRequestResult(result); I think it will be cleaner and you will simply have return statements. https://codereview.chromium.org/2245733004/diff/40001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_request_handler.cc:146: AggregatedRequestResult::PAGE_NOT_FOUND_ON_DISCONNECTED_NETWORK; ON_FLAKY_NETWORK https://codereview.chromium.org/2245733004/diff/40001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_request_handler.cc:183: // Called on IO thread after an offline file path is obtained. nit: tell the reader what it does, reather than when it is called (name sort of indicates that already), please. https://codereview.chromium.org/2245733004/diff/40001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_request_handler.cc:208: // Called on UI thread after an offline page is found. ditto https://codereview.chromium.org/2245733004/diff/40001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_request_handler.cc:225: base::Time::Now() - offline_page->creation_time > Add TODO: this should be policy driven I think: 1. 1 day is very arbitrary 2. Some pages survive longer. E.g. Bookmark pages will be kept for 7 days. No point doing that if we never show pages that are available that long. 3. Even when this is specific to previews, they probably want to control that time better than just in code. Probably worth raising this with Ben https://codereview.chromium.org/2245733004/diff/40001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_request_handler.cc:229: net::FileURLToFilePath(offline_page->GetOfflineURL(), &offline_file_path); move to if web_contents https://codereview.chromium.org/2245733004/diff/40001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_request_handler.cc:336: return nullptr; is default selected if this returns nullptr? https://codereview.chromium.org/2245733004/diff/40001/chrome/browser/android/... File chrome/browser/android/offline_pages/offline_page_request_handler.h (right): https://codereview.chromium.org/2245733004/diff/40001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_request_handler.h:65: AGGREGATED_REQUEST_RESULT_MAX, nit: add a comment that new values are added above this point. https://codereview.chromium.org/2245733004/diff/40001/chrome/browser/android/... File chrome/browser/android/offline_pages/offline_page_request_job.h (right): https://codereview.chromium.org/2245733004/diff/40001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_request_job.h:24: // Will be invoked before the request is restarted. The caller can use this by the caller do you mean the implementing class? https://codereview.chromium.org/2245733004/diff/40001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_request_job.h:25: // opportunity to set a sepcific state in order to prevent the request from nit: specific https://codereview.chromium.org/2245733004/diff/40001/chrome/browser/android/... File chrome/browser/android/offline_pages/offline_page_tab_helper.cc (right): https://codereview.chromium.org/2245733004/diff/40001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_tab_helper.cc:86: OfflinePageRequestHandler::ReportAggregatedRequestResult( Aren't you reporting that from inside of OfflinePageRequestHandler::GetOfflineFileURL? Perhaps one of them is not necessary? https://codereview.chromium.org/2245733004/diff/40001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_tab_helper.cc:87: AggregatedRequestResult::NO_TAB_ID); nit: in case this stays, indentation is off.
Patchset #2 (id:20001) has been deleted
https://codereview.chromium.org/2245733004/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java (right): https://codereview.chromium.org/2245733004/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java:2068: * Gets the URL of the web page in the tab. On 2016/08/15 21:38:32, fgorski wrote: > nit: sinle line: > /** Gets the URL of the web page in the tab. */ Done. https://codereview.chromium.org/2245733004/diff/40001/chrome/browser/android/... File chrome/browser/android/offline_pages/offline_page_request_handler.cc (right): https://codereview.chromium.org/2245733004/diff/40001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_request_handler.cc:98: NetworkState::FLAKY_NETWORK : NetworkState::SKIPPED_NETWORK_CHECK; On 2016/08/15 21:38:32, fgorski wrote: > would it make sense to continue network checks if != error instead of skipping? Done. https://codereview.chromium.org/2245733004/diff/40001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_request_handler.cc:116: return effective_connection_type >= net::EFFECTIVE_CONNECTION_TYPE_OFFLINE && On 2016/08/15 21:38:32, fgorski wrote: > Is the effective connection type enum guaranteed to stay in order of network > quality? I think so. https://codereview.chromium.org/2245733004/diff/40001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_request_handler.cc:122: void ReportRequestResult(RequestResult request_result, On 2016/08/15 21:38:32, fgorski wrote: > Consider splitting it in 2 methods, please. (But up to you) > > One returns AggregatedRequestResult from RequestResult and Network state. > > the other calls it and reports > OfflinePageRequestHandler::ReportAggregatedRequestResult(result); > > I think it will be cleaner and you will simply have return statements. Done. https://codereview.chromium.org/2245733004/diff/40001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_request_handler.cc:146: AggregatedRequestResult::PAGE_NOT_FOUND_ON_DISCONNECTED_NETWORK; On 2016/08/15 21:38:32, fgorski wrote: > ON_FLAKY_NETWORK Done. https://codereview.chromium.org/2245733004/diff/40001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_request_handler.cc:183: // Called on IO thread after an offline file path is obtained. On 2016/08/15 21:38:32, fgorski wrote: > nit: tell the reader what it does, reather than when it is called (name sort of > indicates that already), please. Done. https://codereview.chromium.org/2245733004/diff/40001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_request_handler.cc:208: // Called on UI thread after an offline page is found. On 2016/08/15 21:38:32, fgorski wrote: > ditto Done. https://codereview.chromium.org/2245733004/diff/40001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_request_handler.cc:225: base::Time::Now() - offline_page->creation_time > On 2016/08/15 21:38:32, fgorski wrote: > Add TODO: this should be policy driven I think: > 1. 1 day is very arbitrary > 2. Some pages survive longer. E.g. Bookmark pages will be kept for 7 days. No > point doing that if we never show pages that are available that long. > 3. Even when this is specific to previews, they probably want to control that > time better than just in code. Probably worth raising this with Ben This is ported from existing logic in OfflinePageTabHelper. Added a TODO to make this policy driven. https://codereview.chromium.org/2245733004/diff/40001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_request_handler.cc:229: net::FileURLToFilePath(offline_page->GetOfflineURL(), &offline_file_path); On 2016/08/15 21:38:32, fgorski wrote: > move to if web_contents Done. https://codereview.chromium.org/2245733004/diff/40001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_request_handler.cc:336: return nullptr; On 2016/08/15 21:38:32, fgorski wrote: > is default selected if this returns nullptr? Yes. https://codereview.chromium.org/2245733004/diff/40001/chrome/browser/android/... File chrome/browser/android/offline_pages/offline_page_request_handler.h (right): https://codereview.chromium.org/2245733004/diff/40001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_request_handler.h:65: AGGREGATED_REQUEST_RESULT_MAX, On 2016/08/15 21:38:32, fgorski wrote: > nit: add a comment that new values are added above this point. Such comment is already added at line 48. https://codereview.chromium.org/2245733004/diff/40001/chrome/browser/android/... File chrome/browser/android/offline_pages/offline_page_request_job.h (right): https://codereview.chromium.org/2245733004/diff/40001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_request_job.h:24: // Will be invoked before the request is restarted. The caller can use this On 2016/08/15 21:38:32, fgorski wrote: > by the caller do you mean the implementing class? Updated. https://codereview.chromium.org/2245733004/diff/40001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_request_job.h:25: // opportunity to set a sepcific state in order to prevent the request from On 2016/08/15 21:38:32, fgorski wrote: > nit: specific Done. https://codereview.chromium.org/2245733004/diff/40001/chrome/browser/android/... File chrome/browser/android/offline_pages/offline_page_tab_helper.cc (right): https://codereview.chromium.org/2245733004/diff/40001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_tab_helper.cc:86: OfflinePageRequestHandler::ReportAggregatedRequestResult( On 2016/08/15 21:38:32, fgorski wrote: > Aren't you reporting that from inside of > OfflinePageRequestHandler::GetOfflineFileURL? Perhaps one of them is not > necessary? You're right. We should have already complain about this in request handler. Removed this one. https://codereview.chromium.org/2245733004/diff/40001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_tab_helper.cc:87: AggregatedRequestResult::NO_TAB_ID); On 2016/08/15 21:38:32, fgorski wrote: > nit: in case this stays, indentation is off. Done.
Initial feedback (partial). You will need someone good familiar with Net layer to look at it as well at some point... https://codereview.chromium.org/2245733004/diff/60001/chrome/browser/android/... File chrome/browser/android/offline_pages/offline_page_request_handler.cc (right): https://codereview.chromium.org/2245733004/diff/60001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_request_handler.cc:76: NetworkState GetNetworkState(net::URLRequest* request) { This can be split into several functions and then main one calling those. It's too big otherwise. https://codereview.chromium.org/2245733004/diff/60001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_request_handler.cc:221: if (offline_page) { early return would reduce the nesting level here. https://codereview.chromium.org/2245733004/diff/60001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_request_handler.cc:228: request_result = RequestResult::PAGE_NOT_FRESH; It would be probbaly more readable if this code had structure: if (cond1) { .. ReportRequestResult(...); return; } if (cond2) { .... https://codereview.chromium.org/2245733004/diff/60001/chrome/browser/android/... File chrome/browser/android/offline_pages/offline_page_request_handler.h (right): https://codereview.chromium.org/2245733004/diff/60001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_request_handler.h:26: enum class NetworkState { Some of values here are not the network state but rather presence of a header etc. Perhaps the better name would be ReasonToUseOfflinePage, with CONNECTED_NETWORK -> LOAD_ORIGINAL_PAGE? Or mayeb LoadDesision... Also, is there any reason not to have it as internal in the OfflinePageRequestHandler? https://codereview.chromium.org/2245733004/diff/60001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_request_handler.h:72: can remove this empty line. https://codereview.chromium.org/2245733004/diff/60001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_request_handler.h:146: NetworkState network_state_; I see why you made this a member, you don't want to re-compute it again later. Is this actually that slow to compute? If you compute it again when you created an instance of the class, would it be something noticeable on perf? If not, we can make code simpler and avoid optimization that is not yet necessary.
jianli@chromium.org changed reviewers: + mmenke@chromium.org
mmenke for reviewing url request handling and interception
https://codereview.chromium.org/2245733004/diff/60001/chrome/browser/android/... File chrome/browser/android/offline_pages/offline_page_request_handler.cc (right): https://codereview.chromium.org/2245733004/diff/60001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_request_handler.cc:292: new OfflinePageRequestHandler(network_state)); Rather than attach an object that does stuff to the URLRequest, I'd suggest just attaching a POD type with the data you need. Scope all the extra stuff you need to do to the URLRequestJob subclass, as that's nicely destroyed between redirects, promptly Killed() on cancellation, etc. Then I'd move all the extra logic to do stuff into the job's code, instead of the handler, and do it all on Start(), so you don't have to worry about Start() not being called instantly. https://codereview.chromium.org/2245733004/diff/60001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_request_handler.cc:351: base::Bind(&OfflinePageRequestHandler::GetOfflineFileURL, This do-two-things-at-once behavior seems weird to me. You now have two cases to worry about: The job is started first, or we get the offline file URL first. Currently, we always start a job just after creating it, but seems like we shouldn't rely on it, given that it's a separate method, with no documented guarantee. See earlier comment about a suggested redesign. https://codereview.chromium.org/2245733004/diff/60001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_request_handler.cc:375: content::WebContents* web_contents = web_contents_getter.Run(); web_contents_getter can return nullptr - the tab could have been closed since the request was issued. https://codereview.chromium.org/2245733004/diff/60001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_request_handler.cc:399: NotifyOfflineFilePathOnUI(offline_job_, empty_file_path); BUG: we're on the UI thread, this class is owned by the URLRequest, which lives on the IO thread, and can be deleted at any time. accessing offline_job_ here is not safe. I'd recommend you make all methods called on the UI thread not a member of any class (Or members of a private, ref-counted class, though I don't think that's needed), to avoid this sort of issue. https://codereview.chromium.org/2245733004/diff/60001/chrome/browser/android/... File chrome/browser/android/offline_pages/offline_page_request_job.cc (right): https://codereview.chromium.org/2245733004/diff/60001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_request_job.cc:61: URLRequestFileJob::Start(); Seems like we should have protection against calling this before Start(). We really don't want to call NotifyHeadersComplete() before this job is started. Same goes for FallbackToDefault https://codereview.chromium.org/2245733004/diff/60001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_request_job.cc:81: NotifyHeadersComplete(); We don't have DCHECKs for it (We really should), but you must not call URLRequest::NotifyHeadersCompleted() synchronously in response to a call to Start().
https://codereview.chromium.org/2245733004/diff/60001/chrome/browser/android/... File chrome/browser/android/offline_pages/offline_page_request_handler.cc (right): https://codereview.chromium.org/2245733004/diff/60001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_request_handler.cc:76: NetworkState GetNetworkState(net::URLRequest* request) { On 2016/08/16 20:02:55, Dmitry Titov wrote: > This can be split into several functions and then main one calling those. It's > too big otherwise. Done. https://codereview.chromium.org/2245733004/diff/60001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_request_handler.cc:221: if (offline_page) { On 2016/08/16 20:02:55, Dmitry Titov wrote: > early return would reduce the nesting level here. Done. https://codereview.chromium.org/2245733004/diff/60001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_request_handler.cc:228: request_result = RequestResult::PAGE_NOT_FRESH; On 2016/08/16 20:02:55, Dmitry Titov wrote: > It would be probbaly more readable if this code had structure: > if (cond1) { > .. > ReportRequestResult(...); > return; > } > > if (cond2) { > .... Done. https://codereview.chromium.org/2245733004/diff/60001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_request_handler.cc:292: new OfflinePageRequestHandler(network_state)); On 2016/08/17 17:09:28, mmenke wrote: > Rather than attach an object that does stuff to the URLRequest, I'd suggest just > attaching a POD type with the data you need. Scope all the extra stuff you need > to do to the URLRequestJob subclass, as that's nicely destroyed between > redirects, promptly Killed() on cancellation, etc. Then I'd move all the extra > logic to do stuff into the job's code, instead of the handler, and do it all on > Start(), so you don't have to worry about Start() not being called instantly. Done. https://codereview.chromium.org/2245733004/diff/60001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_request_handler.cc:351: base::Bind(&OfflinePageRequestHandler::GetOfflineFileURL, On 2016/08/17 17:09:28, mmenke wrote: > This do-two-things-at-once behavior seems weird to me. You now have two cases > to worry about: The job is started first, or we get the offline file URL first. > Currently, we always start a job just after creating it, but seems like we > shouldn't rely on it, given that it's a separate method, with no documented > guarantee. See earlier comment about a suggested redesign. Done. https://codereview.chromium.org/2245733004/diff/60001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_request_handler.cc:375: content::WebContents* web_contents = web_contents_getter.Run(); On 2016/08/17 17:09:28, mmenke wrote: > web_contents_getter can return nullptr - the tab could have been closed since > the request was issued. Done. https://codereview.chromium.org/2245733004/diff/60001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_request_handler.cc:399: NotifyOfflineFilePathOnUI(offline_job_, empty_file_path); On 2016/08/17 17:09:28, mmenke wrote: > BUG: we're on the UI thread, this class is owned by the URLRequest, which lives > on the IO thread, and can be deleted at any time. accessing offline_job_ here > is not safe. I'd recommend you make all methods called on the UI thread not a > member of any class (Or members of a private, ref-counted class, though I don't > think that's needed), to avoid this sort of issue. Done. https://codereview.chromium.org/2245733004/diff/60001/chrome/browser/android/... File chrome/browser/android/offline_pages/offline_page_request_handler.h (right): https://codereview.chromium.org/2245733004/diff/60001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_request_handler.h:26: enum class NetworkState { On 2016/08/16 20:02:55, Dmitry Titov wrote: > Some of values here are not the network state but rather presence of a header > etc. Perhaps the better name would be ReasonToUseOfflinePage, with > CONNECTED_NETWORK -> LOAD_ORIGINAL_PAGE? Or mayeb LoadDesision... Also, is there > any reason not to have it as internal in the OfflinePageRequestHandler? The only value is FORCE_OFFLINE_ON_CONNECTED_NETWORK. Also CONNECTED_NETWORK really means for connected network which we don't want to bring up the offline page. I think ReasonToUseOfflinePage is more confusing than NetworkState. Even when the header is present to force loading offline page, we still check network conditions. If disconnected, network state will be DISCONNECTED_NETWORK. Changed to move this inside .cc file. https://codereview.chromium.org/2245733004/diff/60001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_request_handler.h:72: On 2016/08/16 20:02:55, Dmitry Titov wrote: > can remove this empty line. Done. https://codereview.chromium.org/2245733004/diff/60001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_request_handler.h:146: NetworkState network_state_; On 2016/08/16 20:02:55, Dmitry Titov wrote: > I see why you made this a member, you don't want to re-compute it again later. > Is this actually that slow to compute? If you compute it again when you created > an instance of the class, would it be something noticeable on perf? If not, we > can make code simpler and avoid optimization that is not yet necessary. I don't think this is slow to compute. With refactoring per mmneke's feedback, I don't need to cache it any more. https://codereview.chromium.org/2245733004/diff/60001/chrome/browser/android/... File chrome/browser/android/offline_pages/offline_page_request_job.cc (right): https://codereview.chromium.org/2245733004/diff/60001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_request_job.cc:61: URLRequestFileJob::Start(); On 2016/08/17 17:09:28, mmenke wrote: > Seems like we should have protection against calling this before Start(). We > really don't want to call NotifyHeadersComplete() before this job is started. > Same goes for FallbackToDefault Fixed. https://codereview.chromium.org/2245733004/diff/60001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_request_job.cc:81: NotifyHeadersComplete(); On 2016/08/17 17:09:28, mmenke wrote: > We don't have DCHECKs for it (We really should), but you must not call > URLRequest::NotifyHeadersCompleted() synchronously in response to a call to > Start(). Fixed.
https://codereview.chromium.org/2245733004/diff/80001/chrome/browser/android/... File chrome/browser/android/offline_pages/offline_page_request_interceptor.cc (right): https://codereview.chromium.org/2245733004/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_request_interceptor.cc:22: return OfflinePageRequestJob::Create(profile_id_, request, network_delegate); I don't think we need to intercept every single request. That slows down every request made by the network just for this feature, even if it typically won't be much. Could we early reject most requests pages? https://codereview.chromium.org/2245733004/diff/80001/chrome/browser/android/... File chrome/browser/android/offline_pages/offline_page_request_job.cc (right): https://codereview.chromium.org/2245733004/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_request_job.cc:65: int kUserDataKey; // Only address is used. +const https://codereview.chromium.org/2245733004/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_request_job.cc:68: struct OfflinePageRequestInfo : public base::SupportsUserData::Data { A struct inheriting from a class seems weird to me. https://codereview.chromium.org/2245733004/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_request_job.cc:71: bool use_default; nit: This should be after the methods, I believe. https://codereview.chromium.org/2245733004/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_request_job.cc:73: OfflinePageRequestInfo() : use_default(false) {} Classes that use inheritance Should have virtual destructors. https://codereview.chromium.org/2245733004/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_request_job.cc:83: bool ParseOfflineHeader(net::URLRequest* request, std::string* reason) { include <string> https://codereview.chromium.org/2245733004/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_request_job.cc:437: info->use_default = true; Might we want to try to intercept again after being redirected? https://codereview.chromium.org/2245733004/diff/80001/chrome/browser/profiles... File chrome/browser/profiles/profile_impl_io_data.cc (right): https://codereview.chromium.org/2245733004/diff/80001/chrome/browser/profiles... chrome/browser/profiles/profile_impl_io_data.cc:535: request_interceptors.push_back(std::unique_ptr<net::URLRequestInterceptor>( Potential Bug: ServiceWorker. We don't want offline pages to go through a site's ServiceWorker code, which can result in Bad Things (A site caching the offline version of the page, for instance). I'm not all that familiar with ServiceWorker code, so not sure the best way to avoid it. The way ServiceWorker works is a we create a request as normal, but then ServiceWorker's interceptor intercepts that request, sends it to a a renderer, which then may (Or may not) issue another request for the resource, which goes over the network. If we can intercept the initial request, that's fine. If we intercept the second request, we're in trouble. I think ServiceWorker will pass the ResourceType on through, so checking that isn't sufficient, but I'm not positive. https://codereview.chromium.org/2245733004/diff/80001/chrome/browser/renderer... File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc (right): https://codereview.chromium.org/2245733004/diff/80001/chrome/browser/renderer... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:502: request, resource_type); I don't think any code is needed here at all. The OfflinePageRequestJob can grab resource type from ResourceRequestInfo::ForRequest. That means we just need one piece of code to hook up the offline stuff to the network stack.
https://codereview.chromium.org/2245733004/diff/80001/chrome/browser/android/... File chrome/browser/android/offline_pages/offline_page_request_interceptor.cc (right): https://codereview.chromium.org/2245733004/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_request_interceptor.cc:22: return OfflinePageRequestJob::Create(profile_id_, request, network_delegate); On 2016/08/19 15:04:09, mmenke wrote: > I don't think we need to intercept every single request. That slows down every > request made by the network just for this feature, even if it typically won't be > much. Could we early reject most requests pages? I've already put the rejection logic in OfflinePageRequestJob::Create which may return nullptr. Added comment to make this clear. https://codereview.chromium.org/2245733004/diff/80001/chrome/browser/android/... File chrome/browser/android/offline_pages/offline_page_request_job.cc (right): https://codereview.chromium.org/2245733004/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_request_job.cc:65: int kUserDataKey; // Only address is used. On 2016/08/19 15:04:09, mmenke wrote: > +const I can't add const. The compiler will complain about it: uninitialized const 'offline_pages::{anonymous}::kUserDataKey' [-fpermissive] https://codereview.chromium.org/2245733004/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_request_job.cc:68: struct OfflinePageRequestInfo : public base::SupportsUserData::Data { On 2016/08/19 15:04:09, mmenke wrote: > A struct inheriting from a class seems weird to me. URLRerquest::SetUserData only takes an object inherited from SupportsUserData::Data, not POD. The reason for using struct is to avoid adding getters. https://codereview.chromium.org/2245733004/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_request_job.cc:71: bool use_default; On 2016/08/19 15:04:09, mmenke wrote: > nit: This should be after the methods, I believe. Done. https://codereview.chromium.org/2245733004/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_request_job.cc:73: OfflinePageRequestInfo() : use_default(false) {} On 2016/08/19 15:04:09, mmenke wrote: > Classes that use inheritance Should have virtual destructors. Done. https://codereview.chromium.org/2245733004/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_request_job.cc:83: bool ParseOfflineHeader(net::URLRequest* request, std::string* reason) { On 2016/08/19 15:04:09, mmenke wrote: > include <string> Done. https://codereview.chromium.org/2245733004/diff/80001/chrome/browser/profiles... File chrome/browser/profiles/profile_impl_io_data.cc (right): https://codereview.chromium.org/2245733004/diff/80001/chrome/browser/profiles... chrome/browser/profiles/profile_impl_io_data.cc:535: request_interceptors.push_back(std::unique_ptr<net::URLRequestInterceptor>( On 2016/08/19 15:04:09, mmenke wrote: > Potential Bug: ServiceWorker. We don't want offline pages to go through a > site's ServiceWorker code, which can result in Bad Things (A site caching the > offline version of the page, for instance). I'm not all that familiar with > ServiceWorker code, so not sure the best way to avoid it. > > The way ServiceWorker works is a we create a request as normal, but then > ServiceWorker's interceptor intercepts that request, sends it to a a renderer, > which then may (Or may not) issue another request for the resource, which goes > over the network. If we can intercept the initial request, that's fine. If we > intercept the second request, we're in trouble. > > I think ServiceWorker will pass the ResourceType on through, so checking that > isn't sufficient, but I'm not positive. I think ServiceWorker's interceptor will kick in before offline page interceptor. The 2nd request should have ResourceType set which will be used to filter out from offline interception. I will run some tests to verify this is the case. https://codereview.chromium.org/2245733004/diff/80001/chrome/browser/renderer... File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc (right): https://codereview.chromium.org/2245733004/diff/80001/chrome/browser/renderer... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:502: request, resource_type); On 2016/08/19 15:04:09, mmenke wrote: > I don't think any code is needed here at all. The OfflinePageRequestJob can > grab resource type from ResourceRequestInfo::ForRequest. That means we just > need one piece of code to hook up the offline stuff to the network stack. Thanks for the tip. Done.
https://codereview.chromium.org/2245733004/diff/80001/chrome/browser/android/... File chrome/browser/android/offline_pages/offline_page_request_job.cc (right): https://codereview.chromium.org/2245733004/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_request_job.cc:437: info->use_default = true; On 2016/08/19 15:04:09, mmenke wrote: > Might we want to try to intercept again after being redirected? No plan.
https://codereview.chromium.org/2245733004/diff/80001/chrome/browser/profiles... File chrome/browser/profiles/profile_impl_io_data.cc (right): https://codereview.chromium.org/2245733004/diff/80001/chrome/browser/profiles... chrome/browser/profiles/profile_impl_io_data.cc:535: request_interceptors.push_back(std::unique_ptr<net::URLRequestInterceptor>( On 2016/08/19 19:34:56, jianli wrote: > On 2016/08/19 15:04:09, mmenke wrote: > > Potential Bug: ServiceWorker. We don't want offline pages to go through a > > site's ServiceWorker code, which can result in Bad Things (A site caching the > > offline version of the page, for instance). I'm not all that familiar with > > ServiceWorker code, so not sure the best way to avoid it. > > > > The way ServiceWorker works is a we create a request as normal, but then > > ServiceWorker's interceptor intercepts that request, sends it to a a renderer, > > which then may (Or may not) issue another request for the resource, which goes > > over the network. If we can intercept the initial request, that's fine. If > we > > intercept the second request, we're in trouble. > > > > I think ServiceWorker will pass the ResourceType on through, so checking that > > isn't sufficient, but I'm not positive. > > I think ServiceWorker's interceptor will kick in before offline page > interceptor. The 2nd request should have ResourceType set which will be used to > filter out from offline interception. > > I will run some tests to verify this is the case. > > I think the ResourceType will be the same as the original - i.e., if we think it's for the main frame, it will still be RESOURCE_TYPE_MAIN_FRAME (Note that RESOURCE_TYPE_SERVICE_WORKER is just for the service worker's JS). Anyhow, manual testing sounds good to me, but I think we should also have browser tests for it.
On Fri, Aug 19, 2016 at 12:43 PM, <mmenke@chromium.org> wrote: > > https://codereview.chromium.org/2245733004/diff/80001/ > chrome/browser/profiles/profile_impl_io_data.cc > File chrome/browser/profiles/profile_impl_io_data.cc (right): > > https://codereview.chromium.org/2245733004/diff/80001/ > chrome/browser/profiles/profile_impl_io_data.cc#newcode535 > chrome/browser/profiles/profile_impl_io_data.cc:535: > request_interceptors.push_back(std::unique_ptr<net:: > URLRequestInterceptor>( > On 2016/08/19 19:34:56, jianli wrote: > > On 2016/08/19 15:04:09, mmenke wrote: > > > Potential Bug: ServiceWorker. We don't want offline pages to go > through a > > > site's ServiceWorker code, which can result in Bad Things (A site > caching the > > > offline version of the page, for instance). I'm not all that > familiar with > > > ServiceWorker code, so not sure the best way to avoid it. > > > > > > The way ServiceWorker works is a we create a request as normal, but > then > > > ServiceWorker's interceptor intercepts that request, sends it to a a > renderer, > > > which then may (Or may not) issue another request for the resource, > which goes > > > over the network. If we can intercept the initial request, that's > fine. If > > we > > > intercept the second request, we're in trouble. > > > > > > I think ServiceWorker will pass the ResourceType on through, so > checking that > > > isn't sufficient, but I'm not positive. > > > > I think ServiceWorker's interceptor will kick in before offline page > > interceptor. The 2nd request should have ResourceType set which will > be used to > > filter out from offline interception. > > > > I will run some tests to verify this is the case. > > > > > > I think the ResourceType will be the same as the original - i.e., if we > think it's for the main frame, it will still be RESOURCE_TYPE_MAIN_FRAME > (Note that RESOURCE_TYPE_SERVICE_WORKER is just for the service worker's > JS). > > Anyhow, manual testing sounds good to me, but I think we should also > have browser tests for it. > > https://codereview.chromium.org/2245733004/ > I am adding unittest for it. -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Nice progress! https://codereview.chromium.org/2245733004/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java (right): https://codereview.chromium.org/2245733004/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java:2068: private String getOnlineUrlFromTab() { I wonder if this method deserves to exist, it's pretty much the same as getCurrentTab.getUrl() now and subsequent readers will be puzzled why it is called getOnlineUrlFromTab... https://codereview.chromium.org/2245733004/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/pageinfo/WebsiteSettingsPopup.java (right): https://codereview.chromium.org/2245733004/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/pageinfo/WebsiteSettingsPopup.java:304: private WebsiteSettingsPopup(Activity activity, Profile profile, WebContents webContents, While editing signature of the method, could you update the comment above to include all parameters? https://codereview.chromium.org/2245733004/diff/100001/chrome/browser/android... File chrome/browser/android/offline_pages/offline_page_request_job.cc (right): https://codereview.chromium.org/2245733004/diff/100001/chrome/browser/android... chrome/browser/android/offline_pages/offline_page_request_job.cc:190: return AggregatedRequestResult::SHOW_OFFLINE_ON_CONNECTED_NETWORK; Could you add a category here, for FORCE_OFFLINE_ON_CONNECTED_NETWORK? This is "download open", will be interesting to see. https://codereview.chromium.org/2245733004/diff/100001/chrome/browser/android... File chrome/browser/android/offline_pages/offline_page_request_job.h (right): https://codereview.chromium.org/2245733004/diff/100001/chrome/browser/android... chrome/browser/android/offline_pages/offline_page_request_job.h:63: // offline page cannot be served. "cannot or should not be served" would better describe it. https://codereview.chromium.org/2245733004/diff/100001/chrome/browser/android... File chrome/browser/android/offline_pages/offline_page_tab_helper.cc (right): https://codereview.chromium.org/2245733004/diff/100001/chrome/browser/android... chrome/browser/android/offline_pages/offline_page_tab_helper.cc:57: reloading_url_on_net_error_ = false; Would it be better to reset reloading_url_on_net_error_ in DidStartNavigation? It's not very obvious that you are only and always called 1 or 2 times here. https://codereview.chromium.org/2245733004/diff/100001/chrome/browser/android... chrome/browser/android/offline_pages/offline_page_tab_helper.cc:88: // No need to report NO_TAB_ID since it should have already been detected I don't understand that comment. It says no need to report right after it's been reported. https://codereview.chromium.org/2245733004/diff/100001/chrome/browser/android... chrome/browser/android/offline_pages/offline_page_tab_helper.cc:104: if (!offline_page) Do we loose some UMA counts here? This seems to be one of the "NOT_FOUND" variety.
https://codereview.chromium.org/2245733004/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java (right): https://codereview.chromium.org/2245733004/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java:2068: private String getOnlineUrlFromTab() { On 2016/08/20 01:08:13, Dmitry Titov wrote: > I wonder if this method deserves to exist, it's pretty much the same as > getCurrentTab.getUrl() now and subsequent readers will be puzzled why it is > called getOnlineUrlFromTab... I think it is needed since tab might be null. Renamed to getCurrentTabUrl since online doesn't make any sense any more. https://codereview.chromium.org/2245733004/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/pageinfo/WebsiteSettingsPopup.java (right): https://codereview.chromium.org/2245733004/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/pageinfo/WebsiteSettingsPopup.java:304: private WebsiteSettingsPopup(Activity activity, Profile profile, WebContents webContents, On 2016/08/20 01:08:14, Dmitry Titov wrote: > While editing signature of the method, could you update the comment above to > include all parameters? Done. https://codereview.chromium.org/2245733004/diff/100001/chrome/browser/android... File chrome/browser/android/offline_pages/offline_page_request_job.cc (right): https://codereview.chromium.org/2245733004/diff/100001/chrome/browser/android... chrome/browser/android/offline_pages/offline_page_request_job.cc:190: return AggregatedRequestResult::SHOW_OFFLINE_ON_CONNECTED_NETWORK; On 2016/08/20 01:08:14, Dmitry Titov wrote: > Could you add a category here, for FORCE_OFFLINE_ON_CONNECTED_NETWORK? This is > "download open", will be interesting to see. Indeed we should not hit "case NetworkState::CONNECTED_NETWORK". Removed it and add a DCHECK. https://codereview.chromium.org/2245733004/diff/100001/chrome/browser/android... File chrome/browser/android/offline_pages/offline_page_request_job.h (right): https://codereview.chromium.org/2245733004/diff/100001/chrome/browser/android... chrome/browser/android/offline_pages/offline_page_request_job.h:63: // offline page cannot be served. On 2016/08/20 01:08:14, Dmitry Titov wrote: > "cannot or should not be served" would better describe it. Done. https://codereview.chromium.org/2245733004/diff/100001/chrome/browser/android... File chrome/browser/android/offline_pages/offline_page_tab_helper.cc (right): https://codereview.chromium.org/2245733004/diff/100001/chrome/browser/android... chrome/browser/android/offline_pages/offline_page_tab_helper.cc:57: reloading_url_on_net_error_ = false; On 2016/08/20 01:08:14, Dmitry Titov wrote: > Would it be better to reset reloading_url_on_net_error_ in DidStartNavigation? > It's not very obvious that you are only and always called 1 or 2 times here. Done. https://codereview.chromium.org/2245733004/diff/100001/chrome/browser/android... chrome/browser/android/offline_pages/offline_page_tab_helper.cc:88: // No need to report NO_TAB_ID since it should have already been detected On 2016/08/20 01:08:14, Dmitry Titov wrote: > I don't understand that comment. It says no need to report right after it's been > reported. Forgot to remove the report code. Done. https://codereview.chromium.org/2245733004/diff/100001/chrome/browser/android... chrome/browser/android/offline_pages/offline_page_tab_helper.cc:104: if (!offline_page) On 2016/08/20 01:08:14, Dmitry Titov wrote: > Do we loose some UMA counts here? This seems to be one of the "NOT_FOUND" > variety. Done.
Also added tests.
jianli@chromium.org changed reviewers: + holte@chromium.org, nyquist@chromium.org
nyquist for android codes holte for histograms
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: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
https://codereview.chromium.org/2245733004/diff/80001/chrome/browser/android/... File chrome/browser/android/offline_pages/offline_page_request_job.cc (right): https://codereview.chromium.org/2245733004/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_request_job.cc:65: int kUserDataKey; // Only address is used. On 2016/08/19 19:34:56, jianli wrote: > On 2016/08/19 15:04:09, mmenke wrote: > > +const > > I can't add const. The compiler will complain about it: > uninitialized const 'offline_pages::{anonymous}::kUserDataKey' [-fpermissive] Calling something that's not const const violates the style guide. Some code does stuff like "const void* const kUserDataKey = &kUserDataKey;", other code uses 'const char kUserDataKey[] = "offline_page_key"'. I really don't care what we do, as long as we stick to the style guide. https://codereview.chromium.org/2245733004/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_request_job.cc:68: struct OfflinePageRequestInfo : public base::SupportsUserData::Data { On 2016/08/19 19:34:56, jianli wrote: > On 2016/08/19 15:04:09, mmenke wrote: > > A struct inheriting from a class seems weird to me. > > URLRerquest::SetUserData only takes an object inherited from > SupportsUserData::Data, not POD. The reason for using struct is to avoid adding > getters. I think because of inheriting from a class, and the non-virtual destructor, this fall beyond a simple POD class... But not going to fight over that, since I don't own this code. I would if this were in an area I own. https://codereview.chromium.org/2245733004/diff/140001/chrome/chrome_tests_un... File chrome/chrome_tests_unit.gypi (right): https://codereview.chromium.org/2245733004/diff/140001/chrome/chrome_tests_un... chrome/chrome_tests_unit.gypi:1661: 'browser/android/offline_pages/offline_page_request_job_unittest.cc', Looks like this is missing from the uploaded CL.
On 2016/08/19 20:14:09, jianli wrote: > On Fri, Aug 19, 2016 at 12:43 PM, <mailto:mmenke@chromium.org> wrote: > > > > > https://codereview.chromium.org/2245733004/diff/80001/ > > chrome/browser/profiles/profile_impl_io_data.cc > > File chrome/browser/profiles/profile_impl_io_data.cc (right): > > > > https://codereview.chromium.org/2245733004/diff/80001/ > > chrome/browser/profiles/profile_impl_io_data.cc#newcode535 > > chrome/browser/profiles/profile_impl_io_data.cc:535: > > request_interceptors.push_back(std::unique_ptr<net:: > > URLRequestInterceptor>( > > On 2016/08/19 19:34:56, jianli wrote: > > > On 2016/08/19 15:04:09, mmenke wrote: > > > > Potential Bug: ServiceWorker. We don't want offline pages to go > > through a > > > > site's ServiceWorker code, which can result in Bad Things (A site > > caching the > > > > offline version of the page, for instance). I'm not all that > > familiar with > > > > ServiceWorker code, so not sure the best way to avoid it. > > > > > > > > The way ServiceWorker works is a we create a request as normal, but > > then > > > > ServiceWorker's interceptor intercepts that request, sends it to a a > > renderer, > > > > which then may (Or may not) issue another request for the resource, > > which goes > > > > over the network. If we can intercept the initial request, that's > > fine. If > > > we > > > > intercept the second request, we're in trouble. > > > > > > > > I think ServiceWorker will pass the ResourceType on through, so > > checking that > > > > isn't sufficient, but I'm not positive. > > > > > > I think ServiceWorker's interceptor will kick in before offline page > > > interceptor. The 2nd request should have ResourceType set which will > > be used to > > > filter out from offline interception. > > > > > > I will run some tests to verify this is the case. > > > > > > > > > > I think the ResourceType will be the same as the original - i.e., if we > > think it's for the main frame, it will still be RESOURCE_TYPE_MAIN_FRAME > > (Note that RESOURCE_TYPE_SERVICE_WORKER is just for the service worker's > > JS). > > > > Anyhow, manual testing sounds good to me, but I think we should also > > have browser tests for it. > > > > https://codereview.chromium.org/2245733004/ > > > > I am adding unittest for it. And just a heads up that I'm probably going to insist on a pair of browser tests for this case, to protect against regressions. Otherwise, if things break a couple years from now, when ServiceWorker and Offline are both considered complete, and are in maintenance mode, I suspect I'll be the one investigating and fixing it.
In addition to new unittests, I am working on adding browser tests. Are you OK that I will add this in next patch? https://codereview.chromium.org/2245733004/diff/80001/chrome/browser/android/... File chrome/browser/android/offline_pages/offline_page_request_job.cc (right): https://codereview.chromium.org/2245733004/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_request_job.cc:65: int kUserDataKey; // Only address is used. On 2016/08/22 15:12:26, mmenke wrote: > On 2016/08/19 19:34:56, jianli wrote: > > On 2016/08/19 15:04:09, mmenke wrote: > > > +const > > > > I can't add const. The compiler will complain about it: > > uninitialized const 'offline_pages::{anonymous}::kUserDataKey' > [-fpermissive] > > Calling something that's not const const violates the style guide. Some code > does stuff like "const void* const kUserDataKey = &kUserDataKey;", other code > uses 'const char kUserDataKey[] = "offline_page_key"'. I really don't care what > we do, as long as we stick to the style guide. Thanks for the tip. Done. https://codereview.chromium.org/2245733004/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_request_job.cc:68: struct OfflinePageRequestInfo : public base::SupportsUserData::Data { On 2016/08/22 15:12:26, mmenke wrote: > On 2016/08/19 19:34:56, jianli wrote: > > On 2016/08/19 15:04:09, mmenke wrote: > > > A struct inheriting from a class seems weird to me. > > > > URLRerquest::SetUserData only takes an object inherited from > > SupportsUserData::Data, not POD. The reason for using struct is to avoid > adding > > getters. > > I think because of inheriting from a class, and the non-virtual destructor, this > fall beyond a simple POD class... But not going to fight over that, since I > don't own this code. I would if this were in an area I own. Changed struct to class. https://codereview.chromium.org/2245733004/diff/140001/chrome/chrome_tests_un... File chrome/chrome_tests_unit.gypi (right): https://codereview.chromium.org/2245733004/diff/140001/chrome/chrome_tests_un... chrome/chrome_tests_unit.gypi:1661: 'browser/android/offline_pages/offline_page_request_job_unittest.cc', On 2016/08/22 15:12:26, mmenke wrote: > Looks like this is missing from the uploaded CL. Added.
On 2016/08/22 23:20:31, jianli wrote: > In addition to new unittests, I am working on adding browser tests. Are you OK > that I will add this in next patch? I'm fine with that, but if we're going that route, can we not hook it up to ProfileIOData until we have integration testing?
https://codereview.chromium.org/2245733004/diff/180001/chrome/browser/android... File chrome/browser/android/offline_pages/offline_page_request_job_unittest.cc (right): https://codereview.chromium.org/2245733004/diff/180001/chrome/browser/android... chrome/browser/android/offline_pages/offline_page_request_job_unittest.cc:406: &profile_, request.get(), &network_delegate_)); I also recommend against this - I'd suggest hooking up the factory starting jobs through the normal process. It involves more boilerplate, but this logic flow is very different from how things are hooked up in production, and seems very regression prone (There is other testing code that does this, but it really shouldn't) https://codereview.chromium.org/2245733004/diff/180001/chrome/browser/android... chrome/browser/android/offline_pages/offline_page_request_job_unittest.cc:437: "http", std::move(protocol_handler)); I'd recommend against this approach. Think it's better to create a URLRequestInterceptingJobFactory around the URLRequestJobFactoryImpl. Not only does it not require creating another class, but it's less regression prone against future refactors of URLRequestJobFactoryImpl.
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: Try jobs failed on following builders: linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by jianli@chromium.org to run a CQ dry run
Also adding browser test https://codereview.chromium.org/2245733004/diff/180001/chrome/browser/android... File chrome/browser/android/offline_pages/offline_page_request_job_unittest.cc (right): https://codereview.chromium.org/2245733004/diff/180001/chrome/browser/android... chrome/browser/android/offline_pages/offline_page_request_job_unittest.cc:406: &profile_, request.get(), &network_delegate_)); On 2016/08/22 23:40:05, mmenke wrote: > I also recommend against this - I'd suggest hooking up the factory starting jobs > through the normal process. It involves more boilerplate, but this logic flow > is very different from how things are hooked up in production, and seems very > regression prone (There is other testing code that does this, but it really > shouldn't) Done. https://codereview.chromium.org/2245733004/diff/180001/chrome/browser/android... chrome/browser/android/offline_pages/offline_page_request_job_unittest.cc:437: "http", std::move(protocol_handler)); On 2016/08/22 23:40:05, mmenke wrote: > I'd recommend against this approach. Think it's better to create a > URLRequestInterceptingJobFactory around the URLRequestJobFactoryImpl. Not only > does it not require creating another class, but it's less regression prone > against future refactors of URLRequestJobFactoryImpl. 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: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
I've added Java integration test.
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: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
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: 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...)
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...
lgtm from offline_pages, but it looks like Matt will have final say here.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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...)
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...
histograms lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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...)
Apologies in advance, not sure when I'll get to this, working on figuring out a dev blocker.
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...
lgtm with comments. (Renaming of method can happen separately.) https://codereview.chromium.org/2245733004/diff/320001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/pageinfo/WebsiteSettingsPopup.java (right): https://codereview.chromium.org/2245733004/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/pageinfo/WebsiteSettingsPopup.java:300: * @param profile Profile of the tabthat will show the popup. nit: tab that https://codereview.chromium.org/2245733004/diff/320001/chrome/browser/android... File chrome/browser/android/offline_pages/offline_page_request_job.cc (right): https://codereview.chromium.org/2245733004/diff/320001/chrome/browser/android... chrome/browser/android/offline_pages/offline_page_request_job.cc:175: return has_offline_header ? NetworkState::FORCE_OFFLINE_ON_CONNECTED_NETWORK any header other than reason=error will trip this. At least a comment would be useful. Of course explicit check would be better. https://codereview.chromium.org/2245733004/diff/320001/chrome/browser/android... chrome/browser/android/offline_pages/offline_page_request_job.cc:267: RequestResult GetOfflineFilePath( I don't think this name reflects what the function does: * marking page accesed. * checking freshness * setting on tab helper * finally, of course getting the file path. https://codereview.chromium.org/2245733004/diff/320001/chrome/browser/android... chrome/browser/android/offline_pages/offline_page_request_job.cc:278: // |web_contents_getter| is passed from IO thread. We neeed to check if s/neeed/need/ https://codereview.chromium.org/2245733004/diff/320001/chrome/browser/android... chrome/browser/android/offline_pages/offline_page_request_job.cc:284: // If the page is being loaded on a slow network, only use the offline page move this section above the web_contents getting, as it does not use it and can exit quicker. Unless of course you care about the NO_WEB_CONTENTS error more (I wouldn't). https://codereview.chromium.org/2245733004/diff/320001/chrome/browser/android... chrome/browser/android/offline_pages/offline_page_request_job.cc:298: offline_page_model->MarkPageAccessed(offline_page->offline_id); did you check this on incognito?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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...)
The CQ bit was checked by jianli@chromium.org to run a CQ dry run
https://codereview.chromium.org/2245733004/diff/320001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/pageinfo/WebsiteSettingsPopup.java (right): https://codereview.chromium.org/2245733004/diff/320001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/pageinfo/WebsiteSettingsPopup.java:300: * @param profile Profile of the tabthat will show the popup. On 2016/08/24 21:02:36, fgorski wrote: > nit: tab that Done. https://codereview.chromium.org/2245733004/diff/320001/chrome/browser/android... File chrome/browser/android/offline_pages/offline_page_request_job.cc (right): https://codereview.chromium.org/2245733004/diff/320001/chrome/browser/android... chrome/browser/android/offline_pages/offline_page_request_job.cc:175: return has_offline_header ? NetworkState::FORCE_OFFLINE_ON_CONNECTED_NETWORK On 2016/08/24 21:02:37, fgorski wrote: > any header other than reason=error will trip this. > At least a comment would be useful. Of course explicit check would be better. Yes, this is what we want. Added comment. https://codereview.chromium.org/2245733004/diff/320001/chrome/browser/android... chrome/browser/android/offline_pages/offline_page_request_job.cc:267: RequestResult GetOfflineFilePath( On 2016/08/24 21:02:37, fgorski wrote: > I don't think this name reflects what the function does: > * marking page accesed. > * checking freshness > * setting on tab helper > * finally, of course getting the file path. Renamed to AccessOfflineFile https://codereview.chromium.org/2245733004/diff/320001/chrome/browser/android... chrome/browser/android/offline_pages/offline_page_request_job.cc:278: // |web_contents_getter| is passed from IO thread. We neeed to check if On 2016/08/24 21:02:37, fgorski wrote: > s/neeed/need/ Done. https://codereview.chromium.org/2245733004/diff/320001/chrome/browser/android... chrome/browser/android/offline_pages/offline_page_request_job.cc:284: // If the page is being loaded on a slow network, only use the offline page On 2016/08/24 21:02:37, fgorski wrote: > move this section above the web_contents getting, as it does not use it and can > exit quicker. > Unless of course you care about the NO_WEB_CONTENTS error more (I wouldn't). Moving this above will only affect the UMA reporting but the impact should be little. Personally I am more interested in knowing how often we will hit NO_WEB_CONTENTS since it will be very rare. I also think NO_WEB_CONTENTS is the basic error which should surface, instead of detailed error PAGE_NOT_FRESH. https://codereview.chromium.org/2245733004/diff/320001/chrome/browser/android... chrome/browser/android/offline_pages/offline_page_request_job.cc:298: offline_page_model->MarkPageAccessed(offline_page->offline_id); On 2016/08/24 21:02:37, fgorski wrote: > did you check this on incognito? OfflinePageRequestInterceptor will not be created for incognito mode. Added comment.
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: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
jianli@chromium.org changed reviewers: + sky@chromium.org
sky for chrome/test and chrome/browser
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 LGTM chrome/test/data has owners of *, so you don't need more for a data change.
On 2016/08/24 23:37:29, sky wrote: > chrome/browser LGTM > chrome/test/data has owners of *, so you don't need more for a data change. chrome/android - lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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...)
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: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
With test, LGTM, but I really do think you should have a couple integration tests with ServiceWorkers - they're weird enough that they're worth checking directly. https://codereview.chromium.org/2245733004/diff/380001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageRequestTest.java (right): https://codereview.chromium.org/2245733004/diff/380001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageRequestTest.java:92: assertFalse(isOfflinePage(tab)); Can we also shut down the test server here? I'm paranoid.
I will start to work on adding some tests with service worker in a separate patch. https://codereview.chromium.org/2245733004/diff/380001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageRequestTest.java (right): https://codereview.chromium.org/2245733004/diff/380001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageRequestTest.java:92: assertFalse(isOfflinePage(tab)); On 2016/08/25 19:06:09, mmenke (busy) wrote: > Can we also shut down the test server here? I'm paranoid. I think we still need to load about url from test serve at line 95. The test server is indeed shut down when tearDown is called.
I will start to work on adding some tests with service worker in a separate patch.
The CQ bit was checked by jianli@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dimich@chromium.org, holte@chromium.org, fgorski@chromium.org, sky@chromium.org, tedchoc@chromium.org Link to the patchset: https://codereview.chromium.org/2245733004/#ps380001 (title: "Fix test")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2245733004/diff/380001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageRequestTest.java (right): https://codereview.chromium.org/2245733004/diff/380001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageRequestTest.java:92: assertFalse(isOfflinePage(tab)); On 2016/08/25 19:37:35, jianli wrote: > On 2016/08/25 19:06:09, mmenke (busy) wrote: > > Can we also shut down the test server here? I'm paranoid. > > I think we still need to load about url from test serve at line 95. The test > server is indeed shut down when tearDown is called. Oh, I assumed it was an "about:..." URL, but I guess not. Can we shut down the server before we do the final navigation to the offline copy of the page, then, and check the page's contents? I consider that a more robust test than just checking a bool, which also depends on the offline logic. We could be displaying an error page or something, if the code breaks.
The CQ bit was unchecked by jianli@chromium.org
OK. Making change now and will upload a new one. On Thu, Aug 25, 2016 at 12:43 PM, <mmenke@chromium.org> wrote: > > https://codereview.chromium.org/2245733004/diff/380001/ > chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/ > OfflinePageRequestTest.java > File > chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/ > OfflinePageRequestTest.java > (right): > > https://codereview.chromium.org/2245733004/diff/380001/ > chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/ > OfflinePageRequestTest.java#newcode92 > chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/ > OfflinePageRequestTest.java:92: > assertFalse(isOfflinePage(tab)); > On 2016/08/25 19:37:35, jianli wrote: > > On 2016/08/25 19:06:09, mmenke (busy) wrote: > > > Can we also shut down the test server here? I'm paranoid. > > > > I think we still need to load about url from test serve at line 95. > The test > > server is indeed shut down when tearDown is called. > > Oh, I assumed it was an "about:..." URL, but I guess not. Can we shut > down the server before we do the final navigation to the offline copy of > the page, then, and check the page's contents? I consider that a more > robust test than just checking a bool, which also depends on the offline > logic. We could be displaying an error page or something, if the code > breaks. > > https://codereview.chromium.org/2245733004/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
mmenke, I've improved java test per your feedback.
The CQ bit was checked by jianli@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org, holte@chromium.org, fgorski@chromium.org, dimich@chromium.org, tedchoc@chromium.org, mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/2245733004/#ps400001 (title: "Update java test per final feedback")
The CQ bit was unchecked by jianli@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 checked by jianli@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org, holte@chromium.org, fgorski@chromium.org, dimich@chromium.org, tedchoc@chromium.org, mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/2245733004/#ps420001 (title: "Update comment in test")
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: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by jianli@chromium.org
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 #21 (id:420001)
Message was sent while issue was closed.
Description was changed from ========== Serve offline page for online URL on disconnected or bad networks This patch provides the implementation for OfflinePageRequestHandler that may return OfflinePageRequestJob to replace the response with offline content if needed. The original request redirection logic in OfflinePageTabHelper is removed. BUG=636072 ========== to ========== Serve offline page for online URL on disconnected or bad networks This patch provides the implementation for OfflinePageRequestHandler that may return OfflinePageRequestJob to replace the response with offline content if needed. The original request redirection logic in OfflinePageTabHelper is removed. BUG=636072 Committed: https://crrev.com/7b4c282100e0ff3ef68b7e95b520325c7e94d11b Cr-Commit-Position: refs/heads/master@{#414577} ==========
Message was sent while issue was closed.
Patchset 21 (id:??) landed as https://crrev.com/7b4c282100e0ff3ef68b7e95b520325c7e94d11b Cr-Commit-Position: refs/heads/master@{#414577} |
