Save net-internals log dumps directly to disk.

Save net-internals log dumps directly to disk.

BUG=86497
TEST=manual

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

[mmenke, 2011-06-15 22:19:58]

I'm not a big fan of having to create a local file and then download it, but
none of the other approaches are great, either.

The only other two ways I know of to do it using Javascript are with a data URL
(Which gives us length limits, and a name of "Download") and creating a Blob URL
without using the file API, which gives us an absolutely hideous GUID as a file
name, without an extension ("48c9d939-bcba-4cc1-8985-3b67efa87e7a").

Another option is to use a DataSource for the dumps, like the WebUI pages
themselves do.  I have a working implementation of this, but there are two
issues:

1)  We have to stream the data in a bunch of IPCs.

2)  We either have to create a new DataSource for every download, or we have to
be able to have the WebUIs and DataSource for a particular profile be able to
find and talk to each other.  The former violates a TODO you have about not
overwriting old DataSources with new ones.  The latter either requires some sort
of static mapping of profiles to objects, or a way of retrieving particular
DataSources from the ChromeURLDataManager, which is a little messy, giving that
they live on another thread.

I'm open to other ideas.  Unless we do a lot of manual formatting, JSON log
dumps either are just a single line, and drawing them completely kills rendering
performance, or include several times more lines than our current dumps, and
drawing them merely significantly reduces rendering performance.

[eroman, 2011-06-16 00:42:02]

neato!

Your design choice sounds good to me. I don't think saving the temporary file is
a big deal.

I suspect as a next step we can get rid of the text format and just save JSON,
so the "Dump data" and "Load data" features are inverses.

I want to review the changes before approving this, but otherwise LG!

http://codereview.chromium.org/7155031/diff/1/chrome/browser/resources/net_in...
File chrome/browser/resources/net_internals/dataview.js (right):

