Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2278773002:
Modify RecentTabHelper to be always-on and observe the loading of the pages. (Closed)
|
Created:
4 years, 4 months ago by Dmitry Titov 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, dimich+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionModify RecentTabHelper to be always-on and observe the loading of the pages.
Expose is_page_ready_for_snapshot() on it which is set when the page can be successfully captured.
Move the feature flag check from creating the tab helper to before the actual snapshot.
BUG=630817
Committed: https://crrev.com/9ee7a4109cbc763067538c360c36ff2e1ba3ee13
Cr-Commit-Position: refs/heads/master@{#414509}
Patch Set 1 #
Total comments: 17
Patch Set 2 : cr feedback #
Dependent Patchsets: Messages
Total messages: 22 (12 generated)
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...
dimich@chromium.org changed reviewers: + avi@chromium.org, fgorski@chromium.org
fgorski: please look at offline pages avi: please do an OWNER's check of tab_helpers.cc
dimich@chromium.org changed reviewers: + dewittj@chromium.org - fgorski@chromium.org
R:-fgorski +dewittj
https://codereview.chromium.org/2278773002/diff/1/chrome/browser/android/offl... File chrome/browser/android/offline_pages/recent_tab_helper.cc (left): https://codereview.chromium.org/2278773002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/recent_tab_helper.cc:110: weak_ptr_factory_.InvalidateWeakPtrs(); Why is this gone? It seems like there are still places that we'd want to cancel async steps. https://codereview.chromium.org/2278773002/diff/1/chrome/browser/android/offl... File chrome/browser/android/offline_pages/recent_tab_helper.cc (right): https://codereview.chromium.org/2278773002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/recent_tab_helper.cc:82: snapshot_controller_->Stop(); // It is reset when navigation commits. perhaps the snapshot controller should initially be stopped. https://codereview.chromium.org/2278773002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/recent_tab_helper.cc:102: void RecentTabHelper::DidStartNavigation( This seems like it will change behavior for slower connections. Isn't it possible for navigation to start, but not be committed? In that case the "right moment" might still happen on the previous committed page. Is this intentional/expected? I'm worried that this might be more common that we'd want in EM/slow networks type of places. https://codereview.chromium.org/2278773002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/recent_tab_helper.cc:118: EnsureInitialized(); optional nit: conceptually it might make sense to have this method return a bool representing enabled/ok to proceed with snapshot, rather than tracking snapshots_enabled_... wdyt? https://codereview.chromium.org/2278773002/diff/1/chrome/browser/android/offl... File chrome/browser/android/offline_pages/recent_tab_helper.h (right): https://codereview.chromium.org/2278773002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/recent_tab_helper.h:80: OfflinePageModel* page_model_; please document that page_model_ can be null even during normal operation of this tab helper https://codereview.chromium.org/2278773002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/recent_tab_helper.h:93: std::unique_ptr<SnapshotController> snapshot_controller_; also document that this starts out null but then lasts the lifetime of the helper once EnsureInitialized is called. https://codereview.chromium.org/2278773002/diff/1/chrome/browser/android/offl... File chrome/browser/android/offline_pages/recent_tab_helper_unittest.cc (right): https://codereview.chromium.org/2278773002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/recent_tab_helper_unittest.cc:209: base::test::ScopedFeatureList scoped_feature_list; Can I suggest putting this as a member of RecentTabHelperTest, then only having an inner feature list with no features enabled for the Basic and FeatureNotEnabled tests? https://codereview.chromium.org/2278773002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/recent_tab_helper_unittest.cc:212: NavigateAndCommit(kTestPageUrl); may want to separate navigate / commit phases to be able to test the state in weird sequences.
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_...) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by dimich@chromium.org to run a CQ dry run
ptal. https://codereview.chromium.org/2278773002/diff/1/chrome/browser/android/offl... File chrome/browser/android/offline_pages/recent_tab_helper.cc (left): https://codereview.chromium.org/2278773002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/recent_tab_helper.cc:110: weak_ptr_factory_.InvalidateWeakPtrs(); On 2016/08/24 23:01:39, dewittj wrote: > Why is this gone? It seems like there are still places that we'd want to cancel > async steps. Moved back, and before initialization. https://codereview.chromium.org/2278773002/diff/1/chrome/browser/android/offl... File chrome/browser/android/offline_pages/recent_tab_helper.cc (right): https://codereview.chromium.org/2278773002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/recent_tab_helper.cc:82: snapshot_controller_->Stop(); // It is reset when navigation commits. On 2016/08/24 23:01:39, dewittj wrote: > perhaps the snapshot controller should initially be stopped. Perhaps, but this is not the only user of it by now (Background Loader is using it as well, assuming it is in READY state after creation). Looks ok to explicitly set the state though... https://codereview.chromium.org/2278773002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/recent_tab_helper.cc:102: void RecentTabHelper::DidStartNavigation( On 2016/08/24 23:01:39, dewittj wrote: > This seems like it will change behavior for slower connections. Isn't it > possible for navigation to start, but not be committed? In that case the "right > moment" might still happen on the previous committed page. > > Is this intentional/expected? I'm worried that this might be more common that > we'd want in EM/slow networks type of places. Done. I guess I'm being too paranoid about DidFinishNavigation not being called in some cases... https://codereview.chromium.org/2278773002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/recent_tab_helper.cc:118: EnsureInitialized(); On 2016/08/24 23:01:39, dewittj wrote: > optional nit: conceptually it might make sense to have this method return a bool > representing enabled/ok to proceed with snapshot, rather than tracking > snapshots_enabled_... wdyt? I think I'd need to cache the snapshots_enabled to be able to return it every time from EnsureInitialized... I see your point but it doesn't seem to make the code easier to read. https://codereview.chromium.org/2278773002/diff/1/chrome/browser/android/offl... File chrome/browser/android/offline_pages/recent_tab_helper.h (right): https://codereview.chromium.org/2278773002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/recent_tab_helper.h:80: OfflinePageModel* page_model_; On 2016/08/24 23:01:39, dewittj wrote: > please document that page_model_ can be null even during normal operation of > this tab helper Done. https://codereview.chromium.org/2278773002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/recent_tab_helper.h:93: std::unique_ptr<SnapshotController> snapshot_controller_; On 2016/08/24 23:01:39, dewittj wrote: > also document that this starts out null but then lasts the lifetime of the > helper once EnsureInitialized is called. Done. https://codereview.chromium.org/2278773002/diff/1/chrome/browser/android/offl... File chrome/browser/android/offline_pages/recent_tab_helper_unittest.cc (right): https://codereview.chromium.org/2278773002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/recent_tab_helper_unittest.cc:209: base::test::ScopedFeatureList scoped_feature_list; On 2016/08/24 23:01:40, dewittj wrote: > Can I suggest putting this as a member of RecentTabHelperTest, then only having > an inner feature list with no features enabled for the Basic and > FeatureNotEnabled tests? Done. https://codereview.chromium.org/2278773002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/recent_tab_helper_unittest.cc:212: NavigateAndCommit(kTestPageUrl); On 2016/08/24 23:01:39, dewittj wrote: > may want to separate navigate / commit phases to be able to test the state in > weird sequences. Not sure I have a good idea what exact navigate/commit sequences I would use here. I only get DidFinishNavigate and filter for committed...
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm https://codereview.chromium.org/2278773002/diff/1/chrome/browser/android/offl... File chrome/browser/android/offline_pages/recent_tab_helper.cc (right): https://codereview.chromium.org/2278773002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/recent_tab_helper.cc:82: snapshot_controller_->Stop(); // It is reset when navigation commits. On 2016/08/25 00:36:09, Dmitry Titov wrote: > On 2016/08/24 23:01:39, dewittj wrote: > > perhaps the snapshot controller should initially be stopped. > > Perhaps, but this is not the only user of it by now (Background Loader is using > it as well, assuming it is in READY state after creation). Looks ok to > explicitly set the state though... Acknowledged.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Avi, ptal. This is one of the last pieces of Download feature that we try to ship in m54... Thanks!
Go for it! LGTM
The CQ bit was checked by dimich@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 #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Modify RecentTabHelper to be always-on and observe the loading of the pages. Expose is_page_ready_for_snapshot() on it which is set when the page can be successfully captured. Move the feature flag check from creating the tab helper to before the actual snapshot. BUG=630817 ========== to ========== Modify RecentTabHelper to be always-on and observe the loading of the pages. Expose is_page_ready_for_snapshot() on it which is set when the page can be successfully captured. Move the feature flag check from creating the tab helper to before the actual snapshot. BUG=630817 Committed: https://crrev.com/9ee7a4109cbc763067538c360c36ff2e1ba3ee13 Cr-Commit-Position: refs/heads/master@{#414509} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/9ee7a4109cbc763067538c360c36ff2e1ba3ee13 Cr-Commit-Position: refs/heads/master@{#414509} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
