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

chrome/browser/net/net_log_temp_file_unittest.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>

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

Is this needed?

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

Done.

+#include <string>

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

nit:  believe there should be a blank line between C headers and C++ headers.

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

Deleted the include (not needed).

+
+#include "base/basictypes.h"
+#include "base/file_path.h"
+#include "base/file_util.h"
+#include "base/values.h"
+#include "build/build_config.h"
+#include "chrome/browser/net/chrome_net_log.h"
+#include "content/public/test/test_browser_thread.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+using content::BrowserThread;
+
+class NetLogTempFileTest : public ::testing::Test {
+ protected:
+  NetLogTempFileTest() {
+    net_log_.reset(new ChromeNetLog);
+
+    file_user_blocking_thread_.reset(
+        new content::TestBrowserThread(BrowserThread::FILE_USER_BLOCKING));
+    file_user_blocking_thread_->Start();
+
+    net_log_temp_file_ = net_log_->net_log_temp_file();
+  }
+
+  virtual void SetUp() OVERRIDE {
+    // Get a temporary file name for unit tests.
+    FilePath net_export_log;
+    ASSERT_TRUE(file_util::CreateTemporaryFile(&net_export_log));
+
+    // Delete the file to start with no temporary file, but we will use the
+    // temporary file to log NetLog entries.
+    file_util::Delete(net_export_log, false);

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

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.

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

Implemented Subclass approach.
Done.

+
+    FilePath net_export_log_filename = net_export_log.BaseName();
+    net_log_temp_file_->chrome_net_export_ = FILE_PATH_LITERAL("");
+    net_log_temp_file_->log_filename_ = net_export_log_filename.value();
+    net_log_temp_file_->log_path_ = net_export_log;
+  }
+
+  virtual void TearDown() OVERRIDE {
+    // Delete the temporary file we have created.
+    ASSERT_TRUE(file_util::Delete(net_log_temp_file_->log_path(), false));
+  }
+
+  NetLogTempFile* net_log_temp_file_;
+
+ private:
+  scoped_ptr<ChromeNetLog> net_log_;
+  scoped_ptr<content::TestBrowserThread> file_user_blocking_thread_;
+};
+
+TEST_F(NetLogTempFileTest, InitAllowStart) {
+  net_log_temp_file_->Init();
+  base::DictionaryValue* dict = net_log_temp_file_->GetState();
+  std::string state;
+  dict->GetString("state", &state);
+  EXPECT_EQ("ALLOW_START", state);
+  EXPECT_EQ(NetLogTempFile::STATE_ALLOW_START, net_log_temp_file_->state());

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

Think you should check the return value of
net_log_temp_file_->GetFilePath(&net_export_file_path)); at the end of all these
tests.

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

Done.

+}
+
+TEST_F(NetLogTempFileTest, InitAllowStartOrSend) {
+  FilePath log_path = net_log_temp_file_->log_path();
+  // Create the temporary file.
+  FILE* fp = file_util::OpenFile(log_path, "w");
+  CHECK(fp);
+  file_util::CloseFile(fp);
+
+  net_log_temp_file_->Init();
+  base::DictionaryValue* dict = net_log_temp_file_->GetState();
+  std::string state;
+  dict->GetString("state", &state);
+  EXPECT_EQ("ALLOW_START_SEND", state);
+  EXPECT_EQ(NetLogTempFile::STATE_ALLOW_START_SEND,
+            net_log_temp_file_->state());
+}
+
+TEST_F(NetLogTempFileTest, ProcessCommandDoStart) {
+  net_log_temp_file_->Init();

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

We don't need to call this ourselves except in the first two tests, right?  It's
automatically called as needed.

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

Done.

+  // Execute DO_START command via ProcessCommand and verfiy that we have
+  // transitioned to STATE_ALLOW_STOP state.
+  net_log_temp_file_->ProcessCommand(NetLogTempFile::DO_START);
+  base::DictionaryValue* dict = net_log_temp_file_->GetState();
+  std::string state;
+  dict->GetString("state", &state);
+  EXPECT_EQ("ALLOW_STOP", state);
+  EXPECT_EQ(NetLogTempFile::STATE_ALLOW_STOP, net_log_temp_file_->state());
+  FilePath log_path = net_log_temp_file_->log_path();
+  EXPECT_TRUE(file_util::PathExists(log_path));
+  // Execute DO_STOP so that temporary file could be deleted.
+  net_log_temp_file_->ProcessCommand(NetLogTempFile::DO_STOP);
+}
+
+TEST_F(NetLogTempFileTest, ProcessCommandDoStop) {
+  net_log_temp_file_->Init();
+  // Execute DO_START command via ProcessCommand and verfiy that we transition
+  // to ALLOW_STOP state.
+  net_log_temp_file_->ProcessCommand(NetLogTempFile::DO_START);
+  base::DictionaryValue* dict = net_log_temp_file_->GetState();
+  std::string state;
+  dict->GetString("state", &state);
+  EXPECT_EQ("ALLOW_STOP", state);
+  EXPECT_EQ(NetLogTempFile::STATE_ALLOW_STOP, net_log_temp_file_->state());
+  FilePath log_path = net_log_temp_file_->log_path();
+  EXPECT_TRUE(file_util::PathExists(log_path));
+
+  // Execute DO_STOP command via ProcessCommand and verfiy that we have
+  // transitioned to STATE_ALLOW_START_SEND state.
+  net_log_temp_file_->ProcessCommand(NetLogTempFile::DO_STOP);
+  dict = net_log_temp_file_->GetState();
+  dict->GetString("state", &state);
+  EXPECT_EQ("ALLOW_START_SEND", state);
+  EXPECT_EQ(NetLogTempFile::STATE_ALLOW_START_SEND,
+            net_log_temp_file_->state());
+  FilePath log_path1 = net_log_temp_file_->log_path();
+  EXPECT_TRUE(file_util::PathExists(log_path1));
+}
+
+TEST_F(NetLogTempFileTest, GetFilePath) {
+  net_log_temp_file_->Init();
+  // Execute DO_START and DO_STOP commands via ProcessCommand and verfiy that we
+  // have transitioned to STATE_ALLOW_START_SEND state.
+  net_log_temp_file_->ProcessCommand(NetLogTempFile::DO_START);
+  net_log_temp_file_->ProcessCommand(NetLogTempFile::DO_STOP);
+  EXPECT_EQ(NetLogTempFile::STATE_ALLOW_START_SEND,
+            net_log_temp_file_->state());
+
+  FilePath net_export_file_path;
+  EXPECT_TRUE(net_log_temp_file_->GetFilePath(&net_export_file_path));
+  EXPECT_TRUE(file_util::PathExists(net_export_file_path));
+}

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

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]

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]

Added a check the file_size is > 0.
Done.

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

Added a TODO.