Limit the number of captured events held by about:net-internals.

Limit the number of captured events held by about:net-internals.

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

[eroman, 2012-03-02 23:20:26]

Please only view the deltas from patchset 2.

(I based this off the earlier CL which has not yet landed)

Here is what it looks like:
http://i.imgur.com/Jn1Vs.png

I removed the italics help text since it felt too crowded having it for TWO
checkboxes.

I'll try to add unit-tests as a follow-up change!

[mmenke, 2012-03-02 23:28:06]

https://chromiumcodereview.appspot.com/9585027/diff/8001/chrome/browser/resou...
File chrome/browser/resources/net_internals/capture_view.html (right):

https://chromiumcodereview.appspot.com/9585027/diff/8001/chrome/browser/resou...
chrome/browser/resources/net_internals/capture_view.html:4: <input
id=capture-view-limit-checkbox type=checkbox checked />
I worry about making it enabled by default...I have images of someone looking
into a bug, just as an automatic refresh of news.google.com eats half his log.

[eroman, 2012-03-02 23:32:26]

https://chromiumcodereview.appspot.com/9585027/diff/8001/chrome/browser/resou...
File chrome/browser/resources/net_internals/capture_view.html (right):

https://chromiumcodereview.appspot.com/9585027/diff/8001/chrome/browser/resou...
chrome/browser/resources/net_internals/capture_view.html:4: <input
id=capture-view-limit-checkbox type=checkbox checked />
On 2012/03/02 23:28:06, Matt Menke wrote:
> I worry about making it enabled by default...I have images of someone looking
> into a bug, just as an automatic refresh of http://news.google.com eats half
his log.

Yeah I think you right. I did some more experiments and the variance between
number of events is pretty high.

I'll change it to un-checked, and we can recommend it for people doing
long-running experiments (making this kind of like a beefed up
PassiveLogCollector).

[eroman, 2012-03-02 23:36:07]

https://chromiumcodereview.appspot.com/9585027/diff/8001/chrome/browser/resou...
File chrome/browser/resources/net_internals/capture_view.html (right):

https://chromiumcodereview.appspot.com/9585027/diff/8001/chrome/browser/resou...
chrome/browser/resources/net_internals/capture_view.html:4: <input
id=capture-view-limit-checkbox type=checkbox checked />
On 2012/03/02 23:32:26, eroman wrote:
> On 2012/03/02 23:28:06, Matt Menke wrote:
> > I worry about making it enabled by default...I have images of someone
looking
> > into a bug, just as an automatic refresh of http://news.google.com eats half
> his log.
> 
> Yeah I think you right. I did some more experiments and the variance between
> number of events is pretty high.
> 
> I'll change it to un-checked, and we can recommend it for people doing
> long-running experiments (making this kind of like a beefed up
> PassiveLogCollector).

Done.

[eroman, 2012-03-02 23:38:13]

Over time we might also evolve this to filter less important events. For
instance the #bytes read/written, and the cache events might be a candidate for
removal under memory pressure.

[eroman, 2012-03-04 04:51:06]

rebased on head now.

[mmenke, 2012-03-06 03:39:55]

On 2012/03/04 04:51:06, eroman wrote:
> rebased on head now.

Sorry, forgot about this one.  I'll take a look tomorrow.

[mmenke, 2012-03-06 15:21:18]

http://codereview.chromium.org/9585027/diff/16001/chrome/browser/resources/ne...
File chrome/browser/resources/net_internals/capture_view.html (right):

http://codereview.chromium.org/9585027/diff/16001/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/capture_view.html:14: title='This will
result in huge log files, and can expose sensitive data'>
I don't think titles are sufficiently visible for this kind of information. 
They require a brief hover time.  Also, people will generally click the
checkbox, rather than the text, and they'll never see the text when hovering
over the checkbox.

http://codereview.chromium.org/9585027/diff/16001/chrome/browser/resources/ne...
File chrome/browser/resources/net_internals/events_tracker.js (right):

http://codereview.chromium.org/9585027/diff/16001/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/events_tracker.js:62: if
(this.getNumCapturedEvents() >  this.hardLimit_) {
nit:  Remove extra space.

[eroman, 2012-03-06 17:14:14]

http://codereview.chromium.org/9585027/diff/16001/chrome/browser/resources/ne...
File chrome/browser/resources/net_internals/capture_view.html (right):

http://codereview.chromium.org/9585027/diff/16001/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/capture_view.html:14: title='This will
result in huge log files, and can expose sensitive data'>
On 2012/03/06 15:21:18, Matt Menke wrote:
> I don't think titles are sufficiently visible for this kind of information. 
> They require a brief hover time.  Also, people will generally click the
> checkbox, rather than the text, and they'll never see the text when hovering
> over the checkbox.

Restored to italic. Here is what it looks like: http://i.imgur.com/gsLde.png

http://codereview.chromium.org/9585027/diff/16001/chrome/browser/resources/ne...
File chrome/browser/resources/net_internals/events_tracker.js (right):

http://codereview.chromium.org/9585027/diff/16001/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/events_tracker.js:62: if
(this.getNumCapturedEvents() >  this.hardLimit_) {
On 2012/03/06 15:21:18, Matt Menke wrote:
> nit:  Remove extra space.

Done.

[mmenke, 2012-03-06 17:15:59]

LGTM

[commit-bot: I haz the power, 2012-03-06 19:47:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/eroman@chromium.org/9585027/20001

[commit-bot: I haz the power, 2012-03-06 21:21:34]

Change committed as 125225