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

chrome/browser/net/net_log_temp_file.cc

+// Copyright (c) 2013 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/net/net_log_temp_file.h"
+
+#include <stdio.h>
+
+#include "base/bind.h"
+#include "base/file_util.h"
+#include "base/message_loop.h"
+#include "base/string_number_conversions.h"

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

This isn't used here, right?

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

Done.

+#include "base/string_util.h"
+#include "base/utf_string_conversions.h"
+#include "base/values.h"
+#include "chrome/browser/browser_process.h"
+#include "chrome/browser/net/chrome_net_log.h"
+#include "chrome/browser/net/net_log_logger.h"
+#include "content/public/browser/browser_thread.h"
+
+#if defined(OS_ANDROID)
+#include "chrome/browser/android/intent_helper.h"
+#endif
+
+using content::BrowserThread;
+
+NetLogTempFile::NetLogTempFile() : state_(UNINITIALIZED) {
+}
+
+NetLogTempFile::~NetLogTempFile() {
+}
+
+void NetLogTempFile::ProcessCommand(Command command) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE_USER_BLOCKING));

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

Out of paranoia, suggest an Init() call here if uninitialized.

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

Done.

+  switch (command) {
+    case DO_START:
+      StartNetLog();
+      break;
+    case DO_STOP:
+      StopNetLog();
+      break;
+    case DO_SEND:
+      SendNetLog();
+      break;
+    default:
+      break;
+  }
+}
+
+Value* NetLogTempFile::NetLogTempFileToValue() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE_USER_BLOCKING));
+
+  // Determine the state_ if it is not set.
+  if (state_ == UNINITIALIZED)
+    Init();
+
+  base::DictionaryValue* dict = new base::DictionaryValue;
+
+#ifndef NDEBUG
+  dict->SetString("file", log_path_.MaybeAsASCII());
+#endif  // NDEBUG
+
+  switch (state_) {
+    case ALLOW_STOP:
+      dict->SetString("state", "ALLOW_STOP");
+      break;
+    case ALLOW_START_SEND:
+      dict->SetString("state", "ALLOW_START_SEND");
+      break;
+    default:
+      dict->SetString("state", "ALLOW_START");
+      break;
+  }
+  return dict;
+}
+
+bool GetTempNetLogDir(FilePath* path) {

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

This should be in an anonymous namespace.  Suggest putting it up top, too, since
that's the general style.

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

Done.

+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE_USER_BLOCKING));
+
+  FilePath temp_dir;
+  if (!file_util::GetTempDir(&temp_dir))
+    return false;
+
+  FilePath net_log_dir = temp_dir.Append(FILE_PATH_LITERAL("NetLog"));
+  if (file_util::PathExists(net_log_dir)) {
+    *path = net_log_dir;
+    return true;
+  }
+
+  if (!file_util::CreateDirectory(net_log_dir))
+    return false;
+
+  *path = net_log_dir;
+  return true;
+}
+
+void NetLogTempFile::Init() {

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

Suggest either a DCHECK on the state_ or early abort if we're already
initialized.

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

Done.

+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE_USER_BLOCKING));
+
+  FilePath net_log_dir;
+
+  if (!GetTempNetLogDir(&net_log_dir)) {
+    state_ = ALLOW_START;
+    return;
+  }
+  if (file_util::IsDirectoryEmpty(net_log_dir))
+    state_ = ALLOW_START;
+  else
+    state_ = ALLOW_START_SEND;
+}
+
+void NetLogTempFile::StartNetLog() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE_USER_BLOCKING));
+
+  if (!g_browser_process->net_log())
+    return;
+
+  if (state_ == ALLOW_STOP)
+    return;
+
+  FilePath net_log_dir;
+  if (!GetTempNetLogDir(&net_log_dir))
+    return;

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

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?

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

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

Done.

+
+  file_util::FileEnumerator file_iter(
+      net_log_dir, false, file_util::FileEnumerator::FILES);
+  for (FilePath current = file_iter.Next(); !current.empty();
+       current = file_iter.Next()) {
+    file_util::Delete(current, false);
+  }
+
+  if (!file_util::CreateTemporaryFileInDir(net_log_dir, &log_path_))

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

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.

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

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.

+    return;
+
+  net_log_logger_.reset(new NetLogLogger(log_path_));
+  net_log_logger_->StartObserving(g_browser_process->net_log());

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

Rather than messing with globals, suggest having the ChromeNetLog pass itself in
on construction.

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

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

Done.

+  state_ = ALLOW_STOP;
+}
+
+void NetLogTempFile::StopNetLog() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE_USER_BLOCKING));
+
+  if (!g_browser_process->net_log())
+    return;
+
+  if (state_ != ALLOW_STOP)
+    return;
+
+  net_log_logger_->StopObserving();
+  net_log_logger_.reset();
+  state_ = ALLOW_START_SEND;
+}
+
+void NetLogTempFile::SendNetLog() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE_USER_BLOCKING));
+
+  if (state_ != ALLOW_START_SEND)
+    return;
+
+  FilePath net_log_dir;
+  if (!GetTempNetLogDir(&net_log_dir))
+    return;
+
+  FilePath file_to_send;
+  file_util::FileEnumerator file_iter(
+      net_log_dir, false, file_util::FileEnumerator::FILES);
+  for (FilePath current = file_iter.Next(); !current.empty();
+       current = file_iter.Next()) {
+    file_to_send = current;
+    break;
+  }
+  if (file_to_send.empty())
+    return;
+
+#if defined(OS_POSIX)
+  // Users, group and others can read, write and traverse.
+  int mode = file_util::FILE_PERMISSION_MASK;
+  file_util::SetPosixFilePermissions(file_to_send, mode);
+#endif  // defined(OS_POSIX)
+
+  BrowserThread::PostTask(
+      BrowserThread::UI,
+      FROM_HERE,
+      base::Bind(&NetLogTempFile::SendEmail, file_to_send));
+}
+
+// static
+void NetLogTempFile::SendEmail(const FilePath& file_to_send) {

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

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.

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

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
...

[ramant (doing other things), 2013/01/15 02:19:28]

Moved the SendEmail (UI) code to net_export_ui.cc

Done.

+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+
+#if defined(OS_ANDROID)
+  std::string email;
+  std::string subject = "net_internals_log";
+  std::string title = "test_title";
+  std::string body = "Net Internals log data";
+  FilePath::StringType file_to_attach(file_to_send.value());
+  chrome::android::SendEmail(
+      UTF8ToUTF16(email), UTF8ToUTF16(subject),
+      UTF8ToUTF16(body), UTF8ToUTF16(title), UTF8ToUTF16(file_to_attach));
+#endif
+}