First cut at UI for saving net_logs data into a temporary file on

First cut at UI for saving net_logs data into a temporary file on
Mobile (Android and iOS). 

Added a new page chrome://net-export to start/stop/send the net_log
for mobile platforms. net_log entries are saved in
"chrome-net-export-log.json" file created in temporary directory.

Button "Start Saving (after deleting old data)" will start saving to a
temporary file in "NetLog" directory in Temp directory. It deletes all
the old temporary files that were in that directory before creating a
new temporty file. It is a no-op if we are already saving data into a
file.

Button "Stop Saving Data" closes the temporary file so that it could
be mailed. It is a no-op if we haven't started saving data into a
file.

Button "Send Data" emails the data file if a temporary file exists in
"NetLog" directory. It is a no-op if there is no file.

There is only a single temporary file into which we log the
data. NetLog data is saved in the browser process. Logging of net_log
data behaves similar to the command line option --log-net-log.

If chrome://net-export is accessed in multiple tabs, action in first
tab wins. If Stop is clicked in any tab, logging stops. If start is
clicked in any tab, net logging starts.

about:about lists chrome://net-export as one of the links.

unit tests create a temporary file for each test run and they delete the
file at the end of the test run.

mmenke@ and/or eroman@ for chrome\browser\resources and webui changes. 
jar@ for net_log_temp_file changes.
jhawkins@chromium.org for chrome\browser\ui\webui and chrome\browser\resources
OWNERS stamp.

R=jar@chromium.org, mmenke@chromium.org
BUG=151212
TEST=chrome://net-export

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=180026

[ramant (doing other things), 2013-01-10 22:00:22]

Hi Matt,
  Built the new WebUI as per your comments in
https://codereview.chromium.org/11635023/ (somehow this CL got corrupted and
thus uploaded all the changes into a new CL).

jhawkins@ for chrome\browser\ui\webui and chrome\browser\resources OWNERS stamp.

thanks,
raman

[mmenke, 2013-01-11 18:31:12]

A lot of comments, but most of them are pretty minor.

https://codereview.chromium.org/11828036/diff/23001/chrome/browser/net/net_lo...
File chrome/browser/net/net_log_temp_file.cc (right):

https://codereview.chromium.org/11828036/diff/23001/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file.cc:12: #include
"base/string_number_conversions.h"
This isn't used here, right?

https://codereview.chromium.org/11828036/diff/23001/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file.cc:34:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE_USER_BLOCKING));
Out of paranoia, suggest an Init() call here if uninitialized.

