Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2605553002:
Add EffectiveConnectionType enum to the system profile proto (Closed)
DescriptionAdd EffectiveConnectionType enum to the system profile proto
Value of the EffectiveConnectionType enum is provided by the
NetworkMetricsProvider.
A new class EffectiveConnectionTypeObserver has been added which listens
to the changes in the EffectiveConnectionType, and lives on the same
thread as the NetworkQualityEstimator.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
BUG=677158
Review-Url: https://codereview.chromium.org/2605553002
Cr-Commit-Position: refs/heads/master@{#442821}
Committed: https://chromium.googlesource.com/chromium/src/+/9b3dd2bc8783876fdf3d9e242b4240a3537d19e2
Patch Set 1 : ps #Patch Set 2 : Add tests #
Total comments: 17
Patch Set 3 : Rebased, ryansturm comments, fix failing chromeos test #
Total comments: 16
Patch Set 4 : asvitkine comments #Patch Set 5 : fix test #Patch Set 6 : Update proto, Use interface, pass in ctor #
Total comments: 6
Patch Set 7 : asvitkine comments #
Total comments: 6
Patch Set 8 : asvitkine comments, more API cleanup #
Total comments: 4
Patch Set 9 : Use MakeUnique #Patch Set 10 : Rebased #Messages
Total messages: 197 (171 generated)
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== DOO NOT SUBMIT BUG= ========== to ========== ect uma DOO NOT SUBMIT BUG= ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
Description was changed from ========== ect uma DOO NOT SUBMIT BUG= ========== to ========== ect uma DOO NOT SUBMIT CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester BUG= ==========
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
Patchset #4 (id:60001) has been deleted
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #3 (id:40001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
Patchset #3 (id:80001) has been deleted
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #4 (id:120001) has been deleted
Patchset #3 (id:100001) has been deleted
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Patchset #2 (id:160001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #4 (id:220001) has been deleted
Patchset #1 (id:140001) has been deleted
Patchset #1 (id:180001) has been deleted
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:200001) has been deleted
Patchset #2 (id:260001) has been deleted
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #1 (id:240001) has been deleted
Patchset #2 (id:300001) has been deleted
Patchset #2 (id:320001) has been deleted
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== ect uma DOO NOT SUBMIT CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester BUG= ========== to ========== Add EffectiveConnectionType enum to the system profile proto Value of the EffectiveConnectionType enum is provided by the NetworkMetricsProvider. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester BUG= ==========
Patchset #2 (id:340001) has been deleted
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Add EffectiveConnectionType enum to the system profile proto Value of the EffectiveConnectionType enum is provided by the NetworkMetricsProvider. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester BUG= ========== to ========== Add EffectiveConnectionType enum to the system profile proto Value of the EffectiveConnectionType enum is provided by the NetworkMetricsProvider. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester BUG=677158 ==========
Description was changed from ========== Add EffectiveConnectionType enum to the system profile proto Value of the EffectiveConnectionType enum is provided by the NetworkMetricsProvider. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester BUG=677158 ========== to ========== Add EffectiveConnectionType enum to the system profile proto Value of the EffectiveConnectionType enum is provided by the NetworkMetricsProvider. A new class EffectiveConnectionTypeObserver has been added which listens to the changes in the EffectiveConnectionType, and lives on the same class as the NetworkQualityEstimator. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester BUG=677158 ==========
Description was changed from ========== Add EffectiveConnectionType enum to the system profile proto Value of the EffectiveConnectionType enum is provided by the NetworkMetricsProvider. A new class EffectiveConnectionTypeObserver has been added which listens to the changes in the EffectiveConnectionType, and lives on the same class as the NetworkQualityEstimator. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester BUG=677158 ========== to ========== Add EffectiveConnectionType enum to the system profile proto Value of the EffectiveConnectionType enum is provided by the NetworkMetricsProvider. A new class EffectiveConnectionTypeObserver has been added which listens to the changes in the EffectiveConnectionType, and lives on the same thread as the NetworkQualityEstimator. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester BUG=677158 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
Patchset #2 (id:360001) has been deleted
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
Patchset #2 (id:380001) has been deleted
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
tbansal@chromium.org changed reviewers: + ryansturm@chromium.org
ryansturm: ptal at *. Thanks.
Patchset #1 (id:280001) has been deleted
I might be overlooking something. If this all happens on the UI thread, wouldn't using an observer on the UI thread make more sense? UINetworkQualityEstimatorService::AddEffectiveConnectionTypeObserver is an inherited method from net::NetworkQualityProvider and it accepts an net::ECTObserver, so you can pass those objects around as net interfaces and avoid all of the thread hops. It should look similar without all the PostTasks. Tests?
On 2016/12/27 21:14:48, Ryan Sturm wrote: > I might be overlooking something. If this all happens on the UI thread, wouldn't > using an observer on the UI thread make more sense? > > UINetworkQualityEstimatorService::AddEffectiveConnectionTypeObserver is an > inherited method from net::NetworkQualityProvider and it accepts an > net::ECTObserver, so you can pass those objects around as net interfaces and > avoid all of the thread hops. It should look similar without all the PostTasks. The service is profile specific while this is system-specific. e.g., there could be a case when no profile exists, and we still need to fill in the system_profile.proto. > > Tests? Going to add soon.
On 2016/12/27 21:23:15, tbansal1 wrote: > On 2016/12/27 21:14:48, Ryan Sturm wrote: > > I might be overlooking something. If this all happens on the UI thread, > wouldn't > > using an observer on the UI thread make more sense? > > > > UINetworkQualityEstimatorService::AddEffectiveConnectionTypeObserver is an > > inherited method from net::NetworkQualityProvider and it accepts an > > net::ECTObserver, so you can pass those objects around as net interfaces and > > avoid all of the thread hops. It should look similar without all the > PostTasks. > The service is profile specific while this is system-specific. e.g., there could > be a case when no profile exists, and we still need to fill in the > system_profile.proto. Acknowledged. I'll keep reviewing. > > > > Tests? > Going to add soon.
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #3 (id:440001) has been deleted
Patchset #2 (id:420001) has been deleted
Patchset #2 (id:460001) has been deleted
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
Patchset #3 (id:500001) has been deleted
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
2 themes of optional nits: referring to the IO thread in a place that maybe should refer to it as network thread and treating task runners as threads when talking about them in comments. lgtm https://codereview.chromium.org/2605553002/diff/480001/chrome/browser/metrics... File chrome/browser/metrics/chrome_metrics_service_client.cc (right): https://codereview.chromium.org/2605553002/diff/480001/chrome/browser/metrics... chrome/browser/metrics/chrome_metrics_service_client.cc:928: // Provide the |network_quality_estimator| and the thread on which nit: s/the thread/the taskrunner for the thread/ https://codereview.chromium.org/2605553002/diff/480001/components/metrics/net... File components/metrics/net/network_metrics_provider.cc (right): https://codereview.chromium.org/2605553002/diff/480001/components/metrics/net... components/metrics/net/network_metrics_provider.cc:343: // |this| is initialized and used on the IO thread. nit maybe: s/IO/network/ https://codereview.chromium.org/2605553002/diff/480001/components/metrics/net... components/metrics/net/network_metrics_provider.cc:353: // Initializes |this| on IO thread, which is same thread where nit:s/IO/network/ https://codereview.chromium.org/2605553002/diff/480001/components/metrics/net... components/metrics/net/network_metrics_provider.cc:353: // Initializes |this| on IO thread, which is same thread where nit:s/same/the same/ https://codereview.chromium.org/2605553002/diff/480001/components/metrics/net... components/metrics/net/network_metrics_provider.cc:355: void InitOnIOThread(net::NetworkQualityEstimator* network_quality_estimator) { nit maybe: s/InitOnIOThread/InitOnNetworkThread/ https://codereview.chromium.org/2605553002/diff/480001/components/metrics/net... components/metrics/net/network_metrics_provider.cc:372: // |callback_| is ran to notify of changes in the effective connection type. s/|callback_| is ran to notify of changes in the effective connection type/Called when the Effective connection type is changed/ https://codereview.chromium.org/2605553002/diff/480001/components/metrics/net... File components/metrics/net/network_metrics_provider.h (right): https://codereview.chromium.org/2605553002/diff/480001/components/metrics/net... components/metrics/net/network_metrics_provider.h:37: // Sets the network quality estimator which lives on |task_runner|. nit: s/lives on/can be accessed via/ or "which lives on the thread that task_runner is associated with"? https://codereview.chromium.org/2605553002/diff/480001/components/metrics/net... components/metrics/net/network_metrics_provider.h:106: // destroyed on |network_quality_task_runner_|. same nit here. I'm not sure that taskrunner should be talked about as if it were just a thread. https://codereview.chromium.org/2605553002/diff/480001/components/metrics/net... components/metrics/net/network_metrics_provider.h:110: // Task runner on which |effective_connection_type_observer_| is initialized same nit: s/Task runner/Corresponds to the thread/
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
Patchset #4 (id:540001) has been deleted
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #4 (id:560001) has been deleted
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #4 (id:580001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #4 (id:600001) has been deleted
Patchset #3 (id:520001) has been deleted
tbansal@chromium.org changed reviewers: + asvitkine@chromium.org
asvitkine: ptal at chrome/browser/metrics and components/metrics/ Thanks. https://codereview.chromium.org/2605553002/diff/480001/chrome/browser/metrics... File chrome/browser/metrics/chrome_metrics_service_client.cc (right): https://codereview.chromium.org/2605553002/diff/480001/chrome/browser/metrics... chrome/browser/metrics/chrome_metrics_service_client.cc:928: // Provide the |network_quality_estimator| and the thread on which On 2016/12/28 21:53:11, Ryan Sturm wrote: > nit: s/the thread/the taskrunner for the thread/ Done. https://codereview.chromium.org/2605553002/diff/480001/components/metrics/net... File components/metrics/net/network_metrics_provider.cc (right): https://codereview.chromium.org/2605553002/diff/480001/components/metrics/net... components/metrics/net/network_metrics_provider.cc:343: // |this| is initialized and used on the IO thread. On 2016/12/28 21:53:11, Ryan Sturm wrote: > nit maybe: s/IO/network/ Done. https://codereview.chromium.org/2605553002/diff/480001/components/metrics/net... components/metrics/net/network_metrics_provider.cc:353: // Initializes |this| on IO thread, which is same thread where On 2016/12/28 21:53:11, Ryan Sturm wrote: > nit:s/IO/network/ Done. https://codereview.chromium.org/2605553002/diff/480001/components/metrics/net... components/metrics/net/network_metrics_provider.cc:355: void InitOnIOThread(net::NetworkQualityEstimator* network_quality_estimator) { On 2016/12/28 21:53:11, Ryan Sturm wrote: > nit maybe: s/InitOnIOThread/InitOnNetworkThread/ Done. https://codereview.chromium.org/2605553002/diff/480001/components/metrics/net... components/metrics/net/network_metrics_provider.cc:372: // |callback_| is ran to notify of changes in the effective connection type. On 2016/12/28 21:53:11, Ryan Sturm wrote: > s/|callback_| is ran to notify of changes in the effective connection > type/Called when the Effective connection type is changed/ Done. https://codereview.chromium.org/2605553002/diff/480001/components/metrics/net... File components/metrics/net/network_metrics_provider.h (right): https://codereview.chromium.org/2605553002/diff/480001/components/metrics/net... components/metrics/net/network_metrics_provider.h:37: // Sets the network quality estimator which lives on |task_runner|. On 2016/12/28 21:53:11, Ryan Sturm wrote: > nit: s/lives on/can be accessed via/ > or "which lives on the thread that task_runner is associated with"? Done. https://codereview.chromium.org/2605553002/diff/480001/components/metrics/net... components/metrics/net/network_metrics_provider.h:106: // destroyed on |network_quality_task_runner_|. On 2016/12/28 21:53:11, Ryan Sturm wrote: > same nit here. I'm not sure that taskrunner should be talked about as if it were > just a thread. Done. https://codereview.chromium.org/2605553002/diff/480001/components/metrics/net... components/metrics/net/network_metrics_provider.h:110: // Task runner on which |effective_connection_type_observer_| is initialized On 2016/12/28 21:53:11, Ryan Sturm wrote: > same nit: s/Task runner/Corresponds to the thread/ Done.
https://codereview.chromium.org/2605553002/diff/620001/chrome/browser/metrics... File chrome/browser/metrics/chrome_metrics_service_client.cc (right): https://codereview.chromium.org/2605553002/diff/620001/chrome/browser/metrics... chrome/browser/metrics/chrome_metrics_service_client.cc:129: if (!io_thread) In what cases would this be hit? Add a comment or remove if it's not possible. https://codereview.chromium.org/2605553002/diff/620001/chrome/browser/metrics... chrome/browser/metrics/chrome_metrics_service_client.cc:598: weak_ptr_factory_.GetWeakPtr())); Can all of this be internally in NetworksMetricsProvider? Why does it have it be in chrome_metrics_service_client.cc? Is it because of the IO thread reference? If so, perhaps you can just pass in a Callback object to the relevant function to NetworkMetricsProvider ctor and it does the rest with it? https://codereview.chromium.org/2605553002/diff/620001/components/metrics/net... File components/metrics/net/network_metrics_provider.cc (right): https://codereview.chromium.org/2605553002/diff/620001/components/metrics/net... components/metrics/net/network_metrics_provider.cc:52: NOTIMPLEMENTED() << " ECT observer was not deleted successfully"; I think this should be NOTREACHED() rather than NOTIMPLEMENTED(). The latter is meant for when e.g. a platform implementation of an interface doesn't implement a function that it expects to never be called. https://codereview.chromium.org/2605553002/diff/620001/components/metrics/net... File components/metrics/net/network_metrics_provider.h (right): https://codereview.chromium.org/2605553002/diff/620001/components/metrics/net... components/metrics/net/network_metrics_provider.h:39: net::NetworkQualityEstimator* network_quality_estimator, Non-const params should be last. https://codereview.chromium.org/2605553002/diff/620001/components/metrics/pro... File components/metrics/proto/system_profile.proto (right): https://codereview.chromium.org/2605553002/diff/620001/components/metrics/pro... components/metrics/proto/system_profile.proto:283: // Information about the network connection. Add a "Next tag:" comment. https://codereview.chromium.org/2605553002/diff/620001/components/metrics/pro... components/metrics/proto/system_profile.proto:383: // The connection type according to net::NetworkQualityEstimator. Can you expand comment to explain differences with connection_type? https://codereview.chromium.org/2605553002/diff/620001/components/metrics/pro... components/metrics/proto/system_profile.proto:384: optional EffectiveConnectionType effective_connection_type = 6; Please send an internal CL with this change to the proto. It will need to land before this.
asvitkine: ptal. I will work on the server-side CL but it would be good for me to know if this is in a good shape or not. https://codereview.chromium.org/2605553002/diff/620001/chrome/browser/metrics... File chrome/browser/metrics/chrome_metrics_service_client.cc (right): https://codereview.chromium.org/2605553002/diff/620001/chrome/browser/metrics... chrome/browser/metrics/chrome_metrics_service_client.cc:129: if (!io_thread) On 2017/01/03 17:48:48, Alexei Svitkine (slow) wrote: > In what cases would this be hit? Add a comment or remove if it's not possible. Done. https://codereview.chromium.org/2605553002/diff/620001/chrome/browser/metrics... chrome/browser/metrics/chrome_metrics_service_client.cc:598: weak_ptr_factory_.GetWeakPtr())); On 2017/01/03 17:48:48, Alexei Svitkine (slow) wrote: > Can all of this be internally in NetworksMetricsProvider? Why does it have it be > in chrome_metrics_service_client.cc? Is it because of the IO thread reference? Yes, IO thread is in chrome/browser, so IOThread or IOThread::Globals() can't be referenced from components. That means I have to call "io_thread->globals()->network_quality_estimator.get()" somewhere in chrome/browser. Also, that call can only be done on IO thread. So, the only option is to do the PostTask to IO thread here, and get the reference to NQE. > If so, perhaps you can just pass in a Callback object to the relevant function > to NetworkMetricsProvider ctor and it does the rest with it? Probably not. https://codereview.chromium.org/2605553002/diff/620001/components/metrics/net... File components/metrics/net/network_metrics_provider.cc (right): https://codereview.chromium.org/2605553002/diff/620001/components/metrics/net... components/metrics/net/network_metrics_provider.cc:52: NOTIMPLEMENTED() << " ECT observer was not deleted successfully"; On 2017/01/03 17:48:48, Alexei Svitkine (slow) wrote: > I think this should be NOTREACHED() rather than NOTIMPLEMENTED(). > > The latter is meant for when e.g. a platform implementation of an interface > doesn't implement a function that it expects to never be called. Done. https://codereview.chromium.org/2605553002/diff/620001/components/metrics/net... File components/metrics/net/network_metrics_provider.h (right): https://codereview.chromium.org/2605553002/diff/620001/components/metrics/net... components/metrics/net/network_metrics_provider.h:39: net::NetworkQualityEstimator* network_quality_estimator, On 2017/01/03 17:48:48, Alexei Svitkine (slow) wrote: > Non-const params should be last. Done. https://codereview.chromium.org/2605553002/diff/620001/components/metrics/pro... File components/metrics/proto/system_profile.proto (right): https://codereview.chromium.org/2605553002/diff/620001/components/metrics/pro... components/metrics/proto/system_profile.proto:283: // Information about the network connection. On 2017/01/03 17:48:48, Alexei Svitkine (slow) wrote: > Add a "Next tag:" comment. Done. https://codereview.chromium.org/2605553002/diff/620001/components/metrics/pro... components/metrics/proto/system_profile.proto:383: // The connection type according to net::NetworkQualityEstimator. On 2017/01/03 17:48:48, Alexei Svitkine (slow) wrote: > Can you expand comment to explain differences with connection_type? Done. https://codereview.chromium.org/2605553002/diff/620001/components/metrics/pro... components/metrics/proto/system_profile.proto:384: optional EffectiveConnectionType effective_connection_type = 6; On 2017/01/03 17:48:49, Alexei Svitkine (slow) wrote: > Please send an internal CL with this change to the proto. It will need to land > before this. OK, I will do that. But, just to expand on my understanding of protos, are they not forward/backward compatible?
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
asvitkine@google.com changed reviewers: + asvitkine@google.com
https://codereview.chromium.org/2605553002/diff/620001/components/metrics/pro... File components/metrics/proto/system_profile.proto (right): https://codereview.chromium.org/2605553002/diff/620001/components/metrics/pro... components/metrics/proto/system_profile.proto:384: optional EffectiveConnectionType effective_connection_type = 6; On 2017/01/03 18:55:44, tbansal1 wrote: > On 2017/01/03 17:48:49, Alexei Svitkine (slow) wrote: > > Please send an internal CL with this change to the proto. It will need to land > > before this. > > OK, I will do that. But, just to expand on my understanding of protos, are they > not forward/backward compatible? They are compatible, it's just that for metrics protos, our policy is to land internal version first to avoid divergence if two people edit a different one first at the same time and since internal change requires also logs team approval.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
https://codereview.chromium.org/2605553002/diff/620001/chrome/browser/metrics... File chrome/browser/metrics/chrome_metrics_service_client.cc (right): https://codereview.chromium.org/2605553002/diff/620001/chrome/browser/metrics... chrome/browser/metrics/chrome_metrics_service_client.cc:598: weak_ptr_factory_.GetWeakPtr())); On 2017/01/03 18:55:43, tbansal1 wrote: > On 2017/01/03 17:48:48, Alexei Svitkine (slow) wrote: > > Can all of this be internally in NetworksMetricsProvider? Why does it have it > be > > in chrome_metrics_service_client.cc? Is it because of the IO thread reference? > Yes, IO thread is in chrome/browser, so IOThread or IOThread::Globals() can't be > referenced from components. That means I have to call > "io_thread->globals()->network_quality_estimator.get()" somewhere in > chrome/browser. Also, that call can only be done on IO thread. So, the only > option is to do the PostTask to IO thread here, and get the reference to NQE. > > If so, perhaps you can just pass in a Callback object to the relevant function > > to NetworkMetricsProvider ctor and it does the rest with it? > Probably not. > Just to be clear, I would like there to be as little code as possible in this file - as adding more code like this doesn't scale well. So let's find a solution that achieves that. One option is to have an interface defined in network_metrics_provider.h that has functions to get the provider. And then you can have a class implementing that interface in chrome/browser/metrics and in this code you can create that object and pass it to the network_metrics_provider ctor.
On 2017/01/03 19:09:51, asvitkine_google (ooo) wrote: > https://codereview.chromium.org/2605553002/diff/620001/chrome/browser/metrics... > File chrome/browser/metrics/chrome_metrics_service_client.cc (right): > > https://codereview.chromium.org/2605553002/diff/620001/chrome/browser/metrics... > chrome/browser/metrics/chrome_metrics_service_client.cc:598: > weak_ptr_factory_.GetWeakPtr())); > On 2017/01/03 18:55:43, tbansal1 wrote: > > On 2017/01/03 17:48:48, Alexei Svitkine (slow) wrote: > > > Can all of this be internally in NetworksMetricsProvider? Why does it have > it > > be > > > in chrome_metrics_service_client.cc? Is it because of the IO thread > reference? > > Yes, IO thread is in chrome/browser, so IOThread or IOThread::Globals() can't > be > > referenced from components. That means I have to call > > "io_thread->globals()->network_quality_estimator.get()" somewhere in > > chrome/browser. Also, that call can only be done on IO thread. So, the only > > option is to do the PostTask to IO thread here, and get the reference to NQE. > > > If so, perhaps you can just pass in a Callback object to the relevant > function > > > to NetworkMetricsProvider ctor and it does the rest with it? > > Probably not. > > > > Just to be clear, I would like there to be as little code as possible in this > file - as adding more code like this doesn't scale well. > > So let's find a solution that achieves that. One option is to have an interface > defined in network_metrics_provider.h that has functions to get the provider. > And then you can have a class implementing that interface in > chrome/browser/metrics and in this code you can create that object and pass it > to the network_metrics_provider ctor. Sounds good if creating a new class in c/b/metrics is a fair game.
asvitkine@chromium.org changed reviewers: - asvitkine@google.com
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #6 (id:680001) has been deleted
Patchset #6 (id:700001) has been deleted
Patchset #6 (id:720001) has been deleted
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
asvitkine: ptal at PS#6. Thanks.
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: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2605553002/diff/780001/chromecast/browser/met... File chromecast/browser/metrics/cast_metrics_service_client.cc (right): https://codereview.chromium.org/2605553002/diff/780001/chromecast/browser/met... chromecast/browser/metrics/cast_metrics_service_client.cc:363: nullptr /* network_quality_estimator_provider */, Up to you, but you can add a ctor override that doesn't take the two new params - then you don't have to change these other call sites. https://codereview.chromium.org/2605553002/diff/780001/components/metrics/net... File components/metrics/net/network_metrics_provider.h (right): https://codereview.chromium.org/2605553002/diff/780001/components/metrics/net... components/metrics/net/network_metrics_provider.h:57: network_quality_task_runner, Can the task runner ctor param be avoided and just be something that the NetworkQualityEstimatorProvider returns? https://codereview.chromium.org/2605553002/diff/780001/components/metrics/net... File components/metrics/net/network_metrics_provider_unittest.cc (right): https://codereview.chromium.org/2605553002/diff/780001/components/metrics/net... components/metrics/net/network_metrics_provider_unittest.cc:1: // Copyright 2016 The Chromium Authors. All rights reserved. 2017
Patchset #7 (id:760001) has been deleted
Patchset #6 (id:740001) has been deleted
Patchset #7 (id:800001) has been deleted
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
asvitkine: ptal. Thanks for the help. https://codereview.chromium.org/2605553002/diff/780001/chromecast/browser/met... File chromecast/browser/metrics/cast_metrics_service_client.cc (right): https://codereview.chromium.org/2605553002/diff/780001/chromecast/browser/met... chromecast/browser/metrics/cast_metrics_service_client.cc:363: nullptr /* network_quality_estimator_provider */, On 2017/01/04 15:18:59, Alexei Svitkine (slow) wrote: > Up to you, but you can add a ctor override that doesn't take the two new params > - then you don't have to change these other call sites. Done. https://codereview.chromium.org/2605553002/diff/780001/components/metrics/net... File components/metrics/net/network_metrics_provider.h (right): https://codereview.chromium.org/2605553002/diff/780001/components/metrics/net... components/metrics/net/network_metrics_provider.h:57: network_quality_task_runner, On 2017/01/04 15:18:59, Alexei Svitkine (slow) wrote: > Can the task runner ctor param be avoided and just be something that the > NetworkQualityEstimatorProvider returns? Good point. Done. https://codereview.chromium.org/2605553002/diff/780001/components/metrics/net... File components/metrics/net/network_metrics_provider_unittest.cc (right): https://codereview.chromium.org/2605553002/diff/780001/components/metrics/net... components/metrics/net/network_metrics_provider_unittest.cc:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2017/01/04 15:18:59, Alexei Svitkine (slow) wrote: > 2017 Done.
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/2605553002/diff/820001/chrome/browser/metrics... File chrome/browser/metrics/network_quality_estimator_provider_impl.h (right): https://codereview.chromium.org/2605553002/diff/820001/chrome/browser/metrics... chrome/browser/metrics/network_quality_estimator_provider_impl.h:26: // NetworkMetricsProvider::NetworkQualityEstimatorProvider implementation: Nit: For new code, I think this comment syntax is preferred: // NetworkMetricsProvider::NetworkQualityEstimatorProvider: https://codereview.chromium.org/2605553002/diff/820001/chrome/browser/metrics... chrome/browser/metrics/network_quality_estimator_provider_impl.h:28: Nit: Remove blank line. https://codereview.chromium.org/2605553002/diff/820001/components/metrics/net... File components/metrics/net/network_metrics_provider.h (right): https://codereview.chromium.org/2605553002/diff/820001/components/metrics/net... components/metrics/net/network_metrics_provider.h:43: callback) = 0; Can it simply return the NetworkQualityEstimator* and have the caller run the callback? You might have to restructure the calling code (i.e. make a helper anon function) but it would make the interface simpler and this I think would be a win.
Patchset #8 (id:840001) has been deleted
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
ptal. Thanks. https://codereview.chromium.org/2605553002/diff/820001/chrome/browser/metrics... File chrome/browser/metrics/network_quality_estimator_provider_impl.h (right): https://codereview.chromium.org/2605553002/diff/820001/chrome/browser/metrics... chrome/browser/metrics/network_quality_estimator_provider_impl.h:26: // NetworkMetricsProvider::NetworkQualityEstimatorProvider implementation: On 2017/01/04 20:54:21, Alexei Svitkine (slow) wrote: > Nit: For new code, I think this comment syntax is preferred: > > // NetworkMetricsProvider::NetworkQualityEstimatorProvider: Done. https://codereview.chromium.org/2605553002/diff/820001/chrome/browser/metrics... chrome/browser/metrics/network_quality_estimator_provider_impl.h:28: On 2017/01/04 20:54:21, Alexei Svitkine (slow) wrote: > Nit: Remove blank line. Done. https://codereview.chromium.org/2605553002/diff/820001/components/metrics/net... File components/metrics/net/network_metrics_provider.h (right): https://codereview.chromium.org/2605553002/diff/820001/components/metrics/net... components/metrics/net/network_metrics_provider.h:43: callback) = 0; On 2017/01/04 20:54:21, Alexei Svitkine (slow) wrote: > Can it simply return the NetworkQualityEstimator* and have the caller run the > callback? > > You might have to restructure the calling code (i.e. make a helper anon > function) but it would make the interface simpler and this I think would be a > win. Done.
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.
lgtm % comment Please don't submit until the server-side CL is landed. https://codereview.chromium.org/2605553002/diff/860001/chrome/browser/metrics... File chrome/browser/metrics/chrome_metrics_service_client.cc (right): https://codereview.chromium.org/2605553002/diff/860001/chrome/browser/metrics... chrome/browser/metrics/chrome_metrics_service_client.cc:577: std::unique_ptr<metrics::NetworkQualityEstimatorProviderImpl>( Nit: base::WrapUnique() instead https://codereview.chromium.org/2605553002/diff/860001/components/metrics/net... File components/metrics/net/network_metrics_provider.cc (right): https://codereview.chromium.org/2605553002/diff/860001/components/metrics/net... components/metrics/net/network_metrics_provider.cc:144: base::Bind(&EffectiveConnectionTypeObserver::Init, Wondering if it wouldn't be simpler to just have GetAndSetNetworkQualityEstimator to take EffectiveConnectionTypeObserver* as a param and for it to call ->Init(estimator) on it - rather than going through the extra callback hoop?
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
https://codereview.chromium.org/2605553002/diff/860001/chrome/browser/metrics... File chrome/browser/metrics/chrome_metrics_service_client.cc (right): https://codereview.chromium.org/2605553002/diff/860001/chrome/browser/metrics... chrome/browser/metrics/chrome_metrics_service_client.cc:577: std::unique_ptr<metrics::NetworkQualityEstimatorProviderImpl>( On 2017/01/05 16:10:31, Alexei Svitkine (slow) wrote: > Nit: base::WrapUnique() instead Using MakeUnique https://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Prefer... https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/iQgMedVA8-k https://codereview.chromium.org/2605553002/diff/860001/components/metrics/net... File components/metrics/net/network_metrics_provider.cc (right): https://codereview.chromium.org/2605553002/diff/860001/components/metrics/net... components/metrics/net/network_metrics_provider.cc:144: base::Bind(&EffectiveConnectionTypeObserver::Init, On 2017/01/05 16:10:31, Alexei Svitkine (slow) wrote: > Wondering if it wouldn't be simpler to just have > GetAndSetNetworkQualityEstimator to take EffectiveConnectionTypeObserver* as a > param and for it to call ->Init(estimator) on it - rather than going through the > extra callback hoop? Then, I would need to declare EffectiveConnectionTypeObserver as a public class within NetworkMetricsProvider. Right now, it is forward-declared as a private member of NetworkMetricsProvider. I would rather not declare it as public class since then we are leaking the internal implementation details. Let me know if you feel strongly about this.
Yes, I will submit this after the server side change has landed.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
sg - thanks! On Thu, Jan 5, 2017 at 12:21 PM, <tbansal@chromium.org> wrote: > Yes, I will submit this after the server side change has landed. > > https://codereview.chromium.org/2605553002/ > -- 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.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by tbansal@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ryansturm@chromium.org, asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/2605553002/#ps900001 (title: "Rebased")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
tbansal@chromium.org changed reviewers: + hashimoto@chromium.org
hashimoto: ptal at components/metrics/net/DEPS for added dependency on chromeos/dbus. This is being used for test framework in components/metrics/net/network_metrics_provider_unittest.cc. Thanks.,
lgtm
The CQ bit was checked by tbansal@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 900001, "attempt_start_ts": 1484114906388240,
"parent_rev": "a0873eea705ea81161ec71eb3055001920f7b448", "commit_rev":
"9b3dd2bc8783876fdf3d9e242b4240a3537d19e2"}
Message was sent while issue was closed.
Description was changed from ========== Add EffectiveConnectionType enum to the system profile proto Value of the EffectiveConnectionType enum is provided by the NetworkMetricsProvider. A new class EffectiveConnectionTypeObserver has been added which listens to the changes in the EffectiveConnectionType, and lives on the same thread as the NetworkQualityEstimator. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester BUG=677158 ========== to ========== Add EffectiveConnectionType enum to the system profile proto Value of the EffectiveConnectionType enum is provided by the NetworkMetricsProvider. A new class EffectiveConnectionTypeObserver has been added which listens to the changes in the EffectiveConnectionType, and lives on the same thread as the NetworkQualityEstimator. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester BUG=677158 Review-Url: https://codereview.chromium.org/2605553002 Cr-Commit-Position: refs/heads/master@{#442821} Committed: https://chromium.googlesource.com/chromium/src/+/9b3dd2bc8783876fdf3d9e242b42... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:900001) as https://chromium.googlesource.com/chromium/src/+/9b3dd2bc8783876fdf3d9e242b42... |
