Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1974593002:
Make the launch params the default client behavior for UMA 3g (Closed)
|
Created:
4 years, 7 months ago by gayane -on leave until 09-2017 Modified:
4 years, 7 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, asvitkine+watch_chromium.org, sdefresne+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMake the launch params the default client behavior for UMA 3g
BUG=455847
Committed: https://crrev.com/8ff878076934185e5679df6f75a26f0eff47da22
Cr-Commit-Position: refs/heads/master@{#394811}
Patch Set 1 : #
Total comments: 8
Patch Set 2 : #
Total comments: 12
Patch Set 3 : #
Total comments: 8
Patch Set 4 : #
Total comments: 8
Patch Set 5 : change DataUseTracker init logic #
Total comments: 2
Patch Set 6 : unused includes removed #Patch Set 7 : sync #Patch Set 8 : fix build presubmit warning #
Messages
Total messages: 38 (16 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:80001) has been deleted
Description was changed from ========== Add defaults for UMA 3g experiment params ========== to ========== Make the launch params the default client behavior for UMA 3g ==========
gayane@chromium.org changed reviewers: + asvitkine@chromium.org
Please have a look
https://codereview.chromium.org/1974593002/diff/100001/components/metrics/net... File components/metrics/net/cellular_logic_helper.cc (right): https://codereview.chromium.org/1974593002/diff/100001/components/metrics/net... components/metrics/net/cellular_logic_helper.cc:36: return false; I think we want to default to the new behavior when there's no field trial, so remove this if/return please. https://codereview.chromium.org/1974593002/diff/100001/components/metrics/net... components/metrics/net/cellular_logic_helper.cc:60: } Nit: Add // namespace metrics and an empty line above it. https://codereview.chromium.org/1974593002/diff/100001/components/metrics/net... File components/metrics/net/cellular_logic_helper.h (right): https://codereview.chromium.org/1974593002/diff/100001/components/metrics/net... components/metrics/net/cellular_logic_helper.h:12: base::TimeDelta GetUploadInterval(); Nit: Add a comment. https://codereview.chromium.org/1974593002/diff/100001/components/metrics/net... components/metrics/net/cellular_logic_helper.h:17: }; Nit: No ; and add // namespace metrics
Also please associate this with a crbug. On Fri, May 13, 2016 at 11:53 AM, <asvitkine@chromium.org> wrote: > > > https://codereview.chromium.org/1974593002/diff/100001/components/metrics/net... > File components/metrics/net/cellular_logic_helper.cc (right): > > > https://codereview.chromium.org/1974593002/diff/100001/components/metrics/net... > components/metrics/net/cellular_logic_helper.cc:36: return false; > I think we want to default to the new behavior when there's no field > trial, so remove this if/return please. > > > https://codereview.chromium.org/1974593002/diff/100001/components/metrics/net... > components/metrics/net/cellular_logic_helper.cc:60: } > Nit: Add // namespace metrics and an empty line above it. > > > https://codereview.chromium.org/1974593002/diff/100001/components/metrics/net... > File components/metrics/net/cellular_logic_helper.h (right): > > > https://codereview.chromium.org/1974593002/diff/100001/components/metrics/net... > components/metrics/net/cellular_logic_helper.h:12: base::TimeDelta > GetUploadInterval(); > Nit: Add a comment. > > > https://codereview.chromium.org/1974593002/diff/100001/components/metrics/net... > components/metrics/net/cellular_logic_helper.h:17: }; > Nit: No ; and add // namespace metrics > > https://codereview.chromium.org/1974593002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Description was changed from ========== Make the launch params the default client behavior for UMA 3g ========== to ========== Make the launch params the default client behavior for UMA 3g BUG=455847 ==========
Done. https://codereview.chromium.org/1974593002/diff/100001/components/metrics/net... File components/metrics/net/cellular_logic_helper.cc (right): https://codereview.chromium.org/1974593002/diff/100001/components/metrics/net... components/metrics/net/cellular_logic_helper.cc:36: return false; On 2016/05/13 15:53:59, Alexei Svitkine wrote: > I think we want to default to the new behavior when there's no field trial, so > remove this if/return please. Done. https://codereview.chromium.org/1974593002/diff/100001/components/metrics/net... components/metrics/net/cellular_logic_helper.cc:60: } On 2016/05/13 15:53:59, Alexei Svitkine wrote: > Nit: Add // namespace metrics and an empty line above it. Done. https://codereview.chromium.org/1974593002/diff/100001/components/metrics/net... File components/metrics/net/cellular_logic_helper.h (right): https://codereview.chromium.org/1974593002/diff/100001/components/metrics/net... components/metrics/net/cellular_logic_helper.h:12: base::TimeDelta GetUploadInterval(); On 2016/05/13 15:53:59, Alexei Svitkine wrote: > Nit: Add a comment. Done. https://codereview.chromium.org/1974593002/diff/100001/components/metrics/net... components/metrics/net/cellular_logic_helper.h:17: }; On 2016/05/13 15:53:59, Alexei Svitkine wrote: > Nit: No ; and add // namespace metrics Done.
https://codereview.chromium.org/1974593002/diff/120001/components/metrics/dat... File components/metrics/data_use_tracker.cc (right): https://codereview.chromium.org/1974593002/diff/120001/components/metrics/dat... components/metrics/data_use_tracker.cc:17: const int kDefaultUMAWeeklyQuotaBytes = 204800; Nit: Put these in the anon namespace below. Add comments. https://codereview.chromium.org/1974593002/diff/120001/components/metrics/net... File components/metrics/net/cellular_logic_helper.cc (right): https://codereview.chromium.org/1974593002/diff/120001/components/metrics/net... components/metrics/net/cellular_logic_helper.cc:21: const bool kDefaultCellularLogicOptimization = true; These should be in the anon namespace. https://codereview.chromium.org/1974593002/diff/120001/components/metrics/net... components/metrics/net/cellular_logic_helper.cc:33: bool IsCellularLogicEnabled() { Should we make this return false when platform is not android/ios? Otherwise it seems strange that we have an ifdef above on line 24 about it, but other callers of this function may get a different result. https://codereview.chromium.org/1974593002/diff/120001/components/metrics/net... components/metrics/net/cellular_logic_helper.cc:42: is_enabled = enabled == "true"; Nit: How about this structure: bool is_enabled = kDefaultCellularLogicEnabled; if (!enabled.empty()) is_enabled = (enabled == "true"); Same below. https://codereview.chromium.org/1974593002/diff/120001/components/metrics/net... File components/metrics/net/cellular_logic_helper.h (right): https://codereview.chromium.org/1974593002/diff/120001/components/metrics/net... components/metrics/net/cellular_logic_helper.h:17: bool IsCellularLogicEnabled(); Nit: Add an empty line below this. https://codereview.chromium.org/1974593002/diff/120001/components/metrics/net... components/metrics/net/cellular_logic_helper.h:18: } // namespace metrics Nit: 2 spaces before //
Thanks for the review. Have one more look. https://codereview.chromium.org/1974593002/diff/120001/components/metrics/dat... File components/metrics/data_use_tracker.cc (right): https://codereview.chromium.org/1974593002/diff/120001/components/metrics/dat... components/metrics/data_use_tracker.cc:17: const int kDefaultUMAWeeklyQuotaBytes = 204800; On 2016/05/13 18:37:13, Alexei Svitkine wrote: > Nit: Put these in the anon namespace below. Add comments. Done. https://codereview.chromium.org/1974593002/diff/120001/components/metrics/net... File components/metrics/net/cellular_logic_helper.cc (right): https://codereview.chromium.org/1974593002/diff/120001/components/metrics/net... components/metrics/net/cellular_logic_helper.cc:21: const bool kDefaultCellularLogicOptimization = true; On 2016/05/13 18:37:13, Alexei Svitkine wrote: > These should be in the anon namespace. Done. https://codereview.chromium.org/1974593002/diff/120001/components/metrics/net... components/metrics/net/cellular_logic_helper.cc:33: bool IsCellularLogicEnabled() { On 2016/05/13 18:37:13, Alexei Svitkine wrote: > Should we make this return false when platform is not android/ios? > > Otherwise it seems strange that we have an ifdef above on line 24 about it, but > other callers of this function may get a different result. Done. https://codereview.chromium.org/1974593002/diff/120001/components/metrics/net... components/metrics/net/cellular_logic_helper.cc:42: is_enabled = enabled == "true"; On 2016/05/13 18:37:13, Alexei Svitkine wrote: > Nit: How about this structure: > > bool is_enabled = kDefaultCellularLogicEnabled; > if (!enabled.empty()) > is_enabled = (enabled == "true"); > > Same below. Done. https://codereview.chromium.org/1974593002/diff/120001/components/metrics/net... File components/metrics/net/cellular_logic_helper.h (right): https://codereview.chromium.org/1974593002/diff/120001/components/metrics/net... components/metrics/net/cellular_logic_helper.h:17: bool IsCellularLogicEnabled(); On 2016/05/13 18:37:13, Alexei Svitkine wrote: > Nit: Add an empty line below this. Done. https://codereview.chromium.org/1974593002/diff/120001/components/metrics/net... components/metrics/net/cellular_logic_helper.h:18: } // namespace metrics On 2016/05/13 18:37:13, Alexei Svitkine wrote: > Nit: 2 spaces before // Done.
gayane@chromium.org changed reviewers: + lpromero@chromium.org
lpromero@chromium.org: Please review changes in ios/chrome/browser/metrics
https://codereview.chromium.org/1974593002/diff/140001/components/metrics/dat... File components/metrics/data_use_tracker.cc (right): https://codereview.chromium.org/1974593002/diff/140001/components/metrics/dat... components/metrics/data_use_tracker.cc:180: base::FieldTrialList::FindFullName("UMA_EnableCellularLogUpload"); We wanted to not have this experiment check anymore. https://codereview.chromium.org/1974593002/diff/140001/components/metrics/met... File components/metrics/metrics_service.cc (right): https://codereview.chromium.org/1974593002/diff/140001/components/metrics/met... components/metrics/metrics_service.cc:250: probability = kDefaultSamplingProbability; I think we discussed offline that we could just remove this. i.e. since we're not planning to use this sampling mechanism anymore, just get rid of the code for it. https://codereview.chromium.org/1974593002/diff/140001/components/metrics/net... File components/metrics/net/cellular_logic_helper.cc (right): https://codereview.chromium.org/1974593002/diff/140001/components/metrics/net... components/metrics/net/cellular_logic_helper.cc:24: } Nit: Add // namespace Also, an empty line above it. https://codereview.chromium.org/1974593002/diff/140001/components/metrics/net... components/metrics/net/cellular_logic_helper.cc:39: #endif I think you need an #else here else compiler may warn about unreachable code. However, maybe ifdef is not the right thing here. Because iOS we still want the defaults to be different (we don't want to enable this on iOS yet). So I suggest removing this ifdef and putting an ifdef around lines 22 and 23 with a different values for non-Android.
Thanks. PTAL https://codereview.chromium.org/1974593002/diff/140001/components/metrics/dat... File components/metrics/data_use_tracker.cc (right): https://codereview.chromium.org/1974593002/diff/140001/components/metrics/dat... components/metrics/data_use_tracker.cc:180: base::FieldTrialList::FindFullName("UMA_EnableCellularLogUpload"); On 2016/05/13 20:21:09, Alexei Svitkine wrote: > We wanted to not have this experiment check anymore. Done. https://codereview.chromium.org/1974593002/diff/140001/components/metrics/met... File components/metrics/metrics_service.cc (right): https://codereview.chromium.org/1974593002/diff/140001/components/metrics/met... components/metrics/metrics_service.cc:250: probability = kDefaultSamplingProbability; On 2016/05/13 20:21:09, Alexei Svitkine wrote: > I think we discussed offline that we could just remove this. > > i.e. since we're not planning to use this sampling mechanism anymore, just get > rid of the code for it. Done. https://codereview.chromium.org/1974593002/diff/140001/components/metrics/net... File components/metrics/net/cellular_logic_helper.cc (right): https://codereview.chromium.org/1974593002/diff/140001/components/metrics/net... components/metrics/net/cellular_logic_helper.cc:24: } On 2016/05/13 20:21:09, Alexei Svitkine wrote: > Nit: Add // namespace > > Also, an empty line above it. Done. https://codereview.chromium.org/1974593002/diff/140001/components/metrics/net... components/metrics/net/cellular_logic_helper.cc:39: #endif On 2016/05/13 20:21:09, Alexei Svitkine wrote: > I think you need an #else here else compiler may warn about unreachable code. > > However, maybe ifdef is not the right thing here. > > Because iOS we still want the defaults to be different (we don't want to enable > this on iOS yet). So I suggest removing this ifdef and putting an ifdef around > lines 22 and 23 with a different values for non-Android. Done.
https://codereview.chromium.org/1974593002/diff/160001/components/metrics/dat... File components/metrics/data_use_tracker.cc (right): https://codereview.chromium.org/1974593002/diff/160001/components/metrics/dat... components/metrics/data_use_tracker.cc:22: const double kDefaultUMARatio = 0.05; Are these defaults safe for non-Android platforms? I noticed you stopped returning false from either of the functions below. https://codereview.chromium.org/1974593002/diff/160001/components/metrics/dat... components/metrics/data_use_tracker.cc:52: "Uma_Ratio") != "") { We would want this logic to be changed too, since we want this to apply even when the experiment config doesn't exist/specify these params.
https://codereview.chromium.org/1974593002/diff/160001/components/metrics/dat... File components/metrics/data_use_tracker.cc (right): https://codereview.chromium.org/1974593002/diff/160001/components/metrics/dat... components/metrics/data_use_tracker.cc:22: const double kDefaultUMARatio = 0.05; On 2016/05/13 20:46:47, Alexei Svitkine wrote: > Are these defaults safe for non-Android platforms? I noticed you stopped > returning false from either of the functions below. It is not obvious. The functions will be called only if data_use_tracker was initialized and it will not be initialized for non-Android. I can add explicit ifdefs in those functions as well. What do you think ? https://codereview.chromium.org/1974593002/diff/160001/components/metrics/dat... components/metrics/data_use_tracker.cc:52: "Uma_Ratio") != "") { On 2016/05/13 20:46:47, Alexei Svitkine wrote: > We would want this logic to be changed too, since we want this to apply even > when the experiment config doesn't exist/specify these params. Done.
https://codereview.chromium.org/1974593002/diff/160001/components/metrics/dat... File components/metrics/data_use_tracker.cc (right): https://codereview.chromium.org/1974593002/diff/160001/components/metrics/dat... components/metrics/data_use_tracker.cc:22: const double kDefaultUMARatio = 0.05; On 2016/05/13 20:57:12, gayane wrote: > On 2016/05/13 20:46:47, Alexei Svitkine wrote: > > Are these defaults safe for non-Android platforms? I noticed you stopped > > returning false from either of the functions below. > > It is not obvious. The functions will be called only if data_use_tracker was > initialized and it will not be initialized for non-Android. I can add explicit > ifdefs in those functions as well. What do you think ? Just having a comment explaining this above these two defaults should be good fine, but I'm ok with ifdefs too. https://codereview.chromium.org/1974593002/diff/160001/components/metrics/dat... components/metrics/data_use_tracker.cc:52: "Uma_Ratio") != "") { On 2016/05/13 20:57:12, gayane wrote: > On 2016/05/13 20:46:47, Alexei Svitkine wrote: > > We would want this logic to be changed too, since we want this to apply even > > when the experiment config doesn't exist/specify these params. > > Done. (Waiting to see this in your next patchset.)
Sorry forgot to upload previously.
lgtm https://codereview.chromium.org/1974593002/diff/180001/components/metrics/met... File components/metrics/metrics_service.cc (left): https://codereview.chromium.org/1974593002/diff/180001/components/metrics/met... components/metrics/metrics_service.cc:237: bool ShouldUploadLog() { Nit: Can you remove some the includes needed for this now? e.g. variations params API and maybe some of the ones below?
https://codereview.chromium.org/1974593002/diff/160001/components/metrics/dat... File components/metrics/data_use_tracker.cc (right): https://codereview.chromium.org/1974593002/diff/160001/components/metrics/dat... components/metrics/data_use_tracker.cc:22: const double kDefaultUMARatio = 0.05; On 2016/05/13 20:59:43, Alexei Svitkine wrote: > On 2016/05/13 20:57:12, gayane wrote: > > On 2016/05/13 20:46:47, Alexei Svitkine wrote: > > > Are these defaults safe for non-Android platforms? I noticed you stopped > > > returning false from either of the functions below. > > > > It is not obvious. The functions will be called only if data_use_tracker was > > initialized and it will not be initialized for non-Android. I can add explicit > > ifdefs in those functions as well. What do you think ? > > Just having a comment explaining this above these two defaults should be good > fine, but I'm ok with ifdefs too. Done. https://codereview.chromium.org/1974593002/diff/160001/components/metrics/dat... components/metrics/data_use_tracker.cc:52: "Uma_Ratio") != "") { On 2016/05/13 20:59:43, Alexei Svitkine wrote: > On 2016/05/13 20:57:12, gayane wrote: > > On 2016/05/13 20:46:47, Alexei Svitkine wrote: > > > We would want this logic to be changed too, since we want this to apply even > > > when the experiment config doesn't exist/specify these params. > > > > Done. > > (Waiting to see this in your next patchset.) Acknowledged. https://codereview.chromium.org/1974593002/diff/180001/components/metrics/met... File components/metrics/metrics_service.cc (left): https://codereview.chromium.org/1974593002/diff/180001/components/metrics/met... components/metrics/metrics_service.cc:237: bool ShouldUploadLog() { On 2016/05/13 21:11:56, Alexei Svitkine wrote: > Nit: Can you remove some the includes needed for this now? e.g. variations > params API and maybe some of the ones below? Done.
@lpromero ping
lgtm
The CQ bit was checked by gayane@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/1974593002/#ps200001 (title: "unused includes removed")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1974593002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1974593002/200001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-device on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by gayane@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asvitkine@chromium.org, lpromero@chromium.org Link to the patchset: https://codereview.chromium.org/1974593002/#ps240001 (title: "fix build presubmit warning")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1974593002/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1974593002/240001
Message was sent while issue was closed.
Description was changed from ========== Make the launch params the default client behavior for UMA 3g BUG=455847 ========== to ========== Make the launch params the default client behavior for UMA 3g BUG=455847 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:240001)
Message was sent while issue was closed.
Description was changed from ========== Make the launch params the default client behavior for UMA 3g BUG=455847 ========== to ========== Make the launch params the default client behavior for UMA 3g BUG=455847 Committed: https://crrev.com/8ff878076934185e5679df6f75a26f0eff47da22 Cr-Commit-Position: refs/heads/master@{#394811} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/8ff878076934185e5679df6f75a26f0eff47da22 Cr-Commit-Position: refs/heads/master@{#394811} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
