Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2412823002:
Improve the page download: (Closed)
|
Created:
4 years, 2 months ago by Dmitry Titov Modified:
4 years, 2 months ago CC:
chromium-reviews, romax+watch_chromium.org, fgorski+watch_chromium.org, dewittj+watch_chromium.org, petewil+watch_chromium.org, chili+watch_chromium.org, dimich+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImprove the page download:
- observe page loading in the current tab and captrue it when it is ready directly from the tab.
- if tab renavigates/closes, pass the request back to RequestCoordinator
BUG=654955
Committed: https://crrev.com/02d38c0b50c55dadb98039dbc58a70ad2c7c8a15
Cr-Commit-Position: refs/heads/master@{#425077}
Patch Set 1 #
Total comments: 6
Patch Set 2 : merge #
Total comments: 28
Patch Set 3 : CR feedback and fix test #Patch Set 4 : fix compile #
Total comments: 1
Messages
Total messages: 33 (21 generated)
The CQ bit was checked by dimich@chromium.org to run a CQ dry run
dimich@chromium.org changed reviewers: + petewil@chromium.org
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-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...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
dimich@chromium.org changed reviewers: + dewittj@chromium.org
+Justin to look at offline_page_download_bridge.cc - some renaming and logic change.
The CQ bit was checked by dimich@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-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
cool patch, I think this is a great change. https://codereview.chromium.org/2412823002/diff/20001/chrome/browser/android/... File chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc (left): https://codereview.chromium.org/2412823002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc:91: // at a completely different page. My comment here is not internally consistent, my preference is to s/page/URL/ here too :) https://codereview.chromium.org/2412823002/diff/20001/chrome/browser/android/... File chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc (right): https://codereview.chromium.org/2412823002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc:84: void SavePageIfStillOnSamePage(const GURL& original_url, Page is not really right here, that's why I referred to URL previously. So I still prefer the "url" in this name and in the comments because it's closer to the truth. https://codereview.chromium.org/2412823002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc:99: int64_t request_id = 0; this default is suspicious. Needs at least a comment. If it has to be zero, perhaps making it a named constant in the request coordinator or tab helper would be nice for self-documentation. https://codereview.chromium.org/2412823002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc:110: RequestCoordinator::RequestAvailability::DISABLED_FOR_OFFLINER); The comment above doesn't mention that the request is persisted but the disabled status is not persisted, might be useful context here. https://codereview.chromium.org/2412823002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc:119: if (!tab_helper) If no tab helper then perhaps this code should re-enable the save page later request. https://codereview.chromium.org/2412823002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc:380: if you're deleting blank lines may as well also delete this one :) https://codereview.chromium.org/2412823002/diff/20001/chrome/browser/android/... File chrome/browser/android/offline_pages/recent_tab_helper.cc (right): https://codereview.chromium.org/2412823002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/recent_tab_helper.cc:207: ReportSnapshotCompleted(false); nit: either replace bool with an enum class or add comments /* success= */ Also rest of file. https://codereview.chromium.org/2412823002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/recent_tab_helper.cc:254: download_info_->page_snapshot_completed_ = success; could |page_snapshot_completed_| be used instead of the parameter, just checking that down in ReportDownloadRequestStatus? https://codereview.chromium.org/2412823002/diff/20001/chrome/browser/android/... File chrome/browser/android/offline_pages/recent_tab_helper.h (right): https://codereview.chromium.org/2412823002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/recent_tab_helper.h:112: int is_page_ready_for_snapshot_; s/int/bool/ also, why move this? Causing additional review churn. https://codereview.chromium.org/2412823002/diff/20001/components/offline_page... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/2412823002/diff/20001/components/offline_page... components/offline_pages/background/request_coordinator.cc:641: if (disabled_requests_.find(request_id) == disabled_requests_.end()) this is a subtle difference and not covered by any tests; what is the point of this change?
https://codereview.chromium.org/2412823002/diff/1/chrome/browser/android/offl... File chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc (right): https://codereview.chromium.org/2412823002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc:69: void SavePageIfStillOnSamePage(const GURL& original_url, I thought our bridges were supposed to be thin pass throughs - this seems like a lot of logic in the bridge (to be fair, it was also a lot of logic before you change). https://codereview.chromium.org/2412823002/diff/1/chrome/browser/android/offl... File chrome/browser/android/offline_pages/recent_tab_helper.h (right): https://codereview.chromium.org/2412823002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/recent_tab_helper.h:120: std::unique_ptr<DownloadPageInfo> download_info_; Why not just have the struct as a memeber instead of allocating it? You could use request id of 0 to indicate that it is not capturing the current page. That might save a trip to the allocator/deallocator. https://codereview.chromium.org/2412823002/diff/1/components/offline_pages/ba... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/2412823002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator.cc:649: if (disabled_requests_.find(request_id) == disabled_requests_.end()) Ah, nice catch. https://codereview.chromium.org/2412823002/diff/20001/chrome/browser/android/... File chrome/browser/android/offline_pages/recent_tab_helper.cc (right): https://codereview.chromium.org/2412823002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/recent_tab_helper.cc:264: void RecentTabHelper::ReportDownloadRequestStatus(bool completed) { The function name doesn't represent what it does well. Maybe ReportDownloadStatusToRequestCoordinator() ? ReportStatusToRequestCoordinator() ? The function seems to have two duties, to make sure that the status makes it to the notifier, and to take the request off the disabled list. It would be great if the name could represent both of these. https://codereview.chromium.org/2412823002/diff/20001/chrome/browser/android/... File chrome/browser/android/offline_pages/recent_tab_helper_unittest.cc (right): https://codereview.chromium.org/2412823002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/recent_tab_helper_unittest.cc:314: TEST_F(RecentTabHelperTest, DownloadRequest) { Is it possible to add a test where you set up the request coordinator and then call MarkRequestCompleted() to get the RC to resume a request? Maybe that is too difficult to test here, and needs to be added to the request coordinator instead. https://codereview.chromium.org/2412823002/diff/20001/components/offline_page... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/2412823002/diff/20001/components/offline_page... components/offline_pages/background/request_coordinator.cc:641: if (disabled_requests_.find(request_id) == disabled_requests_.end()) Agree with DewittJ that a comment here would help. Suggest (both here and below) // Since the recent tab helper might call multiple times, ignore subsequent calls for a particular request_id.
The CQ bit was checked by dimich@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...
PTAL https://codereview.chromium.org/2412823002/diff/1/chrome/browser/android/offl... File chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc (right): https://codereview.chromium.org/2412823002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc:69: void SavePageIfStillOnSamePage(const GURL& original_url, On 2016/10/12 17:33:36, Pete Williamson wrote: > I thought our bridges were supposed to be thin pass throughs - this seems like a > lot of logic in the bridge (to be fair, it was also a lot of logic before you > change). Indeed. Bridges ideally should be dumb translators between languages. Unfortunately, this complexity was implemented before this patch. I think most of this should actually be in RequestCoordinator, maybe once it becomes PageCaptureJobController or something like that :) One of the reason this logic goes in foo_bridge.cc is because there is no C++ side object that would implement the 'capturing' action end-to-end. We have OfflinePageModel, RequestCoordinator, TabHelper - and there should be something coordinating all these things but we don't have it. Good future refactor. https://codereview.chromium.org/2412823002/diff/1/chrome/browser/android/offl... File chrome/browser/android/offline_pages/recent_tab_helper.h (right): https://codereview.chromium.org/2412823002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/recent_tab_helper.h:120: std::unique_ptr<DownloadPageInfo> download_info_; On 2016/10/12 17:33:36, Pete Williamson wrote: > Why not just have the struct as a memeber instead of allocating it? You could > use request id of 0 to indicate that it is not capturing the current page. That > might save a trip to the allocator/deallocator. I use null here as indicator that tab helper does not capture any page (features disabled etc). request_id of 0 is also used - as indicition that page is captured but may get any id (last_1 or if background loader is disabled for Downloads) https://codereview.chromium.org/2412823002/diff/1/components/offline_pages/ba... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/2412823002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator.cc:649: if (disabled_requests_.find(request_id) == disabled_requests_.end()) On 2016/10/12 17:33:36, Pete Williamson wrote: > Ah, nice catch. Acknowledged. https://codereview.chromium.org/2412823002/diff/20001/chrome/browser/android/... File chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc (left): https://codereview.chromium.org/2412823002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc:91: // at a completely different page. On 2016/10/12 03:27:27, dewittj wrote: > My comment here is not internally consistent, my preference is to s/page/URL/ > here too :) Same as above. https://codereview.chromium.org/2412823002/diff/20001/chrome/browser/android/... File chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc (right): https://codereview.chromium.org/2412823002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc:84: void SavePageIfStillOnSamePage(const GURL& original_url, On 2016/10/12 03:27:27, dewittj wrote: > Page is not really right here, that's why I referred to URL previously. So I > still prefer the "url" in this name and in the comments because it's closer to > the truth. "SamePage" is more established than "SameURL". For example, there is NavigationHandle::IsSamePage() that we use in TabHelper. Also, considering the fragment can change URL but not page and this is what we actually check here, it is more precise to say "same page" rather than "same URL"... Attempted to find a better name. LMKWDYT. https://codereview.chromium.org/2412823002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc:99: int64_t request_id = 0; On 2016/10/12 03:27:27, dewittj wrote: > this default is suspicious. Needs at least a comment. If it has to be zero, > perhaps making it a named constant in the request coordinator or tab helper > would be nice for self-documentation. Done. https://codereview.chromium.org/2412823002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc:110: RequestCoordinator::RequestAvailability::DISABLED_FOR_OFFLINER); On 2016/10/12 03:27:27, dewittj wrote: > The comment above doesn't mention that the request is persisted but the disabled > status is not persisted, might be useful context here. Done. https://codereview.chromium.org/2412823002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc:119: if (!tab_helper) On 2016/10/12 03:27:27, dewittj wrote: > If no tab helper then perhaps this code should re-enable the save page later > request. Done. https://codereview.chromium.org/2412823002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc:380: On 2016/10/12 03:27:27, dewittj wrote: > if you're deleting blank lines may as well also delete this one :) Restoring blank line, wasnt' deliberate. https://codereview.chromium.org/2412823002/diff/20001/chrome/browser/android/... File chrome/browser/android/offline_pages/recent_tab_helper.cc (right): https://codereview.chromium.org/2412823002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/recent_tab_helper.cc:207: ReportSnapshotCompleted(false); On 2016/10/12 03:27:27, dewittj wrote: > nit: either replace bool with an enum class or add comments /* success= */ > > Also rest of file. Done. No parameters. https://codereview.chromium.org/2412823002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/recent_tab_helper.cc:254: download_info_->page_snapshot_completed_ = success; On 2016/10/12 03:27:27, dewittj wrote: > could |page_snapshot_completed_| be used instead of the parameter, just checking > that down in ReportDownloadRequestStatus? Done. This is actually more correct, thanks! https://codereview.chromium.org/2412823002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/recent_tab_helper.cc:264: void RecentTabHelper::ReportDownloadRequestStatus(bool completed) { On 2016/10/12 17:33:36, Pete Williamson wrote: > The function name doesn't represent what it does well. Maybe > ReportDownloadStatusToRequestCoordinator() ? > ReportStatusToRequestCoordinator() ? > > The function seems to have two duties, to make sure that the status makes it to > the notifier, and to take the request off the disabled list. It would be great > if the name could represent both of these. Done. https://codereview.chromium.org/2412823002/diff/20001/chrome/browser/android/... File chrome/browser/android/offline_pages/recent_tab_helper.h (right): https://codereview.chromium.org/2412823002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/recent_tab_helper.h:112: int is_page_ready_for_snapshot_; On 2016/10/12 03:27:27, dewittj wrote: > s/int/bool/ > > also, why move this? Causing additional review churn. Done. https://codereview.chromium.org/2412823002/diff/20001/chrome/browser/android/... File chrome/browser/android/offline_pages/recent_tab_helper_unittest.cc (right): https://codereview.chromium.org/2412823002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/recent_tab_helper_unittest.cc:314: TEST_F(RecentTabHelperTest, DownloadRequest) { On 2016/10/12 17:33:36, Pete Williamson wrote: > Is it possible to add a test where you set up the request coordinator and then > call MarkRequestCompleted() to get the RC to resume a request? Maybe that is > too difficult to test here, and needs to be added to the request coordinator > instead. It should be a part of RequestCoordinator unittest... Is MarkRequestCompleted() tested there? https://codereview.chromium.org/2412823002/diff/20001/components/offline_page... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/2412823002/diff/20001/components/offline_page... components/offline_pages/background/request_coordinator.cc:641: if (disabled_requests_.find(request_id) == disabled_requests_.end()) On 2016/10/12 17:33:36, Pete Williamson wrote: > Agree with DewittJ that a comment here would help. > > Suggest (both here and below) > // Since the recent tab helper might call multiple times, ignore subsequent > calls for a particular request_id. Done. Added comment. Peter, could you consider adding a test?
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...)
lgtm https://codereview.chromium.org/2412823002/diff/20001/chrome/browser/android/... File chrome/browser/android/offline_pages/recent_tab_helper_unittest.cc (right): https://codereview.chromium.org/2412823002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/recent_tab_helper_unittest.cc:314: TEST_F(RecentTabHelperTest, DownloadRequest) { On 2016/10/12 23:11:14, Dmitry Titov wrote: > On 2016/10/12 17:33:36, Pete Williamson wrote: > > Is it possible to add a test where you set up the request coordinator and then > > call MarkRequestCompleted() to get the RC to resume a request? Maybe that is > > too difficult to test here, and needs to be added to the request coordinator > > instead. > > It should be a part of RequestCoordinator unittest... Is MarkRequestCompleted() > tested there? Yes, but the test doesn't actually test that the request is resumed, that's more of a system level test. https://codereview.chromium.org/2412823002/diff/20001/components/offline_page... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/2412823002/diff/20001/components/offline_page... components/offline_pages/background/request_coordinator.cc:641: if (disabled_requests_.find(request_id) == disabled_requests_.end()) On 2016/10/12 23:11:14, Dmitry Titov wrote: > On 2016/10/12 17:33:36, Pete Williamson wrote: > > Agree with DewittJ that a comment here would help. > > > > Suggest (both here and below) > > // Since the recent tab helper might call multiple times, ignore subsequent > > calls for a particular request_id. > > Done. Added comment. Peter, could you consider adding a test? Sure. Open bug?
On 2016/10/12 23:31:41, Pete Williamson wrote: > lgtm > > https://codereview.chromium.org/2412823002/diff/20001/chrome/browser/android/... > File chrome/browser/android/offline_pages/recent_tab_helper_unittest.cc (right): > > https://codereview.chromium.org/2412823002/diff/20001/chrome/browser/android/... > chrome/browser/android/offline_pages/recent_tab_helper_unittest.cc:314: > TEST_F(RecentTabHelperTest, DownloadRequest) { > On 2016/10/12 23:11:14, Dmitry Titov wrote: > > On 2016/10/12 17:33:36, Pete Williamson wrote: > > > Is it possible to add a test where you set up the request coordinator and > then > > > call MarkRequestCompleted() to get the RC to resume a request? Maybe that > is > > > too difficult to test here, and needs to be added to the request coordinator > > > instead. > > > > It should be a part of RequestCoordinator unittest... Is > MarkRequestCompleted() > > tested there? > > Yes, but the test doesn't actually test that the request is resumed, that's more > of a system level test. > > https://codereview.chromium.org/2412823002/diff/20001/components/offline_page... > File components/offline_pages/background/request_coordinator.cc (right): > > https://codereview.chromium.org/2412823002/diff/20001/components/offline_page... > components/offline_pages/background/request_coordinator.cc:641: if > (disabled_requests_.find(request_id) == disabled_requests_.end()) > On 2016/10/12 23:11:14, Dmitry Titov wrote: > > On 2016/10/12 17:33:36, Pete Williamson wrote: > > > Agree with DewittJ that a comment here would help. > > > > > > Suggest (both here and below) > > > // Since the recent tab helper might call multiple times, ignore subsequent > > > calls for a particular request_id. > > > > Done. Added comment. Peter, could you consider adding a test? > > Sure. Open bug? Filed, this captures both the disabled list checks and resume: https://bugs.chromium.org/p/chromium/issues/detail?id=655407
The CQ bit was checked by dimich@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.
lgtm, thanks https://codereview.chromium.org/2412823002/diff/20001/chrome/browser/android/... File chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc (right): https://codereview.chromium.org/2412823002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc:84: void SavePageIfStillOnSamePage(const GURL& original_url, On 2016/10/12 23:11:14, Dmitry Titov wrote: > On 2016/10/12 03:27:27, dewittj wrote: > > Page is not really right here, that's why I referred to URL previously. So I > > still prefer the "url" in this name and in the comments because it's closer to > > the truth. > > "SamePage" is more established than "SameURL". For example, there is > NavigationHandle::IsSamePage() that we use in TabHelper. Also, considering the > fragment can change URL but not page and this is what we actually check here, it > is more precise to say "same page" rather than "same URL"... > > Attempted to find a better name. LMKWDYT. Acknowledged. https://codereview.chromium.org/2412823002/diff/60001/chrome/browser/android/... File chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc (right): https://codereview.chromium.org/2412823002/diff/60001/chrome/browser/android/... chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc:89: int64_t request_id = OfflinePageModel::kInvalidOfflineId; is request id === offline id?
> > https://codereview.chromium.org/2412823002/diff/60001/chrome/browser/android/... > chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc:89: > int64_t request_id = OfflinePageModel::kInvalidOfflineId; > is request id === offline id? It is! When request becomes offline page, it inherits same id. They share the space and are generated same way. This is another reason to merge the database tables into one, eventually.
The CQ bit was checked by dimich@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from petewil@chromium.org Link to the patchset: https://codereview.chromium.org/2412823002/#ps60001 (title: "fix compile")
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 ========== Improve the page download: - observe page loading in the current tab and captrue it when it is ready directly from the tab. - if tab renavigates/closes, pass the request back to RequestCoordinator BUG=654955 ========== to ========== Improve the page download: - observe page loading in the current tab and captrue it when it is ready directly from the tab. - if tab renavigates/closes, pass the request back to RequestCoordinator BUG=654955 Committed: https://crrev.com/02d38c0b50c55dadb98039dbc58a70ad2c7c8a15 Cr-Commit-Position: refs/heads/master@{#425077} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/02d38c0b50c55dadb98039dbc58a70ad2c7c8a15 Cr-Commit-Position: refs/heads/master@{#425077} |