https://codereview.chromium.org/11828036/diff/23001/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file.cc:77: bool GetTempNetLogDir(FilePath*
path) {
This should be in an anonymous namespace.  Suggest putting it up top, too, since
that's the general style.

https://codereview.chromium.org/11828036/diff/23001/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file.cc:97: void NetLogTempFile::Init() {
Suggest either a DCHECK on the state_ or early abort if we're already
initialized.

https://codereview.chromium.org/11828036/diff/23001/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file.cc:123: return;
If the temp directory is in some cache/temp dir where the OS feels free to
delete stuff on a whim, might the directory have already been deleted by the
time StartNetLog is called?

https://codereview.chromium.org/11828036/diff/23001/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file.cc:132: if
(!file_util::CreateTemporaryFileInDir(net_log_dir, &log_path_))
This seems a little weird.

Normally, one uses this function to create a random filename so collisions with
other temp files aren't a problem.

However, if there are any other files in the temp directory, we assume they're
ours during upload.

These seem to be two contradictory behaviors.  I think we should either use a
random name, and not support logs from previous sessions, or we should use a
constant name, to guarantee we find the file.  In the latter case, we may want a
more unique name.


Neither behavior supports parallelization during testing.

https://codereview.chromium.org/11828036/diff/23001/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file.cc:136:
net_log_logger_->StartObserving(g_browser_process->net_log());
Rather than messing with globals, suggest having the ChromeNetLog pass itself in
on construction.

That also allows us to unit test, if so desired.

https://codereview.chromium.org/11828036/diff/23001/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file.cc:188: void
NetLogTempFile::SendEmail(const FilePath& file_to_send) {
Wonder if it would make more sense to put the in net_export_ui.cc instead -
everything else in this file is more file/net based.  This is more UI based, so
perhaps should be in the other file.

The other advantage is this class would then run entirely on one thread, other
than construction / destruction.

https://codereview.chromium.org/11828036/diff/23001/chrome/browser/net/net_lo...
File chrome/browser/net/net_log_temp_file.h (right):

https://codereview.chromium.org/11828036/diff/23001/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file.h:29: // c) Either Send or Start is allowed
(state_ == ALLOW_START_SEND).
Think it's worth mentioning this is created/destroyed on the UI thread, but all
other function calls occur on the FILE_USER_BLOCKING thread.

And that this relies on the UI thread outlasting all other named threads for
thread safety.

https://codereview.chromium.org/11828036/diff/23001/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file.h:40: enum State {
Since these aren't used externally, this should be in the private section.

https://codereview.chromium.org/11828036/diff/23001/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file.h:44: ALLOW_START_SEND,     // Either
OnSendLog or OnStartLog calls are allowed.
Suggest naming the state after what |this| is currently doing, rather than what
commands it allows.

Something along the lines of:

"STATE_UNINITIALIZED",
"STATE_LOGGING",
"STATE_STOPPED_NO_LOG",  (Or STATE_NEVER_STARTED)
"STATE_STOPPED_HAS_LOG"

https://codereview.chromium.org/11828036/diff/23001/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file.h:50: ~NetLogTempFile();
Given how weirdly this class will behave if we have more than one of them, may
be worth making these private and friending ChromeNetLog.  Not a huge fan of
friending, but prefer it to making a class that's fragile.

https://codereview.chromium.org/11828036/diff/23001/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file.h:57: base::Value* NetLogTempFileToValue();
Suggest renaming this "GetStateAsValue()" or even just "GetState()", since
there's no other way of getting the current state.

https://codereview.chromium.org/11828036/diff/23001/chrome/browser/resources/...
File chrome/browser/resources/net_export/net_export.html (right):

https://codereview.chromium.org/11828036/diff/23001/chrome/browser/resources/...
chrome/browser/resources/net_export/net_export.html:16: <style>
This tag isn't needed, since you have one just above.

https://codereview.chromium.org/11828036/diff/23001/chrome/browser/resources/...
chrome/browser/resources/net_export/net_export.html:18: #export-view-start-data
{
Rather than having three rules, can just use one for all buttons:

button {
  ...
}

https://codereview.chromium.org/11828036/diff/23001/chrome/browser/resources/...
chrome/browser/resources/net_export/net_export.html:39: <button
id=export-view-start-data>Start Saving (after deleting old data)
Suggest just "Start Logging to Disk" "Stop Logging" and "Email Log".  I think
"logging" lets us use much better text at the bottom.  Can put "Deletes old log"
in red after the button.

https://codereview.chromium.org/11828036/diff/23001/chrome/browser/resources/...
chrome/browser/resources/net_export/net_export.html:47: </div>
nit:  Fix indent.

https://codereview.chromium.org/11828036/diff/23001/chrome/browser/resources/...
chrome/browser/resources/net_export/net_export.html:53: start saving.</li>
I think "INSTRUCTIONS:  Start logging, reproduce the problem, and then stop
logging.  Make sure to send the email before starting to log again.  Otherwise,
the log will be deleted."

https://codereview.chromium.org/11828036/diff/23001/chrome/browser/resources/...
chrome/browser/resources/net_export/net_export.html:59: </a>
Does help.html contain anything useful beyond the above instructions?  Maybe
just say "Logs can be loaded in net-internals of desktop Chrome"?  (With a link)

https://codereview.chromium.org/11828036/diff/23001/chrome/browser/resources/...
File chrome/browser/resources/net_export/net_export.js (right):

https://codereview.chromium.org/11828036/diff/23001/chrome/browser/resources/...
chrome/browser/resources/net_export/net_export.js:118: this.filePathText_ =
$(FILE_PATH_TEXT_ID);
Suggest just inlining this.

https://codereview.chromium.org/11828036/diff/23001/chrome/browser/resources/...
chrome/browser/resources/net_export/net_export.js:146:
this.filePathText_.textContent = '';
nit:  Not needed.

https://codereview.chromium.org/11828036/diff/23001/chrome/browser/resources/...
chrome/browser/resources/net_export/net_export.js:160:
this.sendDataButton_.disabled = false;
optional:  Think this would be clearer if you used only one set line per button.
 Makes it easier to reason about which buttont is enabled when.  e.g.:

var state = exportNetLogInfo.state;
this.startDataButton_.disabled = (state != 'ALLOW_START');
...

Alternatively, disable all the buttons by default, and only enable them in each
block of code below.  It's clearer if you set what a state can do, rather than
what it can't do.

https://codereview.chromium.org/11828036/diff/23001/chrome/browser/resources/...
chrome/browser/resources/net_export/net_export.js:170: return true;
No need to return anything.  net-internals uses it so tabs without valid data
can be hidden when a log file is loaded.  Not an issue here.

https://codereview.chromium.org/11828036/diff/23001/chrome/browser/ui/webui/n...
File chrome/browser/ui/webui/net_export_ui.cc (right):

https://codereview.chromium.org/11828036/diff/23001/chrome/browser/ui/webui/n...
chrome/browser/ui/webui/net_export_ui.cc:24: #include
"grit/generated_resources.h"
Do we really need generated_resources?

https://codereview.chromium.org/11828036/diff/23001/chrome/browser/ui/webui/n...
chrome/browser/ui/webui/net_export_ui.cc:128: void
NetExportMessageHandler::ProcessNetLogCommand(
This should be static.  Currently, |this| can be destroyed before this command
is called.

https://codereview.chromium.org/11828036/diff/23001/chrome/browser/ui/webui/n...
chrome/browser/ui/webui/net_export_ui.cc:145: SendExportNetLogInfo();
This is redundant - the Javascript code already sends a request at the time time
it sends a command.  Should be removed from one location or the other.

https://codereview.chromium.org/11828036/diff/23001/chrome/browser/ui/webui/n...
chrome/browser/ui/webui/net_export_ui.cc:148: void
NetExportMessageHandler::SendExportNetLogInfo() {
This should also be static.  Suggest you use PostTaskAndReply.  Alternatively,
this can take a weak_ptr<NetExportMessageHandler> as an argument.

https://codereview.chromium.org/11828036/diff/23001/chrome/browser/ui/webui/n...
File chrome/browser/ui/webui/net_export_ui.h (right):

https://codereview.chromium.org/11828036/diff/23001/chrome/browser/ui/webui/n...
chrome/browser/ui/webui/net_export_ui.h:16:
DISALLOW_COPY_AND_ASSIGN(NetExportUI);
Should include basictypes.h for this.

[ramant (doing other things), 2013-01-12 04:34:04]

Hi Matt,
  Many many thanks for the detailed code review and for your time. Made most of
the changes you had suggested (would appreciate if we could better names for
"State" (the names suggested by jar seem ok to me. My comments for it might not
have been as good. Tweaked the comments for it).

https://codereview.chromium.org/11828036/diff/23001/chrome/browser/net/net_lo...
File chrome/browser/net/net_log_temp_file.cc (right):

https://codereview.chromium.org/11828036/diff/23001/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file.cc:12: #include
"base/string_number_conversions.h"
On 2013/01/11 18:31:12, Matt Menke wrote:
> This isn't used here, right?

Done.

https://codereview.chromium.org/11828036/diff/23001/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file.cc:34:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE_USER_BLOCKING));
On 2013/01/11 18:31:12, Matt Menke wrote:
> Out of paranoia, suggest an Init() call here if uninitialized.

Done.

https://codereview.chromium.org/11828036/diff/23001/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file.cc:77: bool GetTempNetLogDir(FilePath*
path) {
On 2013/01/11 18:31:12, Matt Menke wrote:
> This should be in an anonymous namespace.  Suggest putting it up top, too,
since
> that's the general style.

Done.

https://codereview.chromium.org/11828036/diff/23001/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file.cc:97: void NetLogTempFile::Init() {
On 2013/01/11 18:31:12, Matt Menke wrote:
> Suggest either a DCHECK on the state_ or early abort if we're already
> initialized.

Done.

https://codereview.chromium.org/11828036/diff/23001/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file.cc:123: return;
On 2013/01/11 18:31:12, Matt Menke wrote:
> If the temp directory is in some cache/temp dir where the OS feels free to
> delete stuff on a whim, might the directory have already been deleted by the
> time StartNetLog is called?

As we had talked, tried to create "chrome-net-export" directory twice.

Done.

https://codereview.chromium.org/11828036/diff/23001/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file.cc:132: if
(!file_util::CreateTemporaryFileInDir(net_log_dir, &log_path_))
On 2013/01/11 18:31:12, Matt Menke wrote:
> This seems a little weird.
> 
> Normally, one uses this function to create a random filename so collisions
with
> other temp files aren't a problem.
> 
> However, if there are any other files in the temp directory, we assume they're
> ours during upload.
> 
> These seem to be two contradictory behaviors.  I think we should either use a
> random name, and not support logs from previous sessions, or we should use a
> constant name, to guarantee we find the file.  In the latter case, we may want
a
> more unique name.
> 
> 
> Neither behavior supports parallelization during testing.

As we had talked, will create "chrome-net-export/log.json" file in temp folder.
Added an API call for unit tests to change "log.json" filename (each test can
potentially pick a different name).

Done.

https://codereview.chromium.org/11828036/diff/23001/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file.cc:136:
net_log_logger_->StartObserving(g_browser_process->net_log());
On 2013/01/11 18:31:12, Matt Menke wrote:
> Rather than messing with globals, suggest having the ChromeNetLog pass itself
in
> on construction.
> 
> That also allows us to unit test, if so desired.

Done.

https://codereview.chromium.org/11828036/diff/23001/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file.cc:188: void
NetLogTempFile::SendEmail(const FilePath& file_to_send) {
On 2013/01/11 18:31:12, Matt Menke wrote:
> Wonder if it would make more sense to put the in net_export_ui.cc instead -
> everything else in this file is more file/net based.  This is more UI based,
so
> perhaps should be in the other file.
> 
> The other advantage is this class would then run entirely on one thread, other
> than construction / destruction.

Would like to talk to you more about the above.
"SendNetLog" needs to accept, callback and the message_loop on which it could
call the callback function if there is a file that is to be sent via email or
...

https://codereview.chromium.org/11828036/diff/23001/chrome/browser/net/net_lo...
File chrome/browser/net/net_log_temp_file.h (right):

https://codereview.chromium.org/11828036/diff/23001/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file.h:29: // c) Either Send or Start is allowed
(state_ == ALLOW_START_SEND).
On 2013/01/11 18:31:12, Matt Menke wrote:
> Think it's worth mentioning this is created/destroyed on the UI thread, but
all
> other function calls occur on the FILE_USER_BLOCKING thread.
> 
> And that this relies on the UI thread outlasting all other named threads for
> thread safety.

Done.

https://codereview.chromium.org/11828036/diff/23001/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file.h:40: enum State {
On 2013/01/11 18:31:12, Matt Menke wrote:
> Since these aren't used externally, this should be in the private section.

Done.

https://codereview.chromium.org/11828036/diff/23001/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file.h:44: ALLOW_START_SEND,     // Either
OnSendLog or OnStartLog calls are allowed.
On 2013/01/11 18:31:12, Matt Menke wrote:
> Suggest naming the state after what |this| is currently doing, rather than
what
> commands it allows.
> 
> Something along the lines of:
> 
> "STATE_UNINITIALIZED",
> "STATE_LOGGING",
> "STATE_STOPPED_NO_LOG",  (Or STATE_NEVER_STARTED)
> "STATE_STOPPED_HAS_LOG"

Added STATE_ to the names. Updated the comments to show how it is being used
internally. Would like to talk to you more about better names. thanks.

https://codereview.chromium.org/11828036/diff/23001/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file.h:50: ~NetLogTempFile();
On 2013/01/11 18:31:12, Matt Menke wrote:
> Given how weirdly this class will behave if we have more than one of them, may
> be worth making these private and friending ChromeNetLog.  Not a huge fan of
> friending, but prefer it to making a class that's fragile.

left ~NetLogTempFile() as public because of scoped_ptr. Is that ok? Is there a
better way to do this?
Done.

https://codereview.chromium.org/11828036/diff/23001/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file.h:57: base::Value* NetLogTempFileToValue();
On 2013/01/11 18:31:12, Matt Menke wrote:
> Suggest renaming this "GetStateAsValue()" or even just "GetState()", since
> there's no other way of getting the current state.

Done.

https://codereview.chromium.org/11828036/diff/23001/chrome/browser/resources/...
File chrome/browser/resources/net_export/net_export.html (right):

https://codereview.chromium.org/11828036/diff/23001/chrome/browser/resources/...
chrome/browser/resources/net_export/net_export.html:16: <style>
On 2013/01/11 18:31:12, Matt Menke wrote:
> This tag isn't needed, since you have one just above.

Done.

https://codereview.chromium.org/11828036/diff/23001/chrome/browser/resources/...
chrome/browser/resources/net_export/net_export.html:18: #export-view-start-data
{
On 2013/01/11 18:31:12, Matt Menke wrote:
> Rather than having three rules, can just use one for all buttons:
> 
> button {
>   ...
> }

Done.

https://codereview.chromium.org/11828036/diff/23001/chrome/browser/resources/...
chrome/browser/resources/net_export/net_export.html:39: <button
id=export-view-start-data>Start Saving (after deleting old data)
On 2013/01/11 18:31:12, Matt Menke wrote:
> Suggest just "Start Logging to Disk" "Stop Logging" and "Email Log".  I think
> "logging" lets us use much better text at the bottom.  Can put "Deletes old
log"
> in red after the button.

Done.

https://codereview.chromium.org/11828036/diff/23001/chrome/browser/resources/...
chrome/browser/resources/net_export/net_export.html:47: </div>
On 2013/01/11 18:31:12, Matt Menke wrote:
> nit:  Fix indent.

Done.

https://codereview.chromium.org/11828036/diff/23001/chrome/browser/resources/...
chrome/browser/resources/net_export/net_export.html:53: start saving.</li>
On 2013/01/11 18:31:12, Matt Menke wrote:
> I think "INSTRUCTIONS:  Start logging, reproduce the problem, and then stop
> logging.  Make sure to send the email before starting to log again. 
Otherwise,
> the log will be deleted."

Done.

https://codereview.chromium.org/11828036/diff/23001/chrome/browser/resources/...
chrome/browser/resources/net_export/net_export.html:59: </a>
On 2013/01/11 18:31:12, Matt Menke wrote:
> Does help.html contain anything useful beyond the above instructions?  Maybe
> just say "Logs can be loaded in net-internals of desktop Chrome"?  (With a
link)

Done.

https://codereview.chromium.org/11828036/diff/23001/chrome/browser/resources/...
File chrome/browser/resources/net_export/net_export.js (right):

https://codereview.chromium.org/11828036/diff/23001/chrome/browser/resources/...
chrome/browser/resources/net_export/net_export.js:118: this.filePathText_ =
$(FILE_PATH_TEXT_ID);
On 2013/01/11 18:31:12, Matt Menke wrote:
> Suggest just inlining this.

Done.

https://codereview.chromium.org/11828036/diff/23001/chrome/browser/resources/...
chrome/browser/resources/net_export/net_export.js:146:
this.filePathText_.textContent = '';
On 2013/01/11 18:31:12, Matt Menke wrote:
> nit:  Not needed.

Done.

https://codereview.chromium.org/11828036/diff/23001/chrome/browser/resources/...
chrome/browser/resources/net_export/net_export.js:160:
this.sendDataButton_.disabled = false;
On 2013/01/11 18:31:12, Matt Menke wrote:
> optional:  Think this would be clearer if you used only one set line per
button.
>  Makes it easier to reason about which buttont is enabled when.  e.g.:
> 
> var state = exportNetLogInfo.state;
> this.startDataButton_.disabled = (state != 'ALLOW_START');
> ...
> 
> Alternatively, disable all the buttons by default, and only enable them in
each
> block of code below.  It's clearer if you set what a state can do, rather than
> what it can't do.

Did the alternative.
Done.

https://codereview.chromium.org/11828036/diff/23001/chrome/browser/resources/...
chrome/browser/resources/net_export/net_export.js:170: return true;
On 2013/01/11 18:31:12, Matt Menke wrote:
> No need to return anything.  net-internals uses it so tabs without valid data
> can be hidden when a log file is loaded.  Not an issue here.

Done.

https://codereview.chromium.org/11828036/diff/23001/chrome/browser/ui/webui/n...
File chrome/browser/ui/webui/net_export_ui.cc (right):

https://codereview.chromium.org/11828036/diff/23001/chrome/browser/ui/webui/n...
chrome/browser/ui/webui/net_export_ui.cc:24: #include
"grit/generated_resources.h"
On 2013/01/11 18:31:12, Matt Menke wrote:
> Do we really need generated_resources?

Done.

https://codereview.chromium.org/11828036/diff/23001/chrome/browser/ui/webui/n...
chrome/browser/ui/webui/net_export_ui.cc:128: void
NetExportMessageHandler::ProcessNetLogCommand(
On 2013/01/11 18:31:12, Matt Menke wrote:
> This should be static.  Currently, |this| can be destroyed before this command
> is called.

Done.

https://codereview.chromium.org/11828036/diff/23001/chrome/browser/ui/webui/n...
chrome/browser/ui/webui/net_export_ui.cc:145: SendExportNetLogInfo();
On 2013/01/11 18:31:12, Matt Menke wrote:
> This is redundant - the Javascript code already sends a request at the time
time
> it sends a command.  Should be removed from one location or the other.

Deleted from the JS code.

Done.

https://codereview.chromium.org/11828036/diff/23001/chrome/browser/ui/webui/n...
chrome/browser/ui/webui/net_export_ui.cc:148: void
NetExportMessageHandler::SendExportNetLogInfo() {
On 2013/01/11 18:31:12, Matt Menke wrote:
> This should also be static.  Suggest you use PostTaskAndReply.  Alternatively,
> this can take a weak_ptr<NetExportMessageHandler> as an argument.

Done.

https://codereview.chromium.org/11828036/diff/23001/chrome/browser/ui/webui/n...
File chrome/browser/ui/webui/net_export_ui.h (right):

https://codereview.chromium.org/11828036/diff/23001/chrome/browser/ui/webui/n...
chrome/browser/ui/webui/net_export_ui.h:16:
DISALLOW_COPY_AND_ASSIGN(NetExportUI);
On 2013/01/11 18:31:12, Matt Menke wrote:
> Should include basictypes.h for this.

Done.

[ramant (doing other things), 2013-01-15 02:19:27]

https://codereview.chromium.org/11828036/diff/23001/chrome/browser/net/net_lo...
File chrome/browser/net/net_log_temp_file.cc (right):

https://codereview.chromium.org/11828036/diff/23001/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file.cc:188: void
NetLogTempFile::SendEmail(const FilePath& file_to_send) {
On 2013/01/12 04:34:04, ramant wrote:
> On 2013/01/11 18:31:12, Matt Menke wrote:
> > Wonder if it would make more sense to put the in net_export_ui.cc instead -
> > everything else in this file is more file/net based.  This is more UI based,
> so
> > perhaps should be in the other file.
> > 
> > The other advantage is this class would then run entirely on one thread,
other
> > than construction / destruction.
> 
> Would like to talk to you more about the above.
> "SendNetLog" needs to accept, callback and the message_loop on which it could
> call the callback function if there is a file that is to be sent via email or
> ...

Moved the SendEmail (UI) code to net_export_ui.cc

Done.

[mmenke, 2013-01-17 17:49:43]

More mostly minor comments.  I expect to sign off on this on the next round.

https://codereview.chromium.org/11828036/diff/23001/chrome/browser/net/net_lo...
File chrome/browser/net/net_log_temp_file.h (right):

https://codereview.chromium.org/11828036/diff/23001/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file.h:50: ~NetLogTempFile();
On 2013/01/12 04:34:04, ramant wrote:
> On 2013/01/11 18:31:12, Matt Menke wrote:
> > Given how weirdly this class will behave if we have more than one of them,
may
> > be worth making these private and friending ChromeNetLog.  Not a huge fan of
> > friending, but prefer it to making a class that's fragile.
> 
> left ~NetLogTempFile() as public because of scoped_ptr. Is that ok? Is there a
> better way to do this?
> Done.

You could friend the relevant class if you really wanted, but I think this is
fine.  Only construction we really want to protect against.

https://codereview.chromium.org/11828036/diff/42024/chrome/browser/net/net_lo...
File chrome/browser/net/net_log_temp_file.h (right):

https://codereview.chromium.org/11828036/diff/42024/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file.h:23: // NetLogTempFile logs all the
net_log entries into a temporary file created in
nit:  NetLog entries (There is variable called net_log in this file)

https://codereview.chromium.org/11828036/diff/42024/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file.h:32: // c) Either Send or Start is allowed
(state_ == ALLOW_START_SEND).
nit:  States have been renamed.

https://codereview.chromium.org/11828036/diff/42024/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file.h:53: // email. If there is no file to
send, then it returns false.
Don't think there's any need to mention email in this file.

https://codereview.chromium.org/11828036/diff/42024/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file.h:54: bool GetNetLogToSend(FilePath* path);
May want to call this "GetFilePath".  I don't think this class needs any
embedded knowledge of what one is going to do with the log file.

https://codereview.chromium.org/11828036/diff/42024/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file.h:118: // The chrome_net_log_ is owned by
the browser process, cached here to avoid
nit:  Above, you surround state_ in ||'s.  Google style doesn't require, but
does allow, ||'s around member variables.  I'm happy either way, but should be
consistent within a file.

https://codereview.chromium.org/11828036/diff/42024/chrome/browser/net/net_lo...
File chrome/browser/net/net_log_temp_file_unittest.cc (right):

https://codereview.chromium.org/11828036/diff/42024/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file_unittest.cc:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
nit: 2013

https://codereview.chromium.org/11828036/diff/42024/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file_unittest.cc:28: net_log_.reset(new
ChromeNetLog);
Suggest creating this first, and destroying it second.  Not vital, but I believe
the ChromeNetLog is currently created before the FILE_USER_BLOCKING thread (It's
certainly created before the IOThread class is fully initialized).

https://codereview.chromium.org/11828036/diff/42024/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file_unittest.cc:32: // Delete the old temporary
file if it exists.
unit tests are currently run multi-threaded, I believe, so this doesn't work. 
We should be setting the path to a temp file for the currently running test.

https://codereview.chromium.org/11828036/diff/42024/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file_unittest.cc:46:
file_user_blocking_thread_.reset();
We should be deleting the temp file / directory here.

https://codereview.chromium.org/11828036/diff/42024/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file_unittest.cc:46:
file_user_blocking_thread_.reset();
Is explicit destruction needed?

https://codereview.chromium.org/11828036/diff/42024/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file_unittest.cc:51: scoped_ptr<ChromeNetLog>
net_log_;
Suggest just reversing the order of these, to match my suggested change in
creation order.

https://codereview.chromium.org/11828036/diff/42024/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file_unittest.cc:59: net_log_temp_file->Init();
Rather than using internal functions, I suggest only using the public interface
instead.  Can then check the internal state, to make sure it is as expected.

If you want a test with a pre-existing temp file, then can just start logging,
and then delete the chrome_net_log and create a new one (Possibly stopping
logging first - actually, could have one test do each)

https://codereview.chromium.org/11828036/diff/42024/chrome/browser/resources/...
File chrome/browser/resources/net_export/net_export.js (right):

https://codereview.chromium.org/11828036/diff/42024/chrome/browser/resources/...
chrome/browser/resources/net_export/net_export.js:74:
g_mainExportView.onExportNetLogInfoChanged(data);
optional:  Could just make the MainView a singleton like net-internals does. 
Then would just be MainView.GetInstance()...  You'd also have to create the
MainView in onload the same way.

This doesn't really get us much, but I'm not a big fan of globals, when we can
avoid them, despite net-internals use of them.

Would still need to make the browser bridge a global, since we don't want the
callback from C++ to create one.

https://codereview.chromium.org/11828036/diff/42024/chrome/browser/resources/...
chrome/browser/resources/net_export/net_export.js:74:
g_mainExportView.onExportNetLogInfoChanged(data);
nit:  Fix indent.

https://codereview.chromium.org/11828036/diff/42024/chrome/browser/resources/...
chrome/browser/resources/net_export/net_export.js:108:
this.sendDataButton_.onclick = this.onSendData_.bind(this);
optional:  Rather than use member variables, could just use $(blah) everywhere. 
Don't think it matters much, one way or another, just saves a little code, and
makes things more explicit.

https://codereview.chromium.org/11828036/diff/42024/chrome/browser/resources/...
chrome/browser/resources/net_export/net_export.js:148: }
nit:  Suggest a linebreak here.

https://codereview.chromium.org/11828036/diff/42024/chrome/browser/ui/webui/n...
File chrome/browser/ui/webui/net_export_ui.cc (right):

https://codereview.chromium.org/11828036/diff/42024/chrome/browser/ui/webui/n...
chrome/browser/ui/webui/net_export_ui.cc:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
nit:  2013

https://codereview.chromium.org/11828036/diff/42024/chrome/browser/ui/webui/n...
chrome/browser/ui/webui/net_export_ui.cc:72: // Sends net_log data via email if
there is a net_log_temp_file.
There's always a NetLogTempFile class.  May want to be a little clearer.

https://codereview.chromium.org/11828036/diff/42024/chrome/browser/ui/webui/n...
chrome/browser/ui/webui/net_export_ui.cc:99:
g_browser_process->net_log()->net_log_temp_file();
optional:  I'm not a big fan of using g_browser_process everywhere.  How about
storing a pointer to the ChromeNetLog, or even the NetLogTempFile in
NetExportMessageHandler and then passing it around?

It doesn't actually change the thread safety characteristics at all, but I just
prefer to keep direct global accesses to a minimum, and only access it on the UI
thread.

https://codereview.chromium.org/11828036/diff/42024/chrome/browser/ui/webui/n...
chrome/browser/ui/webui/net_export_ui.cc:147:
SendNetLog(weak_ptr_factory_.GetWeakPtr());
optional:  I think this would be a little cleaner if you used PostTaskAndReply
here, and have SendNetLog return a FilePath.  Then can rename SendNetLog to
"GetNetLogFileName".

Could also use the same pattern for the start/stop functions, but think we don't
gain much simplicity there.

https://codereview.chromium.org/11828036/diff/42024/chrome/browser/ui/webui/n...
chrome/browser/ui/webui/net_export_ui.cc:196:
SendExportNetLogInfo(net_export_message_handler);
Has anything changed by calling GetNetLogToSend()?  I don't think this is
needed.

https://codereview.chromium.org/11828036/diff/42024/chrome/browser/ui/webui/n...
chrome/browser/ui/webui/net_export_ui.cc:226: std::string body = "Net Internals
log data";
Suggest default values that encourage users to add some informative text.

Maybe "Issue number: " for the title, for example.

https://codereview.chromium.org/11828036/diff/42024/chrome/browser/ui/webui/n...
chrome/browser/ui/webui/net_export_ui.cc:238: *value.get());
Doesn't just *value work here?

https://codereview.chromium.org/11828036/diff/42024/chrome/browser/ui/webui/n...
File chrome/browser/ui/webui/net_export_ui.h (right):

https://codereview.chromium.org/11828036/diff/42024/chrome/browser/ui/webui/n...
chrome/browser/ui/webui/net_export_ui.h:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
nit:  2013

[ramant (doing other things), 2013-01-18 04:59:52]

Thanks very much for your comments. Made all the changes.

https://codereview.chromium.org/11828036/diff/42024/chrome/browser/net/net_lo...
File chrome/browser/net/net_log_temp_file.h (right):

https://codereview.chromium.org/11828036/diff/42024/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file.h:23: // NetLogTempFile logs all the
net_log entries into a temporary file created in
On 2013/01/17 17:49:43, Matt Menke wrote:
> nit:  NetLog entries (There is variable called net_log in this file)

Done.

https://codereview.chromium.org/11828036/diff/42024/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file.h:32: // c) Either Send or Start is allowed
(state_ == ALLOW_START_SEND).
On 2013/01/17 17:49:43, Matt Menke wrote:
> nit:  States have been renamed.

Done.

https://codereview.chromium.org/11828036/diff/42024/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file.h:53: // email. If there is no file to
send, then it returns false.
On 2013/01/17 17:49:43, Matt Menke wrote:
> Don't think there's any need to mention email in this file.

Done.

https://codereview.chromium.org/11828036/diff/42024/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file.h:54: bool GetNetLogToSend(FilePath* path);
On 2013/01/17 17:49:43, Matt Menke wrote:
> May want to call this "GetFilePath".  I don't think this class needs any
> embedded knowledge of what one is going to do with the log file.

Done.

https://codereview.chromium.org/11828036/diff/42024/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file.h:118: // The chrome_net_log_ is owned by
the browser process, cached here to avoid
On 2013/01/17 17:49:43, Matt Menke wrote:
> nit:  Above, you surround state_ in ||'s.  Google style doesn't require, but
> does allow, ||'s around member variables.  I'm happy either way, but should be
> consistent within a file.

Done.

https://codereview.chromium.org/11828036/diff/42024/chrome/browser/net/net_lo...
File chrome/browser/net/net_log_temp_file_unittest.cc (right):

https://codereview.chromium.org/11828036/diff/42024/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file_unittest.cc:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
On 2013/01/17 17:49:43, Matt Menke wrote:
> nit: 2013

Done.

https://codereview.chromium.org/11828036/diff/42024/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file_unittest.cc:28: net_log_.reset(new
ChromeNetLog);
On 2013/01/17 17:49:43, Matt Menke wrote:
> Suggest creating this first, and destroying it second.  Not vital, but I
believe
> the ChromeNetLog is currently created before the FILE_USER_BLOCKING thread
(It's
> certainly created before the IOThread class is fully initialized).

Done.

https://codereview.chromium.org/11828036/diff/42024/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file_unittest.cc:32: // Delete the old temporary
file if it exists.
On 2013/01/17 17:49:43, Matt Menke wrote:
> unit tests are currently run multi-threaded, I believe, so this doesn't work. 
> We should be setting the path to a temp file for the currently running test.

Done.

https://codereview.chromium.org/11828036/diff/42024/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file_unittest.cc:46:
file_user_blocking_thread_.reset();
On 2013/01/17 17:49:43, Matt Menke wrote:
> Is explicit destruction needed?

Done.

https://codereview.chromium.org/11828036/diff/42024/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file_unittest.cc:46:
file_user_blocking_thread_.reset();
On 2013/01/17 17:49:43, Matt Menke wrote:
> We should be deleting the temp file / directory here.

Deleted the temp file. Didn't delete the directory because other tests could be
running at the same time on that machine?

Done.

https://codereview.chromium.org/11828036/diff/42024/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file_unittest.cc:51: scoped_ptr<ChromeNetLog>
net_log_;
On 2013/01/17 17:49:43, Matt Menke wrote:
> Suggest just reversing the order of these, to match my suggested change in
> creation order.

Done.

https://codereview.chromium.org/11828036/diff/42024/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file_unittest.cc:59: net_log_temp_file->Init();
On 2013/01/17 17:49:43, Matt Menke wrote:
> Rather than using internal functions, I suggest only using the public
interface
> instead.  Can then check the internal state, to make sure it is as expected.
> 
> If you want a test with a pre-existing temp file, then can just start logging,
> and then delete the chrome_net_log and create a new one (Possibly stopping
> logging first - actually, could have one test do each)

Implemented what we had discussed.

Done.

https://codereview.chromium.org/11828036/diff/42024/chrome/browser/resources/...
File chrome/browser/resources/net_export/net_export.js (right):

https://codereview.chromium.org/11828036/diff/42024/chrome/browser/resources/...
chrome/browser/resources/net_export/net_export.js:74:
g_mainExportView.onExportNetLogInfoChanged(data);
On 2013/01/17 17:49:43, Matt Menke wrote:
> nit:  Fix indent.

Done.

https://codereview.chromium.org/11828036/diff/42024/chrome/browser/resources/...
chrome/browser/resources/net_export/net_export.js:74:
g_mainExportView.onExportNetLogInfoChanged(data);
On 2013/01/17 17:49:43, Matt Menke wrote:
> optional:  Could just make the MainView a singleton like net-internals does. 
> Then would just be MainView.GetInstance()...  You'd also have to create the
> MainView in onload the same way.
> 
> This doesn't really get us much, but I'm not a big fan of globals, when we can
> avoid them, despite net-internals use of them.
> 
> Would still need to make the browser bridge a global, since we don't want the
> callback from C++ to create one.

Done.

https://codereview.chromium.org/11828036/diff/42024/chrome/browser/resources/...
chrome/browser/resources/net_export/net_export.js:108:
this.sendDataButton_.onclick = this.onSendData_.bind(this);
On 2013/01/17 17:49:43, Matt Menke wrote:
> optional:  Rather than use member variables, could just use $(blah)
everywhere. 
> Don't think it matters much, one way or another, just saves a little code, and
> makes things more explicit.

Done.

https://codereview.chromium.org/11828036/diff/42024/chrome/browser/resources/...
chrome/browser/resources/net_export/net_export.js:148: }
On 2013/01/17 17:49:43, Matt Menke wrote:
> nit:  Suggest a linebreak here.

Done.

https://codereview.chromium.org/11828036/diff/42024/chrome/browser/ui/webui/n...
File chrome/browser/ui/webui/net_export_ui.cc (right):

https://codereview.chromium.org/11828036/diff/42024/chrome/browser/ui/webui/n...
chrome/browser/ui/webui/net_export_ui.cc:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
On 2013/01/17 17:49:43, Matt Menke wrote:
> nit:  2013

Done.

https://codereview.chromium.org/11828036/diff/42024/chrome/browser/ui/webui/n...
chrome/browser/ui/webui/net_export_ui.cc:72: // Sends net_log data via email if
there is a net_log_temp_file.
On 2013/01/17 17:49:43, Matt Menke wrote:
> There's always a NetLogTempFile class.  May want to be a little clearer.

Done.

https://codereview.chromium.org/11828036/diff/42024/chrome/browser/ui/webui/n...
chrome/browser/ui/webui/net_export_ui.cc:99:
g_browser_process->net_log()->net_log_temp_file();
On 2013/01/17 17:49:43, Matt Menke wrote:
> optional:  I'm not a big fan of using g_browser_process everywhere.  How about
> storing a pointer to the ChromeNetLog, or even the NetLogTempFile in
> NetExportMessageHandler and then passing it around?
> 
> It doesn't actually change the thread safety characteristics at all, but I
just
> prefer to keep direct global accesses to a minimum, and only access it on the
UI
> thread.

Done.

https://codereview.chromium.org/11828036/diff/42024/chrome/browser/ui/webui/n...
chrome/browser/ui/webui/net_export_ui.cc:147:
SendNetLog(weak_ptr_factory_.GetWeakPtr());
On 2013/01/17 17:49:43, Matt Menke wrote:
> optional:  I think this would be a little cleaner if you used PostTaskAndReply
> here, and have SendNetLog return a FilePath.  Then can rename SendNetLog to
> "GetNetLogFileName".
> 
> Could also use the same pattern for the start/stop functions, but think we
don't
> gain much simplicity there.

Done.

https://codereview.chromium.org/11828036/diff/42024/chrome/browser/ui/webui/n...
chrome/browser/ui/webui/net_export_ui.cc:196:
SendExportNetLogInfo(net_export_message_handler);
On 2013/01/17 17:49:43, Matt Menke wrote:
> Has anything changed by calling GetNetLogToSend()?  I don't think this is
> needed.

Done.

https://codereview.chromium.org/11828036/diff/42024/chrome/browser/ui/webui/n...
chrome/browser/ui/webui/net_export_ui.cc:226: std::string body = "Net Internals
log data";
On 2013/01/17 17:49:43, Matt Menke wrote:
> Suggest default values that encourage users to add some informative text.
> 
> Maybe "Issue number: " for the title, for example.

Would appreciate if you could suggest some text.

Done.

https://codereview.chromium.org/11828036/diff/42024/chrome/browser/ui/webui/n...
chrome/browser/ui/webui/net_export_ui.cc:238: *value.get());
On 2013/01/17 17:49:43, Matt Menke wrote:
> Doesn't just *value work here?

NetInternalsMessageHandler::SendJavascriptCommand does the same. Changed it to
*arg.

Done.

https://codereview.chromium.org/11828036/diff/42024/chrome/browser/ui/webui/n...
File chrome/browser/ui/webui/net_export_ui.h (right):

https://codereview.chromium.org/11828036/diff/42024/chrome/browser/ui/webui/n...
chrome/browser/ui/webui/net_export_ui.h:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
On 2013/01/17 17:49:43, Matt Menke wrote:
> nit:  2013

Done.

[mmenke, 2013-01-18 15:58:00]

Still don't think the tests are threadsafe.

https://chromiumcodereview.appspot.com/11828036/diff/72004/chrome/browser/net...
File chrome/browser/net/chrome_net_log.cc (right):

https://chromiumcodereview.appspot.com/11828036/diff/72004/chrome/browser/net...
chrome/browser/net/chrome_net_log.cc:48: net_log_temp_file_.reset();
Think we should have the temp file stop watching the NetLog, if it currently is
doing so.  Can this happen?  It's not clear.  I pretty sure we destroy all tabs
(And thus the WebUI's) before we destroy the FILE_THREAD_USER_BLOCKING, but I'm
not completely positive, and would be more comfortable not relying on that
behavior on all platforms.

Simplest thing would be:

if (net_log_temp_file_->net_log())
  net_log_temp_file_->StopObserving();

https://chromiumcodereview.appspot.com/11828036/diff/72004/chrome/browser/net...
chrome/browser/net/chrome_net_log.cc:48: net_log_temp_file_.reset();
Also, there's no need to delete this.  Just need to make sure it isn't watching
the NetLog.

https://chromiumcodereview.appspot.com/11828036/diff/72004/chrome/browser/net...
File chrome/browser/net/net_log_temp_file.cc (right):

https://chromiumcodereview.appspot.com/11828036/diff/72004/chrome/browser/net...
chrome/browser/net/net_log_temp_file.cc:7: #include <stdio.h>
Is this needed?  We use FILE*'s, but no stdio functions.

https://chromiumcodereview.appspot.com/11828036/diff/72004/chrome/browser/net...
chrome/browser/net/net_log_temp_file.cc:57:
log_filename_(FILE_PATH_LITERAL("log.json")),
Don't think a pathname + a file name really gets us anything, at the moment. 
Suggest just calling it chrome-net-export-log.json to simplify things.  My main
concern is just leaving stuff on the build bots, and having the test fixture
complain, but also simplifies life a little.

https://chromiumcodereview.appspot.com/11828036/diff/72004/chrome/browser/net...
chrome/browser/net/net_log_temp_file.cc:137: FILE* fp =
file_util::OpenFile(net_export_log, "w");
Fine for now, but I'd like to look into a better way of doing this.

https://chromiumcodereview.appspot.com/11828036/diff/72004/chrome/browser/net...
chrome/browser/net/net_log_temp_file.cc:143: file_util::CloseFile(fp);
nit:  Fix indent.

https://chromiumcodereview.appspot.com/11828036/diff/72004/chrome/browser/net...
File chrome/browser/net/net_log_temp_file.h (right):

https://chromiumcodereview.appspot.com/11828036/diff/72004/chrome/browser/net...
chrome/browser/net/net_log_temp_file.h:49: // Accepts the button command and
executes them.
nit:  either "commands and executes them" or "command and executes it"

https://chromiumcodereview.appspot.com/11828036/diff/72004/chrome/browser/net...
File chrome/browser/net/net_log_temp_file_unittest.cc (right):

https://chromiumcodereview.appspot.com/11828036/diff/72004/chrome/browser/net...
chrome/browser/net/net_log_temp_file_unittest.cc:7: #include <stdio.h>
Is this needed?

https://chromiumcodereview.appspot.com/11828036/diff/72004/chrome/browser/net...
chrome/browser/net/net_log_temp_file_unittest.cc:8: #include <string>
nit:  believe there should be a blank line between C headers and C++ headers.

https://chromiumcodereview.appspot.com/11828036/diff/72004/chrome/browser/net...
chrome/browser/net/net_log_temp_file_unittest.cc:40:
file_util::Delete(net_export_log, false);
This isn't thread safe.  While the file doesn't exist, a new call to
CreateTemporararyFile could take the same name.  Windows explicitly mentions
that it uses the time and a check for pre-existing files, so two temp files made
at nearly the same time could theoretically have the same name, if the first one
is deleted.

Here's my suggestion:  Subclass NetLogTempFile and override GetNetExportLog to
create and return a temp file.  Have it return true if the file exists, and
false if it doesn't (And in the cc file, move the file exists check into
GetNetExportLog as well).  Then you can just lie about whether or not the file
exists, based on a class variable.

Unfortunately, we don't test the real file creation / existence checking, but at
least it works.

Alternatively, could mock out existence checking and name selection
independently.

https://chromiumcodereview.appspot.com/11828036/diff/72004/chrome/browser/net...
chrome/browser/net/net_log_temp_file_unittest.cc:66:
EXPECT_EQ(NetLogTempFile::STATE_ALLOW_START, net_log_temp_file_->state());
Think you should check the return value of
net_log_temp_file_->GetFilePath(&net_export_file_path)); at the end of all these
tests.

https://chromiumcodereview.appspot.com/11828036/diff/72004/chrome/browser/net...
chrome/browser/net/net_log_temp_file_unittest.cc:86: net_log_temp_file_->Init();
We don't need to call this ourselves except in the first two tests, right?  It's
automatically called as needed.

https://chromiumcodereview.appspot.com/11828036/diff/72004/chrome/browser/net...
chrome/browser/net/net_log_temp_file_unittest.cc:138: }
Worth testing that DoStart clears the log file?

Could also make sure the file isn't empty (It should contain the constants
table, but not need to check if we have it, just that it gets something, and
assume it's the table).

[mmenke, 2013-01-18 16:02:02]

https://chromiumcodereview.appspot.com/11828036/diff/72004/chrome/browser/net...
File chrome/browser/net/net_log_temp_file_unittest.cc (right):

https://chromiumcodereview.appspot.com/11828036/diff/72004/chrome/browser/net...
chrome/browser/net/net_log_temp_file_unittest.cc:138: }
On 2013/01/18 15:58:00, Matt Menke wrote:
> Worth testing that DoStart clears the log file?
> 
> Could also make sure the file isn't empty (It should contain the constants
> table, but not need to check if we have it, just that it gets something, and
> assume it's the table).

Testing this is actually a little tricky.  Could start logging, add a random
NetLog event, stop logging, load the file, then start/stop logging, and make
sure the file the second time is different (And smaller, but non-empty) from the
output the first time.

This is complicated enough that I'm not sure it's worth it.

[ramant (doing other things), 2013-01-18 23:22:33]

Many many thanks for your comments. Would appreciate if you could take another
look.
-raman

https://codereview.chromium.org/11828036/diff/72004/chrome/browser/net/chrome...
File chrome/browser/net/chrome_net_log.cc (right):

https://codereview.chromium.org/11828036/diff/72004/chrome/browser/net/chrome...
chrome/browser/net/chrome_net_log.cc:48: net_log_temp_file_.reset();
On 2013/01/18 15:58:00, Matt Menke wrote:
> Think we should have the temp file stop watching the NetLog, if it currently
is
> doing so.  Can this happen?  It's not clear.  I pretty sure we destroy all
tabs
> (And thus the WebUI's) before we destroy the FILE_THREAD_USER_BLOCKING, but
I'm
> not completely positive, and would be more comfortable not relying on that
> behavior on all platforms.
> 
> Simplest thing would be:
> 
> if (net_log_temp_file_->net_log())
>   net_log_temp_file_->StopObserving();

We do the above in NetLogTempFile::~NetLogTempFile(). Wanted to keep the
knowledge of Start/Stop  observing to net_log_temp_file. If you feel strongly,
will move that code here and remove the reset.

https://codereview.chromium.org/11828036/diff/72004/chrome/browser/net/net_lo...
File chrome/browser/net/net_log_temp_file.cc (right):

https://codereview.chromium.org/11828036/diff/72004/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file.cc:7: #include <stdio.h>
On 2013/01/18 15:58:00, Matt Menke wrote:
> Is this needed?  We use FILE*'s, but no stdio functions.

Done.

https://codereview.chromium.org/11828036/diff/72004/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file.cc:57:
log_filename_(FILE_PATH_LITERAL("log.json")),
On 2013/01/18 15:58:00, Matt Menke wrote:
> Don't think a pathname + a file name really gets us anything, at the moment. 
> Suggest just calling it chrome-net-export-log.json to simplify things.  My
main
> concern is just leaving stuff on the build bots, and having the test fixture
> complain, but also simplifies life a little.

Done.

https://codereview.chromium.org/11828036/diff/72004/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file.cc:137: FILE* fp =
file_util::OpenFile(net_export_log, "w");
On 2013/01/18 15:58:00, Matt Menke wrote:
> Fine for now, but I'd like to look into a better way of doing this.

Added a TODO.

https://codereview.chromium.org/11828036/diff/72004/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file.cc:143: file_util::CloseFile(fp);
On 2013/01/18 15:58:00, Matt Menke wrote:
> nit:  Fix indent.

Done.

https://codereview.chromium.org/11828036/diff/72004/chrome/browser/net/net_lo...
File chrome/browser/net/net_log_temp_file.h (right):

https://codereview.chromium.org/11828036/diff/72004/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file.h:49: // Accepts the button command and
executes them.
On 2013/01/18 15:58:00, Matt Menke wrote:
> nit:  either "commands and executes them" or "command and executes it"

Done.

https://codereview.chromium.org/11828036/diff/72004/chrome/browser/net/net_lo...
File chrome/browser/net/net_log_temp_file_unittest.cc (right):

https://codereview.chromium.org/11828036/diff/72004/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file_unittest.cc:7: #include <stdio.h>
On 2013/01/18 15:58:00, Matt Menke wrote:
> Is this needed?

Done.

https://codereview.chromium.org/11828036/diff/72004/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file_unittest.cc:8: #include <string>
On 2013/01/18 15:58:00, Matt Menke wrote:
> nit:  believe there should be a blank line between C headers and C++ headers.

Deleted the include (not needed).

https://codereview.chromium.org/11828036/diff/72004/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file_unittest.cc:40:
file_util::Delete(net_export_log, false);
On 2013/01/18 15:58:00, Matt Menke wrote:
> This isn't thread safe.  While the file doesn't exist, a new call to
> CreateTemporararyFile could take the same name.  Windows explicitly mentions
> that it uses the time and a check for pre-existing files, so two temp files
made
> at nearly the same time could theoretically have the same name, if the first
one
> is deleted.
> 
> Here's my suggestion:  Subclass NetLogTempFile and override GetNetExportLog to
> create and return a temp file.  Have it return true if the file exists, and
> false if it doesn't (And in the cc file, move the file exists check into
> GetNetExportLog as well).  Then you can just lie about whether or not the file
> exists, based on a class variable.
> 
> Unfortunately, we don't test the real file creation / existence checking, but
at
> least it works.
> 
> Alternatively, could mock out existence checking and name selection
> independently.

Implemented Subclass approach.
Done.

https://codereview.chromium.org/11828036/diff/72004/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file_unittest.cc:66:
EXPECT_EQ(NetLogTempFile::STATE_ALLOW_START, net_log_temp_file_->state());
On 2013/01/18 15:58:00, Matt Menke wrote:
> Think you should check the return value of
> net_log_temp_file_->GetFilePath(&net_export_file_path)); at the end of all
these
> tests.

Done.

https://codereview.chromium.org/11828036/diff/72004/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file_unittest.cc:86: net_log_temp_file_->Init();
On 2013/01/18 15:58:00, Matt Menke wrote:
> We don't need to call this ourselves except in the first two tests, right? 
It's
> automatically called as needed.

Done.

https://codereview.chromium.org/11828036/diff/72004/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file_unittest.cc:138: }
On 2013/01/18 15:58:00, Matt Menke wrote:
> Worth testing that DoStart clears the log file?
> 
> Could also make sure the file isn't empty (It should contain the constants
> table, but not need to check if we have it, just that it gets something, and
> assume it's the table).

Added a check the file_size is > 0.
Done.

https://codereview.chromium.org/11828036/diff/72004/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file_unittest.cc:138: }
On 2013/01/18 16:02:02, Matt Menke wrote:
> On 2013/01/18 15:58:00, Matt Menke wrote:
> > Worth testing that DoStart clears the log file?
> > 
> > Could also make sure the file isn't empty (It should contain the constants
> > table, but not need to check if we have it, just that it gets something, and
> > assume it's the table).
> 
> Testing this is actually a little tricky.  Could start logging, add a random
> NetLog event, stop logging, load the file, then start/stop logging, and make
> sure the file the second time is different (And smaller, but non-empty) from
the
> output the first time.
> 
> This is complicated enough that I'm not sure it's worth it.

Added a TODO.

[mmenke, 2013-01-23 17:40:59]

Looks like your tests are failing on Windows.  For some reason you're getting a
legacy DOS 8.3 file name in one string, but not the other.  I'll get back to you
later today with feedback (Looks like nits so far).

I'll see if I can spot the issue, but you may have to investigate yourself.

[mmenke, 2013-01-23 19:01:27]

Mostly just minor cleanups and suggestions on unit tests.  My review slate is
much cleaner now, so should be able to get back to you much more quickly next
time.

https://codereview.chromium.org/11828036/diff/80057/chrome/browser/net/net_lo...
File chrome/browser/net/net_log_temp_file.cc (right):

https://codereview.chromium.org/11828036/diff/80057/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file.cc:23: void
NetLogTempFile::ProcessCommand(Command command) {
Think you should DCHECK that all of these are called on the right thread.  Can
just use TestBrowserThread to get the unit tests passing.

https://codereview.chromium.org/11828036/diff/80057/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file.cc:48: dict->SetString("file",
log_path_.MaybeAsASCII());
nit:  Maybe LossyDisplayName() would be better?

https://codereview.chromium.org/11828036/diff/80057/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file.cc:72: state_ = STATE_ALLOW_START;
If file_util::GetTempDir fails, we'll end up in STATE_ALLOW_START with an
invalid log_path_.  If we want to handle GetTempDir failures correctly, we'll
need to slightly rework things.

My suggestion:  Separate out getting the path and checking if the file exists,
using different virtual functions for each.  Then STATE_ALLOW_START can just
call GetNetExportLog (And update log_path_), and if it fails, indicating no temp
directory, exit early without changing the state.

GetFilePath would then just call the function to check for file existence with
log_path_ and just copy log_path_ to path, which I think is a little cleaner as
well.

https://codereview.chromium.org/11828036/diff/80057/chrome/browser/net/net_lo...
File chrome/browser/net/net_log_temp_file.h (right):

https://codereview.chromium.org/11828036/diff/80057/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file.h:53: // send, then it returns false.
nit:  Think it's worth mentioning that it also returns false while actively
logging to the file.

https://codereview.chromium.org/11828036/diff/80057/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file.h:73: friend class CustomNetLogTempFile;
Can we just make anything thing this class accesses protected, and unfriend it? 
I prefer to friend classes only when necessary (The other friends all allow us
not to expose our innards externally.  Doesn't seem to be the case with
CustomNetLogTempFile.

https://codereview.chromium.org/11828036/diff/80057/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file.h:111: FilePath log_path() const { return
log_path_; }
Think we can get rid of this, and not depend on it or log_path_ in the tests.

https://codereview.chromium.org/11828036/diff/80057/chrome/browser/net/net_lo...
File chrome/browser/net/net_log_temp_file_unittest.cc (right):

https://codereview.chromium.org/11828036/diff/80057/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file_unittest.cc:18: class CustomNetLogTempFile
: public NetLogTempFile {
nit:  Think "TestNetLogTempFile" or "NetLogTempFileForTesting" is clearer than
"CustomNetLogTempFile", particularly if we have to friend it in non-test code
(Which I don't think we do, see other comment about that).

https://codereview.chromium.org/11828036/diff/80057/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file_unittest.cc:38: friend class
NetLogTempFileTest;
Don't think we need to friend the test, can just make the constructor public.

https://codereview.chromium.org/11828036/diff/80057/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file_unittest.cc:60:
net_log_temp_file_->log_filename_ = net_export_log_filename.value();
nit:  Could just move this to the constructor, and move TearDown to the
destructor.  Each execution of a test creates a new test fixture.

https://codereview.chromium.org/11828036/diff/80057/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file_unittest.cc:65:
ASSERT_TRUE(file_util::Delete(net_log_temp_file_->log_path(), false));
Suggest we use |net_export_log_| instead of net_log_temp_file_->log_path()
everywhere, so we aren't depending on any NetLogTempFile state.

https://codereview.chromium.org/11828036/diff/80057/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file_unittest.cc:101: TEST_F(NetLogTempFileTest,
ProcessCommandDoStart) {
Not sure this test really gets us anything that DoStop doesn't.

https://codereview.chromium.org/11828036/diff/80057/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file_unittest.cc:129: dict->GetString("state",
&state);
Should use EXPECT_TRUE on calls to dict->GetString.

https://codereview.chromium.org/11828036/diff/80057/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file_unittest.cc:164:
EXPECT_TRUE(file_util::GetFileSize(log_path, &start_file_size));
To keep the length of this test down, sugget deleting a bunch of these expects. 
Everything above this line, except perhaps the state check, GetState checks,
etc, would make this a much more focused test, and easier to follow.

https://codereview.chromium.org/11828036/diff/80057/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file_unittest.cc:185: log_path1,
junk_data.c_str(), junk_data.size()), 0);
Ahh, you weren't following me here...What I was suggesting was that, after the
next DO_START call, we do:

