Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2103293003:
Checks the age of device before initializing HaTS Notification (Closed)
|
Created:
4 years, 5 months ago by malaykeshav Modified:
4 years, 5 months ago CC:
chromium-reviews, davemoore+watch_chromium.org, oshima+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionChecks the age of device before initializing HaTS Notification
- Checks the age of device by comparing the Oobe flag file creation
date.
Design Doc=go/cros-hats-eng
BUG=611781
COMPONENT=Chrome OS, HaTS
Committed: https://crrev.com/4baa85cfcf101dc485866664140d4d8e8e37223b
Cr-Commit-Position: refs/heads/master@{#404464}
Patch Set 1 #
Total comments: 11
Patch Set 2 : Checks the age of device before initializing HaTS Notification #Patch Set 3 : Checks the age of device before initializing HaTS Notification #
Total comments: 10
Patch Set 4 : Checks the age of device before initializing HaTS Notification #
Total comments: 2
Patch Set 5 : Checks the age of device before initializing HaTS Notification #Patch Set 6 : Checks the age of device before initializing HaTS Notification #
Messages
Total messages: 30 (13 generated)
The CQ bit was checked by malaykeshav@chromium.org to run a CQ dry run
malaykeshav@chromium.org changed reviewers: + jdufault@chromium.org
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Checks the age of device before initializing HaTS Notification - Checks the age of device by comparing the Oobe flag file creation date. Design Doc=go/cors-hats-eng BUG=611781 COMPONENT=Chrome OS, HaTS ========== to ========== Checks the age of device before initializing HaTS Notification - Checks the age of device by comparing the Oobe flag file creation date. Design Doc=go/cros-hats-eng BUG=611781 COMPONENT=Chrome OS, HaTS ==========
https://codereview.chromium.org/2103293003/diff/1/chrome/browser/chromeos/hat... File chrome/browser/chromeos/hats/hats_notification_controller.cc (right): https://codereview.chromium.org/2103293003/diff/1/chrome/browser/chromeos/hat... chrome/browser/chromeos/hats/hats_notification_controller.cc:85: if (is_new_device) { It'd be nice if we could move this is_new_device logic to ShouldShowSurveyToProfile so that Initialize only does one thing. Instead of updating the last interaction time, the new device threshold time could be set to kHatsThresholdTime. The problem though is that the OOBE creation time read needs to happen on a different thread. You could make ShouldShowSurveyToProfile an async function that takes a callback that is invoked with the result. It looks like UserSessionManager already has a WeakPtrFactory so it shouldn't be too much work to change. https://codereview.chromium.org/2103293003/diff/1/chrome/browser/chromeos/hat... chrome/browser/chromeos/hats/hats_notification_controller.cc:92: // Add self as an observer to be notified when an internet connection is nit: newline before comment https://codereview.chromium.org/2103293003/diff/1/chrome/browser/chromeos/hat... File chrome/browser/chromeos/hats/hats_notification_controller.h (right): https://codereview.chromium.org/2103293003/diff/1/chrome/browser/chromeos/hat... chrome/browser/chromeos/hats/hats_notification_controller.h:30: static const base::TimeDelta kHatsNewDeviceThresholdTime; Does this need to be exposed publicly, or can it live only in the cc file?
malaykeshav@chromium.org changed reviewers: + stevenjb@chromium.org
https://codereview.chromium.org/2103293003/diff/1/chrome/browser/chromeos/hat... File chrome/browser/chromeos/hats/hats_notification_controller.cc (right): https://codereview.chromium.org/2103293003/diff/1/chrome/browser/chromeos/hat... chrome/browser/chromeos/hats/hats_notification_controller.cc:85: if (is_new_device) { On 2016/06/29 at 19:26:10, jdufault wrote: > It'd be nice if we could move this is_new_device logic to ShouldShowSurveyToProfile so that Initialize only does one thing. > > Instead of updating the last interaction time, the new device threshold time could be set to kHatsThresholdTime. We want to consider the case of a new device as if the user has attempted the survey to reduce clustering of data point at the same point in time, that is 7 days after getting a device.(Part of requirements discussed offline with +omrilio@ and +abodenha@) > > The problem though is that the OOBE creation time read needs to happen on a different thread. You could make ShouldShowSurveyToProfile an async function that takes a callback that is invoked with the result. It looks like UserSessionManager already has a WeakPtrFactory so it shouldn't be too much work to change. 'ShouldShowSurveyToProfile' manages the check for whether to display the notification for a given _profile_. Also moving it adds more complexity and hats related logic to UserSessionManager class. Not sure if thats desirable. https://codereview.chromium.org/2103293003/diff/1/chrome/browser/chromeos/hat... chrome/browser/chromeos/hats/hats_notification_controller.cc:92: // Add self as an observer to be notified when an internet connection is On 2016/06/29 at 19:26:10, jdufault wrote: > nit: newline before comment Done https://codereview.chromium.org/2103293003/diff/1/chrome/browser/chromeos/hat... File chrome/browser/chromeos/hats/hats_notification_controller.h (right): https://codereview.chromium.org/2103293003/diff/1/chrome/browser/chromeos/hat... chrome/browser/chromeos/hats/hats_notification_controller.h:30: static const base::TimeDelta kHatsNewDeviceThresholdTime; On 2016/06/29 at 19:26:10, jdufault wrote: > Does this need to be exposed publicly, or can it live only in the cc file? They are needed for unit tests.
lgtm after comment updates.
https://codereview.chromium.org/2103293003/diff/1/chrome/browser/chromeos/hat... File chrome/browser/chromeos/hats/hats_notification_controller.cc (right): https://codereview.chromium.org/2103293003/diff/1/chrome/browser/chromeos/hat... chrome/browser/chromeos/hats/hats_notification_controller.cc:85: if (is_new_device) { After offline discussion keeping the existing structure lgtm. https://codereview.chromium.org/2103293003/diff/1/chrome/browser/chromeos/hat... chrome/browser/chromeos/hats/hats_notification_controller.cc:86: // If the device is new, then we do not show any survey or notificaiton. Can you update the comment to be more specific, ie, // This device has been chosen for a survey, but it is too new. Instead // of showing the user the survey, just mark it as completed. https://codereview.chromium.org/2103293003/diff/1/chrome/browser/chromeos/hat... chrome/browser/chromeos/hats/hats_notification_controller.cc:100: // Do not show the survey if the HaTS feature is disabled for the device. Please update this comment to explain that the feature flag is controlled by finch and is what lets us run the experiments.
https://codereview.chromium.org/2103293003/diff/1/chrome/browser/chromeos/hat... File chrome/browser/chromeos/hats/hats_notification_controller.cc (right): https://codereview.chromium.org/2103293003/diff/1/chrome/browser/chromeos/hat... chrome/browser/chromeos/hats/hats_notification_controller.cc:86: // If the device is new, then we do not show any survey or notificaiton. On 2016/06/29 at 22:27:03, jdufault wrote: > Can you update the comment to be more specific, ie, > > // This device has been chosen for a survey, but it is too new. Instead > // of showing the user the survey, just mark it as completed. +1 Done https://codereview.chromium.org/2103293003/diff/1/chrome/browser/chromeos/hat... chrome/browser/chromeos/hats/hats_notification_controller.cc:100: // Do not show the survey if the HaTS feature is disabled for the device. On 2016/06/29 22:27:03, jdufault wrote: > Please update this comment to explain that the feature flag is controlled by > finch and is what lets us run the experiments. Done.
https://codereview.chromium.org/2103293003/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/hats/hats_notification_controller.cc (right): https://codereview.chromium.org/2103293003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/hats/hats_notification_controller.cc:44: // Returns true if atleast |threshold| time units have passed since OOBE. This at least https://codereview.chromium.org/2103293003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/hats/hats_notification_controller.cc:45: // is an indirect measure of whether the owner has used the device for atleast at least https://codereview.chromium.org/2103293003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/hats/hats_notification_controller.cc:49: chromeos::StartupUtils::GetTimeSinceOobeFlagFileCreation(); no need for intermediate variable https://codereview.chromium.org/2103293003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/hats/hats_notification_controller.cc:69: base::TimeDelta::FromDays(7); Avoid initializing values like this, it slows down Chrome's startup time. Instead, make these, e.g. const int kHatsThresholdDays = 90; Then when you use it, wrap it with FromDays(). https://codereview.chromium.org/2103293003/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/hats/hats_notification_controller.h (right): https://codereview.chromium.org/2103293003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/hats/hats_notification_controller.h:32: static const char kNotificationId[]; Any of these that are not used outside of hats_notification_controller.cc should be defined as file local variables inside an anonymous namespace.
https://codereview.chromium.org/2103293003/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/hats/hats_notification_controller.cc (right): https://codereview.chromium.org/2103293003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/hats/hats_notification_controller.cc:44: // Returns true if atleast |threshold| time units have passed since OOBE. This On 2016/06/29 at 23:35:55, stevenjb wrote: > at least Done https://codereview.chromium.org/2103293003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/hats/hats_notification_controller.cc:45: // is an indirect measure of whether the owner has used the device for atleast On 2016/06/29 at 23:35:55, stevenjb wrote: > at least Done https://codereview.chromium.org/2103293003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/hats/hats_notification_controller.cc:49: chromeos::StartupUtils::GetTimeSinceOobeFlagFileCreation(); On 2016/06/29 at 23:35:55, stevenjb wrote: > no need for intermediate variable Done https://codereview.chromium.org/2103293003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/hats/hats_notification_controller.cc:69: base::TimeDelta::FromDays(7); On 2016/06/29 at 23:35:55, stevenjb wrote: > Avoid initializing values like this, it slows down Chrome's startup time. > > Instead, make these, e.g. > > const int kHatsThresholdDays = 90; > > Then when you use it, wrap it with FromDays(). Done https://codereview.chromium.org/2103293003/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/hats/hats_notification_controller.h (right): https://codereview.chromium.org/2103293003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/hats/hats_notification_controller.h:32: static const char kNotificationId[]; On 2016/06/29 at 23:35:55, stevenjb wrote: > Any of these that are not used outside of hats_notification_controller.cc should be defined as file local variables inside an anonymous namespace. These are used in the unittests. Would have to move them back here again when writing the test.
Ping
lgtm https://codereview.chromium.org/2103293003/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/hats/hats_notification_controller.cc (right): https://codereview.chromium.org/2103293003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/hats/hats_notification_controller.cc:34: bool DidShowSurveyToProfileRecently(Profile* profile, int threshold) { nit: threshold_days (or just days) https://codereview.chromium.org/2103293003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/hats/hats_notification_controller.cc:47: bool IsNewDevice(int threshold) { nit: threshold_days (or just days)
The CQ bit was checked by malaykeshav@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jdufault@chromium.org, stevenjb@chromium.org Link to the patchset: https://codereview.chromium.org/2103293003/#ps80001 (title: "Checks the age of device before initializing HaTS Notification")
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: linux_chromium_chromeos_ozone_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 malaykeshav@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 malaykeshav@chromium.org
The CQ bit was checked by malaykeshav@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jdufault@chromium.org, stevenjb@chromium.org Link to the patchset: https://codereview.chromium.org/2103293003/#ps100001 (title: "Checks the age of device before initializing HaTS Notification")
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 #6 (id:100001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== Checks the age of device before initializing HaTS Notification - Checks the age of device by comparing the Oobe flag file creation date. Design Doc=go/cros-hats-eng BUG=611781 COMPONENT=Chrome OS, HaTS ========== to ========== Checks the age of device before initializing HaTS Notification - Checks the age of device by comparing the Oobe flag file creation date. Design Doc=go/cros-hats-eng BUG=611781 COMPONENT=Chrome OS, HaTS Committed: https://crrev.com/4baa85cfcf101dc485866664140d4d8e8e37223b Cr-Commit-Position: refs/heads/master@{#404464} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/4baa85cfcf101dc485866664140d4d8e8e37223b Cr-Commit-Position: refs/heads/master@{#404464} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
