Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2699613002:
[remote suggestions] Add separte fetch interval for NTP open trigger (Closed)
|
Created:
3 years, 10 months ago by markusheintz_ Modified:
3 years, 10 months ago Reviewers:
jkrcal CC:
chromium-reviews, noyau+watch_chromium.org, ntp-dev+reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[remote suggestions] Add separte fetch interval for NTP open trigger
BUG=692727
Review-Url: https://codereview.chromium.org/2699613002
Cr-Commit-Position: refs/heads/master@{#451337}
Committed: https://chromium.googlesource.com/chromium/src/+/e85944f082b27f903fcea5491549de058e157528
Patch Set 1 #Patch Set 2 : " #Patch Set 3 : ' #
Total comments: 13
Patch Set 4 : Address comments #Patch Set 5 : Address comments #
Total comments: 4
Patch Set 6 : Address comments #Patch Set 7 : remove extra test - will have a separate CL #
Messages
Total messages: 42 (25 generated)
"
'
Description was changed from ========== [remote suggestions] Add separte fetch interval for NTP open trigger BUG=TBD ========== to ========== [remote suggestions] Add separte fetch interval for NTP open trigger BUG=TBD ==========
markusheintz@chromium.org changed reviewers: + jkrcal@chromium.org
Jan even though I still need to add the variation param please take an early look at this CL. Thanks
I guess you meant the prefs that need to be added? (variation params seem to be in place) Overall it looks good! Good that you reach out early, some naming nits in comments. Please reach out via hangouts if we are not in agreement. https://codereview.chromium.org/2699613002/diff/40001/components/ntp_snippets... File components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc (right): https://codereview.chromium.org/2699613002/diff/40001/components/ntp_snippets... components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc:40: NTP_OPEN, nit: please update consistently with the FetchingSchedule discussion https://codereview.chromium.org/2699613002/diff/40001/components/ntp_snippets... components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc:63: "ntp_open_interval_hours-rare_ntp_user"}; nit: please update consistently with the FetchingSchedule discussion https://codereview.chromium.org/2699613002/diff/40001/components/ntp_snippets... components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc:417: schedule_.interval_soft_on_usage_event = base::TimeDelta::FromInternalValue( we need another pref for the new interval to load the last value... https://codereview.chromium.org/2699613002/diff/40001/components/ntp_snippets... components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc:429: schedule_.interval_soft_on_usage_event.ToInternalValue()); ...and to store it here https://codereview.chromium.org/2699613002/diff/40001/components/ntp_snippets... File components/ntp_snippets/remote/scheduling_remote_suggestions_provider.h (right): https://codereview.chromium.org/2699613002/diff/40001/components/ntp_snippets... components/ntp_snippets/remote/scheduling_remote_suggestions_provider.h:117: base::TimeDelta interval_ntp_open; nit: can it be soft_on_ntp_opened? (opened to make it consistent with the event name; soft_ because it is soft as well) Another possibility is to make it more abstract: soft_on_priority_usage_event I think it actually better expresses the core of being a separate interval. WDYT?
Description was changed from ========== [remote suggestions] Add separte fetch interval for NTP open trigger BUG=TBD ========== to ========== [remote suggestions] Add separte fetch interval for NTP open trigger BUG=692727 ==========
Address comments
https://codereview.chromium.org/2699613002/diff/40001/components/ntp_snippets... File components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc (right): https://codereview.chromium.org/2699613002/diff/40001/components/ntp_snippets... components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc:40: NTP_OPEN, On 2017/02/15 16:35:34, jkrcal wrote: > nit: please update consistently with the FetchingSchedule discussion Done. https://codereview.chromium.org/2699613002/diff/40001/components/ntp_snippets... components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc:63: "ntp_open_interval_hours-rare_ntp_user"}; On 2017/02/15 16:35:34, jkrcal wrote: > nit: please update consistently with the FetchingSchedule discussion Done. https://codereview.chromium.org/2699613002/diff/40001/components/ntp_snippets... components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc:417: schedule_.interval_soft_on_usage_event = base::TimeDelta::FromInternalValue( On 2017/02/15 16:35:34, jkrcal wrote: > we need another pref for the new interval to load the last value... Done. https://codereview.chromium.org/2699613002/diff/40001/components/ntp_snippets... components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc:429: schedule_.interval_soft_on_usage_event.ToInternalValue()); On 2017/02/15 16:35:34, jkrcal wrote: > ...and to store it here Done. https://codereview.chromium.org/2699613002/diff/40001/components/ntp_snippets... components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc:460: case TriggerType::PERSISTENT_SCHEDULER_WAKE_UP: Changed to default in order to prevent having to use case COUNT: https://codereview.chromium.org/2699613002/diff/40001/components/ntp_snippets... File components/ntp_snippets/remote/scheduling_remote_suggestions_provider.h (right): https://codereview.chromium.org/2699613002/diff/40001/components/ntp_snippets... components/ntp_snippets/remote/scheduling_remote_suggestions_provider.h:117: base::TimeDelta interval_ntp_open; On 2017/02/15 16:35:34, jkrcal wrote: > nit: can it be soft_on_ntp_opened? > (opened to make it consistent with the event name; soft_ because it is soft as > well) > > Another possibility is to make it more abstract: > soft_on_priority_usage_event > I think it actually better expresses the core of being a separate interval. > > WDYT? soft_on_ntp_opened SGTM. Name updated.
The CQ bit was checked by markusheintz@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_...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
Thanks! Could you please also add some unit-tests? (changing the default variation param for the new type; look if any open NTP tests need to get updated) https://codereview.chromium.org/2699613002/diff/40001/components/ntp_snippets... File components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc (right): https://codereview.chromium.org/2699613002/diff/40001/components/ntp_snippets... components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc:460: case TriggerType::PERSISTENT_SCHEDULER_WAKE_UP: On 2017/02/15 21:19:29, markusheintz_ wrote: > Changed to default in order to prevent having to use case COUNT: I would actually rather have both of them listed here. This way, when you later add a new entry into the enum, the compiler will force you to add the entry here as well. Not a big deal because if you forget, it will crash here anyway in a debug build so I do not feel strongly.
Address comments
The CQ bit was checked by markusheintz@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...
https://codereview.chromium.org/2699613002/diff/40001/components/ntp_snippets... File components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc (right): https://codereview.chromium.org/2699613002/diff/40001/components/ntp_snippets... components/ntp_snippets/remote/scheduling_remote_suggestions_provider.cc:460: case TriggerType::PERSISTENT_SCHEDULER_WAKE_UP: On 2017/02/16 07:32:54, jkrcal wrote: > On 2017/02/15 21:19:29, markusheintz_ wrote: > > Changed to default in order to prevent having to use case COUNT: > > I would actually rather have both of them listed here. This way, when you later > add a new entry into the enum, the compiler will force you to add the entry here > as well. > > Not a big deal because if you forget, it will crash here anyway in a debug build > so I do not feel strongly. Done
lgtm % nits Thanks! https://codereview.chromium.org/2699613002/diff/80001/components/ntp_snippets... File components/ntp_snippets/remote/scheduling_remote_suggestions_provider_unittest.cc (right): https://codereview.chromium.org/2699613002/diff/80001/components/ntp_snippets... components/ntp_snippets/remote/scheduling_remote_suggestions_provider_unittest.cc:422: ShouldFetchAgainOnNTPOpenedLaterAgain) { nit: Can you please: - either merge this test with your new test? (I think yours is slightly better), - or keep this test and change it so that it focuses on FOREGROUNDING? https://codereview.chromium.org/2699613002/diff/80001/components/ntp_snippets... components/ntp_snippets/remote/scheduling_remote_suggestions_provider_unittest.cc:574: ReschedulesWhenOnUsageEventParamChanges) { nit: Can you please also create an analogous fourth test to this sequence of 3 tests?
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_...)
Address comments
The CQ bit was checked by markusheintz@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.
remove extra test - will have a separate CL
The CQ bit was checked by markusheintz@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_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 markusheintz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jkrcal@chromium.org Link to the patchset: https://codereview.chromium.org/2699613002/#ps120001 (title: "remove extra test - will have a separate CL")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2699613002/diff/80001/components/ntp_snippets... File components/ntp_snippets/remote/scheduling_remote_suggestions_provider_unittest.cc (right): https://codereview.chromium.org/2699613002/diff/80001/components/ntp_snippets... components/ntp_snippets/remote/scheduling_remote_suggestions_provider_unittest.cc:422: ShouldFetchAgainOnNTPOpenedLaterAgain) { On 2017/02/16 14:02:16, jkrcal wrote: > nit: Can you please: > - either merge this test with your new test? (I think yours is slightly > better), > - or keep this test and change it so that it focuses on FOREGROUNDING? Done. https://codereview.chromium.org/2699613002/diff/80001/components/ntp_snippets... components/ntp_snippets/remote/scheduling_remote_suggestions_provider_unittest.cc:574: ReschedulesWhenOnUsageEventParamChanges) { On 2017/02/16 14:02:16, jkrcal wrote: > nit: Can you please also create an analogous fourth test to this sequence of 3 > tests? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by markusheintz@chromium.org
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": 120001, "attempt_start_ts": 1487351363476100,
"parent_rev": "b3d3b8331c1cb0807b916a871b7801788087f713", "commit_rev":
"e85944f082b27f903fcea5491549de058e157528"}
Message was sent while issue was closed.
Description was changed from ========== [remote suggestions] Add separte fetch interval for NTP open trigger BUG=692727 ========== to ========== [remote suggestions] Add separte fetch interval for NTP open trigger BUG=692727 Review-Url: https://codereview.chromium.org/2699613002 Cr-Commit-Position: refs/heads/master@{#451337} Committed: https://chromium.googlesource.com/chromium/src/+/e85944f082b27f903fcea5491549... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/e85944f082b27f903fcea5491549... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