net_log_->AddEvent(net::NetLog::TYPE_CANCELLED);

And then make sure that, after we stop logging, the file is larger than a net
log created without that event.

Could do it in another test that's just a copy of this one.  This test makes
sure the sizes are the same when neither has any events, the other test would
make sure that when one log has events, it's larger (So presumably is correctly
logging events).

https://codereview.chromium.org/11828036/diff/80057/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file_unittest.cc:208: TEST_F(NetLogTempFileTest,
GetFilePath) {
Suggest merging this with InitAllowStart / InitAllowStartStop, for the broadest
coverage.  Check the results of GetFilePath after init, after start, and after
stop.

Could either get rid of this test, and add checks in the other two tests, or
split this test in two, and have one lie about file's initial existence.

This also covers the transition from ALLOW_START to ALLOW_STOP, which none of
these tests currently do.

[mmenke, 2013-01-23 19:05:33]

Sorry for all these rounds of review - most of these I should have noticed and
pointed out earlier.

[ramant (doing other things), 2013-01-24 03:23:55]

Hi Matt,
  Made all the changes (except one of the nits). PTAL.

thanks,
raman

https://codereview.chromium.org/11828036/diff/80057/chrome/browser/net/net_lo...
File chrome/browser/net/net_log_temp_file.cc (right):

https://codereview.chromium.org/11828036/diff/80057/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file.cc:23: void
NetLogTempFile::ProcessCommand(Command command) {
On 2013/01/23 19:01:27, Matt Menke wrote:
> Think you should DCHECK that all of these are called on the right thread.  Can
> just use TestBrowserThread to get the unit tests passing.

Done.

https://codereview.chromium.org/11828036/diff/80057/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file.cc:48: dict->SetString("file",
log_path_.MaybeAsASCII());
On 2013/01/23 19:01:27, Matt Menke wrote:
> nit:  Maybe LossyDisplayName() would be better?

Done.

https://codereview.chromium.org/11828036/diff/80057/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file.cc:72: state_ = STATE_ALLOW_START;
On 2013/01/23 19:01:27, Matt Menke wrote:
> If file_util::GetTempDir fails, we'll end up in STATE_ALLOW_START with an
> invalid log_path_.  If we want to handle GetTempDir failures correctly, we'll
> need to slightly rework things.
> 
> My suggestion:  Separate out getting the path and checking if the file exists,
> using different virtual functions for each.  Then STATE_ALLOW_START can just
> call GetNetExportLog (And update log_path_), and if it fails, indicating no
temp
> directory, exit early without changing the state.
> 
> GetFilePath would then just call the function to check for file existence with
> log_path_ and just copy log_path_ to path, which I think is a little cleaner
as
> well.

Done.

https://codereview.chromium.org/11828036/diff/80057/chrome/browser/net/net_lo...
File chrome/browser/net/net_log_temp_file.h (right):

https://codereview.chromium.org/11828036/diff/80057/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file.h:53: // send, then it returns false.
On 2013/01/23 19:01:27, Matt Menke wrote:
> nit:  Think it's worth mentioning that it also returns false while actively
> logging to the file.

Done.

https://codereview.chromium.org/11828036/diff/80057/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file.h:73: friend class CustomNetLogTempFile;
On 2013/01/23 19:01:27, Matt Menke wrote:
> Can we just make anything thing this class accesses protected, and unfriend
it? 
> I prefer to friend classes only when necessary (The other friends all allow us
> not to expose our innards externally.  Doesn't seem to be the case with
> CustomNetLogTempFile.

Made NetLogTempFile and NetExportLogExists so that unit tests can call
TestNetLogTempFile's methods.

Done.

https://codereview.chromium.org/11828036/diff/80057/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file.h:111: FilePath log_path() const { return
log_path_; }
On 2013/01/23 19:01:27, Matt Menke wrote:
> Think we can get rid of this, and not depend on it or log_path_ in the tests.

Done.

https://codereview.chromium.org/11828036/diff/80057/chrome/browser/net/net_lo...
File chrome/browser/net/net_log_temp_file_unittest.cc (right):

https://codereview.chromium.org/11828036/diff/80057/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file_unittest.cc:18: class CustomNetLogTempFile
: public NetLogTempFile {
On 2013/01/23 19:01:27, Matt Menke wrote:
> nit:  Think "TestNetLogTempFile" or "NetLogTempFileForTesting" is clearer than
> "CustomNetLogTempFile", particularly if we have to friend it in non-test code
> (Which I don't think we do, see other comment about that).

Done.

https://codereview.chromium.org/11828036/diff/80057/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file_unittest.cc:38: friend class
NetLogTempFileTest;
On 2013/01/23 19:01:27, Matt Menke wrote:
> Don't think we need to friend the test, can just make the constructor public.

Made constructor and NetExportLogExists public.

Done.

https://codereview.chromium.org/11828036/diff/80057/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file_unittest.cc:60:
net_log_temp_file_->log_filename_ = net_export_log_filename.value();
On 2013/01/23 19:01:27, Matt Menke wrote:
> nit:  Could just move this to the constructor, and move TearDown to the
> destructor.  Each execution of a test creates a new test fixture.

Would like to call ASSERT_TRUE to check the return value of CreateTemporaryFile.
Constructor doesn't seem to allow it.

https://codereview.chromium.org/11828036/diff/80057/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file_unittest.cc:65:
ASSERT_TRUE(file_util::Delete(net_log_temp_file_->log_path(), false));
On 2013/01/23 19:01:27, Matt Menke wrote:
> Suggest we use |net_export_log_| instead of net_log_temp_file_->log_path()
> everywhere, so we aren't depending on any NetLogTempFile state.

Done.

https://codereview.chromium.org/11828036/diff/80057/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file_unittest.cc:101: TEST_F(NetLogTempFileTest,
ProcessCommandDoStart) {
On 2013/01/23 19:01:27, Matt Menke wrote:
> Not sure this test really gets us anything that DoStop doesn't.

Merged the tests into ProcessCommandDoStartAndStop.

Done.

https://codereview.chromium.org/11828036/diff/80057/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file_unittest.cc:129: dict->GetString("state",
&state);
On 2013/01/23 19:01:27, Matt Menke wrote:
> Should use EXPECT_TRUE on calls to dict->GetString.

Done.

https://codereview.chromium.org/11828036/diff/80057/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file_unittest.cc:164:
EXPECT_TRUE(file_util::GetFileSize(log_path, &start_file_size));
On 2013/01/23 19:01:27, Matt Menke wrote:
> To keep the length of this test down, sugget deleting a bunch of these
expects. 
> Everything above this line, except perhaps the state check, GetState checks,
> etc, would make this a much more focused test, and easier to follow.

Done.

https://codereview.chromium.org/11828036/diff/80057/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file_unittest.cc:185: log_path1,
junk_data.c_str(), junk_data.size()), 0);
On 2013/01/23 19:01:27, Matt Menke wrote:
> Ahh, you weren't following me here...What I was suggesting was that, after the
> next DO_START call, we do:
> 
> net_log_->AddEvent(net::NetLog::TYPE_CANCELLED);
> 
> And then make sure that, after we stop logging, the file is larger than a net
> log created without that event.
> 
> Could do it in another test that's just a copy of this one.  This test makes
> sure the sizes are the same when neither has any events, the other test would
> make sure that when one log has events, it's larger (So presumably is
correctly
> logging events).

