Adds the ability to load JSON log files to about:net-internals.

Adds the ability to load JSON log files to about:net-internals.
Only works with logs created by the new "--log-net-log=file",
which writes NetLog events to the specified file, regardless
of other logging command line parameters.

Using "--log-net-log" without a file name will just write
NetLog events to VLOG(1), as before.

BUG=63687
TEST=None


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

[mmenke, 2011-01-06 21:47:41]

Just wanted to get your take on this.  Some images of the data tab, before and
after loading: 
https://docs0.google.com/document/d/1vIW1DTyriwFN3b8JBp3iZe7TU97CRpTI61qdgNWz...

The read file code is a little funky, but don't see a simpler sane way to do it,
other than just reading the file on the UI thread.  Not sure if you noticed my
comment on the corresponding bug, but the Javascript file I/O functions don't
seem to work, due to security issues.

[eroman, 2011-01-12 05:57:34]

Neat, sorry for taking a while to get to this!

I gave a quick first pass, will give it a more detailed look-over when done
sherifing tommorow.

The overall approach seems reasonable, thanks for the screenshots.

It is really unfortunate we can't do something better for the file reading.

http://codereview.chromium.org/6025017/diff/47001/chrome/browser/net/net_log_...
File chrome/browser/net/net_log_logger.cc (right):

http://codereview.chromium.org/6025017/diff/47001/chrome/browser/net/net_log_...
chrome/browser/net/net_log_logger.cc:34: base::JSONWriter::Write(value.get(),
true, &json);
I don't think we should do pretty printing anymore.

How about simply doing one per line in both cases?

(having it one per line makes it easier to dump inline in the log, then extract
it from the output and feed it into the net-internals import to visualize in the
same manner.)

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

http://codereview.chromium.org/6025017/diff/47001/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/main.js:316:
BrowserBridge.prototype.loadLogFile = function(logLevel) {
the parameter here is unused.

http://codereview.chromium.org/6025017/diff/47001/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/main.js:468: err = err;
is this line intended to be a no-op?

Perhaps we should console.log it in case need to debug.

http://codereview.chromium.org/6025017/diff/47001/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/main.js:483:
entries[i].wasPassivelyCaptured = true;
any reason to initialize this to true rather than false?

[mmenke, 2011-01-13 03:45:29]

On 2011/01/12 05:57:34, eroman wrote:
> Neat, sorry for taking a while to get to this!

Not a problem.  I've been rather busy this week (And I'm sheriffing the next two
days), so not a big deal.

> It is really unfortunate we can't do something better for the file reading.

Yea, I'd much prefer to do it all in Javascript, too.

http://codereview.chromium.org/6025017/diff/47001/chrome/browser/net/net_log_...
File chrome/browser/net/net_log_logger.cc (right):

http://codereview.chromium.org/6025017/diff/47001/chrome/browser/net/net_log_...
chrome/browser/net/net_log_logger.cc:34: base::JSONWriter::Write(value.get(),
true, &json);
On 2011/01/12 05:57:34, eroman wrote:
> I don't think we should do pretty printing anymore.
> 
> How about simply doing one per line in both cases?
> 
> (having it one per line makes it easier to dump inline in the log, then
extract
> it from the output and feed it into the net-internals import to visualize in
the
> same manner.)

Sounds reasonable.  Done.

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