http://codereview.chromium.org/7155031/diff/1/chrome/browser/resources/net_in...
chrome/browser/resources/net_internals/dataview.js:162:
DataView.prototype.EnableExportTextButton_ = function(enabled) {
nit: use lowercaseCamelCase for function names in javascript.

http://codereview.chromium.org/7155031/diff/1/chrome/browser/resources/net_in...
chrome/browser/resources/net_internals/dataview.js:402: var chunkSize = 100 *
1024;
chunkSize is unused.

http://codereview.chromium.org/7155031/diff/1/chrome/browser/resources/net_in...
chrome/browser/resources/net_internals/dataview.js:405: window.TEMPORARY,
10*1024 + flatText.length,
nit: the assumption here is that 1 character = 1 byte, I don't think that is
correct.

It is possible that some of our characters in the string will be non-ascii (for
instance the formatted date strings can contain non-ascii for certain locales).

http://codereview.chromium.org/7155031/diff/1/chrome/browser/resources/net_in...
chrome/browser/resources/net_internals/dataview.js:410:
DataView.prototype.OnFileSystemCreate_ = function(text, fileSystem) {
I believe javascript style stipulates lowercase for function names (this comment
applies to rest of files).

http://codereview.chromium.org/7155031/diff/1/chrome/browser/resources/net_in...
chrome/browser/resources/net_internals/dataview.js:412: 'net_internals_log.txt',
{create: true},
I wander if we should include a unique identifier (like timestamp) in the
filename.

http://codereview.chromium.org/7155031/diff/1/chrome/browser/resources/net_in...
chrome/browser/resources/net_internals/dataview.js:415: }
nit: can you put semicolon after all of these "statements" ?

http://codereview.chromium.org/7155031/diff/1/chrome/browser/resources/net_in...
chrome/browser/resources/net_internals/dataview.js:427:
blobBuilder.append(text);
Do you know how it serializes strings? UTF16? UTF8?

http://codereview.chromium.org/7155031/diff/1/chrome/browser/resources/net_in...
chrome/browser/resources/net_internals/dataview.js:428:
fileWriter.write(blobBuilder.getBlob('octet/stream'));
One annoyance is line ending on Windows are CR LF, however the string we built
up is using LF.

So I imagine if you were to open the resulting file in notepad it would be
smashed into one line.

http://codereview.chromium.org/7155031/diff/1/chrome/browser/resources/net_in...
chrome/browser/resources/net_internals/dataview.js:432: window.location =
fileEntry.toURL();
Not a big deal, but this might cause a flicker in the location bar. Or it might
overwrite it alltogether, i dunno.

If that is a problem, you can create an iframe and set the download target in
the iframe's .src.

This should still cause the download to be triggered, but without affecting our
URL or back/forward history.

http://codereview.chromium.org/7155031/diff/1/chrome/browser/resources/net_in...
chrome/browser/resources/net_internals/dataview.js:438: alert(errorText + '
Error: ' + error.code);
I suggest giving a more informative message, since experience has shown me that
somehow the error cases *always* end up happening :)

Right now this will print something like "Error: 10", which is a bit cryptic.

At a minimum, I suggest mentioning this is a FileError so we know what enum to
lookup the code in.

Even better if we can stringize it. Perhaps this would work (dunno havent
tested):

for (var errorName in FileError) {
  if (error.code == FileError[errorName])
     alert("FileError." + errorName + "(" + error.code + ")");
}

alert("FileError " + error.code);

http://codereview.chromium.org/7155031/diff/1/chrome/browser/resources/net_in...
File chrome/browser/resources/net_internals/index.html (right):

http://codereview.chromium.org/7155031/diff/1/chrome/browser/resources/net_in...
chrome/browser/resources/net_internals/index.html:341: <li>Copy-paste the
resulting selected text to a file.</li>
Please update this help text.

http://codereview.chromium.org/7155031/diff/1/webkit/fileapi/file_system_path...
File webkit/fileapi/file_system_path_manager.cc (right):

http://codereview.chromium.org/7155031/diff/1/webkit/fileapi/file_system_path...
webkit/fileapi/file_system_path_manager.cc:109: url.SchemeIs(kExtensionScheme)
|| url.SchemeIs(kChromeScheme) ||
you will need someone from corresponding OWNERS to review this.

[mmenke, 2011-06-16 16:16:53]

Thanks for the speedy and thorough review.

http://codereview.chromium.org/7155031/diff/1/chrome/browser/resources/net_in...
File chrome/browser/resources/net_internals/dataview.js (right):

http://codereview.chromium.org/7155031/diff/1/chrome/browser/resources/net_in...
chrome/browser/resources/net_internals/dataview.js:162:
DataView.prototype.EnableExportTextButton_ = function(enabled) {
On 2011/06/16 00:42:02, eroman wrote:
> nit: use lowercaseCamelCase for function names in javascript.

Done.

http://codereview.chromium.org/7155031/diff/1/chrome/browser/resources/net_in...
chrome/browser/resources/net_internals/dataview.js:402: var chunkSize = 100 *
1024;
On 2011/06/16 00:42:02, eroman wrote:
> chunkSize is unused.

Removed.  Accidentally left over from one of the aforementioned other
implementations.

http://codereview.chromium.org/7155031/diff/1/chrome/browser/resources/net_in...
chrome/browser/resources/net_internals/dataview.js:405: window.TEMPORARY,
10*1024 + flatText.length,
On 2011/06/16 00:42:02, eroman wrote:
> nit: the assumption here is that 1 character = 1 byte, I don't think that is
> correct.
> 
> It is possible that some of our characters in the string will be non-ascii
(for
> instance the formatted date strings can contain non-ascii for certain
locales).

Fixed.  Thanks for catching that.  Be a pain to run into it in the wild.

http://codereview.chromium.org/7155031/diff/1/chrome/browser/resources/net_in...
chrome/browser/resources/net_internals/dataview.js:410:
DataView.prototype.OnFileSystemCreate_ = function(text, fileSystem) {
On 2011/06/16 00:42:02, eroman wrote:
> I believe javascript style stipulates lowercase for function names (this
comment
> applies to rest of files).

Fixed everywhere.

http://codereview.chromium.org/7155031/diff/1/chrome/browser/resources/net_in...
chrome/browser/resources/net_internals/dataview.js:412: 'net_internals_log.txt',
{create: true},
On 2011/06/16 00:42:02, eroman wrote:
> I wander if we should include a unique identifier (like timestamp) in the
> filename.

I considered that, but wasn't sure it was worth the effort:

We can't easily get a locale dependent version of "m.d.yyyy_hh.mm.ss" or
"d.m.yyyy_hh.mm.ss" from Javscript.  The only functions it has use a combination
of text and numbers, and I have no idea what the text looks like in different
locales, or how else the format may change.

Another issue is then we'd have to be more careful about cleaning up files.  Not
too complicated, but I think we have more than enough callbacks as it is.

http://codereview.chromium.org/7155031/diff/1/chrome/browser/resources/net_in...
chrome/browser/resources/net_internals/dataview.js:415: }
On 2011/06/16 00:42:02, eroman wrote:
> nit: can you put semicolon after all of these "statements" ?

Done.

http://codereview.chromium.org/7155031/diff/1/chrome/browser/resources/net_in...
chrome/browser/resources/net_internals/dataview.js:427:
blobBuilder.append(text);
On 2011/06/16 00:42:02, eroman wrote:
> Do you know how it serializes strings? UTF16? UTF8?

Experimentally, just writes out UTF8.

http://codereview.chromium.org/7155031/diff/1/chrome/browser/resources/net_in...
chrome/browser/resources/net_internals/dataview.js:428:
fileWriter.write(blobBuilder.getBlob('octet/stream'));
On 2011/06/16 00:42:02, eroman wrote:
> One annoyance is line ending on Windows are CR LF, however the string we built
> up is using LF.
> 
> So I imagine if you were to open the resulting file in notepad it would be
> smashed into one line.

Fixed (Use CRLF on Windows only).

http://codereview.chromium.org/7155031/diff/1/chrome/browser/resources/net_in...
chrome/browser/resources/net_internals/dataview.js:432: window.location =
fileEntry.toURL();
On 2011/06/16 00:42:02, eroman wrote:
> Not a big deal, but this might cause a flicker in the location bar. Or it
might
> overwrite it alltogether, i dunno.
> 
> If that is a problem, you can create an iframe and set the download target in
> the iframe's .src.
> 
> This should still cause the download to be triggered, but without affecting
our
> URL or back/forward history.

We don't actually update the address bar until we've decided it's a navigation,
I believe.

http://codereview.chromium.org/7155031/diff/1/chrome/browser/resources/net_in...
chrome/browser/resources/net_internals/dataview.js:438: alert(errorText + '
Error: ' + error.code);
On 2011/06/16 00:42:02, eroman wrote:
> I suggest giving a more informative message, since experience has shown me
that
> somehow the error cases *always* end up happening :)
> 
> Right now this will print something like "Error: 10", which is a bit cryptic.
> 
> At a minimum, I suggest mentioning this is a FileError so we know what enum to
> lookup the code in.
> 
> Even better if we can stringize it. Perhaps this would work (dunno havent
> tested):
> 
> for (var errorName in FileError) {
>   if (error.code == FileError[errorName])
>      alert("FileError." + errorName + "(" + error.code + ")");
> }
> 
> alert("FileError " + error.code);

Done.

http://codereview.chromium.org/7155031/diff/1/chrome/browser/resources/net_in...
File chrome/browser/resources/net_internals/index.html (right):

http://codereview.chromium.org/7155031/diff/1/chrome/browser/resources/net_in...
chrome/browser/resources/net_internals/index.html:341: <li>Copy-paste the
resulting selected text to a file.</li>
On 2011/06/16 00:42:02, eroman wrote:
> Please update this help text.

Done.

http://codereview.chromium.org/7155031/diff/1/webkit/fileapi/file_system_path...
File webkit/fileapi/file_system_path_manager.cc (right):

http://codereview.chromium.org/7155031/diff/1/webkit/fileapi/file_system_path...
webkit/fileapi/file_system_path_manager.cc:109: url.SchemeIs(kExtensionScheme)
|| url.SchemeIs(kChromeScheme) ||
On 2011/06/16 00:42:02, eroman wrote:
> you will need someone from corresponding OWNERS to review this.

Will do.

[mmenke, 2011-06-16 16:18:57]

[+kinuko]

kinuko:  Please review the change to webkit/fileapi/file_system_path_manager.cc.
 Thanks.

[mmenke, 2011-06-16 16:26:58]

http://codereview.chromium.org/7155031/diff/1/chrome/browser/resources/net_in...
File chrome/browser/resources/net_internals/dataview.js (right):

http://codereview.chromium.org/7155031/diff/1/chrome/browser/resources/net_in...
chrome/browser/resources/net_internals/dataview.js:432: window.location =
fileEntry.toURL();
On 2011/06/16 16:16:53, Matt Menke wrote:
> On 2011/06/16 00:42:02, eroman wrote:
> > Not a big deal, but this might cause a flicker in the location bar. Or it
> might
> > overwrite it alltogether, i dunno.
> > 
> > If that is a problem, you can create an iframe and set the download target
in
> > the iframe's .src.
> > 
> > This should still cause the download to be triggered, but without affecting
> our
> > URL or back/forward history.
> 
> We don't actually update the address bar until we've decided it's a
navigation,
> I believe.

Well...I was wrong about this.  I'll go ahead and add the iframe (Window.open
should work, too, but don't want to create a new process)

[eroman, 2011-06-16 19:16:45]

LGTM

http://codereview.chromium.org/7155031/diff/7007/chrome/browser/resources/net...
File chrome/browser/resources/net_internals/dataview.js (right):

http://codereview.chromium.org/7155031/diff/7007/chrome/browser/resources/net...
chrome/browser/resources/net_internals/dataview.js:409: flatText.replace(/\n/g,
'\r\n');
I suspect the file API might have something similar to "text mode" for files,
which does the conversion transparently.

This is fine though.

http://codereview.chromium.org/7155031/diff/7007/chrome/browser/resources/net...
chrome/browser/resources/net_internals/dataview.js:416: window.TEMPORARY,
10*1024 + textBlob.size,
what is the 10kb padding for?

[mmenke, 2011-06-16 19:23:55]

http://codereview.chromium.org/7155031/diff/7007/chrome/browser/resources/net...
File chrome/browser/resources/net_internals/dataview.js (right):

http://codereview.chromium.org/7155031/diff/7007/chrome/browser/resources/net...
chrome/browser/resources/net_internals/dataview.js:409: flatText.replace(/\n/g,
'\r\n');
On 2011/06/16 19:16:45, eroman wrote:
> I suspect the file API might have something similar to "text mode" for files,
> which does the conversion transparently.
> 
> This is fine though.

Specs don't seem to mention it.  I could see it using mime type, but as we want
to force a download, anyways, can't use text/plain.

http://codereview.chromium.org/7155031/diff/7007/chrome/browser/resources/net...
chrome/browser/resources/net_internals/dataview.js:416: window.TEMPORARY,
10*1024 + textBlob.size,
On 2011/06/16 19:16:45, eroman wrote:
> what is the 10kb padding for?

The file API doesn't specify what the size actually means.  I don't trust it to
not include some sort of padding, or file index, etc.  10 KB is a little small
for that, though, I suppose.

[kinuko, 2011-06-17 08:00:16]

fileapi/ change LGTM

http://codereview.chromium.org/7155031/diff/7007/chrome/browser/resources/net...
File chrome/browser/resources/net_internals/dataview.js (right):

http://codereview.chromium.org/7155031/diff/7007/chrome/browser/resources/net...
chrome/browser/resources/net_internals/dataview.js:409: flatText.replace(/\n/g,
'\r\n');
On 2011/06/16 19:23:55, Matt Menke wrote:
> On 2011/06/16 19:16:45, eroman wrote:
> > I suspect the file API might have something similar to "text mode" for
files,
> > which does the conversion transparently.
> > 
> > This is fine though.
> 
> Specs don't seem to mention it.  I could see it using mime type, but as we
want
> to force a download, anyways, can't use text/plain.

BlobBuilder has some support for line-ending conversion.  I think you can use
"native" in this case.

http://dev.w3.org/2009/dap/file-system/file-writer.html#widl-BlobBuilder-appe...

http://codereview.chromium.org/7155031/diff/7007/chrome/browser/resources/net...
chrome/browser/resources/net_internals/dataview.js:416: window.TEMPORARY,
10*1024 + textBlob.size,
On 2011/06/16 19:23:55, Matt Menke wrote:
> On 2011/06/16 19:16:45, eroman wrote:
> > what is the 10kb padding for?
> 
> The file API doesn't specify what the size actually means.  I don't trust it
to
> not include some sort of padding, or file index, etc.  10 KB is a little small
> for that, though, I suppose.

For now this 'size' has no significant meaning but in the future this might be
used to request some quota for the filesystem.  For now any reasonable number
should work as far as you're using TEMPORARY filesystem.  (Or you'll just get
quota error when the physical disk is really getting tight)

[mmenke, 2011-06-17 13:45:59]

Thanks for the useful feedback and information on the API.

http://codereview.chromium.org/7155031/diff/7007/chrome/browser/resources/net...
File chrome/browser/resources/net_internals/dataview.js (right):

http://codereview.chromium.org/7155031/diff/7007/chrome/browser/resources/net...
chrome/browser/resources/net_internals/dataview.js:409: flatText.replace(/\n/g,
'\r\n');
On 2011/06/17 08:00:16, kinuko wrote:
> On 2011/06/16 19:23:55, Matt Menke wrote:
> > On 2011/06/16 19:16:45, eroman wrote:
> > > I suspect the file API might have something similar to "text mode" for
> files,
> > > which does the conversion transparently.
> > > 
> > > This is fine though.
> > 
> > Specs don't seem to mention it.  I could see it using mime type, but as we
> want
> > to force a download, anyways, can't use text/plain.
> 
> BlobBuilder has some support for line-ending conversion.  I think you can use
> "native" in this case.
> 
>
http://dev.w3.org/2009/dap/file-system/file-writer.html#widl-BlobBuilder-appe...

Tried it, and it works.

http://codereview.chromium.org/7155031/diff/7007/chrome/browser/resources/net...
chrome/browser/resources/net_internals/dataview.js:416: window.TEMPORARY,
10*1024 + textBlob.size,
On 2011/06/17 08:00:16, kinuko wrote:
> On 2011/06/16 19:23:55, Matt Menke wrote:
> > On 2011/06/16 19:16:45, eroman wrote:
> > > what is the 10kb padding for?
> > 
> > The file API doesn't specify what the size actually means.  I don't trust it
> to
> > not include some sort of padding, or file index, etc.  10 KB is a little
small
> > for that, though, I suppose.
> 
> For now this 'size' has no significant meaning but in the future this might be
> used to request some quota for the filesystem.  For now any reasonable number
> should work as far as you're using TEMPORARY filesystem.  (Or you'll just get
> quota error when the physical disk is really getting tight)

Ok.  I'll just go ahead and remove the 10 kB, then.

[commit-bot: I haz the power, 2011-06-17 15:48:38]

Change committed as 89491