Done.

https://codereview.chromium.org/11828036/diff/80057/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file_unittest.cc:208: TEST_F(NetLogTempFileTest,
GetFilePath) {
On 2013/01/23 19:01:27, Matt Menke wrote:
> Suggest merging this with InitAllowStart / InitAllowStartStop, for the
broadest
> coverage.  Check the results of GetFilePath after init, after start, and after
> stop.
> 
> Could either get rid of this test, and add checks in the other two tests, or
> split this test in two, and have one lie about file's initial existence.
> 
> This also covers the transition from ALLOW_START to ALLOW_STOP, which none of
> these tests currently do.

Done.

[mmenke, 2013-01-24 03:55:34]

https://codereview.chromium.org/11828036/diff/80057/chrome/browser/net/net_lo...
File chrome/browser/net/net_log_temp_file_unittest.cc (right):

https://codereview.chromium.org/11828036/diff/80057/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file_unittest.cc:60:
net_log_temp_file_->log_filename_ = net_export_log_filename.value();
On 2013/01/24 03:23:55, ramant wrote:
> On 2013/01/23 19:01:27, Matt Menke wrote:
> > nit:  Could just move this to the constructor, and move TearDown to the
> > destructor.  Each execution of a test creates a new test fixture.
> 
> Would like to call ASSERT_TRUE to check the return value of
CreateTemporaryFile.
> Constructor doesn't seem to allow it.

Didn't realize that, fine as-is, then.

https://codereview.chromium.org/11828036/diff/89028/chrome/browser/net/net_lo...
File chrome/browser/net/net_log_temp_file.cc (right):

https://codereview.chromium.org/11828036/diff/89028/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file.cc:88: 
Could you add a:

if (!GetNetExportLog())
  return;

here?  Currently, if it fails in Init(), we end up trying to open an empty path,
which just seems like a bad idea.

https://codereview.chromium.org/11828036/diff/89028/chrome/browser/net/net_lo...
File chrome/browser/net/net_log_temp_file.h (right):

https://codereview.chromium.org/11828036/diff/89028/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file.h:49: explicit NetLogTempFile(ChromeNetLog*
chrome_net_log);
This can still be protected, can't it?

