Add a timeline view to about:net-internals.

Add a timeline view to about:net-internals.

It shows a graph of open/active sockets, open URL requests,
open DNS requests, up/down bandwidth, and disk cache
bandwidth.

BUG=99386
TEST=NetInternalsTest.NetInternalsTimelineView*

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=110553

[mmenke, 2011-11-14 19:33:01]

Take all the time you want reviewing this.  I've tried to keep it simple, but
it's still kinda big and complicated, and there's no rush.

[eroman, 2011-11-15 19:40:34]

excellent, i plan on reviewing this today!

On Mon, Nov 14, 2011 at 11:33 AM, <mmenke@chromium.org> wrote:

> Reviewers: eroman,
>
> Message:
> Take all the time you want reviewing this.  I've tried to keep it simple,
> but
> it's still kinda big and complicated, and there's no rush.
>
> Description:
> Add a timeline view to about:net-internals.
>
> It shows a graph of open/active sockets, open URL requests,
> open DNS requests, up/down bandwidth, and disk cache
> bandwidth.
>
> BUG=99386
> TEST=NetInternalsTest.**NetInternalsTimelineView*
>
> Please review this at
http://codereview.chromium.**org/8474001/<http://codereview.chromium.org/8474...
>
> SVN Base:
svn://svn.chromium.org/chrome/**trunk/src/<http://svn.chromium.org/chrome/trunk/src/>
>
> Affected files:
>  M     chrome/browser/resources/net_**internals/capture_view.js
>  M     chrome/browser/resources/net_**internals/category_tabs.html
>  M     chrome/browser/resources/net_**internals/details_view.js
>  M     chrome/browser/resources/net_**internals/dns_view.js
>  M     chrome/browser/resources/net_**internals/events_view.css
>  M     chrome/browser/resources/net_**internals/events_view.html
>  M     chrome/browser/resources/net_**internals/events_view.js
>  A     chrome/browser/resources/net_**internals/horizontal_**
> scrollbar_view.js
>  M     chrome/browser/resources/net_**internals/index.html
>  M     chrome/browser/resources/net_**internals/index.js
>  M     chrome/browser/resources/net_**internals/main.css
>  M     chrome/browser/resources/net_**internals/main.js
>  M     chrome/browser/resources/net_**internals/proxy_view.js
>  M     chrome/browser/resources/net_**internals/source_tracker.js
>  A     chrome/browser/resources/net_**internals/timeline_data_**series.js
>  A     chrome/browser/resources/net_**internals/timeline_graph_view.**js
>  A     chrome/browser/resources/net_**internals/timeline_view.css
>  A     chrome/browser/resources/net_**internals/timeline_view.html
>  A     chrome/browser/resources/net_**internals/timeline_view.js
>  D     chrome/browser/resources/net_**internals/timeline_view_**painter.js
>  M     chrome/browser/resources/net_**internals/top_mid_bottom_view.**js
>  M     chrome/browser/resources/net_**internals/view.js
>  M     chrome/browser/ui/webui/net_**internals_ui_browsertest.cc
>  M     chrome/test/data/webui/net_**internals/log_util.js
>  M     chrome/test/data/webui/net_**internals/main.js
>  M     chrome/test/data/webui/net_**internals/net_internals_test.**js
>  A     chrome/test/data/webui/net_**internals/timeline_view.js
>
>
>

[eroman, 2011-11-16 04:03:45]

LGTM.

cool!

Thats an impressive amount of code. I did my best to look through it at a high
level, but didn't give a fine-grained pass over everything... I did of course
find the time to provide some nits :)

http://codereview.chromium.org/8474001/diff/21029/chrome/browser/resources/ne...
File chrome/browser/resources/net_internals/details_view.js (right):

http://codereview.chromium.org/8474001/diff/21029/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/details_view.js:12: * The DetailsView is
displays the "log" view. This class keeps track of the
wording: "is displays" --> "displays".

http://codereview.chromium.org/8474001/diff/21029/chrome/browser/resources/ne...
File chrome/browser/resources/net_internals/horizontal_scrollbar_view.js
(right):

