Reland: Added a debug info UI for Blimp

Reland: 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, 614888
TBR=vmpstr@chromium.org

Original CL: https://codereview.chromium.org/1962393004/
Revert CL: https://codereview.chromium.org/2008283004/
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/086802955ffbe67f0a2de987822ae8d18dd5353d
Cr-Commit-Position: refs/heads/master@{#396351}

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

Description was changed from

==========
Reland: 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

Original CL: https://codereview.chromium.org/1962393004/
Revert CL: https://codereview.chromium.org/2008283004/
==========

to

==========
Reland: 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

Original CL: https://codereview.chromium.org/1962393004/
Revert CL: https://codereview.chromium.org/2008283004/
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[shaktisahu, 2016-05-26 23:09:53]

Description was changed from

==========
Reland: 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

Original CL: https://codereview.chromium.org/1962393004/
Revert CL: https://codereview.chromium.org/2008283004/
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Reland: 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
TBR=vmpstr@chromium.org

Original CL: https://codereview.chromium.org/1962393004/
Revert CL: https://codereview.chromium.org/2008283004/
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[shaktisahu, 2016-05-26 23:09:54]

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

[shaktisahu, 2016-05-26 23:11:45]

kmarshall: PTAL at diff between patch set 1 and 2. Patch set is a clean revert
of the revert and patch set 2 contains the fix
vmpstr: TBR cc

[shaktisahu, 2016-05-26 23:11:48]

kmarshall: PTAL at diff between patch set 1 and 2. Patch set is a clean revert
of the revert and patch set 2 contains the fix
vmpstr: TBR cc

[shaktisahu, 2016-05-26 23:14:18]

Description was changed from

==========
Reland: 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
TBR=vmpstr@chromium.org

Original CL: https://codereview.chromium.org/1962393004/
Revert CL: https://codereview.chromium.org/2008283004/
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Reland: 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, 614888
TBR=vmpstr@chromium.org

Original CL: https://codereview.chromium.org/1962393004/
Revert CL: https://codereview.chromium.org/2008283004/
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[Kevin M, 2016-05-26 23:15:30]

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

https://codereview.chromium.org/2019463002/diff/20001/blimp/net/blimp_connect...
blimp/net/blimp_connection_statistics.h:41: // intentionally leaked at the
process termination.
???

Why do we leak them? Is there another task that cleans them up? If so, what is
it?

[Kevin M, 2016-05-26 23:55:01]

lgtm (see histogram.h:64)

[Kevin M, 2016-05-26 23:55:10]

The CQ bit was checked by kmarshall@chromium.org

[commit-bot: I haz the power, 2016-05-26 23:55:58]

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

[commit-bot: I haz the power, 2016-05-27 01:57:44]

Description was changed from

==========
Reland: 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, 614888
TBR=vmpstr@chromium.org

Original CL: https://codereview.chromium.org/1962393004/
Revert CL: https://codereview.chromium.org/2008283004/
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Reland: 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, 614888
TBR=vmpstr@chromium.org

Original CL: https://codereview.chromium.org/1962393004/
Revert CL: https://codereview.chromium.org/2008283004/
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[commit-bot: I haz the power, 2016-05-27 01:57:45]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-05-27 02:00:18]

Description was changed from

==========
Reland: 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, 614888
TBR=vmpstr@chromium.org

Original CL: https://codereview.chromium.org/1962393004/
Revert CL: https://codereview.chromium.org/2008283004/
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Reland: 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, 614888
TBR=vmpstr@chromium.org

Original CL: https://codereview.chromium.org/1962393004/
Revert CL: https://codereview.chromium.org/2008283004/
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/086802955ffbe67f0a2de987822ae8d18dd5353d
Cr-Commit-Position: refs/heads/master@{#396351}
==========

[commit-bot: I haz the power, 2016-05-27 02:00:20]

Patchset 2 (id:??) landed as
https://crrev.com/086802955ffbe67f0a2de987822ae8d18dd5353d
Cr-Commit-Position: refs/heads/master@{#396351}

[Wez, 2016-06-07 00:30:11]

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

[Wez, 2016-06-07 00:30:13]

Belated drive-by.  tl;dr: I don't think we need BlimpConnectionStatistics, and
I'm concerned about the assertion of thread-safety.

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

https://codereview.chromium.org/2019463002/diff/20001/blimp/client/app/androi...
blimp/client/app/android/blimp_client_session_android.cc:125:
stats->Get(BlimpConnectionStatistics::COMMIT)};
As noted in the implementation, you're creating three separate histogram
snapshots to query from - IIUC all you actually want to do is have
GetBlimpConnectionStatistics() return a snapshot that you can query instead.

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

https://codereview.chromium.org/2019463002/diff/20001/blimp/net/blimp_connect...
blimp/net/blimp_connection_statistics.cc:23: return snapshot->GetCount(type);
This doesn't look right; surely the whole point of snapshotting a histogram is
so you can grab the values from that snapshot to get a consistent view?

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

https://codereview.chromium.org/2019463002/diff/20001/blimp/net/blimp_connect...
blimp/net/blimp_connection_statistics.h:19: // it. This class is thread-safe.
I can't find anything in SparseHistogram that actually says it's thread-safe. 
It's also not clear that you need thread-safety - we only update the network
statistics on the network I/O thread, so provided we can cope with fetching
statistics asynchronously, we can have the histograms updated on that thread and
use PostTaskAndReply() to request a snapshot from which to get the values we
want.

AFAICT this class just creates SparseHistogram with specific flags, and defines
three event types - can't we do just do that directly?

https://codereview.chromium.org/2019463002/diff/20001/blimp/net/blimp_connect...
blimp/net/blimp_connection_statistics.h:41: // intentionally leaked at the
process termination.
On 2016/05/26 23:15:30, Kevin M wrote:
> ???
> 
> Why do we leak them? Is there another task that cleans them up? If so, what is
> it?

Did this ever get answered?