https://codereview.chromium.org/11828036/diff/89028/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file.h:62: virtual bool NetExportLogExists();
This can be protected, too - if a subclass overrides a protected function with a
public one, anyone with a subclass pointer can call the subclass's version of
the function.

https://codereview.chromium.org/11828036/diff/89028/chrome/browser/net/net_lo...
File chrome/browser/net/net_log_temp_file_unittest.cc (right):

https://codereview.chromium.org/11828036/diff/89028/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file_unittest.cc:25: 
nit:  "NetLogTempFile implementation:"

https://codereview.chromium.org/11828036/diff/89028/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file_unittest.cc:26: bool NetExportLogExists() {
Add virtual and OVERRIDE

https://codereview.chromium.org/11828036/diff/89028/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file_unittest.cc:48: 
nit:  ::testing::Test implementation:

https://codereview.chromium.org/11828036/diff/89028/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file_unittest.cc:51:
ASSERT_TRUE(file_util::CreateTemporaryFile(&net_export_log_));
Are we guaranteed this will be in the file_util::GetTempDir directory?

https://codereview.chromium.org/11828036/diff/89028/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file_unittest.cc:94:
EXPECT_TRUE(file_util::PathExists(net_export_log_));
So we can't rely on path equality, as a string, on Windows?

https://codereview.chromium.org/11828036/diff/89028/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file_unittest.cc:105: base::DictionaryValue*
dict = net_log_temp_file_->GetState();
These are leaks.  Should store these in scoped_ptrs.