http://codereview.chromium.org/8474001/diff/21029/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/horizontal_scrollbar_view.js:23: function
HorizontalScrollbarView(div, innerDiv, callback) {
nit: innerDiv --> innerDivId; div --> divId

http://codereview.chromium.org/8474001/diff/21029/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/horizontal_scrollbar_view.js:88: // we
were just call from the timer, in which case clearTimeout will
nit: "just call from" --> "just called from".

http://codereview.chromium.org/8474001/diff/21029/chrome/browser/resources/ne...
File chrome/browser/resources/net_internals/main.js (right):

http://codereview.chromium.org/8474001/diff/21029/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/main.js:153: }
nit: add a semicolon.

http://codereview.chromium.org/8474001/diff/21029/chrome/browser/resources/ne...
File chrome/browser/resources/net_internals/source_tracker.js (right):

http://codereview.chromium.org/8474001/diff/21029/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/source_tracker.js:229: * security
stripping changes through:
nit: "through" seems unnecessary, can probably remove it.

http://codereview.chromium.org/8474001/diff/21029/chrome/browser/resources/ne...
File chrome/browser/resources/net_internals/timeline_data_series.js (right):

http://codereview.chromium.org/8474001/diff/21029/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/timeline_data_series.js:62: color:
function() {
nit: for consistency, I suggest calling this getColor()

http://codereview.chromium.org/8474001/diff/21029/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/timeline_data_series.js:70: dataType:
function() {
nit: for consistency I suggest calling this getDataType()

http://codereview.chromium.org/8474001/diff/21029/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/timeline_data_series.js:268: *
TODO(mmenke): Do something better.
I have been having a hard time figuring exactly how the bytes/second stuff
should work too. Maybe as we get some more experiencing using it we will have
other ideas. My suspicion is we may want to average values over a wider span of
time, since the instantaneous bandwidth is going to be very spikey.

http://codereview.chromium.org/8474001/diff/21029/chrome/browser/resources/ne...
File chrome/browser/resources/net_internals/timeline_graph_view.js (right):

http://codereview.chromium.org/8474001/diff/21029/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/timeline_graph_view.js:10: // We inherit
from DivView.
comment out of sync with reality.

http://codereview.chromium.org/8474001/diff/21029/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/timeline_graph_view.js:13: // Default
surrent scale factor, in terms of milliseconds per pixel.
"surrent" ?

http://codereview.chromium.org/8474001/diff/21029/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/timeline_graph_view.js:21: // Horizintal
space between two horitonally placed labels along the bottom
typo horizontal

http://codereview.chromium.org/8474001/diff/21029/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/timeline_graph_view.js:110: +
parseInt(style.paddingLeft);
nit: I recommend putting the + on the preceding line, since I am always afraid
of javscript's auto-semicolon nonsense now.

(To see what I mean, check out:
https://plus.sandbox.google.com/u/0/113677733573072861809/posts/evoSCXjGYYB)

http://codereview.chromium.org/8474001/diff/21029/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/timeline_graph_view.js:129:
this.canvas_.getContext("2d").font = getComputedStyle(this.canvas_).font;
nit: '2d' for consistency with the other strings.

.. Sorry, I couldn't resist :-)

I feel like I need to be writing _some_ comments for this monster change, so if
it isn't going to be about the substance, it may as well be inconsequential
style nits.

http://codereview.chromium.org/8474001/diff/21029/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/timeline_graph_view.js:194: this.endTime_
= (new Date()).getTime();
I wander if we have a helper function for this, seem like a common idiom.

http://codereview.chromium.org/8474001/diff/21029/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/timeline_graph_view.js:254:
MOUSE_WHEEL_SCROLL_RATE * -event.wheelDeltaX / 120);
nit: I noticed the 120 constant in a couple places, is it worth extracting to a
variable?

http://codereview.chromium.org/8474001/diff/21029/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/timeline_graph_view.js:285: // Simplest
jsut to recreate the Graphs.
typo

http://codereview.chromium.org/8474001/diff/21029/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/timeline_graph_view.js:307: var context =
this.canvas_.getContext("2d");
'2d'

http://codereview.chromium.org/8474001/diff/21029/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/timeline_graph_view.js:314: var
fontHeight = parseInt(context.font.match(/[0-9]+px/));
I don't quite understand this --> won't font.match() return an array?

Feels like there should be a capturing group involved, and then you want to
return the match[1] (first capturing group).

http://codereview.chromium.org/8474001/diff/21029/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/timeline_graph_view.js:375: +
LABEL_LABEL_HORIZONTAL_SPACING;
I recommend putting the plus on the preceding line.

http://codereview.chromium.org/8474001/diff/21029/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/timeline_graph_view.js:529: var units =
['B/s', 'kB/s', 'MB/s', 'GB/s', 'TB/s', 'PB/s'];
hehe, somehow I don't think we'll ever be using the PB/s or TB/s labels.... ^.^

http://codereview.chromium.org/8474001/diff/21029/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/timeline_graph_view.js:555: * difference
between two adjacent labels is 10^-|maxDecimalDigits|.
something is wrong with this sentence.

http://codereview.chromium.org/8474001/diff/21029/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/timeline_graph_view.js:584: if
(Math.ceil(maxValue / stepSize) + 1 <= maxLabels)
I'll be honest, I've kinda tuned out here... (no action required)

http://codereview.chromium.org/8474001/diff/21029/chrome/browser/resources/ne...
File chrome/browser/resources/net_internals/timeline_view.js (right):

http://codereview.chromium.org/8474001/diff/21029/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/timeline_view.js:13: // We inherit from
DivView.
comment differs from reality

http://codereview.chromium.org/8474001/diff/21029/chrome/browser/resources/ne...
File chrome/browser/resources/net_internals/top_mid_bottom_view.js (right):

http://codereview.chromium.org/8474001/diff/21029/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/top_mid_bottom_view.js:51: var
topbarHeight = 0;
so the idea here is to make the subviews optional?

http://codereview.chromium.org/8474001/diff/21029/chrome/test/data/webui/net_...
File chrome/test/data/webui/net_internals/net_internals_test.js (right):

http://codereview.chromium.org/8474001/diff/21029/chrome/test/data/webui/net_...
chrome/test/data/webui/net_internals/net_internals_test.js:412: };
nit: no need for semicolon.

http://codereview.chromium.org/8474001/diff/21029/chrome/test/data/webui/net_...
File chrome/test/data/webui/net_internals/timeline_view.js (right):

http://codereview.chromium.org/8474001/diff/21029/chrome/test/data/webui/net_...
chrome/test/data/webui/net_internals/timeline_view.js:159: function
mouseZoomOut() {
nice with the testing!

[mmenke, 2011-11-16 18:39:35]

Thanks so much for the review.  I'm impressed you got to it so quickly.

http://codereview.chromium.org/8474001/diff/21029/chrome/browser/resources/ne...
File chrome/browser/resources/net_internals/details_view.js (right):

http://codereview.chromium.org/8474001/diff/21029/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/details_view.js:12: * The DetailsView is
displays the "log" view. This class keeps track of the
On 2011/11/16 04:03:46, eroman wrote:
> wording: "is displays" --> "displays".

Done.

http://codereview.chromium.org/8474001/diff/21029/chrome/browser/resources/ne...
File chrome/browser/resources/net_internals/horizontal_scrollbar_view.js
(right):

http://codereview.chromium.org/8474001/diff/21029/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/horizontal_scrollbar_view.js:23: function
HorizontalScrollbarView(div, innerDiv, callback) {
On 2011/11/16 04:03:46, eroman wrote:
> nit: innerDiv --> innerDivId; div --> divId

Done.

http://codereview.chromium.org/8474001/diff/21029/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/horizontal_scrollbar_view.js:88: // we
were just call from the timer, in which case clearTimeout will
On 2011/11/16 04:03:46, eroman wrote:
> nit: "just call from" --> "just called from".

Done.

http://codereview.chromium.org/8474001/diff/21029/chrome/browser/resources/ne...
File chrome/browser/resources/net_internals/main.js (right):

http://codereview.chromium.org/8474001/diff/21029/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/main.js:153: }
On 2011/11/16 04:03:46, eroman wrote:
> nit: add a semicolon.

Done.

http://codereview.chromium.org/8474001/diff/21029/chrome/browser/resources/ne...
File chrome/browser/resources/net_internals/source_tracker.js (right):

http://codereview.chromium.org/8474001/diff/21029/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/source_tracker.js:229: * security
stripping changes through:
On 2011/11/16 04:03:46, eroman wrote:
> nit: "through" seems unnecessary, can probably remove it.

Done.

http://codereview.chromium.org/8474001/diff/21029/chrome/browser/resources/ne...
File chrome/browser/resources/net_internals/timeline_data_series.js (right):

http://codereview.chromium.org/8474001/diff/21029/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/timeline_data_series.js:62: color:
function() {
On 2011/11/16 04:03:46, eroman wrote:
> nit: for consistency, I suggest calling this getColor()

Done.

http://codereview.chromium.org/8474001/diff/21029/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/timeline_data_series.js:70: dataType:
function() {
On 2011/11/16 04:03:46, eroman wrote:
> nit: for consistency I suggest calling this getDataType()

Done.

http://codereview.chromium.org/8474001/diff/21029/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/timeline_data_series.js:268: *
TODO(mmenke): Do something better.
On 2011/11/16 04:03:46, eroman wrote:
> I have been having a hard time figuring exactly how the bytes/second stuff
> should work too. Maybe as we get some more experiencing using it we will have
> other ideas. My suspicion is we may want to average values over a wider span
of
> time, since the instantaneous bandwidth is going to be very spikey.

Averaging over a time range is probably the right thing to do, the question is
which time range, and how it should vary relative to stepSize.  Simplest just to
pick some arbitrary value - average over 5 steps in either direction or
something, but could see that not working well at different zoom levels.

We should probably use a weighted average for prettiest results.

http://codereview.chromium.org/8474001/diff/21029/chrome/browser/resources/ne...
File chrome/browser/resources/net_internals/timeline_graph_view.js (right):

http://codereview.chromium.org/8474001/diff/21029/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/timeline_graph_view.js:10: // We inherit
from DivView.
On 2011/11/16 04:03:46, eroman wrote:
> comment out of sync with reality.

Done, though never really cared much for reality.

http://codereview.chromium.org/8474001/diff/21029/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/timeline_graph_view.js:13: // Default
surrent scale factor, in terms of milliseconds per pixel.
On 2011/11/16 04:03:46, eroman wrote:
> "surrent" ?

Done.  Too much comment copy+pasting.

http://codereview.chromium.org/8474001/diff/21029/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/timeline_graph_view.js:21: // Horizintal
space between two horitonally placed labels along the bottom
On 2011/11/16 04:03:46, eroman wrote:
> typo horizontal

Done.

http://codereview.chromium.org/8474001/diff/21029/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/timeline_graph_view.js:110: +
parseInt(style.paddingLeft);
On 2011/11/16 04:03:46, eroman wrote:
> nit: I recommend putting the + on the preceding line, since I am always afraid
> of javscript's auto-semicolon nonsense now.
> 
> (To see what I mean, check out:
> https://plus.sandbox.google.com/u/0/113677733573072861809/posts/evoSCXjGYYB)

Done.  That's what I normally do, but switched since I think it just looks wrong
with minuses.  Your reasoning wins out, though.  Also, the lines with the
wrapping minuses have since been refactored out of existence, anyways.

http://codereview.chromium.org/8474001/diff/21029/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/timeline_graph_view.js:129:
this.canvas_.getContext("2d").font = getComputedStyle(this.canvas_).font;
On 2011/11/16 04:03:46, eroman wrote:
> nit: '2d' for consistency with the other strings.
> 
> .. Sorry, I couldn't resist :-)
> 
> I feel like I need to be writing _some_ comments for this monster change, so
if
> it isn't going to be about the substance, it may as well be inconsequential
> style nits.

Done.  I don't mind, I know the feeling.

http://codereview.chromium.org/8474001/diff/21029/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/timeline_graph_view.js:194: this.endTime_
= (new Date()).getTime();
On 2011/11/16 04:03:46, eroman wrote:
> I wander if we have a helper function for this, seem like a common idiom.

Sounds good to me.  I've added timeutil.getCurrentTime().  Amausingly, it's
longer than inlining it everywhere, but it is clearer.

http://codereview.chromium.org/8474001/diff/21029/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/timeline_graph_view.js:254:
MOUSE_WHEEL_SCROLL_RATE * -event.wheelDeltaX / 120);
On 2011/11/16 04:03:46, eroman wrote:
> nit: I noticed the 120 constant in a couple places, is it worth extracting to
a
> variable?

Done.

http://codereview.chromium.org/8474001/diff/21029/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/timeline_graph_view.js:285: // Simplest
jsut to recreate the Graphs.
On 2011/11/16 04:03:46, eroman wrote:
> typo

Done.

http://codereview.chromium.org/8474001/diff/21029/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/timeline_graph_view.js:307: var context =
this.canvas_.getContext("2d");
On 2011/11/16 04:03:46, eroman wrote:
> '2d'

Done.

http://codereview.chromium.org/8474001/diff/21029/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/timeline_graph_view.js:314: var
fontHeight = parseInt(context.font.match(/[0-9]+px/));
On 2011/11/16 04:03:46, eroman wrote:
> I don't quite understand this --> won't font.match() return an array?
> 
> Feels like there should be a capturing group involved, and then you want to
> return the match[1] (first capturing group).

You're right.  My code actually works, probably because ['10px'].toString() is
'10px', but that's just weird.  Fixed.

http://codereview.chromium.org/8474001/diff/21029/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/timeline_graph_view.js:375: +
LABEL_LABEL_HORIZONTAL_SPACING;
On 2011/11/16 04:03:46, eroman wrote:
> I recommend putting the plus on the preceding line.

Done.

http://codereview.chromium.org/8474001/diff/21029/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/timeline_graph_view.js:529: var units =
['B/s', 'kB/s', 'MB/s', 'GB/s', 'TB/s', 'PB/s'];
On 2011/11/16 04:03:46, eroman wrote:
> hehe, somehow I don't think we'll ever be using the PB/s or TB/s labels....
^.^

I figure it's best to have to many than too few.  My idea of future-proofing is
go as high as is reasonable...Then tack on 3 orders of magnitude.

Though now that you mention it, since reads are generally no more than 32k, and
v8 has a memory limit of...what... 400MB?  800MB?  I think you may be right.  Of
course, if it weren't for the memory limit...  :)

http://codereview.chromium.org/8474001/diff/21029/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/timeline_graph_view.js:555: * difference
between two adjacent labels is 10^-|maxDecimalDigits|.
On 2011/11/16 04:03:46, eroman wrote:
> something is wrong with this sentence.

Done.

http://codereview.chromium.org/8474001/diff/21029/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/timeline_graph_view.js:584: if
(Math.ceil(maxValue / stepSize) + 1 <= maxLabels)
On 2011/11/16 04:03:46, eroman wrote:
> I'll be honest, I've kinda tuned out here... (no action required)

I've added some comments.  It's very uninteresting code.  You didn't miss much.

I'd really like to be able to combine the two places where I decide on label
step sizes, but because of the rounding here, and funky step sizes needed in the
other one, it may be more effort than it's worth.

http://codereview.chromium.org/8474001/diff/21029/chrome/browser/resources/ne...
File chrome/browser/resources/net_internals/timeline_view.js (right):

http://codereview.chromium.org/8474001/diff/21029/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/timeline_view.js:13: // We inherit from
DivView.
On 2011/11/16 04:03:46, eroman wrote:
> comment differs from reality

Done.

http://codereview.chromium.org/8474001/diff/21029/chrome/browser/resources/ne...
File chrome/browser/resources/net_internals/top_mid_bottom_view.js (right):

http://codereview.chromium.org/8474001/diff/21029/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/top_mid_bottom_view.js:51: var
topbarHeight = 0;
On 2011/11/16 04:03:46, eroman wrote:
> so the idea here is to make the subviews optional?

Yea.  I'm actually thinking we may want to allow multiple subviews at both the
top and bottom, which will allow use to have some sort of details pane at the
bottom.  Could also make a horizontal splitter view instead.

http://codereview.chromium.org/8474001/diff/21029/chrome/test/data/webui/net_...
File chrome/test/data/webui/net_internals/net_internals_test.js (right):

http://codereview.chromium.org/8474001/diff/21029/chrome/test/data/webui/net_...
chrome/test/data/webui/net_internals/net_internals_test.js:412: };
On 2011/11/16 04:03:46, eroman wrote:
> nit: no need for semicolon.

Done.

[mmenke, 2011-11-16 22:12:02]

Unless you have any objections / want to look over it some more, I'm going to go
over the code carefully once or twice more, focusing particularly on comments,
beat on it a bit more, and then land it tomorrow.  Know you LGTMed it, I'm just
sufficiently paranoid about big CLs that I feel the need to give a warning.

[commit-bot: I haz the power, 2011-11-17 19:21:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mmenke@chromium.org/8474001/32051

[commit-bot: I haz the power, 2011-11-17 21:18:15]

Change committed as 110553