http://codereview.chromium.org/6025017/diff/47001/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/main.js:316:
BrowserBridge.prototype.loadLogFile = function(logLevel) {
On 2011/01/12 05:57:34, eroman wrote:
> the parameter here is unused.

Oops...Removed.

http://codereview.chromium.org/6025017/diff/47001/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/main.js:468: err = err;
On 2011/01/12 05:57:34, eroman wrote:
> is this line intended to be a no-op?
> 
> Perhaps we should console.log it in case need to debug.

Forgot to remove that line.  Was using it as a break point while debugging.

Blank lines make us end up here.  We get blank lines at the end of the file, and
from any CRLF we split earlier.

Logging non-blank lines sounds like a reasonable idea.  Done (Logging lines that
are valid JSON but aren't valid log lines, too).

http://codereview.chromium.org/6025017/diff/47001/chrome/browser/resources/ne...
chrome/browser/resources/net_internals/main.js:483:
entries[i].wasPassivelyCaptured = true;
On 2011/01/12 05:57:34, eroman wrote:
> any reason to initialize this to true rather than false?

I was just thinking the (P) emphasizes that we're not actively logging anything.
 Loading a log file seems more like getting events from the passive log than
displaying actively logged events as they come in, to me.

If you think it's better to emphasize that the events in the log files more
closely resemble active logs, or think this isn't the way to go for some other
reason, I'm happy to remove the line.  I don't have strong feelings about it,
either way.

[eroman, 2011-01-24 20:45:55]

LGTM.
So sorry it took so long to finish this review!

http://codereview.chromium.org/6025017/diff/56002/chrome/browser/dom_ui/net_i...
File chrome/browser/dom_ui/net_internals_ui.cc (right):

http://codereview.chromium.org/6025017/diff/56002/chrome/browser/dom_ui/net_i...
chrome/browser/dom_ui/net_internals_ui.cc:472:
dom_ui_->RegisterMessageCallback("loadLogFile",
nit: can you break the line here, for consistency with the other
RegisterMessageCallback() lines?

http://codereview.chromium.org/6025017/diff/56002/chrome/browser/net/net_log_...
File chrome/browser/net/net_log_logger.h (right):

http://codereview.chromium.org/6025017/diff/56002/chrome/browser/net/net_log_...
chrome/browser/net/net_log_logger.h:22: // If |log_path| is empty or file
creation fails, writes to VLOG(1) using
I think this comment needs to be updated, no longer pretty-printing.

[mmenke, 2011-01-25 20:08:13]

Thanks.

http://codereview.chromium.org/6025017/diff/56002/chrome/browser/dom_ui/net_i...
File chrome/browser/dom_ui/net_internals_ui.cc (right):

http://codereview.chromium.org/6025017/diff/56002/chrome/browser/dom_ui/net_i...
chrome/browser/dom_ui/net_internals_ui.cc:472:
dom_ui_->RegisterMessageCallback("loadLogFile",
On 2011/01/24 20:45:56, eroman wrote:
> nit: can you break the line here, for consistency with the other
> RegisterMessageCallback() lines?

Done.  It also violated Google style guidelines.

http://codereview.chromium.org/6025017/diff/56002/chrome/browser/net/net_log_...
File chrome/browser/net/net_log_logger.h (right):

http://codereview.chromium.org/6025017/diff/56002/chrome/browser/net/net_log_...
chrome/browser/net/net_log_logger.h:22: // If |log_path| is empty or file
creation fails, writes to VLOG(1) using
On 2011/01/24 20:45:56, eroman wrote:
> I think this comment needs to be updated, no longer pretty-printing.

Done.

[eroman, 2011-01-25 20:09:46]

LGTM

On Tue, Jan 25, 2011 at 12:08 PM, <mmenke@chromium.org> wrote:

> Thanks.
>
>
>
>
>
http://codereview.chromium.org/6025017/diff/56002/chrome/browser/dom_ui/net_i...
> File chrome/browser/dom_ui/net_internals_ui.cc (right):
>
>
>
http://codereview.chromium.org/6025017/diff/56002/chrome/browser/dom_ui/net_i...
> chrome/browser/dom_ui/net_internals_ui.cc:472:
> dom_ui_->RegisterMessageCallback("loadLogFile",
> On 2011/01/24 20:45:56, eroman wrote:
>
>> nit: can you break the line here, for consistency with the other
>> RegisterMessageCallback() lines?
>>
>
> Done.  It also violated Google style guidelines.
>
>
>
>
http://codereview.chromium.org/6025017/diff/56002/chrome/browser/net/net_log_...
> File chrome/browser/net/net_log_logger.h (right):
>
>
>
http://codereview.chromium.org/6025017/diff/56002/chrome/browser/net/net_log_...
> chrome/browser/net/net_log_logger.h:22: // If |log_path| is empty or
> file creation fails, writes to VLOG(1) using
> On 2011/01/24 20:45:56, eroman wrote:
>
>> I think this comment needs to be updated, no longer pretty-printing.
>>
>
> Done.
>
>
> http://codereview.chromium.org/6025017/
>