https://codereview.chromium.org/11828036/diff/89028/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file_unittest.cc:206:
EXPECT_TRUE(file_util::GetFileSize(net_export_log_, &new_stop_file_size));
Are we guaranteed that file_util will see the larger file size when there's an
in-progress write to an open handle?

[mmenke, 2013-01-24 18:07:39]

One more thing...Should we have the tab available on desktop/iOS?  I had
initially thought yes, but given that all we do is create a file in the temp
directory and then never delete it, and hope someone else cleans it up, and
present an email button that doesn't work, perhaps we should disable it on other
platforms until have have something working.

[ramant (doing other things), 2013-01-24 21:56:49]

Could you please look at this change one more time?

https://codereview.chromium.org/11828036/diff/89028/chrome/browser/net/net_lo...
File chrome/browser/net/net_log_temp_file.cc (right):

https://codereview.chromium.org/11828036/diff/89028/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file.cc:88: 
On 2013/01/24 03:55:34, Matt Menke wrote:
> Could you add a:
> 
> if (!GetNetExportLog())
>   return;
> 
> here?  Currently, if it fails in Init(), we end up trying to open an empty
path,
> which just seems like a bad idea.

Good point. Added the following checks and state_ will not change from
STATE_UNINITIALIZED if log_path_ is not set.

  DCHECK_NE(state_, STATE_UNINITIALIZED);
  DCHECK(!log_path_.empty());

https://codereview.chromium.org/11828036/diff/89028/chrome/browser/net/net_lo...
File chrome/browser/net/net_log_temp_file.h (right):

https://codereview.chromium.org/11828036/diff/89028/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file.h:49: explicit NetLogTempFile(ChromeNetLog*
chrome_net_log);
On 2013/01/24 03:55:34, Matt Menke wrote:
> This can still be protected, can't it?

Done.

https://codereview.chromium.org/11828036/diff/89028/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file.h:62: virtual bool NetExportLogExists();
On 2013/01/24 03:55:34, Matt Menke wrote:
> This can be protected, too - if a subclass overrides a protected function with
a
> public one, anyone with a subclass pointer can call the subclass's version of
> the function.

Done.

https://codereview.chromium.org/11828036/diff/89028/chrome/browser/net/net_lo...
File chrome/browser/net/net_log_temp_file_unittest.cc (right):

https://codereview.chromium.org/11828036/diff/89028/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file_unittest.cc:25: 
On 2013/01/24 03:55:34, Matt Menke wrote:
> nit:  "NetLogTempFile implementation:"

Done.

https://codereview.chromium.org/11828036/diff/89028/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file_unittest.cc:26: bool NetExportLogExists() {
On 2013/01/24 03:55:34, Matt Menke wrote:
> Add virtual and OVERRIDE

Done.

https://codereview.chromium.org/11828036/diff/89028/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file_unittest.cc:48: 
On 2013/01/24 03:55:34, Matt Menke wrote:
> nit:  ::testing::Test implementation:

Done.

https://codereview.chromium.org/11828036/diff/89028/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file_unittest.cc:51:
ASSERT_TRUE(file_util::CreateTemporaryFile(&net_export_log_));
On 2013/01/24 03:55:34, Matt Menke wrote:
> Are we guaranteed this will be in the file_util::GetTempDir directory?

Good point.

Added GetNetExportLogDirectory (to get TempDir) and created the temporary file
in that directory.

Done.

https://codereview.chromium.org/11828036/diff/89028/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file_unittest.cc:94:
EXPECT_TRUE(file_util::PathExists(net_export_log_));
On 2013/01/24 03:55:34, Matt Menke wrote:
> So we can't rely on path equality, as a string, on Windows?

Correct.

https://codereview.chromium.org/11828036/diff/89028/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file_unittest.cc:105: base::DictionaryValue*
dict = net_log_temp_file_->GetState();
On 2013/01/24 03:55:34, Matt Menke wrote:
> These are leaks.  Should store these in scoped_ptrs.

Many thanks for catching this.
Done.

https://codereview.chromium.org/11828036/diff/89028/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file_unittest.cc:206:
EXPECT_TRUE(file_util::GetFileSize(net_export_log_, &new_stop_file_size));
On 2013/01/24 03:55:34, Matt Menke wrote:
> Are we guaranteed that file_util will see the larger file size when there's an
> in-progress write to an open handle?

We delete net_log_logger in DoStop, which closes the File handle. Thus checking
the file sizes after DO_STOP is ok.

[mmenke, 2013-01-25 17:39:11]

Still want to do a final pass, particularly looking for missing tests, but only
one of these changes is more than cosmetic.

https://codereview.chromium.org/11828036/diff/84043/chrome/browser/net/net_lo...
File chrome/browser/net/net_log_temp_file.cc (right):

https://codereview.chromium.org/11828036/diff/84043/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file.cc:29: if (!Init())
Suggest renaming this to "EnsureInit()" to better reflect the new behavior.

With this change, think we can remove the "Determine the state_ if it is not
set." comments wherever they appear, too.

https://codereview.chromium.org/11828036/diff/84043/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file.cc:56: switch (state_) {
Think we should always return a state, even when Init() fails, and then display
some sort of error on the page.  Don't think this is likely to happen, but think
it's worth displaying something when it does.

https://codereview.chromium.org/11828036/diff/84043/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file.cc:97: if (!fp)
Wonder if we should surface some error to the user in this case.  Not worth
worrying about in this CL.

https://codereview.chromium.org/11828036/diff/84043/chrome/browser/net/net_lo...
File chrome/browser/net/net_log_temp_file.h (right):

https://codereview.chromium.org/11828036/diff/84043/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file.h:13: #include "base/memory/ref_counted.h"
Don't think we need this.

https://codereview.chromium.org/11828036/diff/84043/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file.h:21: class ChromeNetLog;
nit:  Alphabetize.

https://codereview.chromium.org/11828036/diff/84043/chrome/browser/net/net_lo...
File chrome/browser/net/net_log_temp_file_unittest.cc (right):

https://codereview.chromium.org/11828036/diff/84043/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file_unittest.cc:47: net_log_temp_file_ = new
TestNetLogTempFile(net_log_.get());
Can put these in the initializer list.

https://codereview.chromium.org/11828036/diff/84043/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file_unittest.cc:67: // ::testing::Test
implementation:
nit:  Not needed.  Generally just have one comment above the first function of
the implementation, and all functions together.

https://codereview.chromium.org/11828036/diff/84043/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file_unittest.cc:80: void VerifyNetExportLog() {
Think the three "verify" functions are worth commenting.  The non-empty file
checks, in particular, are non-obvious.

For this one, for instance, I suggest "Make sure the export file has been
created and is non-empty, as net constants will always be written to it on
creation."

For the others, just mention successful export file initialization.

https://codereview.chromium.org/11828036/diff/84043/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file_unittest.cc:82: // convert the path to
include legacy 8.3 filenames.
Looks like we are comparing entire paths here now.

