Expose changes in the network quality to the renderers

content/browser/net/network_quality_observer_impl.cc

+// Copyright (c) 2017 The Chromium Authors. All rights reserved.

[nasko, 2017/05/08 18:27:45]

No "(c)".

[tbansal1, 2017/05/08 19:50:18]

Done.

+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "content/browser/net/network_quality_observer_impl.h"
+
+#include "base/memory/ptr_util.h"
+#include "content/browser/renderer_host/render_process_host_impl.h"
+#include "content/common/view_messages.h"
+#include "content/public/browser/browser_thread.h"
+#include "content/public/browser/notification_observer.h"
+#include "content/public/browser/notification_registrar.h"
+#include "content/public/browser/notification_service.h"
+#include "content/public/browser/notification_types.h"
+#include "content/public/browser/render_process_host.h"
+
+namespace {
+
+// Returns true if the |current_value| is meaningfully different from the
+// |past_value|.
+bool MetricChangedMeaningfully(int32_t past_value, int32_t current_value) {
+  if ((past_value == net::nqe::internal::INVALID_RTT_THROUGHPUT) !=
+      (current_value == net::nqe::internal::INVALID_RTT_THROUGHPUT)) {
+    return true;
+  }
+
+  if (past_value == net::nqe::internal::INVALID_RTT_THROUGHPUT &&
+      current_value == net::nqe::internal::INVALID_RTT_THROUGHPUT) {
+    return false;
+  }
+
+  // Metric has changed meaningfully only if (i) the difference between the two
+  // values exceed the threshold; and, (ii) the ratio of the values also exceeds
+  // the threshold.
+  static const int kMinDifferenceInMetrics = 100;
+  static const float kMinRatio = 1.2f;
+
+  if (std::abs(past_value - current_value) < kMinDifferenceInMetrics) {
+    // The absolute change in the value is not sufficient enough.
+    return false;
+  }
+
+  if (past_value < (kMinRatio * current_value) &&
+      current_value < (kMinRatio * past_value)) {
+    // The relative change in the value is not sufficient enough.
+    return false;
+  }
+
+  return true;
+}
+
+}  // namespace
+
+namespace content {
+
+// UIObserver observes the changes to the network quality on the UI thread, and
+// notifies the renderers of the change in the network quality.
+class NetworkQualityObserverImpl::UIObserver
+    : public content::NotificationObserver {
+ public:
+  UIObserver() {}
+
+  ~UIObserver() override {}
+
+  void InitOnUI() {

[nasko, 2017/05/08 18:27:45]

nit: InitOnUIThread.

[tbansal1, 2017/05/08 19:50:19]

Done.

+    DCHECK_CURRENTLY_ON(BrowserThread::UI);
+    registrar_.Add(this, NOTIFICATION_RENDERER_PROCESS_CREATED,
+                   NotificationService::AllSources());
+  }
+
+  void OnRTTOrThroughputEstimatesComputed(
+      const net::nqe::internal::NetworkQuality& network_quality) {
+    DCHECK_CURRENTLY_ON(BrowserThread::UI);
+
+    last_notified_network_quality_ = network_quality;
+
+    // Notify all the existing renderers of the change in the network quality.
+    for (RenderProcessHost::iterator it(RenderProcessHost::AllHostsIterator());
+         !it.IsAtEnd(); it.Advance()) {
+      it.GetCurrentValue()->GetRendererInterface()->OnNetworkQualityChanged(
+          last_notified_network_quality_.http_rtt().InMilliseconds(),
+          last_notified_network_quality_.transport_rtt().InMilliseconds(),
+          last_notified_network_quality_.downstream_throughput_kbps());
+    }
+  }
+
+ private:
+  // NotificationObserver implementation:
+  void Observe(int type,
+               const NotificationSource& source,
+               const NotificationDetails& details) override {
+    DCHECK_CURRENTLY_ON(BrowserThread::UI);
+    DCHECK_EQ(NOTIFICATION_RENDERER_PROCESS_CREATED, type);
+
+    RenderProcessHost* rph = Source<RenderProcessHost>(source).ptr();
+
+    // Notify the newly created renderer of the current network quality.
+    rph->GetRendererInterface()->OnNetworkQualityChanged(
+        last_notified_network_quality_.http_rtt().InMilliseconds(),
+        last_notified_network_quality_.transport_rtt().InMilliseconds(),
+        last_notified_network_quality_.downstream_throughput_kbps());
+  }
+
+  content::NotificationRegistrar registrar_;
+
+  net::nqe::internal::NetworkQuality last_notified_network_quality_;

[nasko, 2017/05/08 18:27:44]

nit: No need for empty lines between members, if there are no comments.

[tbansal1, 2017/05/08 19:50:19]

Done.

+
+  DISALLOW_COPY_AND_ASSIGN(UIObserver);
+};
+
+NetworkQualityObserverImpl::NetworkQualityObserverImpl(
+    net::NetworkQualityEstimator* network_quality_estimator)
+    : network_quality_estimator_(network_quality_estimator) {
+  network_quality_estimator_->AddRTTAndThroughputEstimatesObserver(this);
+
+  ui_oberver_ = base::WrapUnique(new UIObserver());

[RyanSturm, 2017/05/08 17:37:20]

base::MakeUnique

[tbansal1, 2017/05/08 19:50:18]

Done.

+  BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
+                          base::BindOnce(&UIObserver::InitOnUI,
+                                         base::Unretained(ui_oberver_.get())));
+}
+
+NetworkQualityObserverImpl::~NetworkQualityObserverImpl() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  network_quality_estimator_->RemoveRTTAndThroughputEstimatesObserver(this);
+
+  if (!ui_oberver_)
+    return;
+
+  // |ui_oberver_| must be deleted on UI thread.
+  bool deleted = BrowserThread::DeleteSoon(BrowserThread::UI, FROM_HERE,
+                                           ui_oberver_.release());

