Added a debug info UI for Blimp

Added a debug info UI for Blimp

Added a menu option in toolbar to show/hide debug info. When enabled,
an overlay debug UI would be shown on the bottom left of the screen
showing number of commits, and kilobytes sent and received by the client.
This data is reset when the user moves away or is redirected to a new
page.

BUG=594423
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/03e5b0d97aa1fc6e813725465142978896ed8641
Cr-Commit-Position: refs/heads/master@{#395931}

[shaktisahu, 2016-05-10 23:15:31]

Description was changed from

==========
Rough draft for displaying debug info

BUG=
==========

to

==========
Rough draft for displaying debug info

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[shaktisahu, 2016-05-10 23:29:32]

On 2016/05/10 23:15:31, shaktisahu wrote:
> Description was changed from
> 
> ==========
> Rough draft for displaying debug info
> 
> BUG=
> ==========
> 
> to
> 
> ==========
> Rough draft for displaying debug info
> 
> BUG=
> CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
> ==========

This CL is just for reviewing if it is in right direction and will be later
deleted.

The aim is to calculate the bytes sent and received by the client (client <--->
engine communication)

There is also an ask to calculate the data saving for which I have to calculate
engine <---> target website data (for the next CL probably)

The data makes sense per navigation so I am resetting it to zero in
navigation_feature.h

Also I might have to calculate the commits (cc changes and blimp_compositor
changes are for that)--- eventually I have to hook that all the way upto
BlimpClientSession or BlimpConnectionDetails (?)

Please provide comments on the design.

[Kevin M, 2016-05-10 23:54:23]

kmarshall@chromium.org changed reviewers:
+ kmarshall@chromium.org

[Kevin M, 2016-05-10 23:54:24]

https://codereview.chromium.org/1962393004/diff/1/blimp/client/feature/compos...
File blimp/client/feature/compositor/blimp_compositor.cc (right):

https://codereview.chromium.org/1962393004/diff/1/blimp/client/feature/compos...
blimp/client/feature/compositor/blimp_compositor.cc:110: VLOG(0) << "commit
completed, current count " << ++commit_count_;
nit: Use proper capitalization and a trailing period.
nit: This seems too fine-grained for VLOG 0.

Do we still need this log statement?

https://codereview.chromium.org/1962393004/diff/1/blimp/client/feature/compos...
File blimp/client/feature/compositor/blimp_compositor.h (right):

https://codereview.chromium.org/1962393004/diff/1/blimp/client/feature/compos...
blimp/client/feature/compositor/blimp_compositor.h:208: int commit_count_ = 0;
Add a comment with a similar level of detail as the other fields in this class?

https://codereview.chromium.org/1962393004/diff/1/blimp/client/feature/naviga...
File blimp/client/feature/navigation_feature.cc (right):

https://codereview.chromium.org/1962393004/diff/1/blimp/client/feature/naviga...
blimp/client/feature/navigation_feature.cc:68:
connection_details_->ResetStats();
Consider moving the data collection policy out of here and in to the NetLog
observer object.

https://codereview.chromium.org/1962393004/diff/1/blimp/client/session/blimp_...
File blimp/client/session/blimp_client_session.cc (right):

https://codereview.chromium.org/1962393004/diff/1/blimp/client/session/blimp_...
blimp/client/session/blimp_client_session.cc:77: BlimpConnectionDetails*
blimp_connection_details);
This is racy. |blimp_connection_details| can be destroyed on the UI thread while
it's still being used on the IO thread. You'll need to code a solution that
ensures the object is always accessed and destroyed on the same thread. Weak
pointers w/PostTask or threadsafe refcounted pointers might work here.

https://codereview.chromium.org/1962393004/diff/1/blimp/client/session/blimp_...
blimp/client/session/blimp_client_session.cc:263: void
BlimpClientSession::OnPacketReceived(int bytes) {}
nit: the name says OnPacketReceived, but the parameter is bytes, not packet
count? Should it be OnDataReceived?

nit: shouldn't we encapsulate these telemetry events in a class instead of
adding to the public interface?

major issue: Pushing statistics to Java for every packet sent or received is
really wasteful, since it'll introduce a whole lot of JNI overhead. We should
have Java pull the data instead, e.g. only when the info UI is pulled up.

https://codereview.chromium.org/1962393004/diff/1/blimp/client/session/blimp_...
File blimp/client/session/blimp_client_session.h (right):