https://codereview.chromium.org/11828036/diff/84043/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file_unittest.cc:91: void
VerifyStateAfterDoStart() {
Suggest "VertifyFileAndStateAfterDoStart()", and the corresponding change to the
other.

https://codereview.chromium.org/11828036/diff/84043/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file_unittest.cc:107: TestNetLogTempFile*
net_log_temp_file_;
Should have a note that this must go after the net_log_, so it can stop
obvserving on destruction.

https://codereview.chromium.org/11828036/diff/84043/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file_unittest.cc:107: TestNetLogTempFile*
net_log_temp_file_;
This should be a scoped_ptr.  Currently, it leaks.

[ramant (doing other things), 2013-01-25 20:05:24]

As we had talked, added tests to test the failure clauses for EnsureInit()
method. PTAL. thanks.

https://codereview.chromium.org/11828036/diff/84043/chrome/browser/net/net_lo...
File chrome/browser/net/net_log_temp_file.cc (right):

https://codereview.chromium.org/11828036/diff/84043/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file.cc:29: if (!Init())
On 2013/01/25 17:39:12, Matt Menke wrote:
> Suggest renaming this to "EnsureInit()" to better reflect the new behavior.
> 
> With this change, think we can remove the "Determine the state_ if it is not
> set." comments wherever they appear, too.

Done.

https://codereview.chromium.org/11828036/diff/84043/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file.cc:56: switch (state_) {
On 2013/01/25 17:39:12, Matt Menke wrote:
> Think we should always return a state, even when Init() fails, and then
display
> some sort of error on the page.  Don't think this is likely to happen, but
think
> it's worth displaying something when it does.

Done.

https://codereview.chromium.org/11828036/diff/84043/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file.cc:97: if (!fp)
On 2013/01/25 17:39:12, Matt Menke wrote:
> Wonder if we should surface some error to the user in this case.  Not worth
> worrying about in this CL.

Added this to TODO.

https://codereview.chromium.org/11828036/diff/84043/chrome/browser/net/net_lo...
File chrome/browser/net/net_log_temp_file.h (right):

https://codereview.chromium.org/11828036/diff/84043/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file.h:13: #include "base/memory/ref_counted.h"
On 2013/01/25 17:39:12, Matt Menke wrote:
> Don't think we need this.

Done.

https://codereview.chromium.org/11828036/diff/84043/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file.h:21: class ChromeNetLog;
On 2013/01/25 17:39:12, Matt Menke wrote:
> nit:  Alphabetize.

Done.

https://codereview.chromium.org/11828036/diff/84043/chrome/browser/net/net_lo...
File chrome/browser/net/net_log_temp_file_unittest.cc (right):

https://codereview.chromium.org/11828036/diff/84043/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file_unittest.cc:47: net_log_temp_file_ = new
TestNetLogTempFile(net_log_.get());
On 2013/01/25 17:39:12, Matt Menke wrote:
> Can put these in the initializer list.

Done.

https://codereview.chromium.org/11828036/diff/84043/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file_unittest.cc:67: // ::testing::Test
implementation:
On 2013/01/25 17:39:12, Matt Menke wrote:
> nit:  Not needed.  Generally just have one comment above the first function of
> the implementation, and all functions together.

Done.

https://codereview.chromium.org/11828036/diff/84043/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file_unittest.cc:80: void VerifyNetExportLog() {
On 2013/01/25 17:39:12, Matt Menke wrote:
> Think the three "verify" functions are worth commenting.  The non-empty file
> checks, in particular, are non-obvious.
> 
> For this one, for instance, I suggest "Make sure the export file has been
> created and is non-empty, as net constants will always be written to it on
> creation."
> 
> For the others, just mention successful export file initialization.

Done.

https://codereview.chromium.org/11828036/diff/84043/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file_unittest.cc:82: // convert the path to
include legacy 8.3 filenames.
On 2013/01/25 17:39:12, Matt Menke wrote:
> Looks like we are comparing entire paths here now.

Deleted the comment.
Done.

https://codereview.chromium.org/11828036/diff/84043/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file_unittest.cc:91: void
VerifyStateAfterDoStart() {
On 2013/01/25 17:39:12, Matt Menke wrote:
> Suggest "VertifyFileAndStateAfterDoStart()", and the corresponding change to
the
> other.

Done.

https://codereview.chromium.org/11828036/diff/84043/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file_unittest.cc:107: TestNetLogTempFile*
net_log_temp_file_;
On 2013/01/25 17:39:12, Matt Menke wrote:
> Should have a note that this must go after the net_log_, so it can stop
> obvserving on destruction.

Done.

https://codereview.chromium.org/11828036/diff/84043/chrome/browser/net/net_lo...
chrome/browser/net/net_log_temp_file_unittest.cc:107: TestNetLogTempFile*
net_log_temp_file_;
On 2013/01/25 17:39:12, Matt Menke wrote:
> This should be a scoped_ptr.  Currently, it leaks.

Done.

[mmenke, 2013-01-28 21:51:40]

LGTM.  All but the last are pretty much nits.

https://codereview.chromium.org/11828036/diff/111010/chrome/browser/net/net_l...
File chrome/browser/net/net_log_temp_file.cc (right):

https://codereview.chromium.org/11828036/diff/111010/chrome/browser/net/net_l...
chrome/browser/net/net_log_temp_file.cc:22: if (net_log_logger_.get())
nit:  .get() not needed (And the preferred for is to leave it off)

https://codereview.chromium.org/11828036/diff/111010/chrome/browser/net/net_l...
chrome/browser/net/net_log_temp_file.cc:38: default:
NOTREACHED()?

https://codereview.chromium.org/11828036/diff/111010/chrome/browser/net/net_l...
chrome/browser/net/net_log_temp_file.cc:47: ignore_result(EnsureInit());
I don't beloeve ignore_result is needed unless WARN_UNUSED_RESULT is used in the
function declaration.

https://codereview.chromium.org/11828036/diff/111010/chrome/browser/net/net_l...
chrome/browser/net/net_log_temp_file.cc:91: DCHECK_NE(state_,
STATE_UNINITIALIZED);
nit:  Expected goes first, so this should be:

DCHECK_NE(STATE_UNINITIALIZED, state_);

https://codereview.chromium.org/11828036/diff/111010/chrome/browser/net/net_l...
chrome/browser/net/net_log_temp_file.cc:124: 
optional:  DCHECK(!log_path_.empty());

https://codereview.chromium.org/11828036/diff/111010/chrome/browser/net/net_l...
chrome/browser/net/net_log_temp_file.cc:151:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE_USER_BLOCKING));
DCHECK_NE(STATE_UNINITIALIZED, state_);
DCHECK(!log_path_.empty());

(Alternatively, could just return false if the first isn't true, and then have
the second check).

https://codereview.chromium.org/11828036/diff/111010/chrome/browser/net/net_l...
File chrome/browser/net/net_log_temp_file.h (right):

https://codereview.chromium.org/11828036/diff/111010/chrome/browser/net/net_l...
chrome/browser/net/net_log_temp_file.h:43: DO_STOP,           // Call StopLog.
nit:  Don't think you need quite all that whitespace

https://codereview.chromium.org/11828036/diff/111010/chrome/browser/net/net_l...
File chrome/browser/net/net_log_temp_file_unittest.cc (right):

https://codereview.chromium.org/11828036/diff/111010/chrome/browser/net/net_l...
chrome/browser/net/net_log_temp_file_unittest.cc:55: protected:
nit:  While this works, it's more common just to make these public.

https://codereview.chromium.org/11828036/diff/111010/chrome/browser/net/net_l...
chrome/browser/net/net_log_temp_file_unittest.cc:75:
net_log_temp_file_->GetNetExportLog();
ASSERT_TRUE?

https://codereview.chromium.org/11828036/diff/111010/chrome/browser/net/net_l...
chrome/browser/net/net_log_temp_file_unittest.cc:76: net_export_log_ =
net_log_temp_file_->log_path_;
ASSERT not empty?

https://codereview.chromium.org/11828036/diff/111010/chrome/browser/net/net_l...
chrome/browser/net/net_log_temp_file_unittest.cc:124: // Make sure the export
file has been succeffully initialized.
nit:  successfully

https://codereview.chromium.org/11828036/diff/111010/chrome/browser/net/net_l...
chrome/browser/net/net_log_temp_file_unittest.cc:136: // Make sure the export
file has been succeffully initialized.
nit:  successfully

https://codereview.chromium.org/11828036/diff/111010/chrome/browser/net/net_l...
chrome/browser/net/net_log_temp_file_unittest.cc:144:
EXPECT_TRUE(file_util::PathExists(net_export_file_path));
EXPECT_EQ(net_export_log_, net_export_file_path);

https://codereview.chromium.org/11828036/diff/111010/chrome/browser/net/net_l...
chrome/browser/net/net_log_temp_file_unittest.cc:192:
EXPECT_TRUE(file_util::PathExists(net_export_file_path));
EXPECT_EQ(net_export_log_, net_export_file_path);

https://codereview.chromium.org/11828036/diff/111010/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/net_export_ui.cc (right):

https://codereview.chromium.org/11828036/diff/111010/chrome/browser/ui/webui/...
chrome/browser/ui/webui/net_export_ui.cc:49: // this class's public methods are
expected to run on the UI thread.
Think it's worth mentioning all the functions that run on another thread are
static.

https://codereview.chromium.org/11828036/diff/111010/chrome/browser/ui/webui/...
chrome/browser/ui/webui/net_export_ui.cc:156:
content::BrowserThread::PostTaskAndReply(
If you use PostTaskAndReplyWithResult, you can make GetNetLogFileName return the
FilePath, and get rid of file_to_send here.

https://codereview.chromium.org/11828036/diff/111010/chrome/browser/ui/webui/...
chrome/browser/ui/webui/net_export_ui.cc:216:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
if (file_to_send_arg->empty())
  return;

(Can't ensure the log hasn't started, so can't DCHECK)

[ramant (doing other things), 2013-01-29 04:48:36]

https://codereview.chromium.org/11828036/diff/111010/chrome/browser/net/net_l...
File chrome/browser/net/net_log_temp_file.cc (right):

https://codereview.chromium.org/11828036/diff/111010/chrome/browser/net/net_l...
chrome/browser/net/net_log_temp_file.cc:22: if (net_log_logger_.get())
On 2013/01/28 21:51:41, Matt Menke wrote:
> nit:  .get() not needed (And the preferred for is to leave it off)

Done.

https://codereview.chromium.org/11828036/diff/111010/chrome/browser/net/net_l...
chrome/browser/net/net_log_temp_file.cc:38: default:
On 2013/01/28 21:51:41, Matt Menke wrote:
> NOTREACHED()?

Done.

https://codereview.chromium.org/11828036/diff/111010/chrome/browser/net/net_l...
chrome/browser/net/net_log_temp_file.cc:47: ignore_result(EnsureInit());
On 2013/01/28 21:51:41, Matt Menke wrote:
> I don't beloeve ignore_result is needed unless WARN_UNUSED_RESULT is used in
the
> function declaration.

Done.

https://codereview.chromium.org/11828036/diff/111010/chrome/browser/net/net_l...
chrome/browser/net/net_log_temp_file.cc:91: DCHECK_NE(state_,
STATE_UNINITIALIZED);
On 2013/01/28 21:51:41, Matt Menke wrote:
> nit:  Expected goes first, so this should be:
> 
> DCHECK_NE(STATE_UNINITIALIZED, state_);

Done.

https://codereview.chromium.org/11828036/diff/111010/chrome/browser/net/net_l...
chrome/browser/net/net_log_temp_file.cc:124: 
On 2013/01/28 21:51:41, Matt Menke wrote:
> optional:  DCHECK(!log_path_.empty());

Done.

https://codereview.chromium.org/11828036/diff/111010/chrome/browser/net/net_l...
chrome/browser/net/net_log_temp_file.cc:151:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE_USER_BLOCKING));
On 2013/01/28 21:51:41, Matt Menke wrote:
> DCHECK_NE(STATE_UNINITIALIZED, state_);
> DCHECK(!log_path_.empty());
> 
> (Alternatively, could just return false if the first isn't true, and then have
> the second check).

Added "DCHECK(!log_path_.empty());".

This function could be called when state_ is STATE_UNINITIALIZED from ensureInit
to determine the state_.

The function could be called when state_ is not STATE_UNINITIALIZED (when state_
is STATE_ALLOW_START_SEND) from GetFilePath().  GetFilePath is called when user
clicks on Send. If the file doesn't exist SendEmail doesn't attach the file and
doesn't send email (if user manually deletes the file from the storage area. Not
sure if that is possible on mobile device).

Thus didn't add the first DCHECK.

https://codereview.chromium.org/11828036/diff/111010/chrome/browser/net/net_l...
File chrome/browser/net/net_log_temp_file.h (right):

