Add network throttling as an enterprise policy

chrome/browser/chromeos/net/network_throttling_observer.cc

+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/chromeos/net/network_throttling_observer.h"
+
+#include <memory>
+#include <string>
+
+#include "base/macros.h"
+#include "base/memory/ptr_util.h"
+#include "base/sys_info.h"
+#include "base/values.h"
+#include "chrome/common/pref_names.h"
+#include "chromeos/network/network_state_handler.h"
+#include "components/prefs/pref_member.h"
+#include "components/prefs/pref_registry_simple.h"
+#include "components/prefs/pref_service.h"
+
+namespace chromeos {
+
+NetworkThrottlingObserver::NetworkThrottlingObserver(PrefService* local_state)
+    : local_state_(local_state), weak_ptr_factory_(this) {
+  pref_change_registrar_.Init(local_state_);
+
+  base::Callback<void(const std::string&)> throttle_callback =
+      base::Bind(&NetworkThrottlingObserver::OnPreferenceChanged,
+                 weak_ptr_factory_.GetWeakPtr());

[Andrew T Wilson (Slow), 2016/10/28 14:31:54]

Why is this weak ptr factory required? Can the callback actually ever be invoked
after you call PrefChangeRegistrar::RemoveAll()?

[stevenjb, 2016/10/28 16:27:09]

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).

+
+  pref_change_registrar_.Add(prefs::kNetworkThrottlingEnabled,
+                             throttle_callback);
+}
+
+NetworkThrottlingObserver::~NetworkThrottlingObserver() {
+  pref_change_registrar_.RemoveAll();
+}
+
+void NetworkThrottlingObserver::RegisterPrefs(PrefRegistrySimple* registry) {
+  registry->RegisterDictionaryPref(prefs::kNetworkThrottlingEnabled);
+}
+
+void NetworkThrottlingObserver::OnPreferenceChanged(
+    const std::string& pref_name) {
+  DCHECK(pref_name == prefs::kNetworkThrottlingEnabled);
+
+  const base::DictionaryValue* throttling_policy =
+      local_state_->GetDictionary(prefs::kNetworkThrottlingEnabled);
+
+  if (!throttling_policy)

[Andrew T Wilson (Slow), 2016/10/28 14:31:54]

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?

+    return;
+
+  bool enabled;
+  uint32_t upload_rate, download_rate;
+  throttling_policy->GetBoolean("enabled", &enabled);
+  throttling_policy->GetInteger("upload_rate_kbits",
+                                reinterpret_cast<int*>(&upload_rate));

[Andrew T Wilson (Slow), 2016/10/28 14:31:54]

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.

[stevenjb, 2016/10/28 16:27:09]

I missed this, +1.

+  throttling_policy->GetInteger("download_rate_kbits",
+                                reinterpret_cast<int*>(&download_rate));
+  NetworkHandler::Get()->network_state_handler()->SetNetworkThrottlingStatus(
+      enabled, upload_rate, download_rate);
+}
+
+}  // namespace chromeos