Added a debug info UI for Blimp

blimp/net/blimp_connection_details.cc

Index: blimp/net/blimp_connection_details.cc
diff --git a/blimp/net/blimp_connection_details.cc b/blimp/net/blimp_connection_details.cc
new file mode 100644
index 0000000000000000000000000000000000000000..2fb9e6739dc46f2a913cce78914305e84fcce2cd
--- /dev/null
+++ b/blimp/net/blimp_connection_details.cc
@@ -0,0 +1,74 @@
+// 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 "blimp/net/blimp_connection_details.h"
+
+#include "base/bind.h"
+#include "base/logging.h"
+#include "base/threading/thread.h"
+
+namespace blimp {
+
+BlimpConnectionDetails::BlimpConnectionDetails(

[Kevin M, 2016/05/18 18:38:28]

The name "Connection Details" isn't a good fit here IMO - "details" would
include things like the protocol, address, things like that.
ConnectionStatistics would be more descriptive.

[shaktisahu, 2016/05/19 21:39:19]

Done.

+    base::WeakPtr<NetworkActivityObserver> observer,
+    const scoped_refptr<base::TaskRunner>& main_thread_task_runner,
+    const scoped_refptr<base::TaskRunner>& io_thread_task_runner)
+    : debug_info_enabled_(false),
+      bytes_received_(0),
+      bytes_sent_(0),
+      commits_(0),
+      observer_(observer),
+      main_thread_task_runner_(main_thread_task_runner),
+      io_thread_task_runner_(io_thread_task_runner),
+      weak_factory_(this) {}
+
+BlimpConnectionDetails::~BlimpConnectionDetails() {}
+
+void BlimpConnectionDetails::EnableDebugInfo(bool enable) {
+  debug_info_enabled_ = enable;
+}
+
+void BlimpConnectionDetails::OnPacketReceived(int bytes) {
+  bytes_received_ += bytes;
+  if (debug_info_enabled_) {
+    main_thread_task_runner_->PostTask(
+        FROM_HERE,
+        base::Bind(&NetworkActivityObserver::UpdateDebugInfo, observer_,

[Kevin M, 2016/05/18 18:38:28]

I think that this push approach is really inefficient - for every *packet* that
we receive, we are posting tasks across threads and making calls over JNI.
Developers don't derive much benefit from this level of sampling granularity,
because the values are meant to be read from the screen - does it matter if it
updates 30 times per second, or can it just update every second or every other
second instead? In fact, having a rapidly changing number is probably
detrimental to the feature.

I recommend switching to a pull based model, where Java uses a timer to
periodically pull information from the details object.

[Khushal, 2016/05/18 20:41:10]

I'm not sure how that will run, Java will make a call from the UI thread and the
data is collected on the IO thread. So either java makes a synchronous call,
which is definitely not good, or java makes a call to get the numbers, and then
the io thread responds back with a call that goes to java? If we want to batch
updates we might as well have this class maintain a timer.

If you want to be even more conservative, track whether the view is showing at
the JNI layer and avoid sending updates to java if it isn't. This is only ever
going to be used for debugging, so I don't have a strong opinion on this. We can
put creating the stats collection behind a runtime switch later.

[shaktisahu, 2016/05/19 21:39:19]

Done.

+                   bytes_received_, bytes_sent_, commits_));
+  }
+}
+
+void BlimpConnectionDetails::OnPacketSent(int bytes) {

[Kevin M, 2016/05/18 18:38:28]

There is a lot of code duplication with the way this is set up, and it doesn't
scale well if we add new metrics (or generalize beyond connection stats).

I recommend storing the data in a string- or enum-keyed map.

Add(BlimpConnectionDetails::EventType::DATA_SENT, bytes);
Increment(BlimpConnectionDetails::EventType::COMMIT, 1);
Get(BlimpConnectionDetails::EventType::DataSent)

etc.

[shaktisahu, 2016/05/19 21:39:19]

Done.

+  bytes_sent_ += bytes;
+  if (debug_info_enabled_) {
+    main_thread_task_runner_->PostTask(
+        FROM_HERE,
+        base::Bind(&NetworkActivityObserver::UpdateDebugInfo, observer_,
+                   bytes_received_, bytes_sent_, commits_));
+  }
+}
+
+void BlimpConnectionDetails::OnCommit() {

[Khushal, 2016/05/18 20:41:10]

I think this call is made on the main thread, you don't need to do a PostTask.

[shaktisahu, 2016/05/19 21:39:19]

Done.

+  commits_++;
+  if (debug_info_enabled_) {
+    main_thread_task_runner_->PostTask(
+        FROM_HERE,
+        base::Bind(&NetworkActivityObserver::UpdateDebugInfo, observer_,
+                   bytes_received_, bytes_sent_, commits_));
+  }
+}
+
+void BlimpConnectionDetails::ResetStats() {

[Kevin M, 2016/05/18 18:38:28]

I don't think we should add resetting at this level.

The details object is stored per-connection, which means it's shared across
multiple tabs. That means we'll have navigation events that are contending for
resetting the state. Similarly, control signals and data for other tabs will add
noise to the computed totals - probably not a huge amount, if we are controlling
the benchmarking environment, but just FYI.

[Khushal, 2016/05/18 20:41:10]

We'll either way not have per navigation data even within a single tab. Multiple
compositor streams, in the case of a cross-site navigation, run at the same
time. Only the active compositor acks the commits, but there can always be the
last one in flight.

I think the aim here is to let the developer observe the data consumption rate
for a page and while resetting based on navigation might not be the best
approach, but having an option in the UI to reset sounds like a good thing to
offer. Whether that is done in Java, you can cache the value since last reset
and show the difference with the current value, or here, is up to you.

[shaktisahu, 2016/05/19 21:39:19]

Done.

+  io_thread_task_runner_->PostTask(
+      FROM_HERE, base::Bind(&BlimpConnectionDetails::ResetStatsOnIOThread,
+                            weak_factory_.GetWeakPtr()));
+  commits_ = 0;
+}
+
+void BlimpConnectionDetails::ResetStatsOnIOThread() {
+  bytes_received_ = 0;
+  bytes_sent_ = 0;
+}
+
+}  // namespace blimp