https://codereview.chromium.org/1962393004/diff/1/blimp/client/session/blimp_...
blimp/client/session/blimp_client_session.h:58: NetworkActivityObserver {
public NetworkActivityObserver

https://codereview.chromium.org/1962393004/diff/1/blimp/net/blimp_connection_...
File blimp/net/blimp_connection_details.h (right):

https://codereview.chromium.org/1962393004/diff/1/blimp/net/blimp_connection_...
blimp/net/blimp_connection_details.h:13: class NetworkActivityObserver {
Push a NetLog object down the layers to collect byte transfer events (via
AddByteTransferEvent) and have a NetLog::ThreadSafeObserver subclass add the
events to a cumulative total.

[shaktisahu, 2016-05-11 22:08:00]

https://codereview.chromium.org/1962393004/diff/1/blimp/net/blimp_connection_...
File blimp/net/blimp_connection_details.h (right):

https://codereview.chromium.org/1962393004/diff/1/blimp/net/blimp_connection_...
blimp/net/blimp_connection_details.h:13: class NetworkActivityObserver {
On 2016/05/10 23:54:24, Kevin M wrote:
> Push a NetLog object down the layers to collect byte transfer events (via
> AddByteTransferEvent) and have a NetLog::ThreadSafeObserver subclass add the
> events to a cumulative total.

Apparently the NetLog::ThreadSafeObserver is deprecated and NetLog is used just
for logging purposes and not making any application state related decisions. I
checked with the owner too. Any other recommendations? There is
NetworkActivityMonitor which is very simple to use but it works on lower layer
and I can't reset the byte counter to zero when I navigate away to another page.

[shaktisahu, 2016-05-16 05:24:57]

Description was changed from

==========
Rough draft for displaying debug info

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Added a debug info UI for Blimp

Added an menu option to show/hide debug info in Blimp toolbar. When enabled, an
overlay debug UI would be shown on the bottom left of the screen showing number
of commits, and kilobytes sent and received by the client. This data is reset
when the user moves away or is redirected to a new page.


BUG=
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[shaktisahu, 2016-05-16 05:27:20]

Description was changed from

==========
Added a debug info UI for Blimp

Added an menu option to show/hide debug info in Blimp toolbar. When enabled, an
overlay debug UI would be shown on the bottom left of the screen showing number
of commits, and kilobytes sent and received by the client. This data is reset
when the user moves away or is redirected to a new page.


BUG=
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Added a debug info UI for Blimp

Added a menu option to show/hide debug info in Blimp toolbar. When enabled, an
overlay debug UI would be shown on the bottom left of the screen showing number
of commits, and kilobytes sent and received by the client. This data is reset
when the user moves away or is redirected to a new page.


BUG=
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[shaktisahu, 2016-05-16 05:30:12]

Description was changed from

==========
Added a debug info UI for Blimp

Added a menu option to show/hide debug info in Blimp toolbar. When enabled, an
overlay debug UI would be shown on the bottom left of the screen showing number
of commits, and kilobytes sent and received by the client. This data is reset
when the user moves away or is redirected to a new page.


BUG=
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Added a debug info UI for Blimp

Added a menu option in toolbar to show/hide debug info. When enabled,
an overlay debug UI would be shown on the bottom left of the screen
showing number of commits, and kilobytes sent and received by the client.
This data is reset when the user moves away or is redirected to a new
page.


BUG=594423
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[shaktisahu, 2016-05-16 05:41:37]

shaktisahu@chromium.org changed reviewers:
+ khushalsagar@chromium.org

[shaktisahu, 2016-05-16 05:41:39]

https://codereview.chromium.org/1962393004/diff/1/blimp/client/feature/compos...
File blimp/client/feature/compositor/blimp_compositor.cc (right):

https://codereview.chromium.org/1962393004/diff/1/blimp/client/feature/compos...
blimp/client/feature/compositor/blimp_compositor.cc:110: VLOG(0) << "commit
completed, current count " << ++commit_count_;
On 2016/05/10 23:54:24, Kevin M wrote:
> nit: Use proper capitalization and a trailing period.
> nit: This seems too fine-grained for VLOG 0.
> 
> Do we still need this log statement?

Done.

https://codereview.chromium.org/1962393004/diff/1/blimp/client/feature/compos...
File blimp/client/feature/compositor/blimp_compositor.h (right):

https://codereview.chromium.org/1962393004/diff/1/blimp/client/feature/compos...
blimp/client/feature/compositor/blimp_compositor.h:208: int commit_count_ = 0;
On 2016/05/10 23:54:24, Kevin M wrote:
> Add a comment with a similar level of detail as the other fields in this
class?

Field removed.

https://codereview.chromium.org/1962393004/diff/1/blimp/client/feature/naviga...
File blimp/client/feature/navigation_feature.cc (right):

https://codereview.chromium.org/1962393004/diff/1/blimp/client/feature/naviga...
blimp/client/feature/navigation_feature.cc:68:
connection_details_->ResetStats();
On 2016/05/10 23:54:24, Kevin M wrote:
> Consider moving the data collection policy out of here and in to the NetLog
> observer object.

Keeping this here as I can't use NetLog

https://codereview.chromium.org/1962393004/diff/1/blimp/client/session/blimp_...
File blimp/client/session/blimp_client_session.cc (right):

https://codereview.chromium.org/1962393004/diff/1/blimp/client/session/blimp_...
blimp/client/session/blimp_client_session.cc:77: BlimpConnectionDetails*
blimp_connection_details);
On 2016/05/10 23:54:24, Kevin M wrote:
> This is racy. |blimp_connection_details| can be destroyed on the UI thread
while
> it's still being used on the IO thread. You'll need to code a solution that
> ensures the object is always accessed and destroyed on the same thread. Weak
> pointers w/PostTask or threadsafe refcounted pointers might work here.

Okay. I have redesigned now. |blimp_connection_details| will be created on UI
thread and destroyed on IO thread. The methods will be called from UI thread
(commit count) and IO thread (bytes received/sent) which will be passed to UI
thread through PostTask. Let me know if this is okay.

https://codereview.chromium.org/1962393004/diff/1/blimp/client/session/blimp_...
blimp/client/session/blimp_client_session.cc:263: void
BlimpClientSession::OnPacketReceived(int bytes) {}
On 2016/05/10 23:54:24, Kevin M wrote:
> nit: the name says OnPacketReceived, but the parameter is bytes, not packet
> count? Should it be OnDataReceived?
> 
> nit: shouldn't we encapsulate these telemetry events in a class instead of
> adding to the public interface?
> 
> major issue: Pushing statistics to Java for every packet sent or received is
> really wasteful, since it'll introduce a whole lot of JNI overhead. We should
> have Java pull the data instead, e.g. only when the info UI is pulled up.

Method dropped. Now I have only one method called UpdateDebugInfo.
Also I am sending the data only when debug is enabled from the UI.

[shaktisahu, 2016-05-17 23:14:13]

On 2016/05/17 23:13:56, shaktisahu wrote:

PTAL

[Khushal, 2016-05-18 00:23:01]

https://codereview.chromium.org/1962393004/diff/20001/blimp/client/app/androi...
File blimp/client/app/android/blimp_client_session_android.cc (right):

https://codereview.chromium.org/1962393004/diff/20001/blimp/client/app/androi...
blimp/client/app/android/blimp_client_session_android.cc:124: void
BlimpClientSessionAndroid::EnableDebugInfo(
Hmmm, I don't think we really need to explicitly disable stats collection if the
user hides the debug view. The data can still be recorded and will be visible
when the user shows the view. You can just avoid piping the updates to java if
the view is not showing.

If you want to give the user an explicit option to reset the stats, then you can
add that in the menu.

https://codereview.chromium.org/1962393004/diff/20001/blimp/client/app/androi...
File blimp/client/app/android/blimp_view.cc (right):

https://codereview.chromium.org/1962393004/diff/20001/blimp/client/app/androi...
blimp/client/app/android/blimp_view.cc:59:
blimp_connection_details_(blimp_connection_details) {
DCHECK |blimp_connection_details_| here.

https://codereview.chromium.org/1962393004/diff/20001/blimp/client/app/androi...
File blimp/client/app/android/java/src/org/chromium/blimp/toolbar/Toolbar.java
(right):

https://codereview.chromium.org/1962393004/diff/20001/blimp/client/app/androi...
blimp/client/app/android/java/src/org/chromium/blimp/toolbar/Toolbar.java:58:
private boolean mDebugInfoEnabled = false;
A comment here which says that this denotes whether the DebugInfo view is
visible or not would be nice.

https://codereview.chromium.org/1962393004/diff/20001/blimp/client/app/androi...
blimp/client/app/android/java/src/org/chromium/blimp/toolbar/Toolbar.java:60:
private ArrayAdapter<String> mAdapter;
Rename this mPopupMenuOptionsAdapter so its clear what this stores?

https://codereview.chromium.org/1962393004/diff/20001/blimp/client/app/androi...
blimp/client/app/android/java/src/org/chromium/blimp/toolbar/Toolbar.java:264:
BlimpRendererActivity activity = (BlimpRendererActivity) mContext;
Just a note from our discussion earlier, please don't downcast the context. Use
a delegate interface instead. You wouldn't need to grab a reference to the
BlimpClientSession as well then.

https://codereview.chromium.org/1962393004/diff/20001/blimp/client/feature/na...
File blimp/client/feature/navigation_feature.cc (right):

https://codereview.chromium.org/1962393004/diff/20001/blimp/client/feature/na...
blimp/client/feature/navigation_feature.cc:23: : connection_details_(details) {}
DCHECK the |connection_details_| here.

https://codereview.chromium.org/1962393004/diff/20001/blimp/client/feature/na...
blimp/client/feature/navigation_feature.cc:123: delegate->OnUrlChanged(tab_id,
GURL(details.url()));
The URL may change even if we are on the same page, and may not necessarily
indicate a new navigation. I think your aim here is to reset stats when a new
navigation is initiated?

You should be able to use details.page_load_completed() being false below to
know when a navigation starts. I have a change up to make this event more
accurate, https://codereview.chromium.org/1986883002/

https://codereview.chromium.org/1962393004/diff/20001/blimp/client/feature/na...
File blimp/client/feature/navigation_feature.h (right):

https://codereview.chromium.org/1962393004/diff/20001/blimp/client/feature/na...
blimp/client/feature/navigation_feature.h:68: BlimpConnectionDetails*
connection_details_ = nullptr;
Do you need to initialize it to nullptr here? The class always expects to
receive this in the ctor.

https://codereview.chromium.org/1962393004/diff/20001/blimp/client/feature/na...
File blimp/client/feature/navigation_feature_unittest.cc (right):

https://codereview.chromium.org/1962393004/diff/20001/blimp/client/feature/na...
blimp/client/feature/navigation_feature_unittest.cc:77: NavigationFeatureTest()
Would be nice to have a test to make sure the expected calls to reset stats are
made.

https://codereview.chromium.org/1962393004/diff/20001/blimp/client/session/bl...
File blimp/client/session/blimp_client_session.cc (right):

https://codereview.chromium.org/1962393004/diff/20001/blimp/client/session/bl...
blimp/client/session/blimp_client_session.cc:109: connection_details_(details)
{}
DCHECK |connection_details_| here.

https://codereview.chromium.org/1962393004/diff/20001/blimp/client/session/bl...
blimp/client/session/blimp_client_session.cc:176: blimp_connection_details_ =
base::WrapUnique(new BlimpConnectionDetails(
nit: Use x_.reset(new ...) for member variables of the class for consistency?

https://codereview.chromium.org/1962393004/diff/20001/blimp/engine/session/bl...
File blimp/engine/session/blimp_engine_session.cc (right):

https://codereview.chromium.org/1962393004/diff/20001/blimp/engine/session/bl...
blimp/engine/session/blimp_engine_session.cc:192:
connection->SetBlimpConnectionDetails(nullptr);
Don't really need to do this here. They already have a null
BlimpConnectionDetails.

https://codereview.chromium.org/1962393004/diff/20001/blimp/net/blimp_connect...
File blimp/net/blimp_connection.cc (right):

https://codereview.chromium.org/1962393004/diff/20001/blimp/net/blimp_connect...
blimp/net/blimp_connection.cc:147: if (message_pump_.get()) {
No need to write .get() here.

https://codereview.chromium.org/1962393004/diff/20001/blimp/net/blimp_connect...
File blimp/net/blimp_connection_details.cc (right):

https://codereview.chromium.org/1962393004/diff/20001/blimp/net/blimp_connect...
blimp/net/blimp_connection_details.cc:21:
io_thread_task_runner_(io_thread_task_runner) {}
DCHECK that this is created on the correct thread.

https://codereview.chromium.org/1962393004/diff/20001/blimp/net/blimp_connect...
blimp/net/blimp_connection_details.cc:23:
BlimpConnectionDetails::~BlimpConnectionDetails() {}
ditto for the dtor.

https://codereview.chromium.org/1962393004/diff/20001/blimp/net/blimp_connect...
blimp/net/blimp_connection_details.cc:25: void
BlimpConnectionDetails::EnableDebugInfo(bool enable) {
Since this class is used on multiple threads, please DCHECK that each method is
called on the expected thread.

https://codereview.chromium.org/1962393004/diff/20001/blimp/net/blimp_connect...
blimp/net/blimp_connection_details.cc:62: base::Unretained(this)));
Are you sure its safe to use Unretained here? Can the object be destroyed before
this task runs?

https://codereview.chromium.org/1962393004/diff/20001/blimp/net/blimp_connect...
File blimp/net/blimp_connection_details.h (right):

https://codereview.chromium.org/1962393004/diff/20001/blimp/net/blimp_connect...
blimp/net/blimp_connection_details.h:1: // Copyright 2016 The Chromium Authors.
All rights reserved.
A test for this class?

https://codereview.chromium.org/1962393004/diff/20001/blimp/net/blimp_connect...
blimp/net/blimp_connection_details.h:14: class NetworkActivityObserver {
Which thread are the NetworkActivityObservers expected to be called on?

https://codereview.chromium.org/1962393004/diff/20001/blimp/net/blimp_connect...
blimp/net/blimp_connection_details.h:22: class BLIMP_NET_EXPORT
BlimpConnectionDetails {
Could you add a comment about the lifetime of this class. Which thread it is
created and destroyed on?

https://codereview.chromium.org/1962393004/diff/20001/blimp/net/blimp_connect...
blimp/net/blimp_connection_details.h:33: void OnPacketReceived(int bytes);
Would be nice to just divide the methods into 2 groups, with comments about
which thread they will be called on.

https://codereview.chromium.org/1962393004/diff/20001/blimp/net/blimp_connect...
blimp/net/blimp_connection_details.h:42: void
SetObserver(base::WeakPtr<NetworkActivityObserver> observer) {
Do you really need an Observer here? We usually use the term Observer when a
class has multiple observers that need to be notified of the events.
I think you can call it a Delegate or Client. Also, pass it in the ctor instead?

https://codereview.chromium.org/1962393004/diff/20001/blimp/net/blimp_connect...
blimp/net/blimp_connection_details.h:53: int bytes_received_;
Please mark the thread on which these variables are accessed. I would suggest
put them in a struct and use an accessor to make it obvious.

https://codereview.chromium.org/1962393004/diff/20001/blimp/net/packet_writer.h
File blimp/net/packet_writer.h (right):

https://codereview.chromium.org/1962393004/diff/20001/blimp/net/packet_writer...
blimp/net/packet_writer.h:32: void
SetBlimpConnectionDetails(BlimpConnectionDetails* details) {
nit: If its a simple setter, that's going to be defined in the header, then just
write it that way? set_blimp_connection_detail(...)?

https://codereview.chromium.org/1962393004/diff/20001/cc/trees/remote_channel...
File cc/trees/remote_channel_impl.cc (right):

https://codereview.chromium.org/1962393004/diff/20001/cc/trees/remote_channel...
cc/trees/remote_channel_impl.cc:411: void
RemoteChannelImpl::DidCommitAndDrawFrameOnMain() {
Just a reminder, make sure to update the remote channel tests since this call
will be received twice in the test now.

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

https://codereview.chromium.org/1962393004/diff/80001/blimp/net/blimp_connect...
File blimp/net/blimp_connection_details.cc (right):

https://codereview.chromium.org/1962393004/diff/80001/blimp/net/blimp_connect...
blimp/net/blimp_connection_details.cc:13:
BlimpConnectionDetails::BlimpConnectionDetails(
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.

https://codereview.chromium.org/1962393004/diff/80001/blimp/net/blimp_connect...
blimp/net/blimp_connection_details.cc:37:
base::Bind(&NetworkActivityObserver::UpdateDebugInfo, observer_,
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.

https://codereview.chromium.org/1962393004/diff/80001/blimp/net/blimp_connect...
blimp/net/blimp_connection_details.cc:42: void
BlimpConnectionDetails::OnPacketSent(int bytes) {
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.

https://codereview.chromium.org/1962393004/diff/80001/blimp/net/blimp_connect...
blimp/net/blimp_connection_details.cc:62: void
BlimpConnectionDetails::ResetStats() {
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]

https://codereview.chromium.org/1962393004/diff/20001/blimp/client/app/androi...
File blimp/client/app/android/blimp_client_session_android.cc (right):

https://codereview.chromium.org/1962393004/diff/20001/blimp/client/app/androi...
blimp/client/app/android/blimp_client_session_android.cc:124: void
BlimpClientSessionAndroid::EnableDebugInfo(
On 2016/05/18 00:23:00, Khushal wrote:
> Hmmm, I don't think we really need to explicitly disable stats collection if
the
> user hides the debug view. The data can still be recorded and will be visible

Ignore this part of the comment, I thought you were disabling stats collection.
Might want to rename this as EnableDebugInfoUpdates.

> when the user shows the view. You can just avoid piping the updates to java if
> the view is not showing.
> 
> If you want to give the user an explicit option to reset the stats, then you
can
> add that in the menu.

https://codereview.chromium.org/1962393004/diff/80001/blimp/net/blimp_connect...
File blimp/net/blimp_connection_details.cc (right):

https://codereview.chromium.org/1962393004/diff/80001/blimp/net/blimp_connect...
blimp/net/blimp_connection_details.cc:37:
base::Bind(&NetworkActivityObserver::UpdateDebugInfo, observer_,
On 2016/05/18 18:38:28, Kevin M wrote:
> 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.

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.

https://codereview.chromium.org/1962393004/diff/80001/blimp/net/blimp_connect...
blimp/net/blimp_connection_details.cc:52: void
BlimpConnectionDetails::OnCommit() {
I think this call is made on the main thread, you don't need to do a PostTask.

https://codereview.chromium.org/1962393004/diff/80001/blimp/net/blimp_connect...
blimp/net/blimp_connection_details.cc:62: void
BlimpConnectionDetails::ResetStats() {
On 2016/05/18 18:38:28, Kevin M wrote:
> 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.

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:17]

shaktisahu@chromium.org changed reviewers:
+ vmpstr@chromium.org

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

Moved to a Java based pull logic as per discussion with Kevin.

https://codereview.chromium.org/1962393004/diff/20001/blimp/client/app/androi...
File blimp/client/app/android/blimp_client_session_android.cc (right):

https://codereview.chromium.org/1962393004/diff/20001/blimp/client/app/androi...
blimp/client/app/android/blimp_client_session_android.cc:124: void
BlimpClientSessionAndroid::EnableDebugInfo(
On 2016/05/18 00:23:00, Khushal wrote:
> Hmmm, I don't think we really need to explicitly disable stats collection if
the
> user hides the debug view. The data can still be recorded and will be visible
> when the user shows the view. You can just avoid piping the updates to java if
> the view is not showing.
> 
> If you want to give the user an explicit option to reset the stats, then you
can
> add that in the menu.

I am not disabling the stat collection. The stats are collected but anyway, but
not sent to UI, if it is disabled. May be the name is causing confusion??

https://codereview.chromium.org/1962393004/diff/20001/blimp/client/app/androi...
File blimp/client/app/android/blimp_view.cc (right):

https://codereview.chromium.org/1962393004/diff/20001/blimp/client/app/androi...
blimp/client/app/android/blimp_view.cc:59:
blimp_connection_details_(blimp_connection_details) {
On 2016/05/18 00:23:00, Khushal wrote:
> DCHECK |blimp_connection_details_| here.

Done.

https://codereview.chromium.org/1962393004/diff/20001/blimp/client/app/androi...
File blimp/client/app/android/java/src/org/chromium/blimp/toolbar/Toolbar.java
(right):

https://codereview.chromium.org/1962393004/diff/20001/blimp/client/app/androi...
blimp/client/app/android/java/src/org/chromium/blimp/toolbar/Toolbar.java:58:
private boolean mDebugInfoEnabled = false;
On 2016/05/18 00:23:00, Khushal wrote:
> A comment here which says that this denotes whether the DebugInfo view is
> visible or not would be nice.

Done.

https://codereview.chromium.org/1962393004/diff/20001/blimp/client/app/androi...
blimp/client/app/android/java/src/org/chromium/blimp/toolbar/Toolbar.java:60:
private ArrayAdapter<String> mAdapter;
On 2016/05/18 00:23:00, Khushal wrote:
> Rename this mPopupMenuOptionsAdapter so its clear what this stores?

Done.

https://codereview.chromium.org/1962393004/diff/20001/blimp/client/app/androi...
blimp/client/app/android/java/src/org/chromium/blimp/toolbar/Toolbar.java:264:
BlimpRendererActivity activity = (BlimpRendererActivity) mContext;
On 2016/05/18 00:23:00, Khushal wrote:
> Just a note from our discussion earlier, please don't downcast the context.
Use
> a delegate interface instead. You wouldn't need to grab a reference to the
> BlimpClientSession as well then.

Done.

https://codereview.chromium.org/1962393004/diff/20001/blimp/client/feature/na...
File blimp/client/feature/navigation_feature.cc (right):

https://codereview.chromium.org/1962393004/diff/20001/blimp/client/feature/na...
blimp/client/feature/navigation_feature.cc:23: : connection_details_(details) {}
On 2016/05/18 00:23:00, Khushal wrote:
> DCHECK the |connection_details_| here.

Done.

https://codereview.chromium.org/1962393004/diff/20001/blimp/client/feature/na...
File blimp/client/feature/navigation_feature.h (right):

https://codereview.chromium.org/1962393004/diff/20001/blimp/client/feature/na...
blimp/client/feature/navigation_feature.h:68: BlimpConnectionDetails*
connection_details_ = nullptr;
On 2016/05/18 00:23:00, Khushal wrote:
> Do you need to initialize it to nullptr here? The class always expects to
> receive this in the ctor.

Done.

https://codereview.chromium.org/1962393004/diff/20001/blimp/client/session/bl...
File blimp/client/session/blimp_client_session.cc (right):

https://codereview.chromium.org/1962393004/diff/20001/blimp/client/session/bl...
blimp/client/session/blimp_client_session.cc:109: connection_details_(details)
{}
On 2016/05/18 00:23:00, Khushal wrote:
> DCHECK |connection_details_| here.

Not sure if this saves much, since we invoke the constructor in
BlimpClientSession constructor right after we create connection_details_

https://codereview.chromium.org/1962393004/diff/20001/blimp/client/session/bl...
blimp/client/session/blimp_client_session.cc:176: blimp_connection_details_ =
base::WrapUnique(new BlimpConnectionDetails(
On 2016/05/18 00:23:00, Khushal wrote:
> nit: Use x_.reset(new ...) for member variables of the class for consistency?

Done.

https://codereview.chromium.org/1962393004/diff/20001/blimp/engine/session/bl...
File blimp/engine/session/blimp_engine_session.cc (right):

https://codereview.chromium.org/1962393004/diff/20001/blimp/engine/session/bl...
blimp/engine/session/blimp_engine_session.cc:192:
connection->SetBlimpConnectionDetails(nullptr);
On 2016/05/18 00:23:00, Khushal wrote:
> Don't really need to do this here. They already have a null
> BlimpConnectionDetails.

Done.

https://codereview.chromium.org/1962393004/diff/20001/blimp/net/blimp_connect...
File blimp/net/blimp_connection.cc (right):

https://codereview.chromium.org/1962393004/diff/20001/blimp/net/blimp_connect...
blimp/net/blimp_connection.cc:147: if (message_pump_.get()) {
On 2016/05/18 00:23:00, Khushal wrote:
> No need to write .get() here.

Done.

https://codereview.chromium.org/1962393004/diff/20001/blimp/net/blimp_connect...
File blimp/net/blimp_connection_details.cc (right):

https://codereview.chromium.org/1962393004/diff/20001/blimp/net/blimp_connect...
blimp/net/blimp_connection_details.cc:62: base::Unretained(this)));
On 2016/05/18 00:23:01, Khushal wrote:
> Are you sure its safe to use Unretained here? Can the object be destroyed
before
> this task runs?

Acknowledged.

https://codereview.chromium.org/1962393004/diff/20001/blimp/net/blimp_connect...
File blimp/net/blimp_connection_details.h (right):

https://codereview.chromium.org/1962393004/diff/20001/blimp/net/blimp_connect...
blimp/net/blimp_connection_details.h:14: class NetworkActivityObserver {
On 2016/05/18 00:23:01, Khushal wrote:
> Which thread are the NetworkActivityObservers expected to be called on?

Done.

https://codereview.chromium.org/1962393004/diff/20001/blimp/net/blimp_connect...
blimp/net/blimp_connection_details.h:22: class BLIMP_NET_EXPORT
BlimpConnectionDetails {
On 2016/05/18 00:23:01, Khushal wrote:
> Could you add a comment about the lifetime of this class. Which thread it is
> created and destroyed on?

Done.

https://codereview.chromium.org/1962393004/diff/20001/blimp/net/blimp_connect...
blimp/net/blimp_connection_details.h:33: void OnPacketReceived(int bytes);
On 2016/05/18 00:23:01, Khushal wrote:
> Would be nice to just divide the methods into 2 groups, with comments about
> which thread they will be called on.

Done.

https://codereview.chromium.org/1962393004/diff/20001/blimp/net/blimp_connect...
blimp/net/blimp_connection_details.h:42: void
SetObserver(base::WeakPtr<NetworkActivityObserver> observer) {
On 2016/05/18 00:23:01, Khushal wrote:
> Do you really need an Observer here? We usually use the term Observer when a
> class has multiple observers that need to be notified of the events.
> I think you can call it a Delegate or Client. Also, pass it in the ctor
instead?

Moved it to constructor. Actually I wanted to make it an observer that listens
to these statistics collection events. Theoretically there can be multiple
observers (one for UI, one for something else....).

https://codereview.chromium.org/1962393004/diff/20001/blimp/net/packet_writer.h
File blimp/net/packet_writer.h (right):

https://codereview.chromium.org/1962393004/diff/20001/blimp/net/packet_writer...
blimp/net/packet_writer.h:32: void
SetBlimpConnectionDetails(BlimpConnectionDetails* details) {
On 2016/05/18 00:23:01, Khushal wrote:
> nit: If its a simple setter, that's going to be defined in the header, then
just
> write it that way? set_blimp_connection_detail(...)? 

Done.

https://codereview.chromium.org/1962393004/diff/20001/cc/trees/remote_channel...
File cc/trees/remote_channel_impl.cc (right):

https://codereview.chromium.org/1962393004/diff/20001/cc/trees/remote_channel...
cc/trees/remote_channel_impl.cc:411: void
RemoteChannelImpl::DidCommitAndDrawFrameOnMain() {
On 2016/05/18 00:23:01, Khushal wrote:
> Just a reminder, make sure to update the remote channel tests since this call
> will be received twice in the test now.

Done.

https://codereview.chromium.org/1962393004/diff/80001/blimp/net/blimp_connect...
File blimp/net/blimp_connection_details.cc (right):

https://codereview.chromium.org/1962393004/diff/80001/blimp/net/blimp_connect...
blimp/net/blimp_connection_details.cc:13:
BlimpConnectionDetails::BlimpConnectionDetails(
On 2016/05/18 18:38:28, Kevin M wrote:
> 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.

Done.

https://codereview.chromium.org/1962393004/diff/80001/blimp/net/blimp_connect...
blimp/net/blimp_connection_details.cc:37:
base::Bind(&NetworkActivityObserver::UpdateDebugInfo, observer_,
On 2016/05/18 18:38:28, Kevin M wrote:
> 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.

Done.

https://codereview.chromium.org/1962393004/diff/80001/blimp/net/blimp_connect...
blimp/net/blimp_connection_details.cc:42: void
BlimpConnectionDetails::OnPacketSent(int bytes) {
On 2016/05/18 18:38:28, Kevin M wrote:
> 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.

Done.

https://codereview.chromium.org/1962393004/diff/80001/blimp/net/blimp_connect...
blimp/net/blimp_connection_details.cc:52: void
BlimpConnectionDetails::OnCommit() {
On 2016/05/18 20:41:10, Khushal wrote:
> I think this call is made on the main thread, you don't need to do a PostTask.

Done.

https://codereview.chromium.org/1962393004/diff/80001/blimp/net/blimp_connect...
blimp/net/blimp_connection_details.cc:62: void
BlimpConnectionDetails::ResetStats() {
On 2016/05/18 18:38:28, Kevin M wrote:
> 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.

Done.

[Kevin M, 2016-05-20 01:02:04]

https://codereview.chromium.org/1962393004/diff/100001/blimp/client/app/andro...
File blimp/client/app/android/blimp_client_session_android.cc (right):

https://codereview.chromium.org/1962393004/diff/100001/blimp/client/app/andro...
blimp/client/app/android/blimp_client_session_android.cc:128:
BlimpClientSession::GetDebugInfo();
BlimpClientSession::GetDebugInfo should have a different name, as function
overloading is generally discouraged.

Also, use a callback for returning the result asynchronously (e.g. const
base::Callback<const BlimpConnectionStatistics::StatisticsMap&>&)

https://codereview.chromium.org/1962393004/diff/100001/blimp/client/app/andro...
File
blimp/client/app/android/java/src/org/chromium/blimp/BlimpRendererActivity.java
(right):

https://codereview.chromium.org/1962393004/diff/100001/blimp/client/app/andro...
blimp/client/app/android/java/src/org/chromium/blimp/BlimpRendererActivity.java:40:
// Refresh interval for the debug view.
What's the time unit for this interval?

https://codereview.chromium.org/1962393004/diff/100001/blimp/client/app/andro...
File
blimp/client/app/android/java/src/org/chromium/blimp/session/BlimpClientSession.java
(right):

https://codereview.chromium.org/1962393004/diff/100001/blimp/client/app/andro...
blimp/client/app/android/java/src/org/chromium/blimp/session/BlimpClientSession.java:67:
private int mReceivedOffset;
Offset => Base?

https://codereview.chromium.org/1962393004/diff/100001/blimp/client/app/andro...
blimp/client/app/android/java/src/org/chromium/blimp/session/BlimpClientSession.java:205:
mReceivedOffset = received;
We should probably push the offset and resetting functionality into the observer
(closer to the view), so that the data source isn't throwing away data.
Conflating state change with reset logic seems to be mixing unrelated pieces of
functionality together.

https://codereview.chromium.org/1962393004/diff/100001/blimp/client/app/andro...
blimp/client/app/android/java/src/org/chromium/blimp/session/BlimpClientSession.java:229:
mStatisticsEnabled = false;
What's the use case for statisticsEnabled? If we are only getting 1 update per
request maximum, then do we need to squelch extra updates?

https://codereview.chromium.org/1962393004/diff/100001/blimp/client/app/andro...
File blimp/client/app/android/java/src/org/chromium/blimp/toolbar/Toolbar.java
(right):

https://codereview.chromium.org/1962393004/diff/100001/blimp/client/app/andro...
blimp/client/app/android/java/src/org/chromium/blimp/toolbar/Toolbar.java:98:
mBlimpClientSession.resetDebugStats();
I'm not too keen on making Toolbar also responsible for debug statistics
semantics - it adds side effects to the class that make it less testable and
also makes it harder for readers to comprehend what's in front of them.

I think it would be cleaner to delegate all debug stat state and logic  to the
activity that needs it.
* Decouple this relationship by publishing navigation events to an observer,
e.g. NavigationEventObserver.
* The debug view activity can be an instance of said observer interface. If one
of the methods is triggered, it would reset the stats.

https://codereview.chromium.org/1962393004/diff/100001/blimp/client/app/andro...
File
blimp/client/app/android/java/src/org/chromium/blimp/toolbar/ToolbarMenu.java
(right):

https://codereview.chromium.org/1962393004/diff/100001/blimp/client/app/andro...
blimp/client/app/android/java/src/org/chromium/blimp/toolbar/ToolbarMenu.java:49:
* @return the mDebugInfoEnabled
Can you improve the comments here?

https://codereview.chromium.org/1962393004/diff/100001/blimp/client/app/andro...
blimp/client/app/android/java/src/org/chromium/blimp/toolbar/ToolbarMenu.java:56:
* @param mDebugInfoEnabled the mDebugInfoEnabled to set
and here

https://codereview.chromium.org/1962393004/diff/100001/blimp/client/app/andro...
blimp/client/app/android/java/src/org/chromium/blimp/toolbar/ToolbarMenu.java:65:
public interface ToolbarMenuDelegate {
Should the interface come before the field list? It seems strange to switch
between field and interface declarations.

https://codereview.chromium.org/1962393004/diff/100001/blimp/client/app/andro...
blimp/client/app/android/java/src/org/chromium/blimp/toolbar/ToolbarMenu.java:82:
* Opens up the menu, initializes if uninitialized.
"Opens a lazily-created menu?"

Would it be good to specify *which* menu?

https://codereview.chromium.org/1962393004/diff/100001/blimp/client/app/andro...
blimp/client/app/android/java/src/org/chromium/blimp/toolbar/ToolbarMenu.java:97:
private void initializeMenu(View anchorView) {
Turn this into a creation method instead of an initialization method? (IMO a
return value is more indicative of how the function's I/O behaves)

https://codereview.chromium.org/1962393004/diff/100001/blimp/client/app/andro...
blimp/client/app/android/java/src/org/chromium/blimp/toolbar/ToolbarMenu.java:128:
private void intializeMenuAdapter() {
Comment?

https://codereview.chromium.org/1962393004/diff/100001/blimp/client/app/andro...
blimp/client/app/android/java/src/org/chromium/blimp/toolbar/ToolbarMenu.java:140:
private void openInChrome() {
Comment?

https://codereview.chromium.org/1962393004/diff/100001/blimp/client/feature/c...
File blimp/client/feature/compositor/blimp_compositor_manager.h (right):

https://codereview.chromium.org/1962393004/diff/100001/blimp/client/feature/c...
blimp/client/feature/compositor/blimp_compositor_manager.h:19: class
BlimpCompositorManagerClient {
This should probably be called a Delegate, not a Client.

https://codereview.chromium.org/1962393004/diff/100001/blimp/client/feature/c...
blimp/client/feature/compositor/blimp_compositor_manager.h:22: virtual void
DidCommitAndDrawFrame() = 0;
Can we standardize our naming here, so it's all OnXYZ or DidXYZ, but not both?

https://codereview.chromium.org/1962393004/diff/100001/blimp/client/session/b...
File blimp/client/session/blimp_client_session.cc (right):

https://codereview.chromium.org/1962393004/diff/100001/blimp/client/session/b...
blimp/client/session/blimp_client_session.cc:177:
blimp_connection_statistics_.reset(
Can you move this into ClientNetworkComponents? We consciously chose to relegate
all the IO thread affine stuff into that class for simplicity's sake, so we
limit the mixing of UI thread members and IO thread members in a single object.
That makes destruction pretty error-prone.

https://codereview.chromium.org/1962393004/diff/100001/blimp/client/session/b...
blimp/client/session/blimp_client_session.cc:295: BlimpConnectionStatistics*
BlimpClientSession::GetBlimpConnectionStatistics()
This is risky - we are exposing an IO thread member with this getter.

https://codereview.chromium.org/1962393004/diff/100001/blimp/client/session/b...
File blimp/client/session/blimp_client_session.h (right):

https://codereview.chromium.org/1962393004/diff/100001/blimp/client/session/b...
blimp/client/session/blimp_client_session.h:8: #include
<blimp/net/blimp_connection_statistics.h>
Angle brackets are reserved for standard headers.

Put this in quotes, and move it to the block below.

https://codereview.chromium.org/1962393004/diff/100001/blimp/client/session/b...
blimp/client/session/blimp_client_session.h:81: void GetDebugInfo();
Can we standardize the names here? DebugInfo or ConnectionStatistics, but not
both?

https://codereview.chromium.org/1962393004/diff/100001/blimp/client/session/b...
blimp/client/session/blimp_client_session.h:82: virtual void
UpdateDebugInfo(BlimpConnectionStatistics::StatisticsMap stats);
Just take a const BlimpConnectionStatistics& ?

https://codereview.chromium.org/1962393004/diff/100001/blimp/net/blimp_connec...
File blimp/net/blimp_connection.cc (right):

https://codereview.chromium.org/1962393004/diff/100001/blimp/net/blimp_connec...
blimp/net/blimp_connection.cc:147: if (message_pump_) {
message_pump_ will always be set, ditto for the sender.

https://codereview.chromium.org/1962393004/diff/100001/blimp/net/blimp_connec...
blimp/net/blimp_connection.cc:151:
static_cast<BlimpMessageSender*>(outgoing_msg_processor_.get());
Change the type of outgoing_msg_processor_ to a BlimpMessageSender instead of
using static_cast<>.

https://codereview.chromium.org/1962393004/diff/100001/blimp/net/blimp_connec...
File blimp/net/blimp_connection.h (right):

https://codereview.chromium.org/1962393004/diff/100001/blimp/net/blimp_connec...
blimp/net/blimp_connection.h:46: void
SetBlimpConnectionStatistics(BlimpConnectionStatistics* statistics);
Does it make sense to *not* have connection statistics? Can this be moved into
the constructor?

https://codereview.chromium.org/1962393004/diff/100001/blimp/net/blimp_connec...
File blimp/net/blimp_connection_statistics.cc (right):

https://codereview.chromium.org/1962393004/diff/100001/blimp/net/blimp_connec...
blimp/net/blimp_connection_statistics.cc:35: void
BlimpConnectionStatistics::IncrementOnMain(EventType type) {
No need for this function; increment is basically addition w/1.

https://codereview.chromium.org/1962393004/diff/100001/blimp/net/blimp_connec...
blimp/net/blimp_connection_statistics.cc:43: VLOG(0) << "statistics_" <<
statistics_[BYTES_RECEIVED];
This logging line doesn't look very useful; remove it?

https://codereview.chromium.org/1962393004/diff/100001/blimp/net/blimp_connec...
File blimp/net/blimp_connection_statistics.h (right):

https://codereview.chromium.org/1962393004/diff/100001/blimp/net/blimp_connec...
blimp/net/blimp_connection_statistics.h:36: // Can be called from any thread.
Document the purpose and parameters of these functions

https://codereview.chromium.org/1962393004/diff/100001/blimp/net/blimp_connec...
blimp/net/blimp_connection_statistics.h:37: void Add(EventType type, int data);
We can use a better parameter name instead of "data", maybe "num"? "x"? lol

https://codereview.chromium.org/1962393004/diff/100001/blimp/net/blimp_connec...
blimp/net/blimp_connection_statistics.h:41: void GetDebugInfo(const
base::Callback<void(StatisticsMap stats)>& callback);
If we require that this method be invoked on the correct thread, then the values
are available for synchronous access, therefore there's no need for a callback.

We should probably just add an int Get(EventType type) method, which accesses a
value from the map.

https://codereview.chromium.org/1962393004/diff/100001/blimp/net/blimp_connec...
File blimp/net/blimp_connection_statistics_unittest.cc (right):

https://codereview.chromium.org/1962393004/diff/100001/blimp/net/blimp_connec...
blimp/net/blimp_connection_statistics_unittest.cc:5: #include
<blimp/net/blimp_connection_statistics.h>
No angle brackets for non-standard headers

https://codereview.chromium.org/1962393004/diff/100001/blimp/net/blimp_connec...
blimp/net/blimp_connection_statistics_unittest.cc:29:
std::unique_ptr<BlimpConnectionStatistics> stats_;
DISALLOW_COPY_AND_ASSIGN()

https://codereview.chromium.org/1962393004/diff/100001/blimp/net/blimp_messag...
File blimp/net/blimp_message_pump.h (right):

https://codereview.chromium.org/1962393004/diff/100001/blimp/net/blimp_messag...
blimp/net/blimp_message_pump.h:8: #include
<blimp/net/blimp_connection_statistics.h>
Angle brackets.

https://codereview.chromium.org/1962393004/diff/100001/blimp/net/blimp_messag...
blimp/net/blimp_message_pump.h:49: void SetBlimpConnectionStatistics(
IIRC we can only inline simple setters and getters - if we have to pull in a
header so we can access a class member, then it should go in the .cc file.

https://codereview.chromium.org/1962393004/diff/100001/blimp/net/compressed_p...
File blimp/net/compressed_packet_reader.cc (right):

https://codereview.chromium.org/1962393004/diff/100001/blimp/net/compressed_p...
blimp/net/compressed_packet_reader.cc:96: if (connection_statistics_) {
Test this interaction?

https://codereview.chromium.org/1962393004/diff/100001/blimp/net/compressed_p...
File blimp/net/compressed_packet_writer.cc (right):

https://codereview.chromium.org/1962393004/diff/100001/blimp/net/compressed_p...
blimp/net/compressed_packet_writer.cc:5: #include
<blimp/net/blimp_connection_statistics.h>
Angle brackets

https://codereview.chromium.org/1962393004/diff/100001/blimp/net/compressed_p...
blimp/net/compressed_packet_writer.cc:77: if (connection_statistics_) {
Test this?

https://codereview.chromium.org/1962393004/diff/100001/blimp/net/packet_reader.h
File blimp/net/packet_reader.h (right):

https://codereview.chromium.org/1962393004/diff/100001/blimp/net/packet_reade...
blimp/net/packet_reader.h:8: #include <blimp/net/blimp_connection_statistics.h>
angle brackets

https://codereview.chromium.org/1962393004/diff/100001/blimp/net/packet_reade...
blimp/net/packet_reader.h:30: void
set_blimp_connection_statistics(BlimpConnectionStatistics* statistics) {
set_connection_statistics (use the field name, not the type name)

https://codereview.chromium.org/1962393004/diff/100001/blimp/net/packet_writer.h
File blimp/net/packet_writer.h (right):

https://codereview.chromium.org/1962393004/diff/100001/blimp/net/packet_write...
blimp/net/packet_writer.h:32: void
set_blimp_connection_statistics(BlimpConnectionStatistics* statistics) {
don't use "blimp" here"

https://codereview.chromium.org/1962393004/diff/100001/cc/trees/remote_channe...
File cc/trees/remote_channel_unittest.cc (right):

https://codereview.chromium.org/1962393004/diff/100001/cc/trees/remote_channe...
cc/trees/remote_channel_unittest.cc:151: if
(++calls_received_on_both_server_and_client_ == 4)
I know the previous code was written this way, but we shouldn't be mixing
increment operations with conditionals - plz put them on separate lines.

[Kevin M, 2016-05-20 04:01:52]

https://codereview.chromium.org/1962393004/diff/100001/blimp/net/blimp_connec...
File blimp/net/blimp_connection_statistics.h (right):

https://codereview.chromium.org/1962393004/diff/100001/blimp/net/blimp_connec...
blimp/net/blimp_connection_statistics.h:41: void GetDebugInfo(const
base::Callback<void(StatisticsMap stats)>& callback);
After a bit of thought, I think we need to have the Add and Increment events
work on the UI thread, the reason being is that posting to the UI task runner
for every network event is hugely expensive.

[Kevin M, 2016-05-20 16:37:28]

If the class is changed to support retrieving metrics from another thread, then
disregard my comment about getting one metric at a time with a Get() method. It
would be more effective to simply pass the entire map to the receiving callback
instead, e.g.

base::Callback<void(StatisticsMap)>

[vmpstr, 2016-05-20 23:26:14]

cc rs lgtm

https://codereview.chromium.org/1962393004/diff/100001/cc/trees/remote_channe...
File cc/trees/remote_channel_unittest.cc (right):

https://codereview.chromium.org/1962393004/diff/100001/cc/trees/remote_channe...
cc/trees/remote_channel_unittest.cc:151: if
(++calls_received_on_both_server_and_client_ == 4)
On 2016/05/20 01:02:04, Kevin M wrote:
> I know the previous code was written this way, but we shouldn't be mixing
> increment operations with conditionals - plz put them on separate lines.

(here and in the above lines please. increment in a macro is typically a bad
thing)

[shaktisahu, 2016-05-22 22:36:58]

Kevin, PTAL

https://codereview.chromium.org/1962393004/diff/100001/blimp/client/app/andro...
File blimp/client/app/android/blimp_client_session_android.cc (right):

https://codereview.chromium.org/1962393004/diff/100001/blimp/client/app/andro...
blimp/client/app/android/blimp_client_session_android.cc:128:
BlimpClientSession::GetDebugInfo();
On 2016/05/20 01:02:01, Kevin M wrote:
> BlimpClientSession::GetDebugInfo should have a different name, as function
> overloading is generally discouraged.
> 
> Also, use a callback for returning the result asynchronously (e.g. const
> base::Callback<const BlimpConnectionStatistics::StatisticsMap&>&)

Changed to synchronous, since everything is accessed on UI thread.

https://codereview.chromium.org/1962393004/diff/100001/blimp/client/app/andro...
File
blimp/client/app/android/java/src/org/chromium/blimp/BlimpRendererActivity.java
(right):

https://codereview.chromium.org/1962393004/diff/100001/blimp/client/app/andro...
blimp/client/app/android/java/src/org/chromium/blimp/BlimpRendererActivity.java:40:
// Refresh interval for the debug view.
On 2016/05/20 01:02:01, Kevin M wrote:
> What's the time unit for this interval?

milliseconds.

https://codereview.chromium.org/1962393004/diff/100001/blimp/client/app/andro...
File
blimp/client/app/android/java/src/org/chromium/blimp/session/BlimpClientSession.java
(right):

https://codereview.chromium.org/1962393004/diff/100001/blimp/client/app/andro...
blimp/client/app/android/java/src/org/chromium/blimp/session/BlimpClientSession.java:67:
private int mReceivedOffset;
On 2016/05/20 01:02:02, Kevin M wrote:
> Offset => Base?

Done.

https://codereview.chromium.org/1962393004/diff/100001/blimp/client/app/andro...
blimp/client/app/android/java/src/org/chromium/blimp/session/BlimpClientSession.java:205:
mReceivedOffset = received;
On 2016/05/20 01:02:02, Kevin M wrote:
> We should probably push the offset and resetting functionality into the
observer
> (closer to the view), so that the data source isn't throwing away data.
> Conflating state change with reset logic seems to be mixing unrelated pieces
of
> functionality together.

Yes, I am moving it to BlimpRendererActivity to be cleaner.

https://codereview.chromium.org/1962393004/diff/100001/blimp/client/app/andro...
blimp/client/app/android/java/src/org/chromium/blimp/session/BlimpClientSession.java:229:
mStatisticsEnabled = false;
On 2016/05/20 01:02:02, Kevin M wrote:
> What's the use case for statisticsEnabled? If we are only getting 1 update per
> request maximum, then do we need to squelch extra updates?

Actually this is a flag to the base value of the metrics. See updated comments
and naming.

https://codereview.chromium.org/1962393004/diff/100001/blimp/client/app/andro...
File blimp/client/app/android/java/src/org/chromium/blimp/toolbar/Toolbar.java
(right):

https://codereview.chromium.org/1962393004/diff/100001/blimp/client/app/andro...
blimp/client/app/android/java/src/org/chromium/blimp/toolbar/Toolbar.java:98:
mBlimpClientSession.resetDebugStats();
On 2016/05/20 01:02:02, Kevin M wrote:
> I'm not too keen on making Toolbar also responsible for debug statistics
> semantics - it adds side effects to the class that make it less testable and
> also makes it harder for readers to comprehend what's in front of them.
> 
> I think it would be cleaner to delegate all debug stat state and logic  to the
> activity that needs it.
> * Decouple this relationship by publishing navigation events to an observer,
> e.g. NavigationEventObserver.
> * The debug view activity can be an instance of said observer interface. If
one
> of the methods is triggered, it would reset the stats.

Yea, I will an interface for this. By the way, this class will undergo a lot of
changes in near feature. So not very keen on improving this by a lot.

https://codereview.chromium.org/1962393004/diff/100001/blimp/client/app/andro...
File
blimp/client/app/android/java/src/org/chromium/blimp/toolbar/ToolbarMenu.java
(right):

https://codereview.chromium.org/1962393004/diff/100001/blimp/client/app/andro...
blimp/client/app/android/java/src/org/chromium/blimp/toolbar/ToolbarMenu.java:49:
* @return the mDebugInfoEnabled
On 2016/05/20 01:02:02, Kevin M wrote:
> Can you improve the comments here?

Done.

https://codereview.chromium.org/1962393004/diff/100001/blimp/client/app/andro...
blimp/client/app/android/java/src/org/chromium/blimp/toolbar/ToolbarMenu.java:56:
* @param mDebugInfoEnabled the mDebugInfoEnabled to set
On 2016/05/20 01:02:02, Kevin M wrote:
> and here

Done.

https://codereview.chromium.org/1962393004/diff/100001/blimp/client/app/andro...
blimp/client/app/android/java/src/org/chromium/blimp/toolbar/ToolbarMenu.java:65:
public interface ToolbarMenuDelegate {
On 2016/05/20 01:02:02, Kevin M wrote:
> Should the interface come before the field list? It seems strange to switch
> between field and interface declarations.

Done.

https://codereview.chromium.org/1962393004/diff/100001/blimp/client/app/andro...
blimp/client/app/android/java/src/org/chromium/blimp/toolbar/ToolbarMenu.java:97:
private void initializeMenu(View anchorView) {
On 2016/05/20 01:02:02, Kevin M wrote:
> Turn this into a creation method instead of an initialization method? (IMO a
> return value is more indicative of how the function's I/O behaves)

hmm, initialize sounds good :) ....I think most of the places in android use
'initialize' instead of 'create' for UI initialization.

https://codereview.chromium.org/1962393004/diff/100001/blimp/client/app/andro...
blimp/client/app/android/java/src/org/chromium/blimp/toolbar/ToolbarMenu.java:128:
private void intializeMenuAdapter() {
On 2016/05/20 01:02:02, Kevin M wrote:
> Comment?

Done.

https://codereview.chromium.org/1962393004/diff/100001/blimp/client/app/andro...
blimp/client/app/android/java/src/org/chromium/blimp/toolbar/ToolbarMenu.java:140:
private void openInChrome() {
On 2016/05/20 01:02:02, Kevin M wrote:
> Comment?

Done.

https://codereview.chromium.org/1962393004/diff/100001/blimp/client/feature/c...
File blimp/client/feature/compositor/blimp_compositor_manager.h (right):

https://codereview.chromium.org/1962393004/diff/100001/blimp/client/feature/c...
blimp/client/feature/compositor/blimp_compositor_manager.h:19: class
BlimpCompositorManagerClient {
On 2016/05/20 01:02:03, Kevin M wrote:
> This should probably be called a Delegate, not a Client.

Hmm... Sounds okay to me. Anyway this is not my changes and don't want to change
it.

https://codereview.chromium.org/1962393004/diff/100001/blimp/client/feature/c...
blimp/client/feature/compositor/blimp_compositor_manager.h:22: virtual void
DidCommitAndDrawFrame() = 0;
On 2016/05/20 01:02:03, Kevin M wrote:
> Can we standardize our naming here, so it's all OnXYZ or DidXYZ, but not both?

This is the same callback propagated all the way from cc layer. May be sometimes
one convention makes more sense than the other? e.g. when there are more than
one verb in the method name :)

https://codereview.chromium.org/1962393004/diff/100001/blimp/client/session/b...
File blimp/client/session/blimp_client_session.cc (right):

https://codereview.chromium.org/1962393004/diff/100001/blimp/client/session/b...
blimp/client/session/blimp_client_session.cc:177:
blimp_connection_statistics_.reset(
On 2016/05/20 01:02:03, Kevin M wrote:
> Can you move this into ClientNetworkComponents? We consciously chose to
relegate
> all the IO thread affine stuff into that class for simplicity's sake, so we
> limit the mixing of UI thread members and IO thread members in a single
object.
> That makes destruction pretty error-prone.

This object needs to stay closer to the UI thread since it take care of
threadsafety itself. Also it has data from both IO and UI thread. (Bytes
sent/received from IO thread while commits from UI thread). If I keep it here,
as we saw, I wouldn't have to pass asynchronous callbacks into Java and Java
code would be much simpler.

https://codereview.chromium.org/1962393004/diff/100001/blimp/client/session/b...
blimp/client/session/blimp_client_session.cc:295: BlimpConnectionStatistics*
BlimpClientSession::GetBlimpConnectionStatistics()
On 2016/05/20 01:02:03, Kevin M wrote:
> This is risky - we are exposing an IO thread member with this getter.

Yea, but if it lives on UI thread and is called from UI thread, it should be
okay.

https://codereview.chromium.org/1962393004/diff/100001/blimp/client/session/b...
File blimp/client/session/blimp_client_session.h (right):

https://codereview.chromium.org/1962393004/diff/100001/blimp/client/session/b...
blimp/client/session/blimp_client_session.h:8: #include
<blimp/net/blimp_connection_statistics.h>
On 2016/05/20 01:02:03, Kevin M wrote:
> Angle brackets are reserved for standard headers.
> 
> Put this in quotes, and move it to the block below.

Done.

https://codereview.chromium.org/1962393004/diff/100001/blimp/net/blimp_connec...
File blimp/net/blimp_connection.cc (right):

https://codereview.chromium.org/1962393004/diff/100001/blimp/net/blimp_connec...
blimp/net/blimp_connection.cc:147: if (message_pump_) {
On 2016/05/20 01:02:03, Kevin M wrote:
> message_pump_ will always be set, ditto for the sender.

Done.

https://codereview.chromium.org/1962393004/diff/100001/blimp/net/blimp_connec...
blimp/net/blimp_connection.cc:151:
static_cast<BlimpMessageSender*>(outgoing_msg_processor_.get());
On 2016/05/20 01:02:03, Kevin M wrote:
> Change the type of outgoing_msg_processor_ to a BlimpMessageSender instead of
> using static_cast<>.

In that case I have to forward declare BlimpMessageSender in header file and
remove the namespace wrapping from the .cc file. I don't think it is necessary
to expose this class.

https://codereview.chromium.org/1962393004/diff/100001/blimp/net/blimp_connec...
File blimp/net/blimp_connection.h (right):

https://codereview.chromium.org/1962393004/diff/100001/blimp/net/blimp_connec...
blimp/net/blimp_connection.h:46: void
SetBlimpConnectionStatistics(BlimpConnectionStatistics* statistics);
On 2016/05/20 01:02:03, Kevin M wrote:
> Does it make sense to *not* have connection statistics? Can this be moved into
> the constructor?

No. The BlimpConnection is built at too deep in the call stack where I don't
have access. I have to pass this all the way from BlimpClientSession to specific
PacketReader/PacketWriter implementation, which is pretty terrible. The other
alternative is to make this class a singleton but singletons are not good
patterns in general.

https://codereview.chromium.org/1962393004/diff/100001/blimp/net/blimp_connec...
File blimp/net/blimp_connection_statistics.cc (right):

https://codereview.chromium.org/1962393004/diff/100001/blimp/net/blimp_connec...
blimp/net/blimp_connection_statistics.cc:35: void
BlimpConnectionStatistics::IncrementOnMain(EventType type) {
On 2016/05/20 01:02:03, Kevin M wrote:
> No need for this function; increment is basically addition w/1.

Ok.

https://codereview.chromium.org/1962393004/diff/100001/blimp/net/blimp_connec...
blimp/net/blimp_connection_statistics.cc:43: VLOG(0) << "statistics_" <<
statistics_[BYTES_RECEIVED];
On 2016/05/20 01:02:03, Kevin M wrote:
> This logging line doesn't look very useful; remove it?

Done.

https://codereview.chromium.org/1962393004/diff/100001/blimp/net/blimp_connec...
File blimp/net/blimp_connection_statistics.h (right):

https://codereview.chromium.org/1962393004/diff/100001/blimp/net/blimp_connec...
blimp/net/blimp_connection_statistics.h:37: void Add(EventType type, int data);
On 2016/05/20 01:02:03, Kevin M wrote:
> We can use a better parameter name instead of "data", maybe "num"? "x"? lol

I feel "data" is better than "x".

https://codereview.chromium.org/1962393004/diff/100001/blimp/net/blimp_connec...
blimp/net/blimp_connection_statistics.h:41: void GetDebugInfo(const
base::Callback<void(StatisticsMap stats)>& callback);
On 2016/05/20 01:02:04, Kevin M wrote:
> If we require that this method be invoked on the correct thread, then the
values
> are available for synchronous access, therefore there's no need for a
callback.
> 
> We should probably just add an int Get(EventType type) method, which accesses
a
> value from the map.

Thank you. This would be much better.

https://codereview.chromium.org/1962393004/diff/100001/blimp/net/blimp_connec...
File blimp/net/blimp_connection_statistics_unittest.cc (right):

https://codereview.chromium.org/1962393004/diff/100001/blimp/net/blimp_connec...
blimp/net/blimp_connection_statistics_unittest.cc:5: #include
<blimp/net/blimp_connection_statistics.h>
On 2016/05/20 01:02:04, Kevin M wrote:
> No angle brackets for non-standard headers

Done.

https://codereview.chromium.org/1962393004/diff/100001/blimp/net/blimp_connec...
blimp/net/blimp_connection_statistics_unittest.cc:29:
std::unique_ptr<BlimpConnectionStatistics> stats_;
On 2016/05/20 01:02:04, Kevin M wrote:
> DISALLOW_COPY_AND_ASSIGN()

Done.

https://codereview.chromium.org/1962393004/diff/100001/blimp/net/blimp_messag...
File blimp/net/blimp_message_pump.h (right):

https://codereview.chromium.org/1962393004/diff/100001/blimp/net/blimp_messag...
blimp/net/blimp_message_pump.h:8: #include
<blimp/net/blimp_connection_statistics.h>
On 2016/05/20 01:02:04, Kevin M wrote:
> Angle brackets.

Now that I think about it, I suspect angle bracket was automatically added when
I refactored the class name in Eclipse. Coz, in the previous commit, I din't
have any :)

https://codereview.chromium.org/1962393004/diff/100001/blimp/net/blimp_messag...
blimp/net/blimp_message_pump.h:49: void SetBlimpConnectionStatistics(
On 2016/05/20 01:02:04, Kevin M wrote:
> IIRC we can only inline simple setters and getters - if we have to pull in a
> header so we can access a class member, then it should go in the .cc file.

Done.

https://codereview.chromium.org/1962393004/diff/100001/blimp/net/compressed_p...
File blimp/net/compressed_packet_reader.cc (right):

https://codereview.chromium.org/1962393004/diff/100001/blimp/net/compressed_p...
blimp/net/compressed_packet_reader.cc:96: if (connection_statistics_) {
On 2016/05/20 01:02:04, Kevin M wrote:
> Test this interaction?

Done.

https://codereview.chromium.org/1962393004/diff/100001/blimp/net/compressed_p...
File blimp/net/compressed_packet_writer.cc (right):

https://codereview.chromium.org/1962393004/diff/100001/blimp/net/compressed_p...
blimp/net/compressed_packet_writer.cc:5: #include
<blimp/net/blimp_connection_statistics.h>
On 2016/05/20 01:02:04, Kevin M wrote:
> Angle brackets

Done.

https://codereview.chromium.org/1962393004/diff/100001/blimp/net/compressed_p...
blimp/net/compressed_packet_writer.cc:77: if (connection_statistics_) {
On 2016/05/20 01:02:04, Kevin M wrote:
> Test this?

Done.

https://codereview.chromium.org/1962393004/diff/100001/blimp/net/packet_reader.h
File blimp/net/packet_reader.h (right):

https://codereview.chromium.org/1962393004/diff/100001/blimp/net/packet_reade...
blimp/net/packet_reader.h:8: #include <blimp/net/blimp_connection_statistics.h>
On 2016/05/20 01:02:04, Kevin M wrote:
> angle brackets

Done.

https://codereview.chromium.org/1962393004/diff/100001/blimp/net/packet_reade...
blimp/net/packet_reader.h:30: void
set_blimp_connection_statistics(BlimpConnectionStatistics* statistics) {
On 2016/05/20 01:02:04, Kevin M wrote:
> set_connection_statistics (use the field name, not the type name)

Done.

https://codereview.chromium.org/1962393004/diff/100001/blimp/net/packet_writer.h
File blimp/net/packet_writer.h (right):

https://codereview.chromium.org/1962393004/diff/100001/blimp/net/packet_write...
blimp/net/packet_writer.h:32: void
set_blimp_connection_statistics(BlimpConnectionStatistics* statistics) {
On 2016/05/20 01:02:04, Kevin M wrote:
> don't use "blimp" here"

Done.

https://codereview.chromium.org/1962393004/diff/100001/cc/trees/remote_channe...
File cc/trees/remote_channel_unittest.cc (right):

https://codereview.chromium.org/1962393004/diff/100001/cc/trees/remote_channe...
cc/trees/remote_channel_unittest.cc:151: if
(++calls_received_on_both_server_and_client_ == 4)
On 2016/05/20 01:02:04, Kevin M wrote:
> I know the previous code was written this way, but we shouldn't be mixing
> increment operations with conditionals - plz put them on separate lines.

Done.

https://codereview.chromium.org/1962393004/diff/100001/cc/trees/remote_channe...
cc/trees/remote_channel_unittest.cc:151: if
(++calls_received_on_both_server_and_client_ == 4)
On 2016/05/20 23:26:14, vmpstr wrote:
> On 2016/05/20 01:02:04, Kevin M wrote:
> > I know the previous code was written this way, but we shouldn't be mixing
> > increment operations with conditionals - plz put them on separate lines.
> 
> (here and in the above lines please. increment in a macro is typically a bad
> thing)

Done.

[Kevin M, 2016-05-24 01:02:02]

Sorry for the latency - this is a pretty big CL in terms of # of files touched,
so it takes a while to work through the diff.

https://codereview.chromium.org/1962393004/diff/100001/blimp/client/app/andro...
File blimp/client/app/android/java/src/org/chromium/blimp/toolbar/Toolbar.java
(right):

https://codereview.chromium.org/1962393004/diff/100001/blimp/client/app/andro...
blimp/client/app/android/java/src/org/chromium/blimp/toolbar/Toolbar.java:98:
mBlimpClientSession.resetDebugStats();
> By the way, this class will undergo a lot of
> changes in near feature. So not very keen on improving this by a lot.

Code finds a way to survive, so we should ensure that we mindfully balance
quality and expediency. :)

https://codereview.chromium.org/1962393004/diff/100001/blimp/client/app/andro...
File
blimp/client/app/android/java/src/org/chromium/blimp/toolbar/ToolbarMenu.java
(right):

https://codereview.chromium.org/1962393004/diff/100001/blimp/client/app/andro...
blimp/client/app/android/java/src/org/chromium/blimp/toolbar/ToolbarMenu.java:97:
private void initializeMenu(View anchorView) {
On 2016/05/22 22:36:56, shaktisahu wrote:
> On 2016/05/20 01:02:02, Kevin M wrote:
> > Turn this into a creation method instead of an initialization method? (IMO a
> > return value is more indicative of how the function's I/O behaves)
> 
> hmm, initialize sounds good :) ....I think most of the places in android use
> 'initialize' instead of 'create' for UI initialization.

Acknowledged.

https://codereview.chromium.org/1962393004/diff/120001/blimp/client/app/andro...
File blimp/client/app/android/blimp_client_session_android.cc (right):

https://codereview.chromium.org/1962393004/diff/120001/blimp/client/app/andro...
blimp/client/app/android/blimp_client_session_android.cc:122: int metrics[3] =
{stats->Get(BlimpConnectionStatistics::BYTES_RECEIVED),
Just use metrics[] - the size is computed from the literal.

https://codereview.chromium.org/1962393004/diff/120001/blimp/client/app/andro...
blimp/client/app/android/blimp_client_session_android.cc:125: return
base::android::ToJavaIntArray(env, metrics, 3);
3 => arraysize(metrics)

https://codereview.chromium.org/1962393004/diff/120001/blimp/client/app/andro...
File blimp/client/app/android/blimp_client_session_android.h (right):

https://codereview.chromium.org/1962393004/diff/120001/blimp/client/app/andro...
blimp/client/app/android/blimp_client_session_android.h:37:
base::android::ScopedJavaLocalRef<jintArray> GetDebugInfo(
Document the purpose of this function and the return value incl. the array
member's positional meanings.

https://codereview.chromium.org/1962393004/diff/120001/blimp/client/app/andro...
File blimp/client/app/android/blimp_view.h (right):

https://codereview.chromium.org/1962393004/diff/120001/blimp/client/app/andro...
blimp/client/app/android/blimp_view.h:8: #include
<blimp/net/blimp_connection_statistics.h>
Angle bracket

https://codereview.chromium.org/1962393004/diff/120001/blimp/client/app/andro...
File
blimp/client/app/android/java/src/org/chromium/blimp/BlimpRendererActivity.java
(right):

https://codereview.chromium.org/1962393004/diff/120001/blimp/client/app/andro...
blimp/client/app/android/java/src/org/chromium/blimp/BlimpRendererActivity.java:276:
tv.setText(String.valueOf(received / 1000));
If this is in kilobytes, then this should be 1024.

Even better, is there a named constant somewhere that's descriptive about this
operation? e.g. BYTES_PER_KILO

https://codereview.chromium.org/1962393004/diff/120001/blimp/client/app/andro...
blimp/client/app/android/java/src/org/chromium/blimp/BlimpRendererActivity.java:276:
tv.setText(String.valueOf(received / 1000));
Should we be doing a floating point divide, so that we can observe sub-kilobyte
transfers?

https://codereview.chromium.org/1962393004/diff/120001/blimp/client/app/andro...
File blimp/client/app/android/java/src/org/chromium/blimp/toolbar/Toolbar.java
(right):

https://codereview.chromium.org/1962393004/diff/120001/blimp/client/app/andro...
blimp/client/app/android/java/src/org/chromium/blimp/toolbar/Toolbar.java:48: 
Is the whitespace meaningful here?

https://codereview.chromium.org/1962393004/diff/120001/blimp/client/app/andro...
File
blimp/client/app/android/java/src/org/chromium/blimp/toolbar/ToolbarMenu.java
(right):

https://codereview.chromium.org/1962393004/diff/120001/blimp/client/app/andro...
blimp/client/app/android/java/src/org/chromium/blimp/toolbar/ToolbarMenu.java:147:
// Chrome is probably not installed, so try with the default browser
"probably" - it's a sure thing, if we caught an exception here?

https://codereview.chromium.org/1962393004/diff/120001/blimp/client/session/b...
File blimp/client/session/blimp_client_session.cc (right):

https://codereview.chromium.org/1962393004/diff/120001/blimp/client/session/b...
blimp/client/session/blimp_client_session.cc:197:
io_thread_.task_runner()->DeleteSoon(FROM_HERE,
Move ownership of the statistics object into net_components_ and just hold a
bare pointer here. BlimpNetworkComponents is a container for simplifying object
lifetime on the IO thread.

https://codereview.chromium.org/1962393004/diff/120001/blimp/client/session/b...
File blimp/client/session/blimp_client_session.h (right):

https://codereview.chromium.org/1962393004/diff/120001/blimp/client/session/b...
blimp/client/session/blimp_client_session.h:16: #include
"blimp/net/blimp_connection_statistics.h"
Use a prototype for BlimpConnectionStatistics and move the #include to the .cc.

https://codereview.chromium.org/1962393004/diff/120001/blimp/net/blimp_connec...
File blimp/net/blimp_connection.cc (right):

https://codereview.chromium.org/1962393004/diff/120001/blimp/net/blimp_connec...
blimp/net/blimp_connection.cc:41: DCHECK(writer_);
Not needed - the constructor guarantees that writer_ cannot be null.

https://codereview.chromium.org/1962393004/diff/120001/blimp/net/blimp_connec...
blimp/net/blimp_connection.cc:120:
static_cast<BlimpMessageSender*>(outgoing_msg_processor_.get());
You can remove this cast too, if we make outgoing_msg_processor a
BlimpMessageSender.

https://codereview.chromium.org/1962393004/diff/120001/blimp/net/blimp_connec...
blimp/net/blimp_connection.cc:145: void
BlimpConnection::SetBlimpConnectionStatistics(
We should be setting this at stream creation time, at the StreamPacketReader
level.

So that means:
1) have the TCPClientTransport and SSLClientTransport factories take a
BlimpConnectionStatistics*.

2) pass the pointer in to the StreamPacketReader and StreamPacketWriter objects
at creation time.

We don't have to expose any virtual getters to the packet reader or packet
writer that way, nor do we need to do any kind of two-phase initialization where
we connect and *then* we set it. A nice thing about requiring a statistics
object on construction is that we don't have to check for the existence of the
statistics object before dereferencing it - we'll know for a fact that the
statistics object is set.

https://codereview.chromium.org/1962393004/diff/120001/blimp/net/blimp_connec...
blimp/net/blimp_connection.cc:149:
static_cast<BlimpMessageSender*>(outgoing_msg_processor_.get());
See previous comment about making outgoing_msg_processor_ a BlimpMessageSender.

https://codereview.chromium.org/1962393004/diff/120001/blimp/net/blimp_connec...
File blimp/net/blimp_connection.h (right):

https://codereview.chromium.org/1962393004/diff/120001/blimp/net/blimp_connec...
blimp/net/blimp_connection.h:46: void
SetBlimpConnectionStatistics(BlimpConnectionStatistics* statistics);
Add comment

https://codereview.chromium.org/1962393004/diff/120001/blimp/net/blimp_connec...
blimp/net/blimp_connection.h:58: std::unique_ptr<BlimpMessageProcessor>
outgoing_msg_processor_;
Make this a BlimpMessageSender, and move BlimpMessageSender out of an anonymous
namespace. That way, we preserve type information and don't have to resort to a
static_cast<> later on.

https://codereview.chromium.org/1962393004/diff/120001/blimp/net/blimp_connec...
File blimp/net/blimp_connection_statistics_unittest.cc (right):

https://codereview.chromium.org/1962393004/diff/120001/blimp/net/blimp_connec...
blimp/net/blimp_connection_statistics_unittest.cc:5: #include "base/bind.h"
Not used

https://codereview.chromium.org/1962393004/diff/120001/blimp/net/blimp_connec...
blimp/net/blimp_connection_statistics_unittest.cc:7: #include
"base/message_loop/message_loop.h"
Not used

https://codereview.chromium.org/1962393004/diff/120001/blimp/net/blimp_connec...
blimp/net/blimp_connection_statistics_unittest.cc:8: #include "base/run_loop.h"
Not used

https://codereview.chromium.org/1962393004/diff/120001/blimp/net/blimp_connec...
blimp/net/blimp_connection_statistics_unittest.cc:9: #include
"base/threading/thread.h"
Not used

https://codereview.chromium.org/1962393004/diff/120001/blimp/net/blimp_connec...
blimp/net/blimp_connection_statistics_unittest.cc:11: #include
"testing/gmock/include/gmock/gmock.h"
Not used

https://codereview.chromium.org/1962393004/diff/120001/blimp/net/blimp_connec...
blimp/net/blimp_connection_statistics_unittest.cc:25:
std::unique_ptr<BlimpConnectionStatistics> stats_;
YOu can make this a regular field - no need to make it a unique_ptr

https://codereview.chromium.org/1962393004/diff/120001/blimp/net/blimp_connec...
blimp/net/blimp_connection_statistics_unittest.cc:31:
TEST_F(BlimpConnectionStatisticsTest, AddStatsAndVerify) {
Also verify zeroes beforehand

https://codereview.chromium.org/1962393004/diff/120001/blimp/net/blimp_messag...
File blimp/net/blimp_message_pump.cc (right):

https://codereview.chromium.org/1962393004/diff/120001/blimp/net/blimp_messag...
blimp/net/blimp_message_pump.cc:29: DCHECK(reader_);
Not needed: constructor enforces non-nullness, and the following line will
segfault if it *does* manage to be null

https://codereview.chromium.org/1962393004/diff/120001/blimp/net/blimp_messag...
File blimp/net/blimp_message_pump.h (right):

https://codereview.chromium.org/1962393004/diff/120001/blimp/net/blimp_messag...
blimp/net/blimp_message_pump.h:13: #include
"blimp/net/blimp_connection_statistics.h"
Add a prototype instead of an include in .h

https://codereview.chromium.org/1962393004/diff/120001/blimp/net/blimp_messag...
blimp/net/blimp_message_pump.h:15: #include "blimp/net/packet_reader.h"
Not used? We have a prototype on line 26.

https://codereview.chromium.org/1962393004/diff/120001/blimp/net/blimp_messag...
blimp/net/blimp_message_pump.h:49: void
SetBlimpConnectionStatistics(BlimpConnectionStatistics* statistics);
Add comment

https://codereview.chromium.org/1962393004/diff/120001/blimp/net/compressed_p...
File blimp/net/compressed_packet_reader.cc (right):

https://codereview.chromium.org/1962393004/diff/120001/blimp/net/compressed_p...
blimp/net/compressed_packet_reader.cc:96: if (blimp_connection_statistics_) {
This should be in stream_packet_reader/stream_packet_writer.

https://codereview.chromium.org/1962393004/diff/120001/blimp/net/compressed_p...
File blimp/net/compressed_packet_unittest.cc (right):

https://codereview.chromium.org/1962393004/diff/120001/blimp/net/compressed_p...
blimp/net/compressed_packet_unittest.cc:109:
std::unique_ptr<BlimpConnectionStatistics> blimp_connection_statistics_;
suggestion: make it a regular (non-unique_ptr) field?

https://codereview.chromium.org/1962393004/diff/120001/blimp/net/compressed_p...
blimp/net/compressed_packet_unittest.cc:119:
EXPECT_TRUE(CheckRoundTrip("1234"));
Why duplicate this line?

https://codereview.chromium.org/1962393004/diff/120001/blimp/net/compressed_p...
blimp/net/compressed_packet_unittest.cc:128: EXPECT_EQ((int)compressed.size(),
nit: use static_cast<> instead of C-style cast.

https://codereview.chromium.org/1962393004/diff/120001/blimp/net/compressed_p...
blimp/net/compressed_packet_unittest.cc:132:
BlimpConnectionStatistics::BYTES_SENT));
This formatting looks a little off, run git cl format?

[shaktisahu, 2016-05-24 21:02:45]

Kevin, PTAL.

https://codereview.chromium.org/1962393004/diff/120001/blimp/client/app/andro...
File blimp/client/app/android/blimp_client_session_android.cc (right):

https://codereview.chromium.org/1962393004/diff/120001/blimp/client/app/andro...
blimp/client/app/android/blimp_client_session_android.cc:122: int metrics[3] =
{stats->Get(BlimpConnectionStatistics::BYTES_RECEIVED),
On 2016/05/24 01:02:00, Kevin M wrote:
> Just use metrics[] - the size is computed from the literal.

Done.

https://codereview.chromium.org/1962393004/diff/120001/blimp/client/app/andro...
blimp/client/app/android/blimp_client_session_android.cc:125: return
base::android::ToJavaIntArray(env, metrics, 3);
On 2016/05/24 01:02:00, Kevin M wrote:
> 3 => arraysize(metrics)

Done.

https://codereview.chromium.org/1962393004/diff/120001/blimp/client/app/andro...
File blimp/client/app/android/blimp_view.h (right):

https://codereview.chromium.org/1962393004/diff/120001/blimp/client/app/andro...
blimp/client/app/android/blimp_view.h:8: #include
<blimp/net/blimp_connection_statistics.h>
On 2016/05/24 01:02:00, Kevin M wrote:
> Angle bracket

Done.

https://codereview.chromium.org/1962393004/diff/120001/blimp/client/app/andro...
File
blimp/client/app/android/java/src/org/chromium/blimp/BlimpRendererActivity.java
(right):

https://codereview.chromium.org/1962393004/diff/120001/blimp/client/app/andro...
blimp/client/app/android/java/src/org/chromium/blimp/BlimpRendererActivity.java:276:
tv.setText(String.valueOf(received / 1000));
On 2016/05/24 01:02:00, Kevin M wrote:
> If this is in kilobytes, then this should be 1024.
> 
> Even better, is there a named constant somewhere that's descriptive about this
> operation? e.g. BYTES_PER_KILO

Done.

https://codereview.chromium.org/1962393004/diff/120001/blimp/client/app/andro...
blimp/client/app/android/java/src/org/chromium/blimp/BlimpRendererActivity.java:276:
tv.setText(String.valueOf(received / 1000));
On 2016/05/24 01:02:00, Kevin M wrote:
> If this is in kilobytes, then this should be 1024.
> 
> Even better, is there a named constant somewhere that's descriptive about this
> operation? e.g. BYTES_PER_KILO

Done.

https://codereview.chromium.org/1962393004/diff/120001/blimp/client/app/andro...
File
blimp/client/app/android/java/src/org/chromium/blimp/toolbar/ToolbarMenu.java
(right):

https://codereview.chromium.org/1962393004/diff/120001/blimp/client/app/andro...
blimp/client/app/android/java/src/org/chromium/blimp/toolbar/ToolbarMenu.java:147:
// Chrome is probably not installed, so try with the default browser
On 2016/05/24 01:02:00, Kevin M wrote:
> "probably" - it's a sure thing, if we caught an exception here?

Done.

https://codereview.chromium.org/1962393004/diff/120001/blimp/client/session/b...
File blimp/client/session/blimp_client_session.cc (right):

https://codereview.chromium.org/1962393004/diff/120001/blimp/client/session/b...
blimp/client/session/blimp_client_session.cc:197:
io_thread_.task_runner()->DeleteSoon(FROM_HERE,
On 2016/05/24 01:02:00, Kevin M wrote:
> Move ownership of the statistics object into net_components_ and just hold a
> bare pointer here. BlimpNetworkComponents is a container for simplifying
object
> lifetime on the IO thread.

Done.

https://codereview.chromium.org/1962393004/diff/120001/blimp/client/session/b...
File blimp/client/session/blimp_client_session.h (right):

https://codereview.chromium.org/1962393004/diff/120001/blimp/client/session/b...
blimp/client/session/blimp_client_session.h:16: #include
"blimp/net/blimp_connection_statistics.h"
On 2016/05/24 01:02:00, Kevin M wrote:
> Use a prototype for BlimpConnectionStatistics and move the #include to the
.cc.

Done.

https://codereview.chromium.org/1962393004/diff/120001/blimp/net/blimp_connec...
File blimp/net/blimp_connection.cc (right):

https://codereview.chromium.org/1962393004/diff/120001/blimp/net/blimp_connec...
blimp/net/blimp_connection.cc:41: DCHECK(writer_);
On 2016/05/24 01:02:01, Kevin M wrote:
> Not needed - the constructor guarantees that writer_ cannot be null.


Removed function since we are passing in constructor

https://codereview.chromium.org/1962393004/diff/120001/blimp/net/blimp_connec...
File blimp/net/blimp_connection.h (right):

https://codereview.chromium.org/1962393004/diff/120001/blimp/net/blimp_connec...
blimp/net/blimp_connection.h:46: void
SetBlimpConnectionStatistics(BlimpConnectionStatistics* statistics);
On 2016/05/24 01:02:01, Kevin M wrote:
> Add comment

Removed function since we are passing in constructor

https://codereview.chromium.org/1962393004/diff/120001/blimp/net/blimp_connec...
File blimp/net/blimp_connection_statistics_unittest.cc (right):

https://codereview.chromium.org/1962393004/diff/120001/blimp/net/blimp_connec...
blimp/net/blimp_connection_statistics_unittest.cc:5: #include "base/bind.h"
On 2016/05/24 01:02:01, Kevin M wrote:
> Not used

Done.

https://codereview.chromium.org/1962393004/diff/120001/blimp/net/blimp_connec...
blimp/net/blimp_connection_statistics_unittest.cc:7: #include
"base/message_loop/message_loop.h"
On 2016/05/24 01:02:01, Kevin M wrote:
> Not used

Done.

https://codereview.chromium.org/1962393004/diff/120001/blimp/net/blimp_connec...
blimp/net/blimp_connection_statistics_unittest.cc:11: #include
"testing/gmock/include/gmock/gmock.h"
On 2016/05/24 01:02:01, Kevin M wrote:
> Not used

Done.

https://codereview.chromium.org/1962393004/diff/120001/blimp/net/blimp_connec...
blimp/net/blimp_connection_statistics_unittest.cc:25:
std::unique_ptr<BlimpConnectionStatistics> stats_;
On 2016/05/24 01:02:01, Kevin M wrote:
> YOu can make this a regular field - no need to make it a unique_ptr

Done.

https://codereview.chromium.org/1962393004/diff/120001/blimp/net/blimp_connec...
blimp/net/blimp_connection_statistics_unittest.cc:31:
TEST_F(BlimpConnectionStatisticsTest, AddStatsAndVerify) {
On 2016/05/24 01:02:01, Kevin M wrote:
> Also verify zeroes beforehand

Done.

https://codereview.chromium.org/1962393004/diff/120001/blimp/net/blimp_messag...
File blimp/net/blimp_message_pump.cc (right):

https://codereview.chromium.org/1962393004/diff/120001/blimp/net/blimp_messag...
blimp/net/blimp_message_pump.cc:29: DCHECK(reader_);
On 2016/05/24 01:02:01, Kevin M wrote:
> Not needed: constructor enforces non-nullness, and the following line will
> segfault if it *does* manage to be null


Removed function since we are passing in constructor

https://codereview.chromium.org/1962393004/diff/120001/blimp/net/blimp_messag...
File blimp/net/blimp_message_pump.h (right):

https://codereview.chromium.org/1962393004/diff/120001/blimp/net/blimp_messag...
blimp/net/blimp_message_pump.h:13: #include
"blimp/net/blimp_connection_statistics.h"
On 2016/05/24 01:02:01, Kevin M wrote:
> Add a prototype instead of an include in .h

Done.

https://codereview.chromium.org/1962393004/diff/120001/blimp/net/blimp_messag...
blimp/net/blimp_message_pump.h:15: #include "blimp/net/packet_reader.h"
On 2016/05/24 01:02:01, Kevin M wrote:
> Not used? We have a prototype on line 26.

Done.

https://codereview.chromium.org/1962393004/diff/120001/blimp/net/blimp_messag...
blimp/net/blimp_message_pump.h:49: void
SetBlimpConnectionStatistics(BlimpConnectionStatistics* statistics);
On 2016/05/24 01:02:01, Kevin M wrote:
> Add comment


Removed function since we are passing in constructor

https://codereview.chromium.org/1962393004/diff/120001/blimp/net/compressed_p...
File blimp/net/compressed_packet_reader.cc (right):

https://codereview.chromium.org/1962393004/diff/120001/blimp/net/compressed_p...
blimp/net/compressed_packet_reader.cc:96: if (blimp_connection_statistics_) {
On 2016/05/24 01:02:01, Kevin M wrote:
> This should be in stream_packet_reader/stream_packet_writer.

Done.

https://codereview.chromium.org/1962393004/diff/120001/blimp/net/compressed_p...
File blimp/net/compressed_packet_unittest.cc (right):

https://codereview.chromium.org/1962393004/diff/120001/blimp/net/compressed_p...
blimp/net/compressed_packet_unittest.cc:109:
std::unique_ptr<BlimpConnectionStatistics> blimp_connection_statistics_;
On 2016/05/24 01:02:01, Kevin M wrote:
> suggestion: make it a regular (non-unique_ptr) field?


Removed from this file

https://codereview.chromium.org/1962393004/diff/120001/blimp/net/compressed_p...
blimp/net/compressed_packet_unittest.cc:119:
EXPECT_TRUE(CheckRoundTrip("1234"));
On 2016/05/24 01:02:02, Kevin M wrote:
> Why duplicate this line?

Done.

https://codereview.chromium.org/1962393004/diff/120001/blimp/net/compressed_p...
blimp/net/compressed_packet_unittest.cc:128: EXPECT_EQ((int)compressed.size(),
On 2016/05/24 01:02:01, Kevin M wrote:
> nit: use static_cast<> instead of C-style cast.

Done.

https://codereview.chromium.org/1962393004/diff/120001/blimp/net/compressed_p...
blimp/net/compressed_packet_unittest.cc:132:
BlimpConnectionStatistics::BYTES_SENT));
On 2016/05/24 01:02:01, Kevin M wrote:
> This formatting looks a little off, run git cl format?

Done.

[Kevin M, 2016-05-24 21:49:01]

Almost there...

https://codereview.chromium.org/1962393004/diff/140001/blimp/net/blimp_connec...
File blimp/net/blimp_connection_statistics.cc (right):

https://codereview.chromium.org/1962393004/diff/140001/blimp/net/blimp_connec...
blimp/net/blimp_connection_statistics.cc:7: #include "base/logging.h"
Not used.

https://codereview.chromium.org/1962393004/diff/140001/blimp/net/blimp_connec...
File blimp/net/blimp_connection_statistics_unittest.cc (right):

https://codereview.chromium.org/1962393004/diff/140001/blimp/net/blimp_connec...
blimp/net/blimp_connection_statistics_unittest.cc:7: #include
"testing/gmock/include/gmock/gmock.h"
Not used

https://codereview.chromium.org/1962393004/diff/140001/blimp/net/blimp_connec...
blimp/net/blimp_connection_statistics_unittest.cc:10: using testing::_;
Not used

https://codereview.chromium.org/1962393004/diff/140001/blimp/net/ssl_client_t...
File blimp/net/ssl_client_transport_unittest.cc (right):

https://codereview.chromium.org/1962393004/diff/140001/blimp/net/ssl_client_t...
blimp/net/ssl_client_transport_unittest.cc:65: ip_endpoint, cert, new
BlimpConnectionStatistics(), &net_log_));
This leaks the statistics object, need to pass in an address of a statistics
field or the address of a unique pointer instead.

https://codereview.chromium.org/1962393004/diff/140001/blimp/net/stream_packe...
File blimp/net/stream_packet_reader.h (right):

https://codereview.chromium.org/1962393004/diff/140001/blimp/net/stream_packe...
blimp/net/stream_packet_reader.h:33: // is valid while the reader is in-use (see
ReadPacket below).
Document |statistics| and the lifetime requirement.

https://codereview.chromium.org/1962393004/diff/140001/blimp/net/stream_packe...
File blimp/net/stream_packet_writer.h (right):

https://codereview.chromium.org/1962393004/diff/140001/blimp/net/stream_packe...
blimp/net/stream_packet_writer.h:33: // is valid while the reader is in-use (see
ReadPacket below).
Document |statistics| and lifetime requirements

https://codereview.chromium.org/1962393004/diff/140001/blimp/net/stream_packe...
blimp/net/stream_packet_writer.h:34: explicit
StreamPacketWriter(net::StreamSocket* socket,
Remove "explicit" for >1 arg constructors, in here and in the other .h files
changes by this CL.

https://codereview.chromium.org/1962393004/diff/140001/blimp/net/tcp_engine_t...
File blimp/net/tcp_engine_transport.cc (right):

https://codereview.chromium.org/1962393004/diff/140001/blimp/net/tcp_engine_t...
blimp/net/tcp_engine_transport.cc:64: std::move(accepted_socket_), new
BlimpConnectionStatistics));
This leaks the statistics object.

Maybe consider making the statistics object refcounted. That takes care of any
potential ownership issues and gives you the ability to pass in a valid but
unused object such as the case we have here.

[shaktisahu, 2016-05-24 22:42:14]

https://codereview.chromium.org/1962393004/diff/140001/blimp/net/blimp_connec...
File blimp/net/blimp_connection_statistics.cc (right):

https://codereview.chromium.org/1962393004/diff/140001/blimp/net/blimp_connec...
blimp/net/blimp_connection_statistics.cc:7: #include "base/logging.h"
On 2016/05/24 21:49:00, Kevin M wrote:
> Not used.

Done.

https://codereview.chromium.org/1962393004/diff/140001/blimp/net/blimp_connec...
blimp/net/blimp_connection_statistics.cc:7: #include "base/logging.h"
On 2016/05/24 21:49:00, Kevin M wrote:
> Not used.

Done.

https://codereview.chromium.org/1962393004/diff/140001/blimp/net/blimp_connec...
File blimp/net/blimp_connection_statistics_unittest.cc (right):

https://codereview.chromium.org/1962393004/diff/140001/blimp/net/blimp_connec...
blimp/net/blimp_connection_statistics_unittest.cc:7: #include
"testing/gmock/include/gmock/gmock.h"
On 2016/05/24 21:49:00, Kevin M wrote:
> Not used

Done.

https://codereview.chromium.org/1962393004/diff/140001/blimp/net/blimp_connec...
blimp/net/blimp_connection_statistics_unittest.cc:10: using testing::_;
On 2016/05/24 21:49:00, Kevin M wrote:
> Not used

Done.

https://codereview.chromium.org/1962393004/diff/140001/blimp/net/ssl_client_t...
File blimp/net/ssl_client_transport_unittest.cc (right):

https://codereview.chromium.org/1962393004/diff/140001/blimp/net/ssl_client_t...
blimp/net/ssl_client_transport_unittest.cc:65: ip_endpoint, cert, new
BlimpConnectionStatistics(), &net_log_));
On 2016/05/24 21:49:00, Kevin M wrote:
> This leaks the statistics object, need to pass in an address of a statistics
> field or the address of a unique pointer instead.

Done.

https://codereview.chromium.org/1962393004/diff/140001/blimp/net/stream_packe...
File blimp/net/stream_packet_reader.h (right):

https://codereview.chromium.org/1962393004/diff/140001/blimp/net/stream_packe...
blimp/net/stream_packet_reader.h:33: // is valid while the reader is in-use (see
ReadPacket below).
On 2016/05/24 21:49:00, Kevin M wrote:
> Document |statistics| and the lifetime requirement.

Done.

https://codereview.chromium.org/1962393004/diff/140001/blimp/net/stream_packe...
File blimp/net/stream_packet_writer.h (right):

https://codereview.chromium.org/1962393004/diff/140001/blimp/net/stream_packe...
blimp/net/stream_packet_writer.h:34: explicit
StreamPacketWriter(net::StreamSocket* socket,
On 2016/05/24 21:49:01, Kevin M wrote:
> Remove "explicit" for >1 arg constructors, in here and in the other .h files
> changes by this CL.

Done.

[Kevin M, 2016-05-24 22:43:28]

The CQ bit was checked by kmarshall@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-24 22:44:04]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1962393004/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1962393004/160001

[shaktisahu, 2016-05-24 22:44:48]

The CQ bit was unchecked by shaktisahu@chromium.org

[Kevin M, 2016-05-24 22:56:40]

lgtm % a few changes

https://codereview.chromium.org/1962393004/diff/160001/blimp/client/app/andro...
File blimp/client/app/android/java/res/layout/debug_stats_overlay.xml (right):

https://codereview.chromium.org/1962393004/diff/160001/blimp/client/app/andro...
blimp/client/app/android/java/res/layout/debug_stats_overlay.xml:2: <!--
Copyright 2015 The Chromium Authors. All rights reserved.
2016

https://codereview.chromium.org/1962393004/diff/160001/blimp/client/app/andro...
File
blimp/client/app/android/java/src/org/chromium/blimp/BlimpRendererActivity.java
(right):

https://codereview.chromium.org/1962393004/diff/160001/blimp/client/app/andro...
blimp/client/app/android/java/src/org/chromium/blimp/BlimpRendererActivity.java:275:
* Displays debug metrics upto one decimal place.
up to

https://codereview.chromium.org/1962393004/diff/160001/blimp/client/session/b...
File blimp/client/session/blimp_client_session.cc (right):

https://codereview.chromium.org/1962393004/diff/160001/blimp/client/session/b...
blimp/client/session/blimp_client_session.cc:75: explicit
ClientNetworkComponents(
Not "explicit"

https://codereview.chromium.org/1962393004/diff/160001/blimp/client/session/b...
blimp/client/session/blimp_client_session.cc:77: BlimpConnectionStatistics*
blimp_connection_statistics);
Should take a unique_ptr<> if we are passing ownership.

https://codereview.chromium.org/1962393004/diff/160001/blimp/client/session/b...
blimp/client/session/blimp_client_session.cc:109:
connection_statistics_(statistics) {}
DCHECK(statistics)

https://codereview.chromium.org/1962393004/diff/160001/blimp/net/blimp_connec...
File blimp/net/blimp_connection_statistics.h (right):

https://codereview.chromium.org/1962393004/diff/160001/blimp/net/blimp_connec...
blimp/net/blimp_connection_statistics.h:32: // Accumulates values for a metrics.
a metric

https://codereview.chromium.org/1962393004/diff/160001/blimp/net/stream_packe...
File blimp/net/stream_packet_writer.cc (right):

https://codereview.chromium.org/1962393004/diff/160001/blimp/net/stream_packe...
blimp/net/stream_packet_writer.cc:48: DCHECK(socket_);
DCHECK(statistics_)

https://codereview.chromium.org/1962393004/diff/160001/blimp/net/stream_socke...
File blimp/net/stream_socket_connection.cc (right):

https://codereview.chromium.org/1962393004/diff/160001/blimp/net/stream_socke...
blimp/net/stream_socket_connection.cc:24: DCHECK(socket_);
DCHECK(statistics)

https://codereview.chromium.org/1962393004/diff/160001/blimp/net/stream_socke...
File blimp/net/stream_socket_connection.h (right):

https://codereview.chromium.org/1962393004/diff/160001/blimp/net/stream_socke...
blimp/net/stream_socket_connection.h:23: explicit
StreamSocketConnection(std::unique_ptr<net::StreamSocket> socket,
Remove "explicit"

https://codereview.chromium.org/1962393004/diff/160001/blimp/net/tcp_client_t...
File blimp/net/tcp_client_transport.cc (right):

https://codereview.chromium.org/1962393004/diff/160001/blimp/net/tcp_client_t...
blimp/net/tcp_client_transport.cc:27:
socket_factory_(net::ClientSocketFactory::GetDefaultFactory()) {}
DCHECK(statistics)

https://codereview.chromium.org/1962393004/diff/160001/blimp/net/tcp_engine_t...
File blimp/net/tcp_engine_transport.cc (right):

https://codereview.chromium.org/1962393004/diff/160001/blimp/net/tcp_engine_t...
blimp/net/tcp_engine_transport.cc:24: net_log_(net_log) {}
DCHECK(statistics)

[shaktisahu, 2016-05-24 23:22:51]

The CQ bit was checked by shaktisahu@chromium.org

[shaktisahu, 2016-05-24 23:22:52]

The patchset sent to the CQ was uploaded after l-g-t-m from vmpstr@chromium.org,
kmarshall@chromium.org
Link to the patchset: https://codereview.chromium.org/1962393004/#ps180001
(title: "Addressed nits")

[commit-bot: I haz the power, 2016-05-24 23:23:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1962393004/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1962393004/180001

[commit-bot: I haz the power, 2016-05-24 23:26:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-24 23:26:20]

Try jobs failed on following builders:
  ios-device on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
  ios-simulator on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[shaktisahu, 2016-05-25 00:16:54]

The CQ bit was checked by shaktisahu@chromium.org

[shaktisahu, 2016-05-25 00:16:54]

The patchset sent to the CQ was uploaded after l-g-t-m from vmpstr@chromium.org,
kmarshall@chromium.org
Link to the patchset: https://codereview.chromium.org/1962393004/#ps200001
(title: "rebase")

[commit-bot: I haz the power, 2016-05-25 00:17:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1962393004/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1962393004/200001

[commit-bot: I haz the power, 2016-05-25 00:40:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-25 00:40:19]

Try jobs failed on following builders:
  win_clang on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[shaktisahu, 2016-05-25 01:14:34]

The CQ bit was checked by shaktisahu@chromium.org

[shaktisahu, 2016-05-25 01:14:35]

The patchset sent to the CQ was uploaded after l-g-t-m from vmpstr@chromium.org,
kmarshall@chromium.org
Link to the patchset: https://codereview.chromium.org/1962393004/#ps220001
(title: "Added empty implementation for linux client")

[shaktisahu, 2016-05-25 01:14:38]

The CQ bit was unchecked by shaktisahu@chromium.org

[shaktisahu, 2016-05-25 01:17:25]

The CQ bit was checked by shaktisahu@chromium.org

[commit-bot: I haz the power, 2016-05-25 01:18:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1962393004/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1962393004/220001

[commit-bot: I haz the power, 2016-05-25 01:42:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-25 01:42:18]

Try jobs failed on following builders:
  win_clang on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[shaktisahu, 2016-05-25 17:19:17]

The CQ bit was checked by shaktisahu@chromium.org

[commit-bot: I haz the power, 2016-05-25 17:19:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1962393004/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1962393004/220001

[commit-bot: I haz the power, 2016-05-25 18:07:11]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2016-05-25 18:09:02]

Description was changed from

==========
Added a debug info UI for Blimp

Added a menu option in toolbar to show/hide debug info. When enabled,
an overlay debug UI would be shown on the bottom left of the screen
showing number of commits, and kilobytes sent and received by the client.
This data is reset when the user moves away or is redirected to a new
page.


BUG=594423
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Added a debug info UI for Blimp

Added a menu option in toolbar to show/hide debug info. When enabled,
an overlay debug UI would be shown on the bottom left of the screen
showing number of commits, and kilobytes sent and received by the client.
This data is reset when the user moves away or is redirected to a new
page.

BUG=594423
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/03e5b0d97aa1fc6e813725465142978896ed8641
Cr-Commit-Position: refs/heads/master@{#395931}
==========

[commit-bot: I haz the power, 2016-05-25 18:09:03]

Patchset 12 (id:??) landed as
https://crrev.com/03e5b0d97aa1fc6e813725465142978896ed8641
Cr-Commit-Position: refs/heads/master@{#395931}

[haibinlu, 2016-05-26 01:11:47]

A revert of this CL (patchset #12 id:220001) has been created in
https://codereview.chromium.org/2008283004/ by haibinlu@chromium.org.

The reason for reverting is: crbug.com/614888 broken blimp browsertest on fyi
bot.

.

[shaktisahu, 2016-05-26 22:28:33]

Description was changed from

==========
Added a debug info UI for Blimp

Added a menu option in toolbar to show/hide debug info. When enabled,
an overlay debug UI would be shown on the bottom left of the screen
showing number of commits, and kilobytes sent and received by the client.
This data is reset when the user moves away or is redirected to a new
page.

BUG=594423
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/03e5b0d97aa1fc6e813725465142978896ed8641
Cr-Commit-Position: refs/heads/master@{#395931}
==========

to

==========
Added a debug info UI for Blimp

Added a menu option in toolbar to show/hide debug info. When enabled,
an overlay debug UI would be shown on the bottom left of the screen
showing number of commits, and kilobytes sent and received by the client.
This data is reset when the user moves away or is redirected to a new
page.

BUG=594423
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/03e5b0d97aa1fc6e813725465142978896ed8641
Cr-Commit-Position: refs/heads/master@{#395931}
==========

[shaktisahu, 2016-05-26 22:28:51]

The CQ bit was checked by shaktisahu@chromium.org

[shaktisahu, 2016-05-26 22:28:52]

The patchset sent to the CQ was uploaded after l-g-t-m from vmpstr@chromium.org,
kmarshall@chromium.org
Link to the patchset: https://codereview.chromium.org/1962393004/#ps240001
(title: "Fixed code for browsertests")

[shaktisahu, 2016-05-26 22:28:52]

The CQ bit was unchecked by shaktisahu@chromium.org

[shaktisahu, 2016-05-26 22:45:25]

Patchset #13 (id:240001) has been deleted

[Kevin M, 2016-05-31 20:12:01]

Shakti, can we close this issue if https://codereview.chromium.org/2019463002
relanded it successfully?