Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
922383003:
Enable UMA log uploads for cellular networks. (Closed)
|
Created:
5 years, 10 months ago by gayane -on leave until 09-2017 Modified:
5 years, 10 months ago Reviewers:
Alexei Svitkine (slow) CC:
chromium-reviews, cbentzel+watch_chromium.org, asvitkine+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionEnable UMA log uploads for cellular networks.
For enabling UMA uploads on cellular networks upload interval should be increased to 15min instead of 5min which according to initial experiments showed to decrease average overall size of the upload.
This behavior is enabled for users which are assigned to Finch experiment.
BUG=455847
Committed: https://crrev.com/d52ca402963fc3fc25c9f93b577f79a5eebbb10f
Cr-Commit-Position: refs/heads/master@{#317654}
Patch Set 1 : upload interval based on the connection type #
Total comments: 9
Patch Set 2 : Field trials added #
Total comments: 6
Patch Set 3 : Callback to set connection type in the scheduler #
Total comments: 10
Patch Set 4 : Callback to get connection type #
Total comments: 6
Patch Set 5 : Sending NetworkMetricsProvider as scoped_ptr #
Total comments: 14
Patch Set 6 : Removed old solution, fixed comments #
Total comments: 6
Patch Set 7 : handle the network metricsprovider is null #Patch Set 8 : fix non-android and unittests #Patch Set 9 : fix for IOS #
Total comments: 4
Patch Set 10 : Metrics service is not dependent on net #
Total comments: 6
Patch Set 11 : add checks #
Total comments: 6
Patch Set 12 : comments fixed #Messages
Total messages: 39 (12 generated)
Patchset #1 (id:1) has been deleted
gayane@chromium.org changed reviewers: + asvitkine@chromium.org
For now only upload interval change based on the connection type is implemented.
Approach seems reasonable, but I guess you're still missing: - Field-trial based control. - Hooking up with not sending profiler data. - The UI changes. If you'd like to land things incrementally, I suggest adding field-trial based control to this CL, so that there's no behavior change without the trial and then the CL can be landed without the other bits. https://codereview.chromium.org/922383003/diff/20001/components/metrics/metri... File components/metrics/metrics_reporting_scheduler.cc (right): https://codereview.chromium.org/922383003/diff/20001/components/metrics/metri... components/metrics/metrics_reporting_scheduler.cc:200: return TimeDelta::FromSeconds(kStandardUploadIntervalSeconds); Nit: This is the same return as a below. So you can make the ifdef just around the if statement and its body and share the return line. https://codereview.chromium.org/922383003/diff/20001/components/metrics/metri... components/metrics/metrics_reporting_scheduler.cc:213: network_metrics_provider_->GetConnectionType(); Nit: Since the API allows network_metrics_provider_ to never be set, handle that case in the code. https://codereview.chromium.org/922383003/diff/20001/components/metrics/metri... components/metrics/metrics_reporting_scheduler.cc:215: switch(connection_type) { Nit: space before ( https://codereview.chromium.org/922383003/diff/20001/components/metrics/metri... File components/metrics/metrics_reporting_scheduler.h (right): https://codereview.chromium.org/922383003/diff/20001/components/metrics/metri... components/metrics/metrics_reporting_scheduler.h:49: bool IsCellularConnection(); Nit: Add an empty line after this. https://codereview.chromium.org/922383003/diff/20001/components/metrics/metri... File components/metrics/metrics_service.h (right): https://codereview.chromium.org/922383003/diff/20001/components/metrics/metri... components/metrics/metrics_service.h:248: void SetNetworkMetricsProvider(); Make the provider a param and have the impl of this function call Register on it.
I have fixed your comments and also added field trials, but didn't manage to test these changes on my device. https://codereview.chromium.org/922383003/diff/20001/components/metrics/metri... File components/metrics/metrics_reporting_scheduler.cc (right): https://codereview.chromium.org/922383003/diff/20001/components/metrics/metri... components/metrics/metrics_reporting_scheduler.cc:200: return TimeDelta::FromSeconds(kStandardUploadIntervalSeconds); On 2015/02/13 21:11:40, Alexei Svitkine wrote: > Nit: This is the same return as a below. So you can make the ifdef just around > the if statement and its body and share the return line. Done. https://codereview.chromium.org/922383003/diff/20001/components/metrics/metri... components/metrics/metrics_reporting_scheduler.cc:213: network_metrics_provider_->GetConnectionType(); On 2015/02/13 21:11:40, Alexei Svitkine wrote: > Nit: Since the API allows network_metrics_provider_ to never be set, handle that > case in the code. Done. https://codereview.chromium.org/922383003/diff/20001/components/metrics/metri... components/metrics/metrics_reporting_scheduler.cc:215: switch(connection_type) { On 2015/02/13 21:11:40, Alexei Svitkine wrote: > Nit: space before ( Done. https://codereview.chromium.org/922383003/diff/20001/components/metrics/metri... File components/metrics/metrics_reporting_scheduler.h (right): https://codereview.chromium.org/922383003/diff/20001/components/metrics/metri... components/metrics/metrics_reporting_scheduler.h:49: bool IsCellularConnection(); On 2015/02/13 21:11:41, Alexei Svitkine wrote: > Nit: Add an empty line after this. Done.
https://codereview.chromium.org/922383003/diff/40001/components/metrics/metri... File components/metrics/metrics_reporting_scheduler.h (right): https://codereview.chromium.org/922383003/diff/40001/components/metrics/metri... components/metrics/metrics_reporting_scheduler.h:13: #include "components/metrics/net/network_metrics_provider.h" Hmm, I just realised this is problematic. The NetworkMetricsProvider is in metrics/net, which depends on the network stack. We don't want the normal MetricsService/MetricsReportingScheduler code to have such a dependency. So, instead of passing that object to this class, I suggest passing a callback that can be bound to the function we want to use. https://codereview.chromium.org/922383003/diff/40001/components/metrics/metri... components/metrics/metrics_reporting_scheduler.h:46: const metrics::NetworkMetricsProvider* network_metrics_provider); No metrics:: prefix needed. https://codereview.chromium.org/922383003/diff/40001/components/metrics/metri... components/metrics/metrics_reporting_scheduler.h:101: const metrics::NetworkMetricsProvider* network_metrics_provider_; Ditto.
I have added a callback function in the MetricsService to update a local variable of scheduler with the up-to-date connection type. I did like this because I couldn't pass a nonconst ref to the bind function. https://codereview.chromium.org/922383003/diff/40001/components/metrics/metri... File components/metrics/metrics_reporting_scheduler.h (right): https://codereview.chromium.org/922383003/diff/40001/components/metrics/metri... components/metrics/metrics_reporting_scheduler.h:13: #include "components/metrics/net/network_metrics_provider.h" On 2015/02/13 22:39:50, Alexei Svitkine wrote: > Hmm, I just realised this is problematic. > > > The NetworkMetricsProvider is in metrics/net, which depends on the network > stack. > > We don't want the normal MetricsService/MetricsReportingScheduler code to have > such a dependency. > > So, instead of passing that object to this class, I suggest passing a callback > that can be bound to the function we want to use. Done. https://codereview.chromium.org/922383003/diff/40001/components/metrics/metri... components/metrics/metrics_reporting_scheduler.h:46: const metrics::NetworkMetricsProvider* network_metrics_provider); On 2015/02/13 22:39:50, Alexei Svitkine wrote: > No metrics:: prefix needed. removed https://codereview.chromium.org/922383003/diff/40001/components/metrics/metri... components/metrics/metrics_reporting_scheduler.h:101: const metrics::NetworkMetricsProvider* network_metrics_provider_; On 2015/02/13 22:39:50, Alexei Svitkine wrote: > Ditto. Removed
Hey gayane, I still prefer the approach I suggested in person. I think it might be different than what you tried earlier - so giving you a concrete suggestion of the structure. Let me know if it doesn't work. https://codereview.chromium.org/922383003/diff/60001/components/metrics/metri... File components/metrics/metrics_reporting_scheduler.h (right): https://codereview.chromium.org/922383003/diff/60001/components/metrics/metri... components/metrics/metrics_reporting_scheduler.h:20: explicit MetricsReportingScheduler( Nit: Remove explicit since there's two params. Add a comment explaining the params. https://codereview.chromium.org/922383003/diff/60001/components/metrics/metri... components/metrics/metrics_reporting_scheduler.h:22: const base::Closure& is_cellular_connection_func); Nit: func -> callback https://codereview.chromium.org/922383003/diff/60001/components/metrics/metri... components/metrics/metrics_reporting_scheduler.h:97: // Pointer to |MetricsServcie::IsCellularConnection| function. Can you explain instead what the callback does? https://codereview.chromium.org/922383003/diff/60001/components/metrics/metri... File components/metrics/metrics_service.cc (right): https://codereview.chromium.org/922383003/diff/60001/components/metrics/metri... components/metrics/metrics_service.cc:314: bool Test(){ return false;} Remove this. ;) https://codereview.chromium.org/922383003/diff/60001/components/metrics/metri... components/metrics/metrics_service.cc:356: base::Bind(&MetricsService::UpdateSchedulerConnectionType, Sorry, I don't like this structure. I suggest the following: void MetricsService::GetIsCellularConnection(bool* is_cellular_out) { is_cellular_out = IsCellularConnection(); } and base::Callback<void(bool)> cellular_callback = base::Bind(&MetricsService::GetIsCellularConnection, self_ptr_factory_.GetWeakPtr()); Then, from the schedular, do: bool is_cellular = false; callback.Run(&is_cellular); if (is_cellular) { ... }
Changed the callback as you mentioned. https://codereview.chromium.org/922383003/diff/60001/components/metrics/metri... File components/metrics/metrics_reporting_scheduler.h (right): https://codereview.chromium.org/922383003/diff/60001/components/metrics/metri... components/metrics/metrics_reporting_scheduler.h:20: explicit MetricsReportingScheduler( On 2015/02/17 20:50:04, Alexei Svitkine wrote: > Nit: Remove explicit since there's two params. Add a comment explaining the > params. Done. https://codereview.chromium.org/922383003/diff/60001/components/metrics/metri... components/metrics/metrics_reporting_scheduler.h:22: const base::Closure& is_cellular_connection_func); On 2015/02/17 20:50:04, Alexei Svitkine wrote: > Nit: func -> callback Done. https://codereview.chromium.org/922383003/diff/60001/components/metrics/metri... components/metrics/metrics_reporting_scheduler.h:97: // Pointer to |MetricsServcie::IsCellularConnection| function. On 2015/02/17 20:50:04, Alexei Svitkine wrote: > Can you explain instead what the callback does? Done. https://codereview.chromium.org/922383003/diff/60001/components/metrics/metri... File components/metrics/metrics_service.cc (right): https://codereview.chromium.org/922383003/diff/60001/components/metrics/metri... components/metrics/metrics_service.cc:314: bool Test(){ return false;} On 2015/02/17 20:50:04, Alexei Svitkine wrote: > Remove this. ;) Done. https://codereview.chromium.org/922383003/diff/60001/components/metrics/metri... components/metrics/metrics_service.cc:356: base::Bind(&MetricsService::UpdateSchedulerConnectionType, On 2015/02/17 20:50:04, Alexei Svitkine wrote: > Sorry, I don't like this structure. > > I suggest the following: > > void MetricsService::GetIsCellularConnection(bool* is_cellular_out) { > is_cellular_out = IsCellularConnection(); > } > > and > > base::Callback<void(bool)> cellular_callback = > base::Bind(&MetricsService::GetIsCellularConnection, > self_ptr_factory_.GetWeakPtr()); > > Then, from the schedular, do: > > bool is_cellular = false; > callback.Run(&is_cellular); > if (is_cellular) { > ... > } Done.
https://codereview.chromium.org/922383003/diff/80001/components/metrics/metri... File components/metrics/metrics_reporting_scheduler.cc (right): https://codereview.chromium.org/922383003/diff/80001/components/metrics/metri... components/metrics/metrics_reporting_scheduler.cc:202: if (IsCellularEnabledByExperiment() && is_cellular) Nit: Reverse the order in the if, so we don't need to check experiment if not on cellular. https://codereview.chromium.org/922383003/diff/80001/components/metrics/metri... File components/metrics/metrics_reporting_scheduler.h (right): https://codereview.chromium.org/922383003/diff/80001/components/metrics/metri... components/metrics/metrics_reporting_scheduler.h:71: bool IsCellularEnabledByExperiment(); Make this an anon function inside the .cc. https://codereview.chromium.org/922383003/diff/80001/components/metrics/metri... File components/metrics/metrics_service.h (right): https://codereview.chromium.org/922383003/diff/80001/components/metrics/metri... components/metrics/metrics_service.h:249: NetworkMetricsProvider* network_metrics_provider); Pass the param by scoped_ptr to indicate transfer of ownership. (The member var can still be a raw pointer since ownership is transferred to RegisterMetricsProvider).
Patchset #5 (id:100001) has been deleted
Done. https://codereview.chromium.org/922383003/diff/80001/components/metrics/metri... File components/metrics/metrics_reporting_scheduler.cc (right): https://codereview.chromium.org/922383003/diff/80001/components/metrics/metri... components/metrics/metrics_reporting_scheduler.cc:202: if (IsCellularEnabledByExperiment() && is_cellular) On 2015/02/18 03:26:23, Alexei Svitkine wrote: > Nit: Reverse the order in the if, so we don't need to check experiment if not on > cellular. Done. https://codereview.chromium.org/922383003/diff/80001/components/metrics/metri... File components/metrics/metrics_reporting_scheduler.h (right): https://codereview.chromium.org/922383003/diff/80001/components/metrics/metri... components/metrics/metrics_reporting_scheduler.h:71: bool IsCellularEnabledByExperiment(); On 2015/02/18 03:26:23, Alexei Svitkine wrote: > Make this an anon function inside the .cc. Done. https://codereview.chromium.org/922383003/diff/80001/components/metrics/metri... File components/metrics/metrics_service.h (right): https://codereview.chromium.org/922383003/diff/80001/components/metrics/metri... components/metrics/metrics_service.h:249: NetworkMetricsProvider* network_metrics_provider); On 2015/02/18 03:26:23, Alexei Svitkine wrote: > Pass the param by scoped_ptr to indicate transfer of ownership. (The member var > can still be a raw pointer since ownership is transferred to > RegisterMetricsProvider). Done.
Looking good - almost there! https://codereview.chromium.org/922383003/diff/110007/components/metrics/metr... File components/metrics/metrics_reporting_scheduler.h (right): https://codereview.chromium.org/922383003/diff/110007/components/metrics/metr... components/metrics/metrics_reporting_scheduler.h:49: void SetConnectionType(bool is_cellular); Is this needed anymore? https://codereview.chromium.org/922383003/diff/110007/components/metrics/metr... components/metrics/metrics_reporting_scheduler.h:96: // Callback function from MetricsService used to get current network Don't say "from MetricsService" - just what it's for is good enough. https://codereview.chromium.org/922383003/diff/110007/components/metrics/metr... components/metrics/metrics_reporting_scheduler.h:102: bool is_cellular_connection_; This isn't needed anymore, right? https://codereview.chromium.org/922383003/diff/110007/components/metrics/metr... File components/metrics/metrics_service.cc (right): https://codereview.chromium.org/922383003/diff/110007/components/metrics/metr... components/metrics/metrics_service.cc:1275: Nit: No empty line here. https://codereview.chromium.org/922383003/diff/110007/components/metrics/metr... File components/metrics/metrics_service.h (right): https://codereview.chromium.org/922383003/diff/110007/components/metrics/metr... components/metrics/metrics_service.h:247: // scheduler when it is initialized. This is describing implementation details. I think it can just be: // Sets the network network provider and registers as a provider. https://codereview.chromium.org/922383003/diff/110007/components/metrics/metr... components/metrics/metrics_service.h:254: // Assigns the passed |is_cellular_out| parameter whether current network Nit: wording, "based on whether the current network connection is cellular." https://codereview.chromium.org/922383003/diff/110007/components/metrics/metr... components/metrics/metrics_service.h:497: const NetworkMetricsProvider* network_metrics_provider_; Add a comment.
Removed remains of the old solution and fixed the comments. https://codereview.chromium.org/922383003/diff/110007/components/metrics/metr... File components/metrics/metrics_reporting_scheduler.h (right): https://codereview.chromium.org/922383003/diff/110007/components/metrics/metr... components/metrics/metrics_reporting_scheduler.h:49: void SetConnectionType(bool is_cellular); On 2015/02/18 22:03:09, Alexei Svitkine wrote: > Is this needed anymore? Removed. https://codereview.chromium.org/922383003/diff/110007/components/metrics/metr... components/metrics/metrics_reporting_scheduler.h:96: // Callback function from MetricsService used to get current network On 2015/02/18 22:03:09, Alexei Svitkine wrote: > Don't say "from MetricsService" - just what it's for is good enough. Done. https://codereview.chromium.org/922383003/diff/110007/components/metrics/metr... components/metrics/metrics_reporting_scheduler.h:102: bool is_cellular_connection_; On 2015/02/18 22:03:08, Alexei Svitkine wrote: > This isn't needed anymore, right? Done. https://codereview.chromium.org/922383003/diff/110007/components/metrics/metr... File components/metrics/metrics_service.cc (right): https://codereview.chromium.org/922383003/diff/110007/components/metrics/metr... components/metrics/metrics_service.cc:1275: On 2015/02/18 22:03:09, Alexei Svitkine wrote: > Nit: No empty line here. Done. https://codereview.chromium.org/922383003/diff/110007/components/metrics/metr... File components/metrics/metrics_service.h (right): https://codereview.chromium.org/922383003/diff/110007/components/metrics/metr... components/metrics/metrics_service.h:247: // scheduler when it is initialized. On 2015/02/18 22:03:09, Alexei Svitkine wrote: > This is describing implementation details. > > I think it can just be: > > // Sets the network network provider and registers as a provider. Done. https://codereview.chromium.org/922383003/diff/110007/components/metrics/metr... components/metrics/metrics_service.h:254: // Assigns the passed |is_cellular_out| parameter whether current network On 2015/02/18 22:03:09, Alexei Svitkine wrote: > Nit: wording, "based on whether the current network connection is cellular." Done. https://codereview.chromium.org/922383003/diff/110007/components/metrics/metr... components/metrics/metrics_service.h:497: const NetworkMetricsProvider* network_metrics_provider_; On 2015/02/18 22:03:09, Alexei Svitkine wrote: > Add a comment. Done.
lgtm % remaining comments https://codereview.chromium.org/922383003/diff/130001/components/metrics/metr... File components/metrics/metrics_service.cc (right): https://codereview.chromium.org/922383003/diff/130001/components/metrics/metr... components/metrics/metrics_service.cc:1279: network_metrics_provider_->GetConnectionType(); Since it's optional for SetNetworkMetricsProvider() to be called, please check for it being null. https://codereview.chromium.org/922383003/diff/130001/components/metrics/metr... File components/metrics/metrics_service.h (right): https://codereview.chromium.org/922383003/diff/130001/components/metrics/metr... components/metrics/metrics_service.h:246: // Sets the network metrics provider and registers as a provider. Nit: "registers it as a provider." https://codereview.chromium.org/922383003/diff/130001/components/metrics/metr... components/metrics/metrics_service.h:251: bool IsCellularConnection(); This function and the one below should be private.
New patchsets have been uploaded after l-g-t-m from asvitkine@chromium.org
Done. https://codereview.chromium.org/922383003/diff/130001/components/metrics/metr... File components/metrics/metrics_service.cc (right): https://codereview.chromium.org/922383003/diff/130001/components/metrics/metr... components/metrics/metrics_service.cc:1279: network_metrics_provider_->GetConnectionType(); On 2015/02/19 16:29:12, Alexei Svitkine wrote: > Since it's optional for SetNetworkMetricsProvider() to be called, please check > for it being null. Done. https://codereview.chromium.org/922383003/diff/130001/components/metrics/metr... File components/metrics/metrics_service.h (right): https://codereview.chromium.org/922383003/diff/130001/components/metrics/metr... components/metrics/metrics_service.h:246: // Sets the network metrics provider and registers as a provider. On 2015/02/19 16:29:12, Alexei Svitkine wrote: > Nit: "registers it as a provider." Done. https://codereview.chromium.org/922383003/diff/130001/components/metrics/metr... components/metrics/metrics_service.h:251: bool IsCellularConnection(); On 2015/02/19 16:29:12, Alexei Svitkine wrote: > This function and the one below should be private. Done.
The CQ bit was checked by gayane@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/922383003/150001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_gn_dbg on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by gayane@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/922383003/170001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
Could you please have a look at Patch set #9. For IOS I was getting an error because of the unused variable kStandardUploadIntervalCellularSeconds. That's why I added IOS checks as well. As we will have experiments only for Android, IOS will work with default behavior anyway. Is this ok?
LGTM % comment https://codereview.chromium.org/922383003/diff/190001/components/metrics/metr... File components/metrics/metrics_service.cc (right): https://codereview.chromium.org/922383003/diff/190001/components/metrics/metr... components/metrics/metrics_service.cc:1278: // If network provider is not set than default to cellular connection because Nit: than -> then https://codereview.chromium.org/922383003/diff/190001/components/metrics/metr... components/metrics/metrics_service.cc:1279: // it is more conservative. I don't think we should default to cellular. If we don't have a provider, then we don't have the info so we should return false. Note: This shouldn't matter in practice for Chrome, because Chrome *will* have this provider. It's only non-Chrome users of this component (e.g. possibly Chromecast, possibly ChromeOS system uploader) that will run this codepath. But then presumably they won't be changing their behavior based on this, per the code in the scheduler.
New patchsets have been uploaded after l-g-t-m from asvitkine@chromium.org
Patchset #10 (id:210001) has been deleted
Metrics service is not dependent on net. https://codereview.chromium.org/922383003/diff/190001/components/metrics/metr... File components/metrics/metrics_service.cc (right): https://codereview.chromium.org/922383003/diff/190001/components/metrics/metr... components/metrics/metrics_service.cc:1278: // If network provider is not set than default to cellular connection because On 2015/02/20 22:39:29, Alexei Svitkine wrote: > Nit: than -> then Removed https://codereview.chromium.org/922383003/diff/190001/components/metrics/metr... components/metrics/metrics_service.cc:1279: // it is more conservative. On 2015/02/20 22:39:29, Alexei Svitkine wrote: > I don't think we should default to cellular. > > If we don't have a provider, then we don't have the info so we should return > false. > > Note: This shouldn't matter in practice for Chrome, because Chrome *will* have > this provider. > > It's only non-Chrome users of this component (e.g. possibly Chromecast, possibly > ChromeOS system uploader) that will run this codepath. > > But then presumably they won't be changing their behavior based on this, per the > code in the scheduler. Removed
not lgtm General approach looks great, just a few comments. https://codereview.chromium.org/922383003/diff/230001/components/metrics/metr... File components/metrics/metrics_reporting_scheduler.cc (right): https://codereview.chromium.org/922383003/diff/230001/components/metrics/metr... components/metrics/metrics_reporting_scheduler.cc:211: cellular_callback_.Run(&is_cellular); The callback can be null if never set - please check for this. https://codereview.chromium.org/922383003/diff/230001/components/metrics/metr... File components/metrics/metrics_service.cc (right): https://codereview.chromium.org/922383003/diff/230001/components/metrics/metr... components/metrics/metrics_service.cc:1270: is_cellular_callback_ = is_cellular_callback; Can you add DCHECK() that InitializeMetricsRecordingState() hasn't been called yet? Else, setting this late will not do anything. https://codereview.chromium.org/922383003/diff/230001/components/metrics/net/... File components/metrics/net/network_metrics_provider.h (right): https://codereview.chromium.org/922383003/diff/230001/components/metrics/net/... components/metrics/net/network_metrics_provider.h:35: void GetIsCellularConnection(bool* is_cellular_out); This can be private.
Checks added. https://codereview.chromium.org/922383003/diff/230001/components/metrics/metr... File components/metrics/metrics_reporting_scheduler.cc (right): https://codereview.chromium.org/922383003/diff/230001/components/metrics/metr... components/metrics/metrics_reporting_scheduler.cc:211: cellular_callback_.Run(&is_cellular); On 2015/02/23 18:54:06, Alexei Svitkine wrote: > The callback can be null if never set - please check for this. Done. https://codereview.chromium.org/922383003/diff/230001/components/metrics/metr... File components/metrics/metrics_service.cc (right): https://codereview.chromium.org/922383003/diff/230001/components/metrics/metr... components/metrics/metrics_service.cc:1270: is_cellular_callback_ = is_cellular_callback; On 2015/02/23 18:54:06, Alexei Svitkine wrote: > Can you add DCHECK() that InitializeMetricsRecordingState() hasn't been called > yet? > > Else, setting this late will not do anything. Done. https://codereview.chromium.org/922383003/diff/230001/components/metrics/net/... File components/metrics/net/network_metrics_provider.h (right): https://codereview.chromium.org/922383003/diff/230001/components/metrics/net/... components/metrics/net/network_metrics_provider.h:35: void GetIsCellularConnection(bool* is_cellular_out); On 2015/02/23 18:54:06, Alexei Svitkine wrote: > This can be private. Done.
LGTM with a few final comments. Thanks! https://codereview.chromium.org/922383003/diff/250001/components/metrics/metr... File components/metrics/metrics_service.h (right): https://codereview.chromium.org/922383003/diff/250001/components/metrics/metr... components/metrics/metrics_service.h:246: // Sets the connection type callback. Nit: Mention "that will be passed down to the scheduler." https://codereview.chromium.org/922383003/diff/250001/components/metrics/net/... File components/metrics/net/network_metrics_provider.cc (right): https://codereview.chromium.org/922383003/diff/250001/components/metrics/net/... components/metrics/net/network_metrics_provider.cc:247: base::Bind(&NetworkMetricsProvider::GetIsCellularConnection, Nit: Return this directly. https://codereview.chromium.org/922383003/diff/250001/components/metrics/net/... File components/metrics/net/network_metrics_provider.h (right): https://codereview.chromium.org/922383003/diff/250001/components/metrics/net/... components/metrics/net/network_metrics_provider.h:31: // connection type is cellular. Mention that the callback is bound to a weak pointer to the provider.
Comments fixed. https://codereview.chromium.org/922383003/diff/250001/components/metrics/metr... File components/metrics/metrics_service.h (right): https://codereview.chromium.org/922383003/diff/250001/components/metrics/metr... components/metrics/metrics_service.h:246: // Sets the connection type callback. On 2015/02/23 20:18:15, Alexei Svitkine wrote: > Nit: Mention "that will be passed down to the scheduler." Done. https://codereview.chromium.org/922383003/diff/250001/components/metrics/net/... File components/metrics/net/network_metrics_provider.cc (right): https://codereview.chromium.org/922383003/diff/250001/components/metrics/net/... components/metrics/net/network_metrics_provider.cc:247: base::Bind(&NetworkMetricsProvider::GetIsCellularConnection, On 2015/02/23 20:18:15, Alexei Svitkine wrote: > Nit: Return this directly. Done. https://codereview.chromium.org/922383003/diff/250001/components/metrics/net/... File components/metrics/net/network_metrics_provider.h (right): https://codereview.chromium.org/922383003/diff/250001/components/metrics/net/... components/metrics/net/network_metrics_provider.h:31: // connection type is cellular. On 2015/02/23 20:18:15, Alexei Svitkine wrote: > Mention that the callback is bound to a weak pointer to the provider. Done.
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/922383003/#ps270001 (title: "comments fixed")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/922383003/270001
Message was sent while issue was closed.
Committed patchset #12 (id:270001)
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/d52ca402963fc3fc25c9f93b577f79a5eebbb10f Cr-Commit-Position: refs/heads/master@{#317654} |
