Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2822023002:
[Offline pages]: Move logic for whether to consider the DocumentAvailableInMainFrame signal to the … (Closed)
|
Created:
3 years, 8 months ago by chili Modified:
3 years, 7 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, dewittj+watch_chromium.org, tburkard+watch_chromium.org, fgorski+watch_chromium.org, romax+watch_chromium.org, petewil+watch_chromium.org, gavinp+prer_chromium.org, chili+watch_chromium.org, dimich+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[Offline pages]: Move logic for whether to consider the DocumentAvailableInMainFrame signal to the snapshot controller
Changes include:
-- moving all timing parameters (how long to wait before snapshotting) to the snapshot controller
-- introducing 2 new factory methods for obtaining a snapshot controller
-- more explicit ownership and transfer of the snapshot controller
BUG=712473
Review-Url: https://codereview.chromium.org/2822023002
Cr-Commit-Position: refs/heads/master@{#468765}
Committed: https://chromium.googlesource.com/chromium/src/+/5e17a9358f03488e71cba7c4b6786afba95fed6e
Patch Set 1 #Patch Set 2 : some fix #
Total comments: 15
Patch Set 3 : code review changes #
Total comments: 12
Patch Set 4 : strengthen ownership #Patch Set 5 : update test #Patch Set 6 : test fix: replace snapshot controller at the right place #Messages
Total messages: 52 (37 generated)
The CQ bit was checked by chili@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...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by chili@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...
Patchset #2 (id:20001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
chili@chromium.org changed reviewers: + carlosk@chromium.org, fgorski@chromium.org, romax@chromium.org
This is hard to review without context. Please add a bug and relevant explanation of why this is happening.
mostly looks good, please attach it to a bug :) https://codereview.chromium.org/2822023002/diff/40001/chrome/browser/android/... File chrome/browser/android/offline_pages/prerendering_loader.cc (left): https://codereview.chromium.org/2822023002/diff/40001/chrome/browser/android/... chrome/browser/android/offline_pages/prerendering_loader.cc:20: // Whether to report DomContentLoaded event to the snapshot controller. seems like the delay can also be used as the boolean, like 0 means false? i'm fine with either, but is there a preference on higher-level?
Description was changed from ========== [Offline pages]: Move logic for whether to consider the DocumentAvailableInMainFrame signal to the snapshot controller BUG= ========== to ========== [Offline pages]: Move logic for whether to consider the DocumentAvailableInMainFrame signal to the snapshot controller BUG=712473 ==========
Forgot to attach bug. Added more details in the bug for rationale. https://codereview.chromium.org/2822023002/diff/40001/chrome/browser/android/... File chrome/browser/android/offline_pages/prerendering_loader.cc (left): https://codereview.chromium.org/2822023002/diff/40001/chrome/browser/android/... chrome/browser/android/offline_pages/prerendering_loader.cc:20: // Whether to report DomContentLoaded event to the snapshot controller. On 2017/04/19 17:40:29, romax wrote: > seems like the delay can also be used as the boolean, like 0 means false? i'm > fine with either, but is there a preference on higher-level? I would rather not use delay as a boolean. Delay is answering the question "how many seconds after the event should snapshot happen?". A delay of 0 would therefore answer it as "immediately". The only number that would make sense would be something like MAX_LONG, for "please wait forever". It will also be harder to read than a boolean would, in my opinion.
LGTM with one change needed and a nit. I agree with the choice of not overloading the delay parameter. https://codereview.chromium.org/2822023002/diff/40001/chrome/browser/android/... File chrome/browser/android/offline_pages/background_loader_offliner.cc (right): https://codereview.chromium.org/2822023002/diff/40001/chrome/browser/android/... chrome/browser/android/offline_pages/background_loader_offliner.cc:174: base::ThreadTaskRunnerHandle::Get(), this, kOfflineDomContentLoadedMs, As the DOM content loaded delay is now ignored this constant should be removed. https://codereview.chromium.org/2822023002/diff/40001/components/offline_page... File components/offline_pages/core/snapshot_controller.h (right): https://codereview.chromium.org/2822023002/diff/40001/components/offline_page... components/offline_pages/core/snapshot_controller.h:107: bool consider_document_available_for_snapshot_; nit: it seems to me that prefixing with "consider" makes its intent less less clear than it would be with "ignore".
https://codereview.chromium.org/2822023002/diff/40001/chrome/browser/android/... File chrome/browser/android/offline_pages/background_loader_offliner.cc (right): https://codereview.chromium.org/2822023002/diff/40001/chrome/browser/android/... chrome/browser/android/offline_pages/background_loader_offliner.cc:174: base::ThreadTaskRunnerHandle::Get(), this, kOfflineDomContentLoadedMs, Since this is the only place the constant is used, why not put it into SnapshotController? I have a feeling that this is another case where we duplicated instead of moving it where it belongs. https://codereview.chromium.org/2822023002/diff/40001/chrome/browser/android/... File chrome/browser/android/offline_pages/prerendering_loader.cc (right): https://codereview.chromium.org/2822023002/diff/40001/chrome/browser/android/... chrome/browser/android/offline_pages/prerendering_loader.cc:21: long kOfflinePageDclDelayMs = 25000; Since you are already changing this file, I recommend getting moving this to snapshot controller as well. https://codereview.chromium.org/2822023002/diff/40001/chrome/browser/android/... chrome/browser/android/offline_pages/prerendering_loader.cc:133: false /* consider_document_available_for_snapshot */)); The fact that for both background cases we are passing false tells me that: * we should not expose that as a constructor parameter for the background case. * Perhaps should wrap a single private constructor into 2 factory methods: SnapshotController::CreateForForegroundSnapshot(...) SnapshotController::CreateForBackgroundSnapshot(...) We don't vary parameters now, which does not worry me. When we get to varying parameters I think we should have that well planned. https://codereview.chromium.org/2822023002/diff/40001/components/offline_page... File components/offline_pages/core/snapshot_controller.cc (right): https://codereview.chromium.org/2822023002/diff/40001/components/offline_page... components/offline_pages/core/snapshot_controller.cc:14: const bool kConsiderDocumentAvailableForSnapshot = true; This is where I'd put all constants for foreground and background until we decide to vary within the class. https://codereview.chromium.org/2822023002/diff/40001/components/offline_page... File components/offline_pages/core/snapshot_controller.h (right): https://codereview.chromium.org/2822023002/diff/40001/components/offline_page... components/offline_pages/core/snapshot_controller.h:107: bool consider_document_available_for_snapshot_; I too have hard time reading this variable: document_available_triggers_snapshot_ maybe?
Moved all the constants over to the snapshot controller, per filip's suggestion. PTAL https://codereview.chromium.org/2822023002/diff/40001/chrome/browser/android/... File chrome/browser/android/offline_pages/background_loader_offliner.cc (right): https://codereview.chromium.org/2822023002/diff/40001/chrome/browser/android/... chrome/browser/android/offline_pages/background_loader_offliner.cc:174: base::ThreadTaskRunnerHandle::Get(), this, kOfflineDomContentLoadedMs, On 2017/04/20 16:39:05, fgorski wrote: > Since this is the only place the constant is used, why not put it into > SnapshotController? > > I have a feeling that this is another case where we duplicated instead of moving > it where it belongs. Done. https://codereview.chromium.org/2822023002/diff/40001/chrome/browser/android/... File chrome/browser/android/offline_pages/prerendering_loader.cc (right): https://codereview.chromium.org/2822023002/diff/40001/chrome/browser/android/... chrome/browser/android/offline_pages/prerendering_loader.cc:21: long kOfflinePageDclDelayMs = 25000; On 2017/04/20 16:39:05, fgorski wrote: > Since you are already changing this file, I recommend getting moving this to > snapshot controller as well. Done. https://codereview.chromium.org/2822023002/diff/40001/chrome/browser/android/... chrome/browser/android/offline_pages/prerendering_loader.cc:133: false /* consider_document_available_for_snapshot */)); On 2017/04/20 16:39:05, fgorski wrote: > The fact that for both background cases we are passing false tells me that: > * we should not expose that as a constructor parameter for the background case. > * Perhaps should wrap a single private constructor into 2 factory methods: > SnapshotController::CreateForForegroundSnapshot(...) > SnapshotController::CreateForBackgroundSnapshot(...) > > We don't vary parameters now, which does not worry me. When we get to varying > parameters I think we should have that well planned. Done. https://codereview.chromium.org/2822023002/diff/40001/components/offline_page... File components/offline_pages/core/snapshot_controller.cc (right): https://codereview.chromium.org/2822023002/diff/40001/components/offline_page... components/offline_pages/core/snapshot_controller.cc:14: const bool kConsiderDocumentAvailableForSnapshot = true; On 2017/04/20 16:39:05, fgorski wrote: > This is where I'd put all constants for foreground and background until we > decide to vary within the class. Done. https://codereview.chromium.org/2822023002/diff/40001/components/offline_page... File components/offline_pages/core/snapshot_controller.h (right): https://codereview.chromium.org/2822023002/diff/40001/components/offline_page... components/offline_pages/core/snapshot_controller.h:107: bool consider_document_available_for_snapshot_; On 2017/04/19 23:16:57, carlosk wrote: > nit: it seems to me that prefixing with "consider" makes its intent less less > clear than it would be with "ignore". I think I prefer a "positive" over "negative" link. i.e. If I change the name to "ShouldIgnoreDocumentAvailableForSnapshot", then passing 'false' would imply we shouldn't ignore - aka we should use it. Similarly, a 'true' would mean we shouldn't use it. Filip suggested document_available_triggers_snapshot. Is this ok with you?
Still LGTM and indeed the Create* methods do make the API look clearer. https://codereview.chromium.org/2822023002/diff/40001/components/offline_page... File components/offline_pages/core/snapshot_controller.h (right): https://codereview.chromium.org/2822023002/diff/40001/components/offline_page... components/offline_pages/core/snapshot_controller.h:107: bool consider_document_available_for_snapshot_; On 2017/04/21 00:12:54, chili wrote: > On 2017/04/19 23:16:57, carlosk wrote: > > nit: it seems to me that prefixing with "consider" makes its intent less less > > clear than it would be with "ignore". > > I think I prefer a "positive" over "negative" link. i.e. If I change the name to > "ShouldIgnoreDocumentAvailableForSnapshot", then passing 'false' would imply we > shouldn't ignore - aka we should use it. Similarly, a 'true' would mean we > shouldn't use it. > > Filip suggested document_available_triggers_snapshot. Is this ok with you? It is. I find this naming to be just as clear as the ignore*. https://codereview.chromium.org/2822023002/diff/60001/components/offline_page... File components/offline_pages/core/snapshot_controller.cc (right): https://codereview.chromium.org/2822023002/diff/60001/components/offline_page... components/offline_pages/core/snapshot_controller.cc:15: const bool kDocumentAvailableTriggersSnapshotBackground = false; nit: it seems to me these constants are now unneeded verbosity. Using true/false in the new Create* methods below is still clear enough.
Mostly looks good. My concerns are mostly around stressing the ownership model here. https://codereview.chromium.org/2822023002/diff/60001/chrome/browser/android/... File chrome/browser/android/offline_pages/background_loader_offliner.cc (right): https://codereview.chromium.org/2822023002/diff/60001/chrome/browser/android/... chrome/browser/android/offline_pages/background_loader_offliner.cc:276: void BackgroundLoaderOffliner::SetSnapshotControllerForTest( +1 one to the use of injection that way! I think this provides for a cleaner experience, since now you can create the controller in test. However, because your are taking ownership inside of BLO, you should pass by unique_ptr<>. Please fix. This is very good otherwise. https://codereview.chromium.org/2822023002/diff/60001/components/offline_page... File components/offline_pages/core/snapshot_controller.cc (right): https://codereview.chromium.org/2822023002/diff/60001/components/offline_page... components/offline_pages/core/snapshot_controller.cc:15: const bool kDocumentAvailableTriggersSnapshotBackground = false; On 2017/04/21 01:10:58, carlosk wrote: > nit: it seems to me these constants are now unneeded verbosity. Using true/false > in the new Create* methods below is still clear enough. Alternative (also nit): kDocumentAvailableTriggersSnapshot = true; use it directly for Foreground use it with negation for Background: !kDocumentAvailableTriggersSnapshot https://codereview.chromium.org/2822023002/diff/60001/components/offline_page... components/offline_pages/core/snapshot_controller.cc:20: const int64_t kDefaultDelayAfterDocumentAvailableMs = 7000; Wasn't this set to 25 seconds for Background? I see you are making a change to 7s now. Was that intended? Also documentation would probably need to be updated for respective CreateForBackgroundOfflining if you decide to update. https://codereview.chromium.org/2822023002/diff/60001/components/offline_page... File components/offline_pages/core/snapshot_controller.h (right): https://codereview.chromium.org/2822023002/diff/60001/components/offline_page... components/offline_pages/core/snapshot_controller.h:63: // Creates a SnapshotController with document available delay = 7s nit: please add comma after 7s. https://codereview.chromium.org/2822023002/diff/60001/components/offline_page... components/offline_pages/core/snapshot_controller.h:65: static SnapshotController* CreateForForegroundOfflining( I think these 2 will be safer with a unique_ptr<> as a return type to clearly stress the ownership.
Incorporated the changes. I've mostly gut-feeling understanding of when to use std::move, so please double-check my ownership passing :) https://codereview.chromium.org/2822023002/diff/60001/chrome/browser/android/... File chrome/browser/android/offline_pages/background_loader_offliner.cc (right): https://codereview.chromium.org/2822023002/diff/60001/chrome/browser/android/... chrome/browser/android/offline_pages/background_loader_offliner.cc:276: void BackgroundLoaderOffliner::SetSnapshotControllerForTest( On 2017/04/21 16:05:10, fgorski wrote: > +1 one to the use of injection that way! I think this provides for a cleaner > experience, since now you can create the controller in test. > > However, because your are taking ownership inside of BLO, you should pass by > unique_ptr<>. Please fix. > > This is very good otherwise. Done. https://codereview.chromium.org/2822023002/diff/60001/components/offline_page... File components/offline_pages/core/snapshot_controller.cc (right): https://codereview.chromium.org/2822023002/diff/60001/components/offline_page... components/offline_pages/core/snapshot_controller.cc:15: const bool kDocumentAvailableTriggersSnapshotBackground = false; On 2017/04/21 16:05:10, fgorski wrote: > On 2017/04/21 01:10:58, carlosk wrote: > > nit: it seems to me these constants are now unneeded verbosity. Using > true/false > > in the new Create* methods below is still clear enough. > > Alternative (also nit): > kDocumentAvailableTriggersSnapshot = true; > > use it directly for Foreground > use it with negation for Background: !kDocumentAvailableTriggersSnapshot Done. https://codereview.chromium.org/2822023002/diff/60001/components/offline_page... components/offline_pages/core/snapshot_controller.cc:20: const int64_t kDefaultDelayAfterDocumentAvailableMs = 7000; On 2017/04/21 16:05:10, fgorski wrote: > Wasn't this set to 25 seconds for Background? > I see you are making a change to 7s now. Was that intended? > > Also documentation would probably need to be updated for respective > CreateForBackgroundOfflining if you decide to update. Yes, but the DocumentAvailable signal is ignored anyway, so the 25s is essentially void... https://codereview.chromium.org/2822023002/diff/60001/components/offline_page... File components/offline_pages/core/snapshot_controller.h (right): https://codereview.chromium.org/2822023002/diff/60001/components/offline_page... components/offline_pages/core/snapshot_controller.h:63: // Creates a SnapshotController with document available delay = 7s On 2017/04/21 16:05:10, fgorski wrote: > nit: please add comma after 7s. Done. https://codereview.chromium.org/2822023002/diff/60001/components/offline_page... components/offline_pages/core/snapshot_controller.h:65: static SnapshotController* CreateForForegroundOfflining( On 2017/04/21 16:05:10, fgorski wrote: > I think these 2 will be safer with a unique_ptr<> as a return type to clearly > stress the ownership. Done.
lgtm. I think this patch is great! One more nit though. Please edit the description of the patch to capture what is changing and why. https://codereview.chromium.org/2822023002/diff/60001/components/offline_page... File components/offline_pages/core/snapshot_controller.cc (right): https://codereview.chromium.org/2822023002/diff/60001/components/offline_page... components/offline_pages/core/snapshot_controller.cc:20: const int64_t kDefaultDelayAfterDocumentAvailableMs = 7000; On 2017/04/24 20:34:14, chili wrote: > On 2017/04/21 16:05:10, fgorski wrote: > > Wasn't this set to 25 seconds for Background? > > I see you are making a change to 7s now. Was that intended? > > > > Also documentation would probably need to be updated for respective > > CreateForBackgroundOfflining if you decide to update. > > Yes, but the DocumentAvailable signal is ignored anyway, so the 25s is > essentially void... Acknowledged. Thanks for looking into it. I am glad we have it explained here. It may be worth putting it in bug description in case anyone drills down from source code and wonders why we did that.
Description was changed from ========== [Offline pages]: Move logic for whether to consider the DocumentAvailableInMainFrame signal to the snapshot controller BUG=712473 ========== to ========== [Offline pages]: Move logic for whether to consider the DocumentAvailableInMainFrame signal to the snapshot controller Changes include: -- moving all timing parameters (how long to wait before snapshotting) to the snapshot controller -- introducing 2 new factory methods for obtaining a snapshot controller -- more explicit ownership and transfer of the snapshot controller BUG=712473 ==========
The CQ bit was checked by chili@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from carlosk@chromium.org Link to the patchset: https://codereview.chromium.org/2822023002/#ps80001 (title: "strengthen ownership")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) 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_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) 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_...) win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) 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_...) win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
The CQ bit was checked by chili@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-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
Patchset #5 (id:100001) has been deleted
The CQ bit was checked by chili@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_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) 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 chili@chromium.org to run a CQ dry run
Patchset #5 (id:120001) has been deleted
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_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by chili@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.
The CQ bit was checked by chili@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from fgorski@chromium.org, carlosk@chromium.org Link to the patchset: https://codereview.chromium.org/2822023002/#ps160001 (title: "test fix: replace snapshot controller at the right place")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 160001, "attempt_start_ts": 1493757821299020,
"parent_rev": "4c4a925b1a047a296b7962566ff63918f3eba1c4", "commit_rev":
"5e17a9358f03488e71cba7c4b6786afba95fed6e"}
Message was sent while issue was closed.
Description was changed from ========== [Offline pages]: Move logic for whether to consider the DocumentAvailableInMainFrame signal to the snapshot controller Changes include: -- moving all timing parameters (how long to wait before snapshotting) to the snapshot controller -- introducing 2 new factory methods for obtaining a snapshot controller -- more explicit ownership and transfer of the snapshot controller BUG=712473 ========== to ========== [Offline pages]: Move logic for whether to consider the DocumentAvailableInMainFrame signal to the snapshot controller Changes include: -- moving all timing parameters (how long to wait before snapshotting) to the snapshot controller -- introducing 2 new factory methods for obtaining a snapshot controller -- more explicit ownership and transfer of the snapshot controller BUG=712473 Review-Url: https://codereview.chromium.org/2822023002 Cr-Commit-Position: refs/heads/master@{#468765} Committed: https://chromium.googlesource.com/chromium/src/+/5e17a9358f03488e71cba7c4b678... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:160001) as https://chromium.googlesource.com/chromium/src/+/5e17a9358f03488e71cba7c4b678... |