[RyanSturm, 2017/05/08 17:37:20]

note: Seems odd in general that there is a pattern of std::unique_ptr getting
deleted by DeleteSoon after a release. No matter what the unique_ptr doesn't
call delete, so we might as well have a raw pointer... It's fine the way it is
as well though I guess.

[tbansal1, 2017/05/08 19:50:18]

Yes, it is okay to use a raw pointer as well. Using a unique ptr also seems like
a pattern in content/.

+  DCHECK(deleted);
+  // Silence unused variable warning in release builds.

[RyanSturm, 2017/05/08 17:37:20]

Not sure you really need this DCHECK. I don't know the best way to handle this,
but a lot of places in content/ (maybe all places) trust the DeleteSoon.

[tbansal1, 2017/05/08 19:50:19]

Done.

+  (void)deleted;
+}
+
+void NetworkQualityObserverImpl::OnRTTOrThroughputEstimatesComputed(
+    base::TimeDelta http_rtt,
+    base::TimeDelta transport_rtt,
+    int32_t downstream_throughput_kbps) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+
+  // Check if any of the network quality metrics changed meaningfully.
+  bool http_rtt_changed = MetricChangedMeaningfully(
+      last_notified_network_quality_.http_rtt().InMilliseconds(),
+      http_rtt.InMilliseconds());
+
+  bool transport_rtt_changed = MetricChangedMeaningfully(
+      last_notified_network_quality_.transport_rtt().InMilliseconds(),
+      transport_rtt.InMilliseconds());
+  bool kbps_changed = MetricChangedMeaningfully(
+      last_notified_network_quality_.downstream_throughput_kbps(),
+      downstream_throughput_kbps);
+
+  if (!http_rtt_changed && !transport_rtt_changed && !kbps_changed) {
+    // Return since none of the metrics changed meaningfully. This reduces
+    // the number of notifications to the different renderers every time
+    // the network quality is recomputed.
+    return;
+  }
+
+  last_notified_network_quality_ = net::nqe::internal::NetworkQuality(
+      http_rtt, transport_rtt, downstream_throughput_kbps);
+
+  BrowserThread::PostTask(
+      BrowserThread::UI, FROM_HERE,
+      base::BindOnce(&UIObserver::OnRTTOrThroughputEstimatesComputed,
+                     base::Unretained(ui_oberver_.get()),

[nasko, 2017/05/08 18:27:45]

Why is this Unretained? ui_observer_ is owned by this object and if this object
gets deleted before the task is executed on the UI thread, it could very well be
deleted by then. You are using DeleteSoon as a workaround to that, which seems
very hacky.

All of the work in the UIObserver is done by OnRTTOrThroughputEstimatesComputed,
which can be made a static method and posted to without lifetime concerns. If
the requirement is to have all new processes also notified of the current
condition, then it can also be stored in a static member? I guess overall that
object can just be a singleton for the browser process and doesn't need to be
owned by this object. Does that make sense?

[tbansal1, 2017/05/08 19:50:18]

|ui_observer_| is destroyed only after the destructor of
|this| is invoked. So, IIUC, the code
is safe (although it may be a bit hacky)?
Using a singleton sounds reasonable. But, I thought that in general
singletons are not preferred due to lifetime and testing issues?
May be content/ has different design patterns and tradeoffs. Please let me know
what works best here.

Also, to make the context a bit more clear:
I have to use UI thread because renderers can only be notified
on UI thread and the new renderer notifications are delivered only on UI thread.

I need to have a IO thread object because network quality is available only on
IO thread.

[nasko, 2017/05/09 20:27:47]

It depends. I think what you have here is alternative, but please make sure it
is documented well, as it isn't as common of a pattern.
Yes, that part is clear.

[tbansal1, 2017/05/09 21:37:18]

Done.

+                     last_notified_network_quality_));
+}
+
+std::unique_ptr<net::NetworkQualityEstimator::RTTAndThroughputEstimatesObserver>
+CreateNetworkQualityObserver(
+    net::NetworkQualityEstimator* network_quality_estimator) {
+  return base::MakeUnique<NetworkQualityObserverImpl>(
+      network_quality_estimator);
+}
+
+}  // namespace content