Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2889323004:
Win 10 Inactive toast experiment metrics and storage modifications. (Closed)
DescriptionWin 10 Inactive toast experiment metrics and storage modifications
BUG=717091
Review-Url: https://codereview.chromium.org/2889323004
Cr-Commit-Position: refs/heads/master@{#479571}
Committed: https://chromium.googlesource.com/chromium/src/+/0667367bcef7b8105d76e500c80f265bbda69aed
Patch Set 1 #Patch Set 2 : Win 10 Inactive toast experiment metrics and storage modifications. #Patch Set 3 : Add unittest for experiment.cc #Patch Set 4 : Add missing experiment.* files and experiment_metrics.* files #Patch Set 5 : Apply comments from 2898843002 #Patch Set 6 : Apply comments from 2898843002 #
Total comments: 28
Patch Set 7 : Incorporate review comments #Patch Set 8 : Incorporate review comments #
Total comments: 71
Patch Set 9 : Apply review comments #
Total comments: 22
Patch Set 10 : Apply review comment #Patch Set 11 : Apply review comment #Patch Set 12 : Apply comment #Patch Set 13 : Revert extra files #
Total comments: 2
Patch Set 14 : Apply review comments #
Total comments: 50
Patch Set 15 : Apply review comments #
Total comments: 6
Patch Set 16 : Apply comments #Patch Set 17 : Fix type of returned value from sizeof operator #
Total comments: 2
Patch Set 18 : Apply some comments and try bots errors #Messages
Total messages: 45 (22 generated)
Description was changed from ========== Win 10 Inactive toast experiment metrics and storage modifications (NOT READY YET). Based on CL 2868973002. Need merging after it lands (Diffs from patch is not working). BUG=717091 ========== to ========== Win 10 Inactive toast experiment metrics and storage modifications BUG=717091 ==========
nikunjb@google.com changed reviewers: + grt@chromium.org, nikunjb@google.com, skare@chromium.org
Core code in latest patch is ready for review. PTAL (May add few more unittests).
On 2017/05/24 04:54:22, nikunjb wrote: > Core code in latest patch is ready for review. PTAL > (May add few more unittests). Couldn't find ways to diffbase on 2868973002. So the changes in most of the files are based from that cl (merged to patch 10).
some comments from a partial read https://codereview.chromium.org/2889323004/diff/100001/chrome/installer/util/... File chrome/installer/util/experiment.cc (right): https://codereview.chromium.org/2889323004/diff/100001/chrome/installer/util/... chrome/installer/util/experiment.cc:15: return round(log(x) / log(b)); nit: std::round https://codereview.chromium.org/2889323004/diff/100001/chrome/installer/util/... chrome/installer/util/experiment.cc:22: return exp(log(max_val - 1) / (1 << bits)); std::exp, std::log https://codereview.chromium.org/2889323004/diff/100001/chrome/installer/util/... chrome/installer/util/experiment.cc:41: static_cast<int>(round(pow(log_base, metrics_.last_used_bucket) - 1)); std::pow https://codereview.chromium.org/2889323004/diff/100001/chrome/installer/util/... File chrome/installer/util/experiment_metrics.cc (right): https://codereview.chromium.org/2889323004/diff/100001/chrome/installer/util/... chrome/installer/util/experiment_metrics.cc:9: // static not static https://codereview.chromium.org/2889323004/diff/100001/chrome/installer/util/... File chrome/installer/util/experiment_metrics.h (right): https://codereview.chromium.org/2889323004/diff/100001/chrome/installer/util/... chrome/installer/util/experiment_metrics.h:8: #include "base/time/time.h" unused https://codereview.chromium.org/2889323004/diff/100001/chrome/installer/util/... File chrome/installer/util/experiment_storage.cc (right): https://codereview.chromium.org/2889323004/diff/100001/chrome/installer/util/... chrome/installer/util/experiment_storage.cc:51: static constexpr int kSessionLengthLowestBit = 0; nit: omit "static" in an unnamed namespace https://codereview.chromium.org/2889323004/diff/100001/chrome/installer/util/... chrome/installer/util/experiment_storage.cc:280: << "Can not read an int of length " << len << " from " << value; oops :-) https://codereview.chromium.org/2889323004/diff/100001/chrome/installer/util/... File chrome/installer/util/experiment_storage_unittest.cc (right): https://codereview.chromium.org/2889323004/diff/100001/chrome/installer/util/... chrome/installer/util/experiment_storage_unittest.cc:32: const install_static::InstallDetails& details = please make this a parameterized test with a bool parameter for user vs. system level, and use install_static::ScopedInstallDetails in the test fixture to test both variants (see GoogleUpdateWinTest for an example of this). https://codereview.chromium.org/2889323004/diff/100001/chrome/installer/util/... chrome/installer/util/experiment_storage_unittest.cc:35: registry_util::RegistryOverrideManager override_manager; this needs to be a member variable of the class so that the override is alive for the actual test. https://codereview.chromium.org/2889323004/diff/100001/chrome/installer/util/... chrome/installer/util/experiment_storage_unittest.cc:37: GoogleUpdateSettings::SetGoogleUpdateIntegrationForTesting(true); i propose getting rid of this in favor of moving the kUseGoogleUpdateIntegration check from {Set,Read}ExperimentLabels into chrome/browser/metrics/variations/chrome_variations_service_client.cc. i think this will allow TestLoadStoreMetrics and TestLoadStoreExperiment to work in Chromium builds. wdyt? https://codereview.chromium.org/2889323004/diff/100001/chrome/installer/util/... chrome/installer/util/experiment_storage_unittest.cc:52: ASSERT_EQ(encoded_metrics, base::ASCIIToUTF16("5BIMD4IA")); move this up so it immediately follows line 48, and change it to EXPECT_EQ https://codereview.chromium.org/2889323004/diff/100001/chrome/installer/util/... chrome/installer/util/experiment_storage_unittest.cc:55: ASSERT_EQ(encoded_metrics, decoded_metrics_str); EXPECT rather than ASSERT, and inline the call to ExperimentStorage::EncodeMetrics(decoded_metrics). https://codereview.chromium.org/2889323004/diff/100001/chrome/installer/util/... chrome/installer/util/experiment_storage_unittest.cc:59: #if defined(GOOGLE_CHROME_BUILD) i don't think this is needed here https://codereview.chromium.org/2889323004/diff/100001/chrome/installer/util/... chrome/installer/util/experiment_storage_unittest.cc:99: } // namespace installer nit: blank line before this https://codereview.chromium.org/2889323004/diff/100001/chrome/installer/util/... File chrome/installer/util/experiment_unittest.cc (right): https://codereview.chromium.org/2889323004/diff/100001/chrome/installer/util/... chrome/installer/util/experiment_unittest.cc:36: ASSERT_EQ(metrics.state, experiment.state()); please use EXPECT_ rather than ASSERT_ for failures that aren't truly fatal. for these, i don't see an obvious reason why the test should abort on the first failure.
Ready for second pass review. https://codereview.chromium.org/2889323004/diff/100001/chrome/installer/util/... File chrome/installer/util/experiment.cc (right): https://codereview.chromium.org/2889323004/diff/100001/chrome/installer/util/... chrome/installer/util/experiment.cc:15: return round(log(x) / log(b)); On 2017/05/24 13:43:38, grt (UTC plus 2) wrote: > nit: std::round Done. https://codereview.chromium.org/2889323004/diff/100001/chrome/installer/util/... chrome/installer/util/experiment.cc:22: return exp(log(max_val - 1) / (1 << bits)); On 2017/05/24 13:43:38, grt (UTC plus 2) wrote: > std::exp, std::log Done. https://codereview.chromium.org/2889323004/diff/100001/chrome/installer/util/... chrome/installer/util/experiment.cc:41: static_cast<int>(round(pow(log_base, metrics_.last_used_bucket) - 1)); On 2017/05/24 13:43:38, grt (UTC plus 2) wrote: > std::pow Done. https://codereview.chromium.org/2889323004/diff/100001/chrome/installer/util/... File chrome/installer/util/experiment_metrics.cc (right): https://codereview.chromium.org/2889323004/diff/100001/chrome/installer/util/... chrome/installer/util/experiment_metrics.cc:9: // static On 2017/05/24 13:43:38, grt (UTC plus 2) wrote: > not static Done. https://codereview.chromium.org/2889323004/diff/100001/chrome/installer/util/... File chrome/installer/util/experiment_metrics.h (right): https://codereview.chromium.org/2889323004/diff/100001/chrome/installer/util/... chrome/installer/util/experiment_metrics.h:8: #include "base/time/time.h" On 2017/05/24 13:43:38, grt (UTC plus 2) wrote: > unused Done. https://codereview.chromium.org/2889323004/diff/100001/chrome/installer/util/... File chrome/installer/util/experiment_storage.cc (right): https://codereview.chromium.org/2889323004/diff/100001/chrome/installer/util/... chrome/installer/util/experiment_storage.cc:51: static constexpr int kSessionLengthLowestBit = 0; On 2017/05/24 13:43:38, grt (UTC plus 2) wrote: > nit: omit "static" in an unnamed namespace Done. https://codereview.chromium.org/2889323004/diff/100001/chrome/installer/util/... chrome/installer/util/experiment_storage.cc:280: << "Can not read an int of length " << len << " from " << value; On 2017/05/24 13:43:38, grt (UTC plus 2) wrote: > oops :-) Done. https://codereview.chromium.org/2889323004/diff/100001/chrome/installer/util/... File chrome/installer/util/experiment_storage_unittest.cc (right): https://codereview.chromium.org/2889323004/diff/100001/chrome/installer/util/... chrome/installer/util/experiment_storage_unittest.cc:32: const install_static::InstallDetails& details = On 2017/05/24 13:43:39, grt (UTC plus 2) wrote: > please make this a parameterized test with a bool parameter for user vs. system > level, and use install_static::ScopedInstallDetails in the test fixture to test > both variants (see GoogleUpdateWinTest for an example of this). Done. https://codereview.chromium.org/2889323004/diff/100001/chrome/installer/util/... chrome/installer/util/experiment_storage_unittest.cc:35: registry_util::RegistryOverrideManager override_manager; On 2017/05/24 13:43:39, grt (UTC plus 2) wrote: > this needs to be a member variable of the class so that the override is alive > for the actual test. Done. https://codereview.chromium.org/2889323004/diff/100001/chrome/installer/util/... chrome/installer/util/experiment_storage_unittest.cc:37: GoogleUpdateSettings::SetGoogleUpdateIntegrationForTesting(true); On 2017/05/24 13:43:39, grt (UTC plus 2) wrote: > i propose getting rid of this in favor of moving the kUseGoogleUpdateIntegration > check from {Set,Read}ExperimentLabels into > chrome/browser/metrics/variations/chrome_variations_service_client.cc. i think > this will allow TestLoadStoreMetrics and TestLoadStoreExperiment to work in > Chromium builds. wdyt? SGTM. https://codereview.chromium.org/2889323004/diff/100001/chrome/installer/util/... chrome/installer/util/experiment_storage_unittest.cc:52: ASSERT_EQ(encoded_metrics, base::ASCIIToUTF16("5BIMD4IA")); On 2017/05/24 13:43:39, grt (UTC plus 2) wrote: > move this up so it immediately follows line 48, and change it to EXPECT_EQ Done. https://codereview.chromium.org/2889323004/diff/100001/chrome/installer/util/... chrome/installer/util/experiment_storage_unittest.cc:55: ASSERT_EQ(encoded_metrics, decoded_metrics_str); On 2017/05/24 13:43:38, grt (UTC plus 2) wrote: > EXPECT rather than ASSERT, and inline the call to > ExperimentStorage::EncodeMetrics(decoded_metrics). Done. https://codereview.chromium.org/2889323004/diff/100001/chrome/installer/util/... chrome/installer/util/experiment_storage_unittest.cc:59: #if defined(GOOGLE_CHROME_BUILD) On 2017/05/24 13:43:38, grt (UTC plus 2) wrote: > i don't think this is needed here Done.
this is coming along nicely! https://codereview.chromium.org/2889323004/diff/140001/chrome/browser/metrics... File chrome/browser/metrics/variations/chrome_variations_service_client.cc (right): https://codereview.chromium.org/2889323004/diff/140001/chrome/browser/metrics... chrome/browser/metrics/variations/chrome_variations_service_client.cc:19: #include "chrome/installer/util/google_update_settings.h" #include "chrome/install_static/install_modes.h" https://codereview.chromium.org/2889323004/diff/140001/chrome/installer/util/... File chrome/installer/util/experiment.cc (right): https://codereview.chromium.org/2889323004/diff/140001/chrome/installer/util/... chrome/installer/util/experiment.cc:13: // Returns closest integer of logarithm of |x| with base |b|. put these helpers in an unnamed namespace (and drop "static"). https://codereview.chromium.org/2889323004/diff/140001/chrome/installer/util/... chrome/installer/util/experiment.cc:35: void Experiment::InitializeFromMetrics(const ExperimentMetrics& metrics) { this will only be called when |metrics| is in an initial state so it doesn't need to be able to reconstitute any of the fields that are set from group assignment on. may as well add a DCHECK that |metrics| is in the initial state here. https://codereview.chromium.org/2889323004/diff/140001/chrome/installer/util/... chrome/installer/util/experiment.cc:91: metrics_.SetState(state); may has well drop this setter and use metrics_.state = state; for consistency with the other setters. https://codereview.chromium.org/2889323004/diff/140001/chrome/installer/util/... chrome/installer/util/experiment.cc:118: metrics_.last_used_bucket = LogFloor(1 + days, log_base); how about: LogFloor(std::min(days, ExperimentMetrics::kMaxLastUsed) + 1, log_base); and get rid of the if below? comment applies to other setters below, too https://codereview.chromium.org/2889323004/diff/140001/chrome/installer/util/... chrome/installer/util/experiment.cc:129: metrics_.toast_count = count; std::min(count, ExperimentMetrics::kMaxToastCount) also, wdyt of moving this into a setter on ExperimentMetrics? or maybe creating a setter that DCHECKs that |count| <= kMaxToastCount? comment applies to all EM fields. https://codereview.chromium.org/2889323004/diff/140001/chrome/installer/util/... chrome/installer/util/experiment.cc:135: void Experiment::SetDisplayTime(const base::Time& time) { Time and TimeTicks should be passed by value rather than constref as per comment in base/time.h https://codereview.chromium.org/2889323004/diff/140001/chrome/installer/util/... File chrome/installer/util/experiment.h (right): https://codereview.chromium.org/2889323004/diff/140001/chrome/installer/util/... chrome/installer/util/experiment.h:53: // Time delta between user session start and action taken on toast display. this is the delta between presentation and action, no? https://codereview.chromium.org/2889323004/diff/140001/chrome/installer/util/... chrome/installer/util/experiment.h:93: base::TimeDelta action_delay_; hmm, i'm not sure why i called this action_delay_ here but display_time_bucket in metrics. i think i like display_time_ better than action_delay_. wdyt? https://codereview.chromium.org/2889323004/diff/140001/chrome/installer/util/... File chrome/installer/util/experiment_metrics.h (right): https://codereview.chromium.org/2889323004/diff/140001/chrome/installer/util/... chrome/installer/util/experiment_metrics.h:101: // into cohorts for analysing retention. (25 May 2017 00:00:00 PST) PST -> UTC? base::Time is in UTC https://codereview.chromium.org/2889323004/diff/140001/chrome/installer/util/... chrome/installer/util/experiment_metrics.h:102: static constexpr int64_t kExperimentStartSeconds = 1495695600L; nit: remove 'L' https://codereview.chromium.org/2889323004/diff/140001/chrome/installer/util/... chrome/installer/util/experiment_metrics.h:146: // The number of days that have passed since 2017-05-20 on the first day the 20 -> 25 https://codereview.chromium.org/2889323004/diff/140001/chrome/installer/util/... File chrome/installer/util/experiment_storage.cc (right): https://codereview.chromium.org/2889323004/diff/140001/chrome/installer/util/... chrome/installer/util/experiment_storage.cc:294: for (int i = 0; i < metrics_data.size(); ++i) { int -> size_t https://codereview.chromium.org/2889323004/diff/140001/chrome/installer/util/... chrome/installer/util/experiment_storage.cc:365: for (int i = 0; i < metrics_data.size(); ++i) { int -> size_t https://codereview.chromium.org/2889323004/diff/140001/chrome/installer/util/... File chrome/installer/util/experiment_storage.h (right): https://codereview.chromium.org/2889323004/diff/140001/chrome/installer/util/... chrome/installer/util/experiment_storage.h:110: static void ExperimentStorage::SetUint64Bits(int value, nit: omit "ExperimentStorage::" https://codereview.chromium.org/2889323004/diff/140001/chrome/installer/util/... chrome/installer/util/experiment_storage.h:143: // Returns the instance's task runner for COM usage. The runner is created on oops -- please delete this and the task_runner_ member (and clean up the forward decls/includes). they are leftover from a failed attempt to update the "client" value. https://codereview.chromium.org/2889323004/diff/140001/chrome/installer/util/... chrome/installer/util/experiment_storage.h:150: FRIEND_TEST_ALL_PREFIXES(ExperimentStorageTest, TestEncodeDecodeMetrics); nit: move these up to the top of the private: section https://codereview.chromium.org/2889323004/diff/140001/chrome/installer/util/... File chrome/installer/util/experiment_storage_unittest.cc (right): https://codereview.chromium.org/2889323004/diff/140001/chrome/installer/util/... chrome/installer/util/experiment_storage_unittest.cc:19: class ExperimentStorageTest : public ::testing::TestWithParam<bool> { please document the parameter https://codereview.chromium.org/2889323004/diff/140001/chrome/installer/util/... chrome/installer/util/experiment_storage_unittest.cc:25: void TestExperimentMetrics(ExperimentMetrics* metrics) { same comment regarding this function name as in other file https://codereview.chromium.org/2889323004/diff/140001/chrome/installer/util/... chrome/installer/util/experiment_storage_unittest.cc:36: void SetUp() { nit: override https://codereview.chromium.org/2889323004/diff/140001/chrome/installer/util/... chrome/installer/util/experiment_storage_unittest.cc:44: key.Create( ASSERT_EQ(ERROR_SUCCESS, key.Create(..)); https://codereview.chromium.org/2889323004/diff/140001/chrome/installer/util/... chrome/installer/util/experiment_storage_unittest.cc:47: KEY_QUERY_VALUE); KEY_WOW64_32KEY | KEY_QUERY_VALUE https://codereview.chromium.org/2889323004/diff/140001/chrome/installer/util/... chrome/installer/util/experiment_storage_unittest.cc:54: }; DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/2889323004/diff/140001/chrome/installer/util/... chrome/installer/util/experiment_storage_unittest.cc:60: EXPECT_EQ(encoded_metrics, base::ASCIIToUTF16("5BIMD4IA")); base::ASCIIToUTF16("5BIMD4IA") -> L"5BIMD4IA" https://codereview.chromium.org/2889323004/diff/140001/chrome/installer/util/... chrome/installer/util/experiment_storage_unittest.cc:64: EXPECT_EQ(encoded_metrics, ExperimentStorage::EncodeMetrics(decoded_metrics)); ? EXPECT_EQ(metrics, decoded_metrics); https://codereview.chromium.org/2889323004/diff/140001/chrome/installer/util/... chrome/installer/util/experiment_storage_unittest.cc:65: } could you add a test for an instance with every field at max value? https://codereview.chromium.org/2889323004/diff/140001/chrome/installer/util/... chrome/installer/util/experiment_storage_unittest.cc:86: EXPECT_EQ(stored_experiment.state(), ExperimentMetrics::kGroupAssigned); can you compare the two instances directly (EXPECT_EQ(experiment, stored_experiment);)? https://codereview.chromium.org/2889323004/diff/140001/chrome/installer/util/... chrome/installer/util/experiment_storage_unittest.cc:98: EXPECT_EQ(ExperimentStorage::EncodeMetrics(stored_metrics), could you compare the instances rather than their encodings? https://codereview.chromium.org/2889323004/diff/140001/chrome/installer/util/... chrome/installer/util/experiment_storage_unittest.cc:109: } // namespace installer nit: blank line above this https://codereview.chromium.org/2889323004/diff/140001/chrome/installer/util/... File chrome/installer/util/experiment_unittest.cc (right): https://codereview.chromium.org/2889323004/diff/140001/chrome/installer/util/... chrome/installer/util/experiment_unittest.cc:17: void TestExperimentMetrics(ExperimentMetrics* metrics) { nit: this name makes me think that this function is somehow testing |metrics|. how about PopulateMetricsWithTestData? alternatively, move this initialization into TestInitializeFromMetrics since that's the only place it's used. doing so makes it easier to eyeball the various checks there. https://codereview.chromium.org/2889323004/diff/140001/chrome/installer/util/... chrome/installer/util/experiment_unittest.cc:36: ASSERT_EQ(metrics.state, experiment.state()); please use EXPECT_* rather than ASSERT_* for cases where it's okay for the test to proceed on failure. in these cases here, there's no reason not to evaluate all of the conditions if any one fails. ASSERT_* is great for making sure the test stops if some prerequisite isn't satisfied. https://codereview.chromium.org/2889323004/diff/140001/chrome/installer/util/... chrome/installer/util/experiment_unittest.cc:53: TEST_F(ExperimentTest, TestSetters) { would you break this up into individual test functions for each Experiment method? it's easier to audit them and will be more sensible should one of them start failing down the road. https://codereview.chromium.org/2889323004/diff/140001/chrome/installer/util/... chrome/installer/util/experiment_unittest.cc:67: experiment.SetDisplayTime(test_display_time); please add a test to confirm that SetDisplayTime(25 may 2017) results in experiment.metrics().first_toast_offset == 0. https://codereview.chromium.org/2889323004/diff/140001/chrome/installer/util/... chrome/installer/util/experiment_unittest.cc:88: ASSERT_EQ(3, experiment.metrics().toast_hour); this one will be hard to test since it's tz-dependent. rather than testing an explicit value, how about testing that the value from two toastings an hour apart differ by one? alternatively, use Time::FromLocalExploded to compute a display time specifically for testing this value. https://codereview.chromium.org/2889323004/diff/140001/chrome/installer/util/... File chrome/installer/util/google_update_settings.cc (left): https://codereview.chromium.org/2889323004/diff/140001/chrome/installer/util/... chrome/installer/util/google_update_settings.cc:854: // There is nothing to do if this brand does not support integration with please update GoogleUpdateSettingsTest so that it tests these functions in Chromium builds. also, please pull this part of the CL out into its own change. it is a good general improvement that stands on its own.
Ready for next pass https://codereview.chromium.org/2889323004/diff/140001/chrome/browser/metrics... File chrome/browser/metrics/variations/chrome_variations_service_client.cc (right): https://codereview.chromium.org/2889323004/diff/140001/chrome/browser/metrics... chrome/browser/metrics/variations/chrome_variations_service_client.cc:19: #include "chrome/installer/util/google_update_settings.h" On 2017/05/31 12:24:09, grt (UTC plus 2) wrote: > #include "chrome/install_static/install_modes.h" Done. (Moving to separate CL) https://codereview.chromium.org/2889323004/diff/140001/chrome/installer/util/... File chrome/installer/util/experiment.cc (right): https://codereview.chromium.org/2889323004/diff/140001/chrome/installer/util/... chrome/installer/util/experiment.cc:13: // Returns closest integer of logarithm of |x| with base |b|. On 2017/05/31 12:24:09, grt (UTC plus 2) wrote: > put these helpers in an unnamed namespace (and drop "static"). Done. https://codereview.chromium.org/2889323004/diff/140001/chrome/installer/util/... chrome/installer/util/experiment.cc:35: void Experiment::InitializeFromMetrics(const ExperimentMetrics& metrics) { On 2017/05/31 12:24:09, grt (UTC plus 2) wrote: > this will only be called when |metrics| is in an initial state so it doesn't > need to be able to reconstitute any of the fields that are set from group > assignment on. may as well add a DCHECK that |metrics| is in the initial state > here. Done. https://codereview.chromium.org/2889323004/diff/140001/chrome/installer/util/... chrome/installer/util/experiment.cc:91: metrics_.SetState(state); On 2017/05/31 12:24:09, grt (UTC plus 2) wrote: > may has well drop this setter and use metrics_.state = state; for consistency > with the other setters. Done. https://codereview.chromium.org/2889323004/diff/140001/chrome/installer/util/... chrome/installer/util/experiment.cc:118: metrics_.last_used_bucket = LogFloor(1 + days, log_base); On 2017/05/31 12:24:09, grt (UTC plus 2) wrote: > how about: > LogFloor(std::min(days, ExperimentMetrics::kMaxLastUsed) + 1, log_base); > and get rid of the if below? > > comment applies to other setters below, too Done. https://codereview.chromium.org/2889323004/diff/140001/chrome/installer/util/... chrome/installer/util/experiment.cc:129: metrics_.toast_count = count; On 2017/05/31 12:24:09, grt (UTC plus 2) wrote: > std::min(count, ExperimentMetrics::kMaxToastCount) > > also, wdyt of moving this into a setter on ExperimentMetrics? or maybe creating > a setter that DCHECKs that |count| <= kMaxToastCount? comment applies to all EM > fields. Done. https://codereview.chromium.org/2889323004/diff/140001/chrome/installer/util/... chrome/installer/util/experiment.cc:135: void Experiment::SetDisplayTime(const base::Time& time) { On 2017/05/31 12:24:09, grt (UTC plus 2) wrote: > Time and TimeTicks should be passed by value rather than constref as per comment > in base/time.h Done. https://codereview.chromium.org/2889323004/diff/140001/chrome/installer/util/... File chrome/installer/util/experiment.h (right): https://codereview.chromium.org/2889323004/diff/140001/chrome/installer/util/... chrome/installer/util/experiment.h:53: // Time delta between user session start and action taken on toast display. On 2017/05/31 12:24:09, grt (UTC plus 2) wrote: > this is the delta between presentation and action, no? Done. https://codereview.chromium.org/2889323004/diff/140001/chrome/installer/util/... chrome/installer/util/experiment.h:93: base::TimeDelta action_delay_; On 2017/05/31 12:24:09, grt (UTC plus 2) wrote: > hmm, i'm not sure why i called this action_delay_ here but display_time_bucket > in metrics. i think i like display_time_ better than action_delay_. wdyt? The name of registry key is ActionDelay, so, I like action_delay as better name. Modified to action_delay (and action_delay_bucket) in all places. https://codereview.chromium.org/2889323004/diff/140001/chrome/installer/util/... File chrome/installer/util/experiment_metrics.h (right): https://codereview.chromium.org/2889323004/diff/140001/chrome/installer/util/... chrome/installer/util/experiment_metrics.h:101: // into cohorts for analysing retention. (25 May 2017 00:00:00 PST) On 2017/05/31 12:24:09, grt (UTC plus 2) wrote: > PST -> UTC? base::Time is in UTC The value is used to create TimeDelta which is tz independent. This is subtracted from display time before setting metrics.first_toast_offset (unit is days), so having this aligned with midnight PST when logs are cutoff means for daily analysis the cohort will be almost ready the next day. https://codereview.chromium.org/2889323004/diff/140001/chrome/installer/util/... chrome/installer/util/experiment_metrics.h:102: static constexpr int64_t kExperimentStartSeconds = 1495695600L; On 2017/05/31 12:24:09, grt (UTC plus 2) wrote: > nit: remove 'L' Done. https://codereview.chromium.org/2889323004/diff/140001/chrome/installer/util/... chrome/installer/util/experiment_metrics.h:146: // The number of days that have passed since 2017-05-20 on the first day the On 2017/05/31 12:24:09, grt (UTC plus 2) wrote: > 20 -> 25 Done. https://codereview.chromium.org/2889323004/diff/140001/chrome/installer/util/... File chrome/installer/util/experiment_storage.cc (right): https://codereview.chromium.org/2889323004/diff/140001/chrome/installer/util/... chrome/installer/util/experiment_storage.cc:294: for (int i = 0; i < metrics_data.size(); ++i) { On 2017/05/31 12:24:09, grt (UTC plus 2) wrote: > int -> size_t Done. https://codereview.chromium.org/2889323004/diff/140001/chrome/installer/util/... chrome/installer/util/experiment_storage.cc:365: for (int i = 0; i < metrics_data.size(); ++i) { On 2017/05/31 12:24:09, grt (UTC plus 2) wrote: > int -> size_t Done. https://codereview.chromium.org/2889323004/diff/140001/chrome/installer/util/... File chrome/installer/util/experiment_storage.h (right): https://codereview.chromium.org/2889323004/diff/140001/chrome/installer/util/... chrome/installer/util/experiment_storage.h:110: static void ExperimentStorage::SetUint64Bits(int value, On 2017/05/31 12:24:10, grt (UTC plus 2) wrote: > nit: omit "ExperimentStorage::" Done. https://codereview.chromium.org/2889323004/diff/140001/chrome/installer/util/... chrome/installer/util/experiment_storage.h:143: // Returns the instance's task runner for COM usage. The runner is created on On 2017/05/31 12:24:09, grt (UTC plus 2) wrote: > oops -- please delete this and the task_runner_ member (and clean up the forward > decls/includes). they are leftover from a failed attempt to update the "client" > value. Done. https://codereview.chromium.org/2889323004/diff/140001/chrome/installer/util/... chrome/installer/util/experiment_storage.h:150: FRIEND_TEST_ALL_PREFIXES(ExperimentStorageTest, TestEncodeDecodeMetrics); On 2017/05/31 12:24:09, grt (UTC plus 2) wrote: > nit: move these up to the top of the private: section Done. https://codereview.chromium.org/2889323004/diff/140001/chrome/installer/util/... File chrome/installer/util/experiment_storage_unittest.cc (right): https://codereview.chromium.org/2889323004/diff/140001/chrome/installer/util/... chrome/installer/util/experiment_storage_unittest.cc:19: class ExperimentStorageTest : public ::testing::TestWithParam<bool> { On 2017/05/31 12:24:10, grt (UTC plus 2) wrote: > please document the parameter Done. https://codereview.chromium.org/2889323004/diff/140001/chrome/installer/util/... chrome/installer/util/experiment_storage_unittest.cc:25: void TestExperimentMetrics(ExperimentMetrics* metrics) { On 2017/05/31 12:24:10, grt (UTC plus 2) wrote: > same comment regarding this function name as in other file Done. https://codereview.chromium.org/2889323004/diff/140001/chrome/installer/util/... chrome/installer/util/experiment_storage_unittest.cc:36: void SetUp() { On 2017/05/31 12:24:10, grt (UTC plus 2) wrote: > nit: override Done. https://codereview.chromium.org/2889323004/diff/140001/chrome/installer/util/... chrome/installer/util/experiment_storage_unittest.cc:44: key.Create( On 2017/05/31 12:24:10, grt (UTC plus 2) wrote: > ASSERT_EQ(ERROR_SUCCESS, key.Create(..)); Done. https://codereview.chromium.org/2889323004/diff/140001/chrome/installer/util/... chrome/installer/util/experiment_storage_unittest.cc:47: KEY_QUERY_VALUE); On 2017/05/31 12:24:10, grt (UTC plus 2) wrote: > KEY_WOW64_32KEY | KEY_QUERY_VALUE Done. https://codereview.chromium.org/2889323004/diff/140001/chrome/installer/util/... chrome/installer/util/experiment_storage_unittest.cc:54: }; On 2017/05/31 12:24:10, grt (UTC plus 2) wrote: > DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/2889323004/diff/140001/chrome/installer/util/... chrome/installer/util/experiment_storage_unittest.cc:60: EXPECT_EQ(encoded_metrics, base::ASCIIToUTF16("5BIMD4IA")); On 2017/05/31 12:24:10, grt (UTC plus 2) wrote: > base::ASCIIToUTF16("5BIMD4IA") -> L"5BIMD4IA" Done. https://codereview.chromium.org/2889323004/diff/140001/chrome/installer/util/... chrome/installer/util/experiment_storage_unittest.cc:64: EXPECT_EQ(encoded_metrics, ExperimentStorage::EncodeMetrics(decoded_metrics)); On 2017/05/31 12:24:10, grt (UTC plus 2) wrote: > ? EXPECT_EQ(metrics, decoded_metrics); Was checking by re-encoding the decoding the metrics. Although agree it is better to check the individual values directly. Done. https://codereview.chromium.org/2889323004/diff/140001/chrome/installer/util/... chrome/installer/util/experiment_storage_unittest.cc:65: } On 2017/05/31 12:24:10, grt (UTC plus 2) wrote: > could you add a test for an instance with every field at max value? Done. Added one for max and one for all 0. https://codereview.chromium.org/2889323004/diff/140001/chrome/installer/util/... chrome/installer/util/experiment_storage_unittest.cc:86: EXPECT_EQ(stored_experiment.state(), ExperimentMetrics::kGroupAssigned); On 2017/05/31 12:24:10, grt (UTC plus 2) wrote: > can you compare the two instances directly (EXPECT_EQ(experiment, > stored_experiment);)? Done. (Tested with a small number of values set for experiment, since it is only testing experiment Load/store are working. Added more cases for Experiment class setters separately) https://codereview.chromium.org/2889323004/diff/140001/chrome/installer/util/... chrome/installer/util/experiment_storage_unittest.cc:98: EXPECT_EQ(ExperimentStorage::EncodeMetrics(stored_metrics), On 2017/05/31 12:24:10, grt (UTC plus 2) wrote: > could you compare the instances rather than their encodings? Done. Added an operator== in ExperimentMetrics (Since it is a struct storing plain data it can be generally useful outside test). https://codereview.chromium.org/2889323004/diff/140001/chrome/installer/util/... chrome/installer/util/experiment_storage_unittest.cc:109: } // namespace installer On 2017/05/31 12:24:10, grt (UTC plus 2) wrote: > nit: blank line above this Done. https://codereview.chromium.org/2889323004/diff/140001/chrome/installer/util/... File chrome/installer/util/experiment_unittest.cc (right): https://codereview.chromium.org/2889323004/diff/140001/chrome/installer/util/... chrome/installer/util/experiment_unittest.cc:17: void TestExperimentMetrics(ExperimentMetrics* metrics) { On 2017/05/31 12:24:10, grt (UTC plus 2) wrote: > nit: this name makes me think that this function is somehow testing |metrics|. > how about PopulateMetricsWithTestData? alternatively, move this initialization > into TestInitializeFromMetrics since that's the only place it's used. doing so > makes it easier to eyeball the various checks there. This is no longer needed for this test. https://codereview.chromium.org/2889323004/diff/140001/chrome/installer/util/... chrome/installer/util/experiment_unittest.cc:36: ASSERT_EQ(metrics.state, experiment.state()); On 2017/05/31 12:24:10, grt (UTC plus 2) wrote: > please use EXPECT_* rather than ASSERT_* for cases where it's okay for the test > to proceed on failure. in these cases here, there's no reason not to evaluate > all of the conditions if any one fails. ASSERT_* is great for making sure the > test stops if some prerequisite isn't satisfied. Done. https://codereview.chromium.org/2889323004/diff/140001/chrome/installer/util/... chrome/installer/util/experiment_unittest.cc:53: TEST_F(ExperimentTest, TestSetters) { On 2017/05/31 12:24:10, grt (UTC plus 2) wrote: > would you break this up into individual test functions for each Experiment > method? it's easier to audit them and will be more sensible should one of them > start failing down the road. Done. Added separate test. Left this test as it is since it is one overall test with all setter. https://codereview.chromium.org/2889323004/diff/140001/chrome/installer/util/... chrome/installer/util/experiment_unittest.cc:67: experiment.SetDisplayTime(test_display_time); On 2017/05/31 12:24:10, grt (UTC plus 2) wrote: > please add a test to confirm that SetDisplayTime(25 may 2017) results in > experiment.metrics().first_toast_offset == 0. Added in TestSetDisplayTime. (Although note that this is an invalid value, first valid cohort is 26th May. I haven't added a DCHECK for this because this is almost always the case since the day has passed.) https://codereview.chromium.org/2889323004/diff/140001/chrome/installer/util/... chrome/installer/util/experiment_unittest.cc:88: ASSERT_EQ(3, experiment.metrics().toast_hour); On 2017/05/31 12:24:10, grt (UTC plus 2) wrote: > this one will be hard to test since it's tz-dependent. rather than testing an > explicit value, how about testing that the value from two toastings an hour > apart differ by one? alternatively, use Time::FromLocalExploded to compute a > display time specifically for testing this value. Removed toast hour check from here and added a separate test for individual setter (TestSetDisplayTime). https://codereview.chromium.org/2889323004/diff/140001/chrome/installer/util/... File chrome/installer/util/google_update_settings.cc (left): https://codereview.chromium.org/2889323004/diff/140001/chrome/installer/util/... chrome/installer/util/google_update_settings.cc:854: // There is nothing to do if this brand does not support integration with On 2017/05/31 12:24:10, grt (UTC plus 2) wrote: > please update GoogleUpdateSettingsTest so that it tests these functions in > Chromium builds. > > also, please pull this part of the CL out into its own change. it is a good > general improvement that stands on its own. Done. (New CL for this https://codereview.chromium.org/2919963002)
this is looking really good. https://codereview.chromium.org/2889323004/diff/140001/chrome/installer/util/... File chrome/installer/util/experiment_metrics.h (right): https://codereview.chromium.org/2889323004/diff/140001/chrome/installer/util/... chrome/installer/util/experiment_metrics.h:101: // into cohorts for analysing retention. (25 May 2017 00:00:00 PST) On 2017/06/02 05:11:23, nikunjb wrote: > On 2017/05/31 12:24:09, grt (UTC plus 2) wrote: > > PST -> UTC? base::Time is in UTC > > The value is used to create TimeDelta which is tz independent. > > This is subtracted from display time before setting metrics.first_toast_offset > (unit is days), so having this aligned with midnight PST when logs are cutoff > means for daily analysis the cohort will be almost ready the next day. Groovy. Since it's an offset from the Unix epoch (which is UTC), I just wanted to verify that you really wanted midnight PST. It makes sense. https://codereview.chromium.org/2889323004/diff/150001/chrome/installer/util/... File chrome/installer/util/experiment.cc (right): https://codereview.chromium.org/2889323004/diff/150001/chrome/installer/util/... chrome/installer/util/experiment.cc:15: // Returns closest integer of logarithm of |x| with base |b|. nit: blank line before comment https://codereview.chromium.org/2889323004/diff/150001/chrome/installer/util/... chrome/installer/util/experiment.cc:28: int BucketToValue(int bucket, double bucket_base) { unused https://codereview.chromium.org/2889323004/diff/150001/chrome/installer/util/... chrome/installer/util/experiment.cc:103: metrics_.first_toast_offset = std::min( nit: reverse these since the std::min isn't needed if the offset is < 0 if (metrics_.first_toast_offset < 0) { metrics_.first_toast_offset = ExperimentMetrics::kMaxFirstToastOffset; } else { metrics_.first_toast_offset = std::min(...); } https://codereview.chromium.org/2889323004/diff/150001/chrome/installer/util/... chrome/installer/util/experiment.cc:108: metrics_.toast_hour = (time - time.LocalMidnight()).InHours(); do we want this only for the first toast rather than the latest one? https://codereview.chromium.org/2889323004/diff/150001/chrome/installer/util/... File chrome/installer/util/experiment_metrics.h (right): https://codereview.chromium.org/2889323004/diff/150001/chrome/installer/util/... chrome/installer/util/experiment_metrics.h:95: // Overloaded == operator which deep compare the contents of this instance. nit: in following with the "If this is trivial, just skip the comment." sentiment in https://google.github.io/styleguide/cppguide.html#Function_Comments, i think this comment should be removed. https://codereview.chromium.org/2889323004/diff/150001/chrome/installer/util/... File chrome/installer/util/experiment_storage.h (right): https://codereview.chromium.org/2889323004/diff/150001/chrome/installer/util/... chrome/installer/util/experiment_storage.h:11: #include "base/memory/ref_counted.h" unused https://codereview.chromium.org/2889323004/diff/150001/chrome/installer/util/... File chrome/installer/util/experiment_storage_unittest.cc (right): https://codereview.chromium.org/2889323004/diff/150001/chrome/installer/util/... chrome/installer/util/experiment_storage_unittest.cc:20: // Individual tests provide a parameter, which is true if chrome is installed chrome -> Chrome https://codereview.chromium.org/2889323004/diff/150001/chrome/installer/util/... chrome/installer/util/experiment_storage_unittest.cc:135: experiment.SetState(ExperimentMetrics::kGroupAssigned); remove -- AssignGroup() does this https://codereview.chromium.org/2889323004/diff/150001/chrome/installer/util/... File chrome/installer/util/experiment_unittest.cc (right): https://codereview.chromium.org/2889323004/diff/150001/chrome/installer/util/... chrome/installer/util/experiment_unittest.cc:15: class ExperimentTest : public ::testing::Test {}; nit: remove empty class and use TEST rather than TEST_F below https://codereview.chromium.org/2889323004/diff/150001/chrome/installer/util/... chrome/installer/util/experiment_unittest.cc:29: experiment.SetState(ExperimentMetrics::kGroupAssigned); nit: add an expectation above here about the state before setting it. maybe that it's unintiailized?
The CQ bit was checked by nikunjb@google.com 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/2889323004/diff/150001/chrome/installer/util/... File chrome/installer/util/experiment.cc (right): https://codereview.chromium.org/2889323004/diff/150001/chrome/installer/util/... chrome/installer/util/experiment.cc:15: // Returns closest integer of logarithm of |x| with base |b|. On 2017/06/02 11:30:46, grt (no reviews June 5) wrote: > nit: blank line before comment Done. https://codereview.chromium.org/2889323004/diff/150001/chrome/installer/util/... chrome/installer/util/experiment.cc:28: int BucketToValue(int bucket, double bucket_base) { On 2017/06/02 11:30:45, grt (no reviews June 5) wrote: > unused Done. https://codereview.chromium.org/2889323004/diff/150001/chrome/installer/util/... chrome/installer/util/experiment.cc:103: metrics_.first_toast_offset = std::min( On 2017/06/02 11:30:45, grt (no reviews June 5) wrote: > nit: reverse these since the std::min isn't needed if the offset is < 0 > if (metrics_.first_toast_offset < 0) { > metrics_.first_toast_offset = ExperimentMetrics::kMaxFirstToastOffset; > } else { > metrics_.first_toast_offset = std::min(...); > } Done. https://codereview.chromium.org/2889323004/diff/150001/chrome/installer/util/... chrome/installer/util/experiment.cc:108: metrics_.toast_hour = (time - time.LocalMidnight()).InHours(); On 2017/06/02 11:30:46, grt (no reviews June 5) wrote: > do we want this only for the first toast rather than the latest one? I was actually not sure, My reasoning for only doing it with first was that since metrics_ doesn't currently use latest_display_time, toast_hour will be more consistent with what is seen in metrics. Although since for this experiment if we are not going to retoast a user it makes sense to move this outside. https://codereview.chromium.org/2889323004/diff/150001/chrome/installer/util/... File chrome/installer/util/experiment_metrics.h (right): https://codereview.chromium.org/2889323004/diff/150001/chrome/installer/util/... chrome/installer/util/experiment_metrics.h:95: // Overloaded == operator which deep compare the contents of this instance. On 2017/06/02 11:30:46, grt (no reviews June 5) wrote: > nit: in following with the "If this is trivial, just skip the comment." > sentiment in > https://google.github.io/styleguide/cppguide.html#Function_Comments, i think > this comment should be removed. Done. https://codereview.chromium.org/2889323004/diff/150001/chrome/installer/util/... File chrome/installer/util/experiment_storage.h (right): https://codereview.chromium.org/2889323004/diff/150001/chrome/installer/util/... chrome/installer/util/experiment_storage.h:11: #include "base/memory/ref_counted.h" On 2017/06/02 11:30:46, grt (no reviews June 5) wrote: > unused Done. https://codereview.chromium.org/2889323004/diff/150001/chrome/installer/util/... File chrome/installer/util/experiment_storage_unittest.cc (right): https://codereview.chromium.org/2889323004/diff/150001/chrome/installer/util/... chrome/installer/util/experiment_storage_unittest.cc:20: // Individual tests provide a parameter, which is true if chrome is installed On 2017/06/02 11:30:46, grt (no reviews June 5) wrote: > chrome -> Chrome Done. https://codereview.chromium.org/2889323004/diff/150001/chrome/installer/util/... chrome/installer/util/experiment_storage_unittest.cc:135: experiment.SetState(ExperimentMetrics::kGroupAssigned); On 2017/06/02 11:30:46, grt (no reviews June 5) wrote: > remove -- AssignGroup() does this Done. https://codereview.chromium.org/2889323004/diff/150001/chrome/installer/util/... File chrome/installer/util/experiment_unittest.cc (right): https://codereview.chromium.org/2889323004/diff/150001/chrome/installer/util/... chrome/installer/util/experiment_unittest.cc:15: class ExperimentTest : public ::testing::Test {}; On 2017/06/02 11:30:46, grt (no reviews June 5) wrote: > nit: remove empty class and use TEST rather than TEST_F below Done. https://codereview.chromium.org/2889323004/diff/150001/chrome/installer/util/... chrome/installer/util/experiment_unittest.cc:29: experiment.SetState(ExperimentMetrics::kGroupAssigned); On 2017/06/02 11:30:46, grt (no reviews June 5) wrote: > nit: add an expectation above here about the state before setting it. maybe that > it's unintiailized? Done.
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 nikunjb@google.com 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.
https://codereview.chromium.org/2889323004/diff/150001/chrome/installer/util/... File chrome/installer/util/experiment_storage.cc (right): https://codereview.chromium.org/2889323004/diff/150001/chrome/installer/util/... chrome/installer/util/experiment_storage.cc:216: metrics.state != ExperimentMetrics::kUninitialized) { I this this addition is incorrect. LoadMetricsUnsafe will return true with an uninitialized |metrics| if there is no value in the registry. The body of this "if" clause is the right thing in this case -- return an uninitialized experiment.
Latest patch set 13 https://codereview.chromium.org/2889323004/diff/150001/chrome/installer/util/... File chrome/installer/util/experiment_storage.cc (right): https://codereview.chromium.org/2889323004/diff/150001/chrome/installer/util/... chrome/installer/util/experiment_storage.cc:216: metrics.state != ExperimentMetrics::kUninitialized) { On 2017/06/08 11:06:23, grt (UTC plus 2) wrote: > I this this addition is incorrect. LoadMetricsUnsafe will return true with an > uninitialized |metrics| if there is no value in the registry. The body of this > "if" clause is the right thing in this case -- return an uninitialized > experiment. Okay. Thanks for catching. Modified it. The reason for adding this was - preventing Initializing Experiment from Uninitialized ExperimentMetrics
are you waiting on anything else for this to undergo final review + commit? https://codereview.chromium.org/2889323004/diff/230001/chrome/installer/util/... File chrome/installer/util/experiment_storage.cc (right): https://codereview.chromium.org/2889323004/diff/230001/chrome/installer/util/... chrome/installer/util/experiment_storage.cc:218: if (metrics.state != ExperimentMetrics::kUninitialized) { i think this should be removed in favor of making InitializeFromMetrics do the right thing.
Yes I think it is ready for final review + commit. I am not blocked on anything currently (Uploaded new patch). https://codereview.chromium.org/2889323004/diff/230001/chrome/installer/util/... File chrome/installer/util/experiment_storage.cc (right): https://codereview.chromium.org/2889323004/diff/230001/chrome/installer/util/... chrome/installer/util/experiment_storage.cc:218: if (metrics.state != ExperimentMetrics::kUninitialized) { On 2017/06/09 08:38:06, grt (UTC plus 2) wrote: > i think this should be removed in favor of making InitializeFromMetrics do the > right thing. Done.
The CQ bit was checked by nikunjb@google.com 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.
some comments. more to come. thanks! https://codereview.chromium.org/2889323004/diff/250001/chrome/installer/util/... File chrome/installer/util/experiment.cc (right): https://codereview.chromium.org/2889323004/diff/250001/chrome/installer/util/... chrome/installer/util/experiment.cc:40: // Ignore uninitialized metrics. it shouldn't ignore, but rather make the state of |this| consistent with |metrics|. https://codereview.chromium.org/2889323004/diff/250001/chrome/installer/util/... File chrome/installer/util/experiment_metrics.h (right): https://codereview.chromium.org/2889323004/diff/250001/chrome/installer/util/... chrome/installer/util/experiment_metrics.h:102: // into cohorts for analysing retention. (25 May 2017 00:00:00 PST) wdyt of bumping this to today so that we have a little more runway in the experiment? https://codereview.chromium.org/2889323004/diff/250001/chrome/installer/util/... chrome/installer/util/experiment_metrics.h:111: // Maximum value of last used bucket. (7 bits, log scale). "in days and log scale" for consistency with the values below? https://codereview.chromium.org/2889323004/diff/250001/chrome/installer/util/... chrome/installer/util/experiment_metrics.h:149: int first_toast_offset = 0; nit: ?first_toast_offset_days? if you agree, please change kMaxFirstToastOffset to kMaxFirstToastOffsetDays as well. https://codereview.chromium.org/2889323004/diff/250001/chrome/installer/util/... File chrome/installer/util/experiment_storage.cc (right): https://codereview.chromium.org/2889323004/diff/250001/chrome/installer/util/... chrome/installer/util/experiment_storage.cc:87: *path = install_details.GetClientStateKeyPath().append(kRegKeyRetention); yikes! i think this needs a '\\' between the two components. how about putting the leading separator into the kRegKeyRetention constant and removing the one on line 94. i think we could use unittest coverage for this. could you add a test that calls StoreExperiment and then verifies that there's a key named "Retention" under either ClientState or ClientStateMedium (depending on whether the test is running for a user-level or a system-level install)? https://codereview.chromium.org/2889323004/diff/250001/chrome/installer/util/... chrome/installer/util/experiment_storage.cc:139: if (qword_value > std::numeric_limits<int64_t>::max()) huh. not sure why i wrote this -- it can't possibly be true. maybe i had uint64_t once upon a time. anyway, please remove this and the next line. handling qword_value < 0 is an interesting case. it's valid for a TimeDelta to be negative, but we would never expect that to be the case. how do you think we should handle that here? should this return false if qword_value < 0? should there be range checking in Experiment::SetUserSessionUptime and Experiment::SetActionDelay to set their fields to 0 if the delta is negative? https://codereview.chromium.org/2889323004/diff/250001/chrome/installer/util/... chrome/installer/util/experiment_storage.cc:141: *result = T::FromInternalValue(static_cast<int64_t>(qword_value)); this cast isn't needed, either. https://codereview.chromium.org/2889323004/diff/250001/chrome/installer/util/... chrome/installer/util/experiment_storage.cc:270: DCHECK(len > 0 && len <= 32 && low_bit + len <= 64); nit: 64 -> sizeof(source) * 8 https://codereview.chromium.org/2889323004/diff/250001/chrome/installer/util/... chrome/installer/util/experiment_storage.cc:270: DCHECK(len > 0 && len <= 32 && low_bit + len <= 64); nit: 32 -> sizeof(int) * 8 https://codereview.chromium.org/2889323004/diff/250001/chrome/installer/util/... chrome/installer/util/experiment_storage.cc:279: uint64_t* source) { nit: source->target https://codereview.chromium.org/2889323004/diff/250001/chrome/installer/util/... chrome/installer/util/experiment_storage.cc:280: DCHECK(len > 0 && len <= 32); nit: 32 -> sizeof(value) * 8 https://codereview.chromium.org/2889323004/diff/250001/chrome/installer/util/... File chrome/installer/util/experiment_storage.h (right): https://codereview.chromium.org/2889323004/diff/250001/chrome/installer/util/... chrome/installer/util/experiment_storage.h:96: // Returns exclusive access to the experiment storage. please add to this that the ExperimentStorage instance must outlive the returned Lock.
this is super. comments below. https://codereview.chromium.org/2889323004/diff/250001/chrome/installer/util/... File chrome/installer/util/experiment_storage.cc (right): https://codereview.chromium.org/2889323004/diff/250001/chrome/installer/util/... chrome/installer/util/experiment_storage.cc:269: int ExperimentStorage::ReadUint64Bits(uint64_t source, int len, int low_bit) { nit: len -> bit_length in keeping with the "don't abbreviate" bit in the style guide, and to make the "length in what?" question explicit in the name. same comment for SetUint64Bits below. https://codereview.chromium.org/2889323004/diff/250001/chrome/installer/util/... chrome/installer/util/experiment_storage.cc:271: uint64_t bit_mask = (uint64_t(1) << len) - 1; i think we generally use either reinterpret_cast or static_cast (as appropriate) rather than the functional notation you have here. in this case, you can change uint64_t(1) to 1ULL. please do the same on line 281 below. https://codereview.chromium.org/2889323004/diff/250001/chrome/installer/util/... chrome/installer/util/experiment_storage.cc:282: *source |= ((uint64_t(value) & bit_mask) << low_bit); please use static_cast<uint64_t>(value) here https://codereview.chromium.org/2889323004/diff/250001/chrome/installer/util/... chrome/installer/util/experiment_storage.cc:296: for (size_t i = 0; i < metrics_data.size(); ++i) { optional nit: omit braces https://codereview.chromium.org/2889323004/diff/250001/chrome/installer/util/... chrome/installer/util/experiment_storage.cc:328: int lowest_unused_bit = ExperimentMetrics::kGroupBits + kGroupLowestBit; nit: define this as kLowestUnusedBit up on line 67 so that it'll be obvious to update it if new fields are added. https://codereview.chromium.org/2889323004/diff/250001/chrome/installer/util/... chrome/installer/util/experiment_storage.cc:329: if (ReadUint64Bits(metrics_value, 64 - lowest_unused_bit, 64 -> sizeof(metrics_value) * 8 https://codereview.chromium.org/2889323004/diff/250001/chrome/installer/util/... chrome/installer/util/experiment_storage.cc:330: lowest_unused_bit) != 0) nit: add braces for multi-line conditional. alternatively, use ! rather than " != 0" if that makes it fit on one line. https://codereview.chromium.org/2889323004/diff/250001/chrome/installer/util/... File chrome/installer/util/experiment_storage_unittest.cc (right): https://codereview.chromium.org/2889323004/diff/250001/chrome/installer/util/... chrome/installer/util/experiment_storage_unittest.cc:6: unused? https://codereview.chromium.org/2889323004/diff/250001/chrome/installer/util/... chrome/installer/util/experiment_storage_unittest.cc:8: #include "base/strings/utf_string_conversions.h" unused? https://codereview.chromium.org/2889323004/diff/250001/chrome/installer/util/... File chrome/installer/util/experiment_unittest.cc (right): https://codereview.chromium.org/2889323004/diff/250001/chrome/installer/util/... chrome/installer/util/experiment_unittest.cc:1: // Copyright 2017 The Chromium Authors. All rights reserved. this set of tests is downright awesome!
https://codereview.chromium.org/2889323004/diff/250001/chrome/installer/util/... File chrome/installer/util/experiment_metrics.h (right): https://codereview.chromium.org/2889323004/diff/250001/chrome/installer/util/... chrome/installer/util/experiment_metrics.h:65: // User selected 'No Thanks' button from UI after toast was shown and No Thanks -> Open Chrome
PatchSet 15 with comments applied. https://codereview.chromium.org/2889323004/diff/250001/chrome/installer/util/... File chrome/installer/util/experiment.cc (right): https://codereview.chromium.org/2889323004/diff/250001/chrome/installer/util/... chrome/installer/util/experiment.cc:40: // Ignore uninitialized metrics. On 2017/06/09 14:04:22, grt (UTC plus 2) wrote: > it shouldn't ignore, but rather make the state of |this| consistent with > |metrics|. Done. One point to note regarding kUninitialized is that in our encoding we don't have space for kUninitialized (its value is -1). I am thinking to rely on uninitialized experiment label to be unique so as to filter pings from such labels if they get recorded. Added a test that InitializeFromMetrics unsets all Experiment states. https://codereview.chromium.org/2889323004/diff/250001/chrome/installer/util/... File chrome/installer/util/experiment_metrics.h (right): https://codereview.chromium.org/2889323004/diff/250001/chrome/installer/util/... chrome/installer/util/experiment_metrics.h:65: // User selected 'No Thanks' button from UI after toast was shown and On 2017/06/12 09:40:40, grt (UTC plus 2) wrote: > No Thanks -> Open Chrome Done. https://codereview.chromium.org/2889323004/diff/250001/chrome/installer/util/... chrome/installer/util/experiment_metrics.h:102: // into cohorts for analysing retention. (25 May 2017 00:00:00 PST) On 2017/06/09 14:04:22, grt (UTC plus 2) wrote: > wdyt of bumping this to today so that we have a little more runway in the > experiment? Done. Putting Tue Jun 13th. https://codereview.chromium.org/2889323004/diff/250001/chrome/installer/util/... chrome/installer/util/experiment_metrics.h:111: // Maximum value of last used bucket. (7 bits, log scale). On 2017/06/09 14:04:22, grt (UTC plus 2) wrote: > "in days and log scale" for consistency with the values below? Done. https://codereview.chromium.org/2889323004/diff/250001/chrome/installer/util/... chrome/installer/util/experiment_metrics.h:149: int first_toast_offset = 0; On 2017/06/09 14:04:22, grt (UTC plus 2) wrote: > nit: ?first_toast_offset_days? if you agree, please change kMaxFirstToastOffset > to kMaxFirstToastOffsetDays as well. Done. https://codereview.chromium.org/2889323004/diff/250001/chrome/installer/util/... File chrome/installer/util/experiment_storage.cc (right): https://codereview.chromium.org/2889323004/diff/250001/chrome/installer/util/... chrome/installer/util/experiment_storage.cc:87: *path = install_details.GetClientStateKeyPath().append(kRegKeyRetention); On 2017/06/09 14:04:22, grt (UTC plus 2) wrote: > yikes! i think this needs a '\\' between the two components. how about putting > the leading separator into the kRegKeyRetention constant and removing the one on > line 94. i think we could use unittest coverage for this. could you add a test > that calls StoreExperiment and then verifies that there's a key named > "Retention" under either ClientState or ClientStateMedium (depending on whether > the test is running for a user-level or a system-level install)? Done. https://codereview.chromium.org/2889323004/diff/250001/chrome/installer/util/... chrome/installer/util/experiment_storage.cc:139: if (qword_value > std::numeric_limits<int64_t>::max()) On 2017/06/09 14:04:22, grt (UTC plus 2) wrote: > huh. not sure why i wrote this -- it can't possibly be true. maybe i had > uint64_t once upon a time. anyway, please remove this and the next line. > > handling qword_value < 0 is an interesting case. it's valid for a TimeDelta to > be negative, but we would never expect that to be the case. how do you think we > should handle that here? should this return false if qword_value < 0? should > there be range checking in Experiment::SetUserSessionUptime and > Experiment::SetActionDelay to set their fields to 0 if the delta is negative? So, a general plan I have tried to follow is fine grained data in Experiment is not validated and stored raw. Validation is done only when converting Experiment data into metrics_. Also the way I thought of handling invalid Time is by assigning it to the max bucket instead of 0. (So essentially using max bucket as both overflow and underflow bucket). During analysis, I am planning to discard this bucket totally. The rationale is 0 is a bucket of importance for this analysis (e.g. I am hoping a lot of user with action delay of 0 minutes). The rationale for not validating Experiment data is not very big, but if we ever found a valid reason to upload invalid time, it will be hard but possible since raw values are kept. So, in this case I modified SetUserSessionUptime and SetActionDelay to handle negative TimeDelta before taking LogFloor. Do you think it is sufficient? https://codereview.chromium.org/2889323004/diff/250001/chrome/installer/util/... chrome/installer/util/experiment_storage.cc:141: *result = T::FromInternalValue(static_cast<int64_t>(qword_value)); On 2017/06/09 14:04:23, grt (UTC plus 2) wrote: > this cast isn't needed, either. Done. https://codereview.chromium.org/2889323004/diff/250001/chrome/installer/util/... chrome/installer/util/experiment_storage.cc:269: int ExperimentStorage::ReadUint64Bits(uint64_t source, int len, int low_bit) { On 2017/06/09 20:02:27, grt (UTC plus 2) wrote: > nit: len -> bit_length in keeping with the "don't abbreviate" bit in the style > guide, and to make the "length in what?" question explicit in the name. same > comment for SetUint64Bits below. Done. https://codereview.chromium.org/2889323004/diff/250001/chrome/installer/util/... chrome/installer/util/experiment_storage.cc:270: DCHECK(len > 0 && len <= 32 && low_bit + len <= 64); On 2017/06/09 14:04:22, grt (UTC plus 2) wrote: > nit: 32 -> sizeof(int) * 8 Done. https://codereview.chromium.org/2889323004/diff/250001/chrome/installer/util/... chrome/installer/util/experiment_storage.cc:270: DCHECK(len > 0 && len <= 32 && low_bit + len <= 64); On 2017/06/09 14:04:22, grt (UTC plus 2) wrote: > nit: 32 -> sizeof(int) * 8 Done. https://codereview.chromium.org/2889323004/diff/250001/chrome/installer/util/... chrome/installer/util/experiment_storage.cc:271: uint64_t bit_mask = (uint64_t(1) << len) - 1; On 2017/06/09 20:02:27, grt (UTC plus 2) wrote: > i think we generally use either reinterpret_cast or static_cast (as appropriate) > rather than the functional notation you have here. in this case, you can change > uint64_t(1) to 1ULL. please do the same on line 281 below. Done. https://codereview.chromium.org/2889323004/diff/250001/chrome/installer/util/... chrome/installer/util/experiment_storage.cc:279: uint64_t* source) { On 2017/06/09 14:04:22, grt (UTC plus 2) wrote: > nit: source->target Done. https://codereview.chromium.org/2889323004/diff/250001/chrome/installer/util/... chrome/installer/util/experiment_storage.cc:280: DCHECK(len > 0 && len <= 32); On 2017/06/09 14:04:22, grt (UTC plus 2) wrote: > nit: 32 -> sizeof(value) * 8 Done. https://codereview.chromium.org/2889323004/diff/250001/chrome/installer/util/... chrome/installer/util/experiment_storage.cc:282: *source |= ((uint64_t(value) & bit_mask) << low_bit); On 2017/06/09 20:02:27, grt (UTC plus 2) wrote: > please use static_cast<uint64_t>(value) here Done. https://codereview.chromium.org/2889323004/diff/250001/chrome/installer/util/... chrome/installer/util/experiment_storage.cc:296: for (size_t i = 0; i < metrics_data.size(); ++i) { On 2017/06/09 20:02:27, grt (UTC plus 2) wrote: > optional nit: omit braces Done. https://codereview.chromium.org/2889323004/diff/250001/chrome/installer/util/... chrome/installer/util/experiment_storage.cc:328: int lowest_unused_bit = ExperimentMetrics::kGroupBits + kGroupLowestBit; On 2017/06/09 20:02:27, grt (UTC plus 2) wrote: > nit: define this as kLowestUnusedBit up on line 67 so that it'll be obvious to > update it if new fields are added. Done. https://codereview.chromium.org/2889323004/diff/250001/chrome/installer/util/... chrome/installer/util/experiment_storage.cc:329: if (ReadUint64Bits(metrics_value, 64 - lowest_unused_bit, On 2017/06/09 20:02:27, grt (UTC plus 2) wrote: > 64 -> sizeof(metrics_value) * 8 Done. https://codereview.chromium.org/2889323004/diff/250001/chrome/installer/util/... chrome/installer/util/experiment_storage.cc:330: lowest_unused_bit) != 0) On 2017/06/09 20:02:27, grt (UTC plus 2) wrote: > nit: add braces for multi-line conditional. alternatively, use ! rather than " > != 0" if that makes it fit on one line. Done. https://codereview.chromium.org/2889323004/diff/250001/chrome/installer/util/... File chrome/installer/util/experiment_storage.h (right): https://codereview.chromium.org/2889323004/diff/250001/chrome/installer/util/... chrome/installer/util/experiment_storage.h:96: // Returns exclusive access to the experiment storage. On 2017/06/09 14:04:23, grt (UTC plus 2) wrote: > please add to this that the ExperimentStorage instance must outlive the returned > Lock. Done. For my understanding: What will happen if there are two instance of ExperimentStorage being created and try to mutate Experiment? My understanding: One of them will create the mutex in registry and other will get a handle to this. The first one to call AcquireLock will lock the registry mutex and second AcquireLock will wait for the Lock to be released which will happen when the return ptr for first goes out of scope. Is this accurate? https://codereview.chromium.org/2889323004/diff/250001/chrome/installer/util/... File chrome/installer/util/experiment_storage_unittest.cc (right): https://codereview.chromium.org/2889323004/diff/250001/chrome/installer/util/... chrome/installer/util/experiment_storage_unittest.cc:6: On 2017/06/09 20:02:27, grt (UTC plus 2) wrote: > unused? Done. https://codereview.chromium.org/2889323004/diff/250001/chrome/installer/util/... chrome/installer/util/experiment_storage_unittest.cc:8: #include "base/strings/utf_string_conversions.h" On 2017/06/09 20:02:27, grt (UTC plus 2) wrote: > unused? No, used for implicit conversion from string16 to StringPiece16 in DecodeMetrics https://codereview.chromium.org/2889323004/diff/250001/chrome/installer/util/... File chrome/installer/util/experiment_unittest.cc (right): https://codereview.chromium.org/2889323004/diff/250001/chrome/installer/util/... chrome/installer/util/experiment_unittest.cc:1: // Copyright 2017 The Chromium Authors. All rights reserved. On 2017/06/09 20:02:27, grt (UTC plus 2) wrote: > this set of tests is downright awesome! Acknowledged. Thanks.
https://codereview.chromium.org/2889323004/diff/250001/chrome/installer/util/... File chrome/installer/util/experiment.cc (right): https://codereview.chromium.org/2889323004/diff/250001/chrome/installer/util/... chrome/installer/util/experiment.cc:40: // Ignore uninitialized metrics. On 2017/06/13 00:27:35, nikunjb wrote: > On 2017/06/09 14:04:22, grt (UTC plus 2) wrote: > > it shouldn't ignore, but rather make the state of |this| consistent with > > |metrics|. > > Done. > > One point to note regarding kUninitialized is that in our encoding we don't have > space for kUninitialized (its value is -1). Right. We should never store uninitialized metrics to the registry, so this value should never be reported. > I am thinking to rely on > uninitialized experiment label to be unique so as to filter pings from such > labels if they get recorded. > > Added a test that InitializeFromMetrics unsets all Experiment states. https://codereview.chromium.org/2889323004/diff/250001/chrome/installer/util/... File chrome/installer/util/experiment_metrics.h (right): https://codereview.chromium.org/2889323004/diff/250001/chrome/installer/util/... chrome/installer/util/experiment_metrics.h:65: // User selected 'No Thanks' button from UI after toast was shown and On 2017/06/13 00:27:35, nikunjb wrote: > On 2017/06/12 09:40:40, grt (UTC plus 2) wrote: > > No Thanks -> Open Chrome > > Done. Looks like it didn't stick. ;-) https://codereview.chromium.org/2889323004/diff/250001/chrome/installer/util/... File chrome/installer/util/experiment_storage.h (right): https://codereview.chromium.org/2889323004/diff/250001/chrome/installer/util/... chrome/installer/util/experiment_storage.h:96: // Returns exclusive access to the experiment storage. On 2017/06/13 00:27:36, nikunjb wrote: > On 2017/06/09 14:04:23, grt (UTC plus 2) wrote: > > please add to this that the ExperimentStorage instance must outlive the > returned > > Lock. > > Done. > > For my understanding: > > What will happen if there are two instance of ExperimentStorage being created > and try to mutate Experiment? > > My understanding: > One of them will create the mutex in registry and other will get a handle to > this. The first one to call AcquireLock will lock the registry mutex and second > AcquireLock will wait for the Lock to be released which will happen when the > return ptr for first goes out of scope. > > Is this accurate? Yup! https://codereview.chromium.org/2889323004/diff/250001/chrome/installer/util/... File chrome/installer/util/experiment_storage_unittest.cc (right): https://codereview.chromium.org/2889323004/diff/250001/chrome/installer/util/... chrome/installer/util/experiment_storage_unittest.cc:8: #include "base/strings/utf_string_conversions.h" On 2017/06/13 00:27:36, nikunjb wrote: > On 2017/06/09 20:02:27, grt (UTC plus 2) wrote: > > unused? > > No, used for implicit conversion from string16 to StringPiece16 in DecodeMetrics seems like base/strings/string_piece.h should be included rather than utf_string_conversions.h, then. https://codereview.chromium.org/2889323004/diff/270001/chrome/installer/util/... File chrome/installer/util/experiment_metrics.h (right): https://codereview.chromium.org/2889323004/diff/270001/chrome/installer/util/... chrome/installer/util/experiment_metrics.h:147: // The number of days that have passed since (25 May 2017 00:00:00 PST) on 13 Jun https://codereview.chromium.org/2889323004/diff/270001/chrome/installer/util/... File chrome/installer/util/experiment_storage.cc (right): https://codereview.chromium.org/2889323004/diff/270001/chrome/installer/util/... chrome/installer/util/experiment_storage.cc:282: DCHECK(bit_length > 0 && bit_length <= sizeof(int) * 8 && sizeof(int) -> sizeof(value) https://codereview.chromium.org/2889323004/diff/270001/chrome/installer/util/... File chrome/installer/util/experiment_storage.h (right): https://codereview.chromium.org/2889323004/diff/270001/chrome/installer/util/... chrome/installer/util/experiment_storage.h:117: uint64_t* source); source -> target
https://codereview.chromium.org/2889323004/diff/270001/chrome/installer/util/... File chrome/installer/util/experiment_metrics.h (right): https://codereview.chromium.org/2889323004/diff/270001/chrome/installer/util/... chrome/installer/util/experiment_metrics.h:147: // The number of days that have passed since (25 May 2017 00:00:00 PST) on On 2017/06/13 12:24:48, grt (UTC plus 2) wrote: > 13 Jun Done. https://codereview.chromium.org/2889323004/diff/270001/chrome/installer/util/... File chrome/installer/util/experiment_storage.cc (right): https://codereview.chromium.org/2889323004/diff/270001/chrome/installer/util/... chrome/installer/util/experiment_storage.cc:282: DCHECK(bit_length > 0 && bit_length <= sizeof(int) * 8 && On 2017/06/13 12:24:48, grt (UTC plus 2) wrote: > sizeof(int) -> sizeof(value) Done. https://codereview.chromium.org/2889323004/diff/270001/chrome/installer/util/... File chrome/installer/util/experiment_storage.h (right): https://codereview.chromium.org/2889323004/diff/270001/chrome/installer/util/... chrome/installer/util/experiment_storage.h:117: uint64_t* source); On 2017/06/13 12:24:48, grt (UTC plus 2) wrote: > source -> target Done.
lgtm!
The CQ bit was checked by nikunjb@google.com 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_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
https://codereview.chromium.org/2889323004/diff/310001/chrome/installer/util/... File chrome/installer/util/experiment.cc (right): https://codereview.chromium.org/2889323004/diff/310001/chrome/installer/util/... chrome/installer/util/experiment.cc:51: return; nit: omit
Also fixed a small error caught by trybot when using sizeof(*) operator. https://codereview.chromium.org/2889323004/diff/310001/chrome/installer/util/... File chrome/installer/util/experiment.cc (right): https://codereview.chromium.org/2889323004/diff/310001/chrome/installer/util/... chrome/installer/util/experiment.cc:51: return; On 2017/06/14 21:13:51, grt (UTC plus 2) wrote: > nit: omit Done.
The CQ bit was checked by nikunjb@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from grt@chromium.org Link to the patchset: https://codereview.chromium.org/2889323004/#ps330001 (title: "Apply some comments and try bots errors")
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": 330001, "attempt_start_ts": 1497483742792210,
"parent_rev": "a238408784081bac273d488f8339ab45cc630aaa", "commit_rev":
"0667367bcef7b8105d76e500c80f265bbda69aed"}
Message was sent while issue was closed.
Description was changed from ========== Win 10 Inactive toast experiment metrics and storage modifications BUG=717091 ========== to ========== Win 10 Inactive toast experiment metrics and storage modifications BUG=717091 Review-Url: https://codereview.chromium.org/2889323004 Cr-Commit-Position: refs/heads/master@{#479571} Committed: https://chromium.googlesource.com/chromium/src/+/0667367bcef7b8105d76e500c80f... ==========
Message was sent while issue was closed.
Committed patchset #18 (id:330001) as https://chromium.googlesource.com/chromium/src/+/0667367bcef7b8105d76e500c80f... |
