Exposing NQE on the Browser UI thread

chrome/browser/net/nqe/ui_network_quality_estimator_service.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/net/nqe/ui_network_quality_estimator_service.h"
+
+#include "base/bind.h"
+#include "base/message_loop/message_loop.h"
+#include "base/task_runner.h"
+#include "chrome/browser/browser_process.h"
+#include "chrome/browser/io_thread.h"
+
+namespace chrome_browser_net {
+
+class UINetworkQualityEstimatorService::IONetworkQualityObserver

[tbansal1, 2016/07/13 00:07:27]

Add class comments. Something along the lines of: this class listens to changes
in effective connection type. It is created on UI thread, but moved to IO
thread.

[RyanSturm, 2016/07/13 17:28:14]

Done.

+    : public net::NetworkQualityEstimator::EffectiveConnectionTypeObserver {
+ public:
+  IONetworkQualityObserver(
+      scoped_refptr<base::SingleThreadTaskRunner> ui_task_runner,
+      base::WeakPtr<UINetworkQualityEstimatorService> service)
+      : ui_task_runner_(ui_task_runner), service_(service) {
+        // This is created on the UI thread, but used only on the UI thread.
+        thread_checker_.DetachFromThread();

[tbansal1, 2016/07/13 00:07:28]

DCHECK(ui_task_runner_);

[RyanSturm, 2016/07/13 17:28:14]

Done.

+      }
+
+  ~IONetworkQualityObserver() override {
+    DCHECK(thread_checker_.CalledOnValidThread());
+    if (network_quality_estimator_)
+      network_quality_estimator_->RemoveEffectiveConnectionTypeObserver(this);
+  }
+
+  void InitializeOnIOThread(IOThread* io_thread) {
+    DCHECK(thread_checker_.CalledOnValidThread());
+    if (!io_thread->globals()->network_quality_estimator)
+      return;
+    network_quality_estimator_ =
+        io_thread->globals()->network_quality_estimator->GetWeakPtr();

[tbansal1, 2016/07/13 00:07:28]

Just use the raw pointer.

[RyanSturm, 2016/07/13 17:28:14]

I'm not sure that the
network_quality_estimator_->RemoveEffectiveConnectionTypeObserver above will be
safe without a WeakPtr. If the IO process globals are deleted before the UI
process KeyedServices, couldn't we get a use after free?

[tbansal1, 2016/07/13 17:56:11]

The IOThread::globals are destroyed in IOThread::Cleanup() which is called
after the message loop ends.
https://cs.chromium.org/chromium/src/base/threading/thread.h?rcl=1468408847&l...

[tbansal1, 2016/07/13 18:08:13]

Because of the above, you can remove the WeakPtr, and use the raw pointer from
here and
from NQE class.
Generally, it is considered an anti-pattern to expose WeakPtrs broadly.

+    network_quality_estimator_->AddEffectiveConnectionTypeObserver(this);

[tbansal1, 2016/07/13 00:07:28]

Check if network_quality_estimator_ is non-null before you call this.

[RyanSturm, 2016/07/13 17:28:14]

Done.

+    ui_task_runner_->PostTask(
+        FROM_HERE,
+        base::Bind(
+            &UINetworkQualityEstimatorService::EffectiveConnectionTypeChanged,
+            service_,
+            network_quality_estimator_->GetEffectiveConnectionType()));
+  }
+
+ protected:
+  void OnEffectiveConnectionTypeChanged(

[tbansal1, 2016/07/13 00:07:28]

Why protected?

Also, add comment:
// net::NetworkQualityEstimator::EffectiveConnectionTypeObserver implementation:

[RyanSturm, 2016/07/13 17:28:14]

Done.

+      net::NetworkQualityEstimator::EffectiveConnectionType type) override {
+    DCHECK(thread_checker_.CalledOnValidThread());
+    ui_task_runner_->PostTask(
+        FROM_HERE,
+        base::Bind(
+            &UINetworkQualityEstimatorService::EffectiveConnectionTypeChanged,
+            service_, type));
+  }
+
+ private:
+  scoped_refptr<base::SingleThreadTaskRunner> ui_task_runner_;
+  base::WeakPtr<UINetworkQualityEstimatorService> service_;
+  base::WeakPtr<net::NetworkQualityEstimator> network_quality_estimator_;
+  base::ThreadChecker thread_checker_;
+
+  DISALLOW_COPY_AND_ASSIGN(IONetworkQualityObserver);
+};
+
+UINetworkQualityEstimatorService::UINetworkQualityEstimatorService(
+    Profile* profile)
+    : type_(net::NetworkQualityEstimator::EFFECTIVE_CONNECTION_TYPE_UNKNOWN),
+      io_observer_(nullptr),
+      weak_factory_(this) {}
+
+UINetworkQualityEstimatorService::~UINetworkQualityEstimatorService() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK(io_task_runner_->DeleteSoon(FROM_HERE, io_observer_));
+}
+
+void UINetworkQualityEstimatorService::Initialize(
+    scoped_refptr<base::SingleThreadTaskRunner> ui_task_runner,
+    scoped_refptr<base::SingleThreadTaskRunner> io_task_runner) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  io_task_runner_ = io_task_runner;
+  io_observer_ =
+      new IONetworkQualityObserver(ui_task_runner, weak_factory_.GetWeakPtr());
+  io_task_runner_->PostTask(
+      FROM_HERE, base::Bind(&IONetworkQualityObserver::InitializeOnIOThread,
+                            base::Unretained(io_observer_),
+                            g_browser_process->io_thread()));
+}
+
+void UINetworkQualityEstimatorService::Shutdown() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  weak_factory_.InvalidateWeakPtrs();
+}
+
+void UINetworkQualityEstimatorService::EffectiveConnectionTypeChanged(
+    net::NetworkQualityEstimator::EffectiveConnectionType type) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  type_ = type;
+}
+
+}  // namespace chrome_browser_net