Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2364703002:
Add network throttling as an enterprise policy (Closed)
|
Created:
4 years, 3 months ago by kirtika Modified:
3 years, 9 months ago Reviewers:
bartfab (slow), stevenjb, michaelpg, Mattias Nissler (ping if slow), kirtika1, Lei Zhang, gab, Andrew T Wilson (Slow), Alexei Svitkine (slow), brettw, Ilya Sherman CC:
sduraisamy, Kevin Cernekee, Sameer Nanda Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd support for an enterprise policy that lets the admin
turn on network throttling, and specify the upload and download rates in kbps to throttle to.
BUG=642811
TEST=(1) Unit-tests
(2) Added network throttling policy to YAPS server,
enabled throttling with various values, disabled,
and checked that changes propagate to client device.
Signed-off-by: Kirtika Ruchandani <kirtika@google.com>;
Committed: https://crrev.com/7db3c2b899d448dc0235e03a248df83aa5778ebf
Cr-Commit-Position: refs/heads/master@{#428565}
Patch Set 1 #Patch Set 2 : Work-in-progress: Testing policy addition #Patch Set 3 : Add network throttling as an enterprise policy #Patch Set 4 : Add network bandwidth throttling as an enterprise policy #
Total comments: 1
Patch Set 5 : Add network bandwidth throttling as an enterprise policy #Patch Set 6 : Add network bandwidth throttling as an enterprise policy #Patch Set 7 : Add network bandwidth throttling as an enterprise policy #
Total comments: 43
Patch Set 8 : Add network bandwidth throttling as an enterprise policy #
Total comments: 18
Patch Set 9 : Add network bandwidth throttling as an enterprise policy #Patch Set 10 : Add network bandwidth throttling as an enterprise policy #
Total comments: 4
Patch Set 11 : Add network bandwidth throttling as an enterprise policy #Patch Set 12 : Add network bandwidth throttling as an enterprise policy #
Total comments: 8
Patch Set 13 : Add network bandwidth throttling as an enterprise policy #Patch Set 14 : Add network bandwidth throttling as an enterprise policy #
Total comments: 1
Patch Set 15 : Add network bandwidth throttling as an enterprise policy #
Total comments: 11
Patch Set 16 : Add network bandwidth throttling as an enterprise policy #Patch Set 17 : Add network bandwidth throttling as an enterprise policy #
Messages
Total messages: 119 (65 generated)
Message was sent while issue was closed.
Description was changed from
==========
Test commit
BUG=Foo
==========
to
==========
Add support for an enterprise policy that lets the admin
turn on network throttling, and specify the upload and download rates in kbps to
throttle to.
BUG=642811
==========
kirtika@google.com changed reviewers: + bartfab@chromium.org
Description was changed from
==========
Add support for an enterprise policy that lets the admin
turn on network throttling, and specify the upload and download rates in kbps to
throttle to.
BUG=642811
==========
to
==========
[Pre-review: Proof of concept only. Seeking a yay/nay on whether the pieces are
plumbed in the right places.]
Add support for an enterprise policy that lets the admin
turn on network throttling, and specify the upload and download rates in kbps to
throttle to.
BUG=642811
Signed-off-by: Kirtika Ruchandani <kirtika@google.com>
==========
kirtika@google.com changed reviewers: + stevenjb@chromium.org
The CQ bit was checked by kirtika@google.com
The CQ bit was unchecked by kirtika@google.com
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
No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
kirtika@google.com changed reviewers: + michaelpg@chromium.org
The CQ bit was checked by kirtika@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: 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...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
On 2016/10/06 at 10:12:56, commit-bot wrote: > No L-G-T-M from a valid reviewer yet. > CQ run can only be started by full committers or once the patch has > received an L-G-T-M from a full committer. > Even if an L-G-T-M may have been provided, it was from a non-committer, > _not_ a full super star committer. > See http://www.chromium.org/getting-involved/become-a-committer > Note that this has nothing to do with OWNERS files. 1) We should add NetworkStateHandler::SetThrottling(...), similarly to NetworkStateHandler::SetWakeOnLanEnabled(). This will remove any Shill dependencies from the src/chrome code. 2) NetworkThrottlingManager can then just be NetworkThrottlingObserver (or something) which would just set the initial state and observe any preference changes.
> 1) We should add NetworkStateHandler::SetThrottling(...), similarly to
> NetworkStateHandler::SetWakeOnLanEnabled(). This will remove any Shill
> dependencies from the src/chrome code.
> 2) NetworkThrottlingManager can then just be NetworkThrottlingObserver (or
> something) which would just set the initial state and observe any preference
> changes.
Addressed both in PS#4. Does this look OK?
There are still several TODOs, I'll follow up in PS#5. Two main TODOs apart from
documentation and unit-tests are things I blindly borrowed from
wake_on_wifi_manager.cc's design:
(1) Its constructor checks for user not logged in. Do I need this? (Does wake on
wifi need it because
its a user set preference?)
CHECK(!base::SysInfo::IsRunningOnChromeOS() ||
!LoginState::Get()->IsUserLoggedIn());
(2) Initializing network_throttling_observer in the browser main thread seems
overkill.
Do we know of a better home for it?
I plumbed it through NetworkStateHandler as asked, but worth noting that there
are 3 wrapper
layers over a single dbus call now, hope that is ok (NetworkThrottlingObserver
-> NetworkStateHandler -> ShillManagerClient)
mnissler@chromium.org changed reviewers: + mnissler@chromium.org
https://codereview.chromium.org/2364703002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/preferences.cc (right): https://codereview.chromium.org/2364703002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/preferences.cc:635: if (reason == REASON_INITIALIZATION || If you want this code to trigger when the pref changes, why isn't this testing for reason == REASON_PREF_CHANGED?
The CQ bit was checked by kirtika@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: 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-...)
Description was changed from
==========
[Pre-review: Proof of concept only. Seeking a yay/nay on whether the pieces are
plumbed in the right places.]
Add support for an enterprise policy that lets the admin
turn on network throttling, and specify the upload and download rates in kbps to
throttle to.
BUG=642811
Signed-off-by: Kirtika Ruchandani <kirtika@google.com>
==========
to
==========
[Pre-review: Proof of concept only. Seeking a yay/nay on whether the pieces are
plumbed in the right places.]
Add support for an enterprise policy that lets the admin
turn on network throttling, and specify the upload and download rates in kbps to
throttle to.
BUG=642811
Signed-off-by: Kirtika Ruchandani <kirtika@google.com>
==========
Description was changed from ========== [Pre-review: Proof of concept only. Seeking a yay/nay on whether the pieces are plumbed in the right places.] Add support for an enterprise policy that lets the admin turn on network throttling, and specify the upload and download rates in kbps to throttle to. BUG=642811 Signed-off-by: Kirtika Ruchandani <kirtika@google.com> ========== to ========== Add support for an enterprise policy that lets the admin turn on network throttling, and specify the upload and download rates in kbps to throttle to. While there, fix some linter warnings in files touched. BUG=642811 Signed-off-by: Kirtika Ruchandani <kirtika@google.com> ==========
atwilson@chromium.org changed reviewers: + atwilson@chromium.org
atwilson@chromium.org changed reviewers: + atwilson@chromium.org
Why is .chroot_lock part of this CL? Also, the NetworkThrottlingObserver is kind of a meaningless class as it stands. Either you need to move the pref-observing code into that class, or else nuke the class entirely and have chromeos::Preferences() call NetworkHandler directly to apply changes to the throttling pref. https://codereview.chromium.org/2364703002/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right): https://codereview.chromium.org/2364703002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:414: network_throttling_observer_.reset(new NetworkThrottlingObserver()); Why are we constructing and not using Get()? https://codereview.chromium.org/2364703002/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/net/network_throttling_observer.cc (right): https://codereview.chromium.org/2364703002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/net/network_throttling_observer.cc:39: // borrowed from wake_on_wifi_manager - check if it is really needed here. What is this? What is the purpose here? Let's not check this in without explaining what this code does. https://codereview.chromium.org/2364703002/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/net/network_throttling_observer.h (right): https://codereview.chromium.org/2364703002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/net/network_throttling_observer.h:25: class NetworkThrottlingObserver { This is a weird class - it's owned by ChromeBrowserParts but has a getter here, so it's kind of a singleton owned by ChromeBrowserParts. You really need to document the lifetime and purpose of the getter below better than this. https://codereview.chromium.org/2364703002/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/preferences.cc (right): https://codereview.chromium.org/2364703002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/preferences.cc:127: registry->RegisterDictionaryPref(prefs::kNetworkThrottlingEnabled); Agreed with Mattias that I don't understand why we need this registered in multiple places. https://codereview.chromium.org/2364703002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/preferences.cc:641: if (!throttling_policy) { empty clause? https://codereview.chromium.org/2364703002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/preferences.cc:650: NetworkThrottlingObserver::Get()->OnPreferenceChanged( This is kind of ugly - can you have NetworkThrottlingObserver do its own observing? Why do we need logic here? https://codereview.chromium.org/2364703002/diff/120001/chromeos/dbus/fake_shi... File chromeos/dbus/fake_shill_manager_client.cc (right): https://codereview.chromium.org/2364703002/diff/120001/chromeos/dbus/fake_shi... chromeos/dbus/fake_shill_manager_client.cc:10: #include <memory> Why are these needed? https://codereview.chromium.org/2364703002/diff/120001/chromeos/dbus/shill_ma... File chromeos/dbus/shill_manager_client.h (right): https://codereview.chromium.org/2364703002/diff/120001/chromeos/dbus/shill_ma... chromeos/dbus/shill_manager_client.h:227: virtual void SetNetworkThrottlingStatus( Add comment/documentation about how to use this. https://codereview.chromium.org/2364703002/diff/120001/chromeos/network/netwo... File chromeos/network/network_state_handler.cc (right): https://codereview.chromium.org/2364703002/diff/120001/chromeos/network/netwo... chromeos/network/network_state_handler.cc:8: #include <utility> Why is this here? https://codereview.chromium.org/2364703002/diff/120001/chromeos/network/netwo... chromeos/network/network_state_handler.cc:395: " , " + base::IntToString(download_rate_kbits)) nit, typically put a space after the comma but not before it. https://codereview.chromium.org/2364703002/diff/120001/chromeos/network/shill... File chromeos/network/shill_property_handler.cc (right): https://codereview.chromium.org/2364703002/diff/120001/chromeos/network/shill... chromeos/network/shill_property_handler.cc:9: #include <memory> You don't seem to have added any code that requires these includes? https://codereview.chromium.org/2364703002/diff/120001/components/policy/reso... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/2364703002/diff/120001/components/policy/reso... components/policy/resources/policy_templates.json:8796: If set to true, the system is throttled to achieve the provided upload and download rates (in kbits/s).''', Should we update the documentation to make it clear this applies to all users on the device? https://codereview.chromium.org/2364703002/diff/120001/components/prefs/pref_... File components/prefs/pref_service.cc (right): https://codereview.chromium.org/2364703002/diff/120001/components/prefs/pref_... components/prefs/pref_service.cc:538: const base::Value* result = pref_service_->GetPreferenceValue(name_); Please don't bring files into your CL just to fix typos - if you want to fix this, move it to a separate CL so you don't have to balloon the set of reviewers.
Why is .chroot_lock part of this CL? Also, the NetworkThrottlingObserver is kind of a meaningless class as it stands. Either you need to move the pref-observing code into that class, or else nuke the class entirely and have chromeos::Preferences() call NetworkHandler directly to apply changes to the throttling pref. https://codereview.chromium.org/2364703002/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right): https://codereview.chromium.org/2364703002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:414: network_throttling_observer_.reset(new NetworkThrottlingObserver()); Why are we constructing and not using Get()? https://codereview.chromium.org/2364703002/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/net/network_throttling_observer.cc (right): https://codereview.chromium.org/2364703002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/net/network_throttling_observer.cc:39: // borrowed from wake_on_wifi_manager - check if it is really needed here. What is this? What is the purpose here? Let's not check this in without explaining what this code does. https://codereview.chromium.org/2364703002/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/net/network_throttling_observer.h (right): https://codereview.chromium.org/2364703002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/net/network_throttling_observer.h:25: class NetworkThrottlingObserver { This is a weird class - it's owned by ChromeBrowserParts but has a getter here, so it's kind of a singleton owned by ChromeBrowserParts. You really need to document the lifetime and purpose of the getter below better than this. https://codereview.chromium.org/2364703002/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/preferences.cc (right): https://codereview.chromium.org/2364703002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/preferences.cc:127: registry->RegisterDictionaryPref(prefs::kNetworkThrottlingEnabled); Agreed with Mattias that I don't understand why we need this registered in multiple places. https://codereview.chromium.org/2364703002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/preferences.cc:641: if (!throttling_policy) { empty clause? https://codereview.chromium.org/2364703002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/preferences.cc:650: NetworkThrottlingObserver::Get()->OnPreferenceChanged( This is kind of ugly - can you have NetworkThrottlingObserver do its own observing? Why do we need logic here? https://codereview.chromium.org/2364703002/diff/120001/chromeos/dbus/fake_shi... File chromeos/dbus/fake_shill_manager_client.cc (right): https://codereview.chromium.org/2364703002/diff/120001/chromeos/dbus/fake_shi... chromeos/dbus/fake_shill_manager_client.cc:10: #include <memory> Why are these needed? https://codereview.chromium.org/2364703002/diff/120001/chromeos/dbus/shill_ma... File chromeos/dbus/shill_manager_client.h (right): https://codereview.chromium.org/2364703002/diff/120001/chromeos/dbus/shill_ma... chromeos/dbus/shill_manager_client.h:227: virtual void SetNetworkThrottlingStatus( Add comment/documentation about how to use this. https://codereview.chromium.org/2364703002/diff/120001/chromeos/network/netwo... File chromeos/network/network_state_handler.cc (right): https://codereview.chromium.org/2364703002/diff/120001/chromeos/network/netwo... chromeos/network/network_state_handler.cc:8: #include <utility> Why is this here? https://codereview.chromium.org/2364703002/diff/120001/chromeos/network/netwo... chromeos/network/network_state_handler.cc:395: " , " + base::IntToString(download_rate_kbits)) nit, typically put a space after the comma but not before it. https://codereview.chromium.org/2364703002/diff/120001/chromeos/network/shill... File chromeos/network/shill_property_handler.cc (right): https://codereview.chromium.org/2364703002/diff/120001/chromeos/network/shill... chromeos/network/shill_property_handler.cc:9: #include <memory> You don't seem to have added any code that requires these includes? https://codereview.chromium.org/2364703002/diff/120001/components/policy/reso... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/2364703002/diff/120001/components/policy/reso... components/policy/resources/policy_templates.json:8796: If set to true, the system is throttled to achieve the provided upload and download rates (in kbits/s).''', Should we update the documentation to make it clear this applies to all users on the device? https://codereview.chromium.org/2364703002/diff/120001/components/prefs/pref_... File components/prefs/pref_service.cc (right): https://codereview.chromium.org/2364703002/diff/120001/components/prefs/pref_... components/prefs/pref_service.cc:538: const base::Value* result = pref_service_->GetPreferenceValue(name_); Please don't bring files into your CL just to fix typos - if you want to fix this, move it to a separate CL so you don't have to balloon the set of reviewers.
Thanks for putting this into NetworkStateHandler. My primary question now is whether the Shill call affects just the current profile (i.e. if the change reverts when a user logs out), and whether we want this to be available to device and/or to user policies. https://codereview.chromium.org/2364703002/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/net/network_throttling_observer.cc (right): https://codereview.chromium.org/2364703002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/net/network_throttling_observer.cc:33: } nit: blank line https://codereview.chromium.org/2364703002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/net/network_throttling_observer.cc:39: // borrowed from wake_on_wifi_manager - check if it is really needed here. On 2016/10/21 13:27:30, Andrew T Wilson (Slow) wrote: > What is this? What is the purpose here? Let's not check this in without > explaining what this code does. This should only be necessary if network throttling needs to be applied to the user profile in Shill (instead of the device profile). That is the case for wake on wifi which is why it does this check. Either remove this or update the comment. https://codereview.chromium.org/2364703002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/net/network_throttling_observer.cc:49: g_network_throttling_observer = NULL; s/NULL/nullptr (but see other comments) https://codereview.chromium.org/2364703002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/net/network_throttling_observer.cc:54: DCHECK(g_network_throttling_observer); Use CHECK instead of DCHECK, we want to make sure global singletons are never accessed after destruction. https://codereview.chromium.org/2364703002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/net/network_throttling_observer.cc:63: throttling_enabled, upload_rate_kbits, download_rate_kbits); This class isn't really an observer, i.e. it's not implementing some other Observer base class, so it is not clear to me that we even need it. Why not just call NetworkStateHandler::SetNetworkThrottlingStatus directly instead of NetworkThrottlingObserver::OnPreferenceChanged()? https://codereview.chromium.org/2364703002/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/net/network_throttling_observer.h (right): https://codereview.chromium.org/2364703002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/net/network_throttling_observer.h:25: class NetworkThrottlingObserver { On 2016/10/21 13:27:30, Andrew T Wilson (Slow) wrote: > This is a weird class - it's owned by ChromeBrowserParts but has a getter here, > so it's kind of a singleton owned by ChromeBrowserParts. You really need to > document the lifetime and purpose of the getter below better than this. +1. We generally use one of two patterns: 1) The class is owned by ChromeBrowserMainParts and passed to any other classes that need to access it. This is generally preferred, but sometimes inconvenient for classes that are accessed frequently. 2) The class has static Initialize(), Shutdown(), and Get() methods that manage the global instance. Initialize() and Shutdown() is called in ChromeBrowserMainParts(). Get() should assert if the instance is uninitailized. e.g. see NetworkHandler. https://codereview.chromium.org/2364703002/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/preferences.cc (right): https://codereview.chromium.org/2364703002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/preferences.cc:650: NetworkThrottlingObserver::Get()->OnPreferenceChanged( On 2016/10/21 13:27:30, Andrew T Wilson (Slow) wrote: > This is kind of ugly - can you have NetworkThrottlingObserver do its own > observing? Why do we need logic here? +1. NetworkThrottlingObserver should be a proper pref observer that gets called when the pref changes. Otherwise we may as well just call the NetworkState method here directly, https://codereview.chromium.org/2364703002/diff/120001/chromeos/dbus/fake_shi... File chromeos/dbus/fake_shill_manager_client.h (right): https://codereview.chromium.org/2364703002/diff/120001/chromeos/dbus/fake_shi... chromeos/dbus/fake_shill_manager_client.h:105: elim blank line https://codereview.chromium.org/2364703002/diff/120001/chromeos/dbus/fake_shi... chromeos/dbus/fake_shill_manager_client.h:111: const ErrorCallback& error_callback) override {} Don't provide a default implementation in the header. https://codereview.chromium.org/2364703002/diff/120001/chromeos/dbus/shill_ma... File chromeos/dbus/shill_manager_client.h (right): https://codereview.chromium.org/2364703002/diff/120001/chromeos/dbus/shill_ma... chromeos/dbus/shill_manager_client.h:227: virtual void SetNetworkThrottlingStatus( On 2016/10/21 13:27:30, Andrew T Wilson (Slow) wrote: > Add comment/documentation about how to use this. +1. Please document all 3 args. https://codereview.chromium.org/2364703002/diff/120001/chromeos/network/netwo... File chromeos/network/network_state_handler.h (right): https://codereview.chromium.org/2364703002/diff/120001/chromeos/network/netwo... chromeos/network/network_state_handler.h:230: uint32_t download_rate_kbits); Document, including args. Even though this happens to map to a similar Shill method, it may not always and we don't wan't callers of this to have to look up the implementation, so the arguments need to be documented in both places. https://codereview.chromium.org/2364703002/diff/120001/chromeos/network/shill... File chromeos/network/shill_property_handler.cc (right): https://codereview.chromium.org/2364703002/diff/120001/chromeos/network/shill... chromeos/network/shill_property_handler.cc:9: #include <memory> On 2016/10/21 13:27:31, Andrew T Wilson (Slow) wrote: > You don't seem to have added any code that requires these includes? <memory> is in the header so we don't need to repeat it here. (It does belong in the header since we use std::unique_ptr threre). https://codereview.chromium.org/2364703002/diff/120001/components/policy/reso... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/2364703002/diff/120001/components/policy/reso... components/policy/resources/policy_templates.json:8796: If set to true, the system is throttled to achieve the provided upload and download rates (in kbits/s).''', On 2016/10/21 13:27:31, Andrew T Wilson (Slow) wrote: > Should we update the documentation to make it clear this applies to all users on > the device? +1 https://codereview.chromium.org/2364703002/diff/120001/components/prefs/pref_... File components/prefs/pref_service.cc (right): https://codereview.chromium.org/2364703002/diff/120001/components/prefs/pref_... components/prefs/pref_service.cc:538: const base::Value* result = pref_service_->GetPreferenceValue(name_); On 2016/10/21 13:27:31, Andrew T Wilson (Slow) wrote: > Please don't bring files into your CL just to fix typos - if you want to fix > this, move it to a separate CL so you don't have to balloon the set of > reviewers. +1. And in general, trivial changes like this should be ignored unless this file is changed in some other way.
> Thanks for putting this into NetworkStateHandler. My primary question now is > whether the Shill call affects just the current profile (i.e. if the change > reverts when a user logs out), and whether we want this to be available to > device and/or to user policies. The shill call is persistent, we want this to be system-wide. So it will persist until the policy changes. The shill-side CL for this adds a libpolicy dependency, making shill query libpolicy on startup for the throttling status and apply it. > > This should only be necessary if network throttling needs to be applied to the > user profile in Shill (instead of the device profile). That is the case for wake > on wifi which is why it does this check. Either remove this or update the > comment. Will remove the check. > chrome/browser/chromeos/net/network_throttling_observer.cc:63: > throttling_enabled, upload_rate_kbits, download_rate_kbits); > This class isn't really an observer, i.e. it's not implementing some other > Observer base class, so it is not clear to me that we even need it. Why not just > call NetworkStateHandler::SetNetworkThrottlingStatus directly instead of > NetworkThrottlingObserver::OnPreferenceChanged()? If its acceptable to call NetworkStateHandler directly from preferences.cc, I'll change it to that. The only (misplaced) reason I had NetworkThrottlingObserver was that there didn't seem to be precedence for doing work directly in ApplyPreferences. > <memory> is in the header so we don't need to repeat it here. (It does belong in > the header since we use std::unique_ptr threre). This and other includes were added to silence 'git-cl lint'. I will remove them.
> Thanks for putting this into NetworkStateHandler. My primary question now is > whether the Shill call affects just the current profile (i.e. if the change > reverts when a user logs out), and whether we want this to be available to > device and/or to user policies. The shill call is persistent, we want this to be system-wide. So it will persist until the policy changes. The shill-side CL for this adds a libpolicy dependency, making shill query libpolicy on startup for the throttling status and apply it. > > This should only be necessary if network throttling needs to be applied to the > user profile in Shill (instead of the device profile). That is the case for wake > on wifi which is why it does this check. Either remove this or update the > comment. Will remove the check. > chrome/browser/chromeos/net/network_throttling_observer.cc:63: > throttling_enabled, upload_rate_kbits, download_rate_kbits); > This class isn't really an observer, i.e. it's not implementing some other > Observer base class, so it is not clear to me that we even need it. Why not just > call NetworkStateHandler::SetNetworkThrottlingStatus directly instead of > NetworkThrottlingObserver::OnPreferenceChanged()? If its acceptable to call NetworkStateHandler directly from preferences.cc, I'll change it to that. The only (misplaced) reason I had NetworkThrottlingObserver was that there didn't seem to be precedence for doing work directly in ApplyPreferences. > <memory> is in the header so we don't need to repeat it here. (It does belong in > the header since we use std::unique_ptr threre). This and other includes were added to silence 'git-cl lint'. I will remove them.
On 2016/10/21 20:08:18, kirtika1 wrote: > > Thanks for putting this into NetworkStateHandler. My primary question now is > > whether the Shill call affects just the current profile (i.e. if the change > > reverts when a user logs out), and whether we want this to be available to > > device and/or to user policies. > > The shill call is persistent, we want this to be system-wide. > So it will persist until the policy changes. The shill-side CL for this > adds a libpolicy dependency, making shill query libpolicy on startup > for the throttling status and apply it. > > > > > > > This should only be necessary if network throttling needs to be applied to the > > user profile in Shill (instead of the device profile). That is the case for > wake > > on wifi which is why it does this check. Either remove this or update the > > comment. > > Will remove the check. > > > chrome/browser/chromeos/net/network_throttling_observer.cc:63: > > throttling_enabled, upload_rate_kbits, download_rate_kbits); > > This class isn't really an observer, i.e. it's not implementing some other > > Observer base class, so it is not clear to me that we even need it. Why not > just > > call NetworkStateHandler::SetNetworkThrottlingStatus directly instead of > > NetworkThrottlingObserver::OnPreferenceChanged()? > > If its acceptable to call NetworkStateHandler directly from preferences.cc, I'll > > change it to that. The only (misplaced) reason I had NetworkThrottlingObserver > was > that there didn't seem to be precedence for doing work directly in > ApplyPreferences. > > > > <memory> is in the header so we don't need to repeat it here. (It does belong > in > > the header since we use std::unique_ptr threre). > > This and other includes were added to silence 'git-cl lint'. > I will remove them. I agree that calling into NetworkStateHandler directly from preferences.cc is not a good idea, but calling into NetworkThrottlingObserver is not really any better. Please look into making NetworkThrottlingObserver a pref observer instead.
https://codereview.chromium.org/2364703002/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/net/network_throttling_observer.cc (right): https://codereview.chromium.org/2364703002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/net/network_throttling_observer.cc:39: // borrowed from wake_on_wifi_manager - check if it is really needed here. On 2016/10/21 19:41:20, stevenjb wrote: > On 2016/10/21 13:27:30, Andrew T Wilson (Slow) wrote: > > What is this? What is the purpose here? Let's not check this in without > > explaining what this code does. > > This should only be necessary if network throttling needs to be applied to the > user profile in Shill (instead of the device profile). That is the case for wake > on wifi which is why it does this check. Either remove this or update the > comment. > I see now that the policy code is specifically in device_policy_decoder_chromeos.cc, so that seems fine. We shouldn't need the logged in state check here in that case.
https://codereview.chromium.org/2364703002/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/net/network_throttling_observer.h (right): https://codereview.chromium.org/2364703002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/net/network_throttling_observer.h:25: class NetworkThrottlingObserver { On 2016/10/21 19:41:20, stevenjb wrote: > On 2016/10/21 13:27:30, Andrew T Wilson (Slow) wrote: > > This is a weird class - it's owned by ChromeBrowserParts but has a getter > here, > > so it's kind of a singleton owned by ChromeBrowserParts. You really need to > > document the lifetime and purpose of the getter below better than this. > > +1. We generally use one of two patterns: > > 1) The class is owned by ChromeBrowserMainParts and passed to any other classes > that need to access it. This is generally preferred, but sometimes inconvenient > for classes that are accessed frequently. > > 2) The class has static Initialize(), Shutdown(), and Get() methods that manage > the global instance. Initialize() and Shutdown() is called in > ChromeBrowserMainParts(). Get() should assert if the instance is uninitailized. > e.g. see NetworkHandler. If we make this a pref observer we shouldn't need Get() at all since this class will register itself as a pref observer on construction and OnPreferenceChanged does not need to be called explicitly.
https://codereview.chromium.org/2364703002/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right): https://codereview.chromium.org/2364703002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:414: network_throttling_observer_.reset(new NetworkThrottlingObserver()); On 2016/10/21 13:27:30, Andrew T Wilson (Slow) wrote: > Why are we constructing and not using Get()? The next patch-set removes Get(). I was emulating wake_on_wifi_manager_ right above, whose Get() does not construct, simply gives the pointer, so constructing here was necessary. https://codereview.chromium.org/2364703002/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/net/network_throttling_observer.cc (right): https://codereview.chromium.org/2364703002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/net/network_throttling_observer.cc:33: } On 2016/10/21 19:41:20, stevenjb wrote: > nit: blank line Done. https://codereview.chromium.org/2364703002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/net/network_throttling_observer.cc:39: // borrowed from wake_on_wifi_manager - check if it is really needed here. On 2016/10/21 13:27:30, Andrew T Wilson (Slow) wrote: > What is this? What is the purpose here? Let's not check this in without > explaining what this code does. Done. https://codereview.chromium.org/2364703002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/net/network_throttling_observer.cc:49: g_network_throttling_observer = NULL; On 2016/10/21 19:41:20, stevenjb wrote: > s/NULL/nullptr (but see other comments) Done. https://codereview.chromium.org/2364703002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/net/network_throttling_observer.cc:54: DCHECK(g_network_throttling_observer); On 2016/10/21 19:41:20, stevenjb wrote: > Use CHECK instead of DCHECK, we want to make sure global singletons are never > accessed after destruction. Done. https://codereview.chromium.org/2364703002/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/net/network_throttling_observer.h (right): https://codereview.chromium.org/2364703002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/net/network_throttling_observer.h:25: class NetworkThrottlingObserver { On 2016/10/21 19:41:20, stevenjb wrote: > On 2016/10/21 13:27:30, Andrew T Wilson (Slow) wrote: > > This is a weird class - it's owned by ChromeBrowserParts but has a getter > here, > > so it's kind of a singleton owned by ChromeBrowserParts. You really need to > > document the lifetime and purpose of the getter below better than this. > > +1. We generally use one of two patterns: > > 1) The class is owned by ChromeBrowserMainParts and passed to any other classes > that need to access it. This is generally preferred, but sometimes inconvenient > for classes that are accessed frequently. > > 2) The class has static Initialize(), Shutdown(), and Get() methods that manage > the global instance. Initialize() and Shutdown() is called in > ChromeBrowserMainParts(). Get() should assert if the instance is uninitailized. > e.g. see NetworkHandler. Done, used (1) https://codereview.chromium.org/2364703002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/net/network_throttling_observer.h:25: class NetworkThrottlingObserver { > If we make this a pref observer we shouldn't need Get() at all since this class > will register itself as a pref observer on construction and OnPreferenceChanged > does not need to be called explicitly. Thanks for that suggestion!Done. https://codereview.chromium.org/2364703002/diff/120001/chromeos/dbus/fake_shi... File chromeos/dbus/fake_shill_manager_client.h (right): https://codereview.chromium.org/2364703002/diff/120001/chromeos/dbus/fake_shi... chromeos/dbus/fake_shill_manager_client.h:111: const ErrorCallback& error_callback) override {} On 2016/10/21 19:41:21, stevenjb wrote: > Don't provide a default implementation in the header. Done. https://codereview.chromium.org/2364703002/diff/120001/chromeos/dbus/shill_ma... File chromeos/dbus/shill_manager_client.h (right): https://codereview.chromium.org/2364703002/diff/120001/chromeos/dbus/shill_ma... chromeos/dbus/shill_manager_client.h:227: virtual void SetNetworkThrottlingStatus( On 2016/10/21 19:41:21, stevenjb wrote: > On 2016/10/21 13:27:30, Andrew T Wilson (Slow) wrote: > > Add comment/documentation about how to use this. > > +1. Please document all 3 args. Done. https://codereview.chromium.org/2364703002/diff/120001/chromeos/network/netwo... File chromeos/network/network_state_handler.cc (right): https://codereview.chromium.org/2364703002/diff/120001/chromeos/network/netwo... chromeos/network/network_state_handler.cc:8: #include <utility> On 2016/10/21 13:27:30, Andrew T Wilson (Slow) wrote: > Why is this here? These and other includes were added to silence cpplint.py for the files I touched (existing errors made it hard for me to find linter errors I introduced). These have been removed. https://codereview.chromium.org/2364703002/diff/120001/chromeos/network/netwo... chromeos/network/network_state_handler.cc:395: " , " + base::IntToString(download_rate_kbits)) On 2016/10/21 13:27:30, Andrew T Wilson (Slow) wrote: > nit, typically put a space after the comma but not before it. Done. https://codereview.chromium.org/2364703002/diff/120001/chromeos/network/shill... File chromeos/network/shill_property_handler.cc (right): https://codereview.chromium.org/2364703002/diff/120001/chromeos/network/shill... chromeos/network/shill_property_handler.cc:9: #include <memory> On 2016/10/21 13:27:31, Andrew T Wilson (Slow) wrote: > You don't seem to have added any code that requires these includes? Done. https://codereview.chromium.org/2364703002/diff/120001/components/policy/reso... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/2364703002/diff/120001/components/policy/reso... components/policy/resources/policy_templates.json:8796: If set to true, the system is throttled to achieve the provided upload and download rates (in kbits/s).''', On 2016/10/21 13:27:31, Andrew T Wilson (Slow) wrote: > Should we update the documentation to make it clear this applies to all users on > the device? Done. https://codereview.chromium.org/2364703002/diff/120001/components/prefs/pref_... File components/prefs/pref_service.cc (right): https://codereview.chromium.org/2364703002/diff/120001/components/prefs/pref_... components/prefs/pref_service.cc:538: const base::Value* result = pref_service_->GetPreferenceValue(name_); On 2016/10/21 19:41:21, stevenjb wrote: > On 2016/10/21 13:27:31, Andrew T Wilson (Slow) wrote: > > Please don't bring files into your CL just to fix typos - if you want to fix > > this, move it to a separate CL so you don't have to balloon the set of > > reviewers. > > +1. And in general, trivial changes like this should be ignored unless this file > is changed in some other way. Done.
Much better, thanks. We need a test for this also. You can do this using a mock NetworkStateHandler similar to what wake_on_wifi_manager_unittest.cc does. https://codereview.chromium.org/2364703002/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right): https://codereview.chromium.org/2364703002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:803: wake_on_wifi_manager_.reset(); We should explicitly reset network_throttling_observer_ also to reduce the chances of destruction order related edge cases. https://codereview.chromium.org/2364703002/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/net/network_throttling_observer.cc (right): https://codereview.chromium.org/2364703002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/net/network_throttling_observer.cc:15: #include "chrome/browser/profiles/profile.h" Needed? https://codereview.chromium.org/2364703002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/net/network_throttling_observer.cc:27: NetworkThrottlingObserver* g_network_throttling_observer = nullptr; You shouldn't need this any more. https://codereview.chromium.org/2364703002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/net/network_throttling_observer.cc:36: pref_change_registrar_.Init(g_browser_process->local_state()); Instead of accessing g_browser_process, pass g_browser_process->local_state() to the constructor as a PrefService* and store it as a member, e.g. local_state_ for use in OnPreferenceChanged. That way this class does not have any chrome/browser dependencies and we can more easily move it later if we decide we need to. https://codereview.chromium.org/2364703002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/net/network_throttling_observer.cc:39: &NetworkThrottlingObserver::OnPreferenceChanged, base::Unretained(this)); base::Unretained is dangerous here. If this class is destroyed before PrefService the callback could occur after the class is destroyed. You need to use base::WeakPtr here (See WakeOnWifiManager for an example). https://codereview.chromium.org/2364703002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/net/network_throttling_observer.cc:62: if (throttling_policy) { nit: early exit: if (!throttling_policy) return; https://codereview.chromium.org/2364703002/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/net/network_throttling_observer.h (right): https://codereview.chromium.org/2364703002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/net/network_throttling_observer.h:21: // changes in Chrome down to shill which implements by calling 'tc' in the s/shill/Shill/ https://codereview.chromium.org/2364703002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/net/network_throttling_observer.h:24: // being called. The last sentence isn't really necessary, that level of detail is better gained by looking at the code. Describing ownership and purpose is sufficient. https://codereview.chromium.org/2364703002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/net/network_throttling_observer.h:27: // -> ShillManagerClient Also not necessary.
I've made the corrections stevenjb@ suggested, and added unit-tests for network throttling observer and shill manager client. Network state handler and shill property handler do not have unit-tests for methods similar to what I added (both wrapper functions), I skipped those. https://codereview.chromium.org/2364703002/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right): https://codereview.chromium.org/2364703002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:803: wake_on_wifi_manager_.reset(); On 2016/10/24 17:14:16, stevenjb wrote: > We should explicitly reset network_throttling_observer_ also to reduce the > chances of destruction order related edge cases. Done. https://codereview.chromium.org/2364703002/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/net/network_throttling_observer.cc (right): https://codereview.chromium.org/2364703002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/net/network_throttling_observer.cc:15: #include "chrome/browser/profiles/profile.h" On 2016/10/24 17:14:16, stevenjb wrote: > Needed? Removed. https://codereview.chromium.org/2364703002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/net/network_throttling_observer.cc:27: NetworkThrottlingObserver* g_network_throttling_observer = nullptr; On 2016/10/24 17:14:16, stevenjb wrote: > You shouldn't need this any more. Done. https://codereview.chromium.org/2364703002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/net/network_throttling_observer.cc:36: pref_change_registrar_.Init(g_browser_process->local_state()); On 2016/10/24 17:14:16, stevenjb wrote: > Instead of accessing g_browser_process, pass g_browser_process->local_state() to > the constructor as a PrefService* and store it as a member, e.g. local_state_ > for use in OnPreferenceChanged. That way this class does not have any > chrome/browser dependencies and we can more easily move it later if we decide we > need to. Done. https://codereview.chromium.org/2364703002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/net/network_throttling_observer.cc:39: &NetworkThrottlingObserver::OnPreferenceChanged, base::Unretained(this)); On 2016/10/24 17:14:16, stevenjb wrote: > base::Unretained is dangerous here. If this class is destroyed before > PrefService the callback could occur after the class is destroyed. You need to > use base::WeakPtr here (See WakeOnWifiManager for an example). > Done. I could follow your explanation for this specific case, but am still confused about general usage. This link does not list WeakPtr as a possible option, though lots of Chromium (and Shill, and others) code uses that binding? https://cs.chromium.org/chromium/src/base/bind_helpers.h?q=base::Unretained&s... https://codereview.chromium.org/2364703002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/net/network_throttling_observer.cc:62: if (throttling_policy) { On 2016/10/24 17:14:16, stevenjb wrote: > nit: early exit: > if (!throttling_policy) > return; Done. https://codereview.chromium.org/2364703002/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/net/network_throttling_observer.h (right): https://codereview.chromium.org/2364703002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/net/network_throttling_observer.h:21: // changes in Chrome down to shill which implements by calling 'tc' in the On 2016/10/24 17:14:16, stevenjb wrote: > s/shill/Shill/ > Done. https://codereview.chromium.org/2364703002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/net/network_throttling_observer.h:24: // being called. On 2016/10/24 17:14:16, stevenjb wrote: > The last sentence isn't really necessary, that level of detail is better gained > by looking at the code. Describing ownership and purpose is sufficient. Done. https://codereview.chromium.org/2364703002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/net/network_throttling_observer.h:27: // -> ShillManagerClient On 2016/10/24 17:14:16, stevenjb wrote: > Also not necessary. Done.
kirtika@google.com changed reviewers: + c-readability-approvers@google.com
Nice test, thanks for adding it. LGTM! https://codereview.chromium.org/2364703002/diff/180001/chrome/browser/chromeo... File chrome/browser/chromeos/net/network_throttling_observer.cc (right): https://codereview.chromium.org/2364703002/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/net/network_throttling_observer.cc:14: #include "chrome/browser/browser_process.h" This dependency should be removed now? https://codereview.chromium.org/2364703002/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/net/network_throttling_observer.cc:52: } nit: {} not needed
The CQ bit was checked by kirtika@google.com
The CQ bit was unchecked by kirtika@google.com
The CQ bit was checked by kirtika@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from stevenjb@chromium.org Link to the patchset: https://codereview.chromium.org/2364703002/#ps200001 (title: "Add network bandwidth throttling as an enterprise policy")
lgtm https://codereview.chromium.org/2364703002/diff/180001/chrome/browser/chromeo... File chrome/browser/chromeos/net/network_throttling_observer.cc (right): https://codereview.chromium.org/2364703002/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/net/network_throttling_observer.cc:14: #include "chrome/browser/browser_process.h" On 2016/10/27 22:54:38, stevenjb wrote: > This dependency should be removed now? Done. https://codereview.chromium.org/2364703002/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/net/network_throttling_observer.cc:52: } On 2016/10/27 22:54:38, stevenjb wrote: > nit: {} not needed Done.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by kirtika@chromium.org
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...)
kirtika@google.com changed reviewers: + gab@chromium.org
On 2016/10/27 23:16:00, commit-bot: I haz the power wrote: > 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...) Trybots failed due to missing lgtm from the following reviewers (much thanks to stevenjb@ for covering the rest). ** Presubmit ERRORS ** Missing LGTM from an OWNER for these files: chrome/browser/policy/configuration_policy_handler_list_factory.cc chrome/browser/prefs/browser_prefs.cc components/policy/resources/policy_templates.json tools/metrics/histograms/histograms.xml Added gab@ for help with prefs. atwilson@ / mnissler@ - could you help with an LGTM for the policy part?
On 2016/10/27 23:16:00, commit-bot: I haz the power wrote: > 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...) Trybots failed due to missing lgtm from the following reviewers (much thanks to stevenjb@ for covering the rest). ** Presubmit ERRORS ** Missing LGTM from an OWNER for these files: chrome/browser/policy/configuration_policy_handler_list_factory.cc chrome/browser/prefs/browser_prefs.cc components/policy/resources/policy_templates.json tools/metrics/histograms/histograms.xml Added gab@ for help with prefs. atwilson@ / mnissler@ - could you help with an LGTM for the policy part?
On 2016/10/27 23:37:48, kirtika1 wrote: > > Added gab@ for help with prefs. > atwilson@ / mnissler@ - could you help with an LGTM for the policy part? I'll take a look later today, but FYI looks like there are compile errors?
The CQ bit was checked by kirtika@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from stevenjb@chromium.org, kirtika@google.com Link to the patchset: https://codereview.chromium.org/2364703002/#ps220001 (title: "Add network bandwidth throttling as an enterprise policy")
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: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
c/b/prefs rs lgtm
The CQ bit was checked by kirtika@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from stevenjb@chromium.org, kirtika@google.com, gab@chromium.org Link to the patchset: https://codereview.chromium.org/2364703002/#ps240001 (title: "Add network bandwidth throttling as an enterprise policy")
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...)
The CQ bit was checked by kirtika@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Mostly LG, had a few questions about behavior when policy is removed. https://codereview.chromium.org/2364703002/diff/220001/chrome/browser/chromeo... File chrome/browser/chromeos/net/network_throttling_observer.cc (right): https://codereview.chromium.org/2364703002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/net/network_throttling_observer.cc:28: weak_ptr_factory_.GetWeakPtr()); Why is this weak ptr factory required? Can the callback actually ever be invoked after you call PrefChangeRegistrar::RemoveAll()? https://codereview.chromium.org/2364703002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/net/network_throttling_observer.cc:49: if (!throttling_policy) What does it mean if throttling_policy is null - does it mean that the admin has removed the policy? Don't we want to call SetNetworkThrottlingStatus(false, 0, 0) in that case? https://codereview.chromium.org/2364703002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/net/network_throttling_observer.cc:56: reinterpret_cast<int*>(&upload_rate)); I'm OK with this, but this falls apart if int != 32 bits, or if int < 0. It would be cleaner to actually read an int, then do some manual conversion of values after range checking. If you don't think it's possible for any of these attributes (like upload_rate_kbits) to be missing, then CHECK() to make sure GetXXXX() returns true. Otherwise, handle the error. https://codereview.chromium.org/2364703002/diff/220001/chrome/browser/chromeo... File chrome/browser/chromeos/net/network_throttling_observer.h (right): https://codereview.chromium.org/2364703002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/net/network_throttling_observer.h:31: void OnPreferenceChanged(const std::string& pref_name); I think OnPreferenceChanged() should be private, correct? https://codereview.chromium.org/2364703002/diff/220001/chrome/browser/chromeo... File chrome/browser/chromeos/net/network_throttling_observer_unittest.cc (right): https://codereview.chromium.org/2364703002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/net/network_throttling_observer_unittest.cc:59: observer_->OnPreferenceChanged(prefs::kNetworkThrottlingEnabled); This test is a bad reason to make OnPreferenceChanged() public. Why do you need to call this directly here? https://codereview.chromium.org/2364703002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/net/network_throttling_observer_unittest.cc:75: local_state_->Set(prefs::kNetworkThrottlingEnabled, Can you test what happens if you then clear/remove this preference? Shill should be called again to disable throttling, right?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2364703002/diff/220001/chrome/browser/chromeo... File chrome/browser/chromeos/net/network_throttling_observer.cc (right): https://codereview.chromium.org/2364703002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/net/network_throttling_observer.cc:28: weak_ptr_factory_.GetWeakPtr()); On 2016/10/28 14:31:54, Andrew T Wilson (Slow) wrote: > Why is this weak ptr factory required? Can the callback actually ever be invoked > after you call PrefChangeRegistrar::RemoveAll()? This was my fault actually, I haven't used PrefChangeRegistrar myself until recently. I didn't notice that it removes observers on destruction, making weak_ptr unnecessary here. kirtika@ you can go ahead and clean that up with your next change to this code (or in a separate follow up if it is complete). https://codereview.chromium.org/2364703002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/net/network_throttling_observer.cc:56: reinterpret_cast<int*>(&upload_rate)); On 2016/10/28 14:31:54, Andrew T Wilson (Slow) wrote: > I'm OK with this, but this falls apart if int != 32 bits, or if int < 0. It > would be cleaner to actually read an int, then do some manual conversion of > values after range checking. > > If you don't think it's possible for any of these attributes (like > upload_rate_kbits) to be missing, then CHECK() to make sure GetXXXX() returns > true. Otherwise, handle the error. I missed this, +1.
The CQ bit was checked by kirtika@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.
Description was changed from
==========
Add support for an enterprise policy that lets the admin
turn on network throttling, and specify the upload and download rates in kbps to
throttle to.
While there, fix some linter warnings in files touched.
BUG=642811
Signed-off-by: Kirtika Ruchandani <kirtika@google.com>
==========
to
==========
Add support for an enterprise policy that lets the admin
turn on network throttling, and specify the upload and download rates in kbps to
throttle to.
BUG=642811
TEST=(1) Unit-tests
(2) Added network throttling policy to YAPS server,
enabled throttling with various values, disabled,
and checked that changes propagate to client device.
Signed-off-by: Kirtika Ruchandani <kirtika@google.com>
==========
kirtika@google.com changed reviewers: + asvitkine@chromium.org, brettw@chromium.org, isherman@chromium.org - c-readability-approvers@google.com
Added more owners for help with LGTM. PTAL. - brettw@ - for chrome/browser/policy/configuration_policy_handler_list_factory.cc - isherman@ or asvitkine@ for histograms.xml
histograms.xml lgtm
The CQ bit was checked by kirtika@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
LGTM with one style nit https://codereview.chromium.org/2364703002/diff/260001/chrome/browser/chromeo... File chrome/browser/chromeos/net/network_throttling_observer.cc (right): https://codereview.chromium.org/2364703002/diff/260001/chrome/browser/chromeo... chrome/browser/chromeos/net/network_throttling_observer.cc:54: if (throttling_policy->GetInteger("upload_rate_kbits", &upload_rate_read) && If you have a multi-line if statement, you need to use braces. So: if (foo) bar; is fine but if (foo && wrapped_expr) { bar; } Is what you need to do here and below.
LGTM with one style nit https://codereview.chromium.org/2364703002/diff/260001/chrome/browser/chromeo... File chrome/browser/chromeos/net/network_throttling_observer.cc (right): https://codereview.chromium.org/2364703002/diff/260001/chrome/browser/chromeo... chrome/browser/chromeos/net/network_throttling_observer.cc:54: if (throttling_policy->GetInteger("upload_rate_kbits", &upload_rate_read) && If you have a multi-line if statement, you need to use braces. So: if (foo) bar; is fine but if (foo && wrapped_expr) { bar; } Is what you need to do here and below.
The CQ bit was checked by kirtika@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from stevenjb@chromium.org, gab@chromium.org, kirtika@google.com, atwilson@chromium.org, isherman@chromium.org Link to the patchset: https://codereview.chromium.org/2364703002/#ps280001 (title: "Add network bandwidth throttling as an enterprise policy")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
All I looked at was configuration_policy_handler_list_factory.cc. That LGTM.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by kirtika@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from
==========
Add support for an enterprise policy that lets the admin
turn on network throttling, and specify the upload and download rates in kbps to
throttle to.
BUG=642811
TEST=(1) Unit-tests
(2) Added network throttling policy to YAPS server,
enabled throttling with various values, disabled,
and checked that changes propagate to client device.
Signed-off-by: Kirtika Ruchandani <kirtika@google.com>
==========
to
==========
Add support for an enterprise policy that lets the admin
turn on network throttling, and specify the upload and download rates in kbps to
throttle to.
BUG=642811
TEST=(1) Unit-tests
(2) Added network throttling policy to YAPS server,
enabled throttling with various values, disabled,
and checked that changes propagate to client device.
Signed-off-by: Kirtika Ruchandani <kirtika@google.com>
==========
Message was sent while issue was closed.
Committed patchset #15 (id:280001)
Message was sent while issue was closed.
Description was changed from
==========
Add support for an enterprise policy that lets the admin
turn on network throttling, and specify the upload and download rates in kbps to
throttle to.
BUG=642811
TEST=(1) Unit-tests
(2) Added network throttling policy to YAPS server,
enabled throttling with various values, disabled,
and checked that changes propagate to client device.
Signed-off-by: Kirtika Ruchandani <kirtika@google.com>
==========
to
==========
Add support for an enterprise policy that lets the admin
turn on network throttling, and specify the upload and download rates in kbps to
throttle to.
BUG=642811
TEST=(1) Unit-tests
(2) Added network throttling policy to YAPS server,
enabled throttling with various values, disabled,
and checked that changes propagate to client device.
Signed-off-by: Kirtika Ruchandani <kirtika@google.com>
Committed: https://crrev.com/7db3c2b899d448dc0235e03a248df83aa5778ebf
Cr-Commit-Position: refs/heads/master@{#428565}
==========
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/7db3c2b899d448dc0235e03a248df83aa5778ebf Cr-Commit-Position: refs/heads/master@{#428565}
Message was sent while issue was closed.
thestig@chromium.org changed reviewers: + thestig@chromium.org
Message was sent while issue was closed.
There is a persistent red bot on the waterfall that lead me here. I commented on some of the problems in network_throttling_observer_unittest.cc and left a few style nits. Hope you can fix the problem before sheriffs decide to revert this CL. https://codereview.chromium.org/2364703002/diff/280001/chrome/browser/chromeo... File chrome/browser/chromeos/net/network_throttling_observer.cc (right): https://codereview.chromium.org/2364703002/diff/280001/chrome/browser/chromeo... chrome/browser/chromeos/net/network_throttling_observer.cc:48: uint32_t upload_rate = 0, download_rate = 0; nit: 1 declaration per line please https://codereview.chromium.org/2364703002/diff/280001/chrome/browser/chromeo... File chrome/browser/chromeos/net/network_throttling_observer.h (right): https://codereview.chromium.org/2364703002/diff/280001/chrome/browser/chromeo... chrome/browser/chromeos/net/network_throttling_observer.h:10: #include "base/memory/weak_ptr.h" nit: not used https://codereview.chromium.org/2364703002/diff/280001/chrome/browser/chromeo... chrome/browser/chromeos/net/network_throttling_observer.h:18: // NetworkThrottlingObserver is a singleton, owned by Usually a singleton would not have a public constructor. Instead there's only a public static Get() method that's backed by a base::LazyInstance, or base::Singleton. *shrug* https://codereview.chromium.org/2364703002/diff/280001/chrome/browser/chromeo... File chrome/browser/chromeos/net/network_throttling_observer_unittest.cc (right): https://codereview.chromium.org/2364703002/diff/280001/chrome/browser/chromeo... chrome/browser/chromeos/net/network_throttling_observer_unittest.cc:29: DBusThreadManager::Initialize(); This is missing a matching call to DBusThreadManager::Shutdown(); at the end. Thus there is a red bot here complaining about the resulting memory leak(s): https://build.chromium.org/p/chromium.memory/builders/Linux%20Chromium%20OS%2... https://codereview.chromium.org/2364703002/diff/280001/chrome/browser/chromeo... chrome/browser/chromeos/net/network_throttling_observer_unittest.cc:34: std::unique_ptr<ShillManagerClient>(mock_manager_client_)); I'm rather worried about this. Is this telling SetShillManagerClient() that it can take ownership of|mock_manager_client_| ? Because |mock_manager_client_| also gets deleted on line 43. https://codereview.chromium.org/2364703002/diff/280001/chrome/browser/chromeo... chrome/browser/chromeos/net/network_throttling_observer_unittest.cc:40: observer_.reset(new NetworkThrottlingObserver(local_state_)); nit: foo.reset(new Foo(bar)) -> foo = base::MakeUnique<Foo>(bar) https://codereview.chromium.org/2364703002/diff/280001/chromeos/dbus/shill_ma... File chromeos/dbus/shill_manager_client_unittest.cc (right): https://codereview.chromium.org/2364703002/diff/280001/chromeos/dbus/shill_ma... chromeos/dbus/shill_manager_client_unittest.cc:63: EXPECT_EQ(throttling_enabled_actual, throttling_enabled_expected); nit: EXPECT_EQ(expected, actual) - reverse the arguments.
Message was sent while issue was closed.
https://codereview.chromium.org/2364703002/diff/280001/chrome/browser/chromeo... File chrome/browser/chromeos/net/network_throttling_observer.cc (right): https://codereview.chromium.org/2364703002/diff/280001/chrome/browser/chromeo... chrome/browser/chromeos/net/network_throttling_observer.cc:48: uint32_t upload_rate = 0, download_rate = 0; On 2016/10/29 16:04:04, Lei Zhang wrote: > nit: 1 declaration per line please Done. https://codereview.chromium.org/2364703002/diff/280001/chrome/browser/chromeo... File chrome/browser/chromeos/net/network_throttling_observer_unittest.cc (right): https://codereview.chromium.org/2364703002/diff/280001/chrome/browser/chromeo... chrome/browser/chromeos/net/network_throttling_observer_unittest.cc:29: DBusThreadManager::Initialize(); On 2016/10/29 16:04:05, Lei Zhang wrote: > This is missing a matching call to DBusThreadManager::Shutdown(); at the end. > > Thus there is a red bot here complaining about the resulting memory leak(s): > https://build.chromium.org/p/chromium.memory/builders/Linux%20Chromium%20OS%2... Naive question - is adding DBusThreadManager::Shutdown to the destructor enough? This file is expected to contain only one test until NetworkThrottlingObserver's functionality changes. My assumption is that as long as cleanup happens at the end of the test, it should be fine - does that assumption and the fix patch look right? https://codereview.chromium.org/2364703002/diff/280001/chrome/browser/chromeo... chrome/browser/chromeos/net/network_throttling_observer_unittest.cc:34: std::unique_ptr<ShillManagerClient>(mock_manager_client_)); On 2016/10/29 16:04:04, Lei Zhang wrote: > I'm rather worried about this. Is this telling SetShillManagerClient() that it > can take ownership of|mock_manager_client_| ? Because |mock_manager_client_| > also gets deleted on line 43. Done. https://codereview.chromium.org/2364703002/diff/280001/chrome/browser/chromeo... chrome/browser/chromeos/net/network_throttling_observer_unittest.cc:40: observer_.reset(new NetworkThrottlingObserver(local_state_)); On 2016/10/29 16:04:04, Lei Zhang wrote: > nit: foo.reset(new Foo(bar)) -> foo = base::MakeUnique<Foo>(bar) Done.
Message was sent while issue was closed.
Message was sent while issue was closed.
Description was changed from ========== Add support for an enterprise policy that lets the admin turn on network throttling, and specify the upload and download rates in kbps to throttle to. BUG=642811 TEST=(1) Unit-tests (2) Added network throttling policy to YAPS server, enabled throttling with various values, disabled, and checked that changes propagate to client device. Signed-off-by: Kirtika Ruchandani <kirtika@google.com> Committed: https://crrev.com/7db3c2b899d448dc0235e03a248df83aa5778ebf Cr-Commit-Position: refs/heads/master@{#428565} ========== to ========== Add support for an enterprise policy that lets the admin turn on network throttling, and specify the upload and download rates in kbps to throttle to. BUG=642811 TEST=(1) Unit-tests (2) Added network throttling policy to YAPS server, enabled throttling with various values, disabled, and checked that changes propagate to client device. Signed-off-by: Kirtika Ruchandani <kirtika@google.com> Committed: https://crrev.com/7db3c2b899d448dc0235e03a248df83aa5778ebf Cr-Commit-Position: refs/heads/master@{#428565} ==========
The CQ bit was checked by kirtika@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from stevenjb@chromium.org, kirtika@google.com, gab@chromium.org, atwilson@chromium.org, brettw@chromium.org, isherman@chromium.org Link to the patchset: https://codereview.chromium.org/2364703002/#ps300001 (title: "Add network bandwidth throttling as an enterprise policy")
The CQ bit was checked by kirtika@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by kirtika@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/10/29 16:04:05, Lei Zhang wrote: > There is a persistent red bot on the waterfall that lead me here. I commented on > some of the problems in network_throttling_observer_unittest.cc and left a few > style nits. Hope you can fix the problem before sheriffs decide to revert this > CL. Lei, thanks a lot for alerting about this.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
This CL has already landed. You should start a new CL instead of continuing on this one.
On 2016/10/30 04:01:50, Lei Zhang wrote: > This CL has already landed. You should start a new CL instead of continuing on > this one. Added https://codereview.chromium.org/2465743003/, closing this one.
Message was sent while issue was closed.
On 2016/10/31 04:39:10, kirtika1 wrote: > On 2016/10/30 04:01:50, Lei Zhang wrote: > > This CL has already landed. You should start a new CL instead of continuing on > > this one. > > Added https://codereview.chromium.org/2465743003/, closing this one. I was about to revert this one, but looks like you have a fix going on so I'll give that a change to land.
Message was sent while issue was closed.
On 2016/10/31 09:23:41, phoglund_chrome wrote: > On 2016/10/31 04:39:10, kirtika1 wrote: > > On 2016/10/30 04:01:50, Lei Zhang wrote: > > > This CL has already landed. You should start a new CL instead of continuing > on > > > this one. > > > > Added https://codereview.chromium.org/2465743003/, closing this one. > > I was about to revert this one, but looks like you have a fix going on so I'll > give that a change to land. Also, next time, please revert+fix rather than fixing forward. Fixing forward was OK in the past, but now you're causing so many red builds and possible lost productivity for others that it's never worth it.
Message was sent while issue was closed.
On 2016/10/31 09:26:00, phoglund_chrome wrote: > On 2016/10/31 09:23:41, phoglund_chrome wrote: > > On 2016/10/31 04:39:10, kirtika1 wrote: > > > On 2016/10/30 04:01:50, Lei Zhang wrote: > > > > This CL has already landed. You should start a new CL instead of > continuing > > on > > > > this one. > > > > > > Added https://codereview.chromium.org/2465743003/, closing this one. > > > > I was about to revert this one, but looks like you have a fix going on so I'll > > give that a change to land. > > Also, next time, please revert+fix rather than fixing forward. Fixing forward > was OK in the past, but now you're causing so many red builds and possible lost > productivity for others that it's never worth it. ... And it turns out your fix didn't fix the problem. That's annoying. Did you test locally? I'm reverting both your patches now.
Message was sent while issue was closed.
A revert of this CL (patchset #17 id:320001) has been created in https://codereview.chromium.org/2463023002/ by phoglund@chromium.org. The reason for reverting is: Introduces memory leak; see discussion in https://bugs.chromium.org/p/chromium/issues/detail?id=660769..
Message was sent while issue was closed.
A revert of this CL (patchset #17 id:320001) has been created in https://codereview.chromium.org/2461383002/ by tommycli@chromium.org. The reason for reverting is: Reverting because this broke CrOS ASan bots: First breaking build: https://build.chromium.org/p/chromium.memory/builders/Linux%20Chromium%20OS%2... Thanks!.
Message was sent while issue was closed.
On 2016/10/31 15:20:05, tommycli wrote: > A revert of this CL (patchset #17 id:320001) has been created in > https://codereview.chromium.org/2461383002/ by mailto:tommycli@chromium.org. > > The reason for reverting is: Reverting because this broke CrOS ASan bots: > > First breaking build: > > https://build.chromium.org/p/chromium.memory/builders/Linux%20Chromium%20OS%2... > > Thanks!. Re-landed here: https://codereview.chromium.org/2466693002 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