https://codereview.chromium.org/11828036/diff/111010/chrome/browser/net/net_l...
chrome/browser/net/net_log_temp_file.h:43: DO_STOP,           // Call StopLog.
On 2013/01/28 21:51:41, Matt Menke wrote:
> nit:  Don't think you need quite all that whitespace

Done.

https://codereview.chromium.org/11828036/diff/111010/chrome/browser/net/net_l...
File chrome/browser/net/net_log_temp_file_unittest.cc (right):

https://codereview.chromium.org/11828036/diff/111010/chrome/browser/net/net_l...
chrome/browser/net/net_log_temp_file_unittest.cc:55: protected:
On 2013/01/28 21:51:41, Matt Menke wrote:
> nit:  While this works, it's more common just to make these public.

Done.

https://codereview.chromium.org/11828036/diff/111010/chrome/browser/net/net_l...
chrome/browser/net/net_log_temp_file_unittest.cc:75:
net_log_temp_file_->GetNetExportLog();
On 2013/01/28 21:51:41, Matt Menke wrote:
> ASSERT_TRUE?

Done.

https://codereview.chromium.org/11828036/diff/111010/chrome/browser/net/net_l...
chrome/browser/net/net_log_temp_file_unittest.cc:76: net_export_log_ =
net_log_temp_file_->log_path_;
On 2013/01/28 21:51:41, Matt Menke wrote:
> ASSERT not empty?

Done.

https://codereview.chromium.org/11828036/diff/111010/chrome/browser/net/net_l...
chrome/browser/net/net_log_temp_file_unittest.cc:124: // Make sure the export
file has been succeffully initialized.
On 2013/01/28 21:51:41, Matt Menke wrote:
> nit:  successfully

Done.

https://codereview.chromium.org/11828036/diff/111010/chrome/browser/net/net_l...
chrome/browser/net/net_log_temp_file_unittest.cc:136: // Make sure the export
file has been succeffully initialized.
On 2013/01/28 21:51:41, Matt Menke wrote:
> nit:  successfully

Done.

https://codereview.chromium.org/11828036/diff/111010/chrome/browser/net/net_l...
chrome/browser/net/net_log_temp_file_unittest.cc:144:
EXPECT_TRUE(file_util::PathExists(net_export_file_path));
On 2013/01/28 21:51:41, Matt Menke wrote:
> EXPECT_EQ(net_export_log_, net_export_file_path);

Done.

https://codereview.chromium.org/11828036/diff/111010/chrome/browser/net/net_l...
chrome/browser/net/net_log_temp_file_unittest.cc:192:
EXPECT_TRUE(file_util::PathExists(net_export_file_path));
On 2013/01/28 21:51:41, Matt Menke wrote:
> EXPECT_EQ(net_export_log_, net_export_file_path);

Done.

https://codereview.chromium.org/11828036/diff/111010/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/net_export_ui.cc (right):

https://codereview.chromium.org/11828036/diff/111010/chrome/browser/ui/webui/...
chrome/browser/ui/webui/net_export_ui.cc:49: // this class's public methods are
expected to run on the UI thread.
On 2013/01/28 21:51:41, Matt Menke wrote:
> Think it's worth mentioning all the functions that run on another thread are
> static.

Done.

https://codereview.chromium.org/11828036/diff/111010/chrome/browser/ui/webui/...
chrome/browser/ui/webui/net_export_ui.cc:156:
content::BrowserThread::PostTaskAndReply(
On 2013/01/28 21:51:41, Matt Menke wrote:
> If you use PostTaskAndReplyWithResult, you can make GetNetLogFileName return
the
> FilePath, and get rid of file_to_send here.

Done.

https://codereview.chromium.org/11828036/diff/111010/chrome/browser/ui/webui/...
chrome/browser/ui/webui/net_export_ui.cc:216:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
On 2013/01/28 21:51:41, Matt Menke wrote:
> if (file_to_send_arg->empty())
>   return;
> 
> (Can't ensure the log hasn't started, so can't DCHECK)

Done.

[ramant (doing other things), 2013-01-29 04:51:03]

jhawkins@ can I get owner's rubber stamp please? thanks

[James Hawkins, 2013-01-29 22:43:04]

On 2013/01/29 04:51:03, ramant wrote:
> jhawkins@ can I get owner's rubber stamp please? thanks

No, but you can get a real review :-)

[James Hawkins, 2013-01-29 22:48:34]

https://codereview.chromium.org/11828036/diff/129007/chrome/browser/resources...
File chrome/browser/resources/net_export/net_export.html (right):

https://codereview.chromium.org/11828036/diff/129007/chrome/browser/resources...
chrome/browser/resources/net_export/net_export.html:9: <style>
Move styling to CSS file.

https://codereview.chromium.org/11828036/diff/129007/chrome/browser/resources...
chrome/browser/resources/net_export/net_export.html:29: <div style="margin:
8px">
Remove inline styling (use CSS, id or class instead).

https://codereview.chromium.org/11828036/diff/129007/chrome/browser/resources...
chrome/browser/resources/net_export/net_export.html:41: 
nit: Remove blank line.

https://codereview.chromium.org/11828036/diff/129007/chrome/browser/resources...
File chrome/browser/resources/net_export/net_export.js (right):

https://codereview.chromium.org/11828036/diff/129007/chrome/browser/resources...
chrome/browser/resources/net_export/net_export.js:64: chrome.send('sendNetLog');
Just inline chrome.send instead of using this bridge.

https://codereview.chromium.org/11828036/diff/129007/chrome/browser/resources...
chrome/browser/resources/net_export/net_export.js:138: message = 'NetLog data is
collected in: ';
nit: No braces on single-line blocks.

https://codereview.chromium.org/11828036/diff/129007/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/net_export_ui.cc (right):

https://codereview.chromium.org/11828036/diff/129007/chrome/browser/ui/webui/...
chrome/browser/ui/webui/net_export_ui.cc:74: static FilePath
GetNetLogFileName(NetLogTempFile* net_log_temp_file);
nit: Document method.

https://codereview.chromium.org/11828036/diff/129007/chrome/browser/ui/webui/...
chrome/browser/ui/webui/net_export_ui.cc:76: // Helper function to send
state/file information from NetLogTempFile.
Optional nit: No need to actually say 'Helper function'. Just say what it does
like any other method.

[ramant (doing other things), 2013-01-30 02:35:42]

Many many thanks jhawkins@. Made all the changes you had suggested. PTAL.

https://codereview.chromium.org/11828036/diff/129007/chrome/browser/resources...
File chrome/browser/resources/net_export/net_export.html (right):

https://codereview.chromium.org/11828036/diff/129007/chrome/browser/resources...
chrome/browser/resources/net_export/net_export.html:9: <style>
On 2013/01/29 22:48:34, James Hawkins wrote:
> Move styling to CSS file.

Done.

https://codereview.chromium.org/11828036/diff/129007/chrome/browser/resources...
chrome/browser/resources/net_export/net_export.html:29: <div style="margin:
8px">
On 2013/01/29 22:48:34, James Hawkins wrote:
> Remove inline styling (use CSS, id or class instead).

Done.

https://codereview.chromium.org/11828036/diff/129007/chrome/browser/resources...
chrome/browser/resources/net_export/net_export.html:41: 
On 2013/01/29 22:48:34, James Hawkins wrote:
> nit: Remove blank line.

Done.

https://codereview.chromium.org/11828036/diff/129007/chrome/browser/resources...
File chrome/browser/resources/net_export/net_export.js (right):

https://codereview.chromium.org/11828036/diff/129007/chrome/browser/resources...
chrome/browser/resources/net_export/net_export.js:64: chrome.send('sendNetLog');
On 2013/01/29 22:48:34, James Hawkins wrote:
> Just inline chrome.send instead of using this bridge.

Done.

https://codereview.chromium.org/11828036/diff/129007/chrome/browser/resources...
chrome/browser/resources/net_export/net_export.js:138: message = 'NetLog data is
collected in: ';
On 2013/01/29 22:48:34, James Hawkins wrote:
> nit: No braces on single-line blocks.

Done.

https://codereview.chromium.org/11828036/diff/129007/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/net_export_ui.cc (right):

https://codereview.chromium.org/11828036/diff/129007/chrome/browser/ui/webui/...
chrome/browser/ui/webui/net_export_ui.cc:74: static FilePath
GetNetLogFileName(NetLogTempFile* net_log_temp_file);
On 2013/01/29 22:48:34, James Hawkins wrote:
> nit: Document method.

Done.

https://codereview.chromium.org/11828036/diff/129007/chrome/browser/ui/webui/...
chrome/browser/ui/webui/net_export_ui.cc:76: // Helper function to send
state/file information from NetLogTempFile.
On 2013/01/29 22:48:34, James Hawkins wrote:
> Optional nit: No need to actually say 'Helper function'. Just say what it does
> like any other method.

Done.

[James Hawkins, 2013-01-31 18:58:46]

https://codereview.chromium.org/11828036/diff/130058/chrome/browser/resources...
File chrome/browser/resources/net_export/net_export.html (right):

https://codereview.chromium.org/11828036/diff/130058/chrome/browser/resources...
chrome/browser/resources/net_export/net_export.html:9: 
nit: Remove blank line.

https://codereview.chromium.org/11828036/diff/130058/chrome/browser/resources...
chrome/browser/resources/net_export/net_export.html:15: <button
id=export-view-start-data>Start Logging to Disk</button>
nit: Quotes around id name.  Here and elsewhere.

https://codereview.chromium.org/11828036/diff/130058/chrome/browser/resources...
File chrome/browser/resources/net_export/net_export.js (right):

https://codereview.chromium.org/11828036/diff/130058/chrome/browser/resources...
chrome/browser/resources/net_export/net_export.js:41: receivedData:
function(data) {
Same thing: why do you need a 'bridge'?

https://codereview.chromium.org/11828036/diff/130058/chrome/browser/resources...
chrome/browser/resources/net_export/net_export.js:65: var START_DATA_BUTTON_ID =
'export-view-start-data';
/** @const */

https://codereview.chromium.org/11828036/diff/130058/chrome/browser/resources...
chrome/browser/resources/net_export/net_export.js:110:
onExportNetLogInfoChanged: function(exportNetLogInfo) {
nit: Document method.

[ramant (doing other things), 2013-01-31 19:58:26]

Hi James Hawkins,
  Made all the changes you have suggested. PTAL.

https://codereview.chromium.org/11828036/diff/130058/chrome/browser/resources...
File chrome/browser/resources/net_export/net_export.html (right):

https://codereview.chromium.org/11828036/diff/130058/chrome/browser/resources...
chrome/browser/resources/net_export/net_export.html:9: 
On 2013/01/31 18:58:46, James Hawkins wrote:
> nit: Remove blank line.

Done.

https://codereview.chromium.org/11828036/diff/130058/chrome/browser/resources...
chrome/browser/resources/net_export/net_export.html:15: <button
id=export-view-start-data>Start Logging to Disk</button>
On 2013/01/31 18:58:46, James Hawkins wrote:
> nit: Quotes around id name.  Here and elsewhere.

Done.

https://codereview.chromium.org/11828036/diff/130058/chrome/browser/resources...
File chrome/browser/resources/net_export/net_export.js (right):

https://codereview.chromium.org/11828036/diff/130058/chrome/browser/resources...
chrome/browser/resources/net_export/net_export.js:41: receivedData:
function(data) {
On 2013/01/31 18:58:46, James Hawkins wrote:
> Same thing: why do you need a 'bridge'?

Called the JS function directly from C++ code.

Done.

https://codereview.chromium.org/11828036/diff/130058/chrome/browser/resources...
chrome/browser/resources/net_export/net_export.js:65: var START_DATA_BUTTON_ID =
'export-view-start-data';
On 2013/01/31 18:58:46, James Hawkins wrote:
> /** @const */

Done.

https://codereview.chromium.org/11828036/diff/130058/chrome/browser/resources...
chrome/browser/resources/net_export/net_export.js:110:
onExportNetLogInfoChanged: function(exportNetLogInfo) {
On 2013/01/31 18:58:46, James Hawkins wrote:
> nit: Document method.

Done.

[James Hawkins, 2013-01-31 20:00:16]

lgtm

[commit-bot: I haz the power, 2013-01-31 21:14:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rtenneti@chromium.org/11828036/127037

[commit-bot: I haz the power, 2013-01-31 21:14:41]

Presubmit check for 11828036-127037 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
The file build/ios/grit_whitelist.txt must be sorted.

Presubmit checks took 2.8s to calculate.

[commit-bot: I haz the power, 2013-01-31 21:27:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rtenneti@chromium.org/11828036/132033

[commit-bot: I haz the power, 2013-02-01 01:11:46]

Change committed as 180026