Added WARN_UNUSED_RESULT to ScopedTempDir methods.

Added WARN_UNUSED_RESULT to ScopedTempDir methods.

BUG=NONE
TEST=all targets build, tests pass.

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

[cbentzel, 2011-01-06 12:54:57]

This change adds WARN_UNUSED_RESULT to ScopedTempDir, and adjusts the users of
that to match. The users are pretty spread out, so reviewers only need to look
at small pieces:

erikkay: scoped_temp_dir.h and scoped_temp_dir_unittest.cc
aa: convert_web_app_unittest.cc
ukai: extension_websocket_apitest.cc
skerner: sandboxed_extension_unpacket_unittest.cc
willchan: process_singleton_linux.cc, process_singleton_uitest.cc

[cbentzel, 2011-01-06 13:01:02]

More reviewers [pressed enter too quickly]

shess: process_singleton_mac_unittest
sanjeevr: service_utility_process_host
nirnimesh: automation_proxy_uitest
phajdan.jr: ui_test_utils, worker_uitest, extension_websocket_apitest, general
testing comments
kinuko: file_system_operation_unittest, file_system_path_manager_unittest
dumi: simple_database_system

[Sam Kerner (Chrome), 2011-01-06 14:54:56]

On 2011/01/06 12:54:57, cbentzel wrote:
> This change adds WARN_UNUSED_RESULT to ScopedTempDir, and adjusts the users of
> that to match. The users are pretty spread out, so reviewers only need to look
> at small pieces:
> 
> erikkay: scoped_temp_dir.h and scoped_temp_dir_unittest.cc
> aa: convert_web_app_unittest.cc
> ukai: extension_websocket_apitest.cc
> skerner: sandboxed_extension_unpacket_unittest.cc

The files with my name Look Good To Me.  Took a quick look at the other files
and saw no issues.

> willchan: process_singleton_linux.cc, process_singleton_uitest.cc

[Erik does not do reviews, 2011-01-06 19:01:34]

scoped_temp_dir* LGTM

[Scott Hess - ex-Googler, 2011-01-06 19:17:26]

My name is Scott Hess, and I support this change.

http://codereview.chromium.org/6042009/diff/8001/webkit/fileapi/file_system_p...
File webkit/fileapi/file_system_path_manager_unittest.cc (right):

http://codereview.chromium.org/6042009/diff/8001/webkit/fileapi/file_system_p...
webkit/fileapi/file_system_path_manager_unittest.cc:169:
ASSERT_TRUE(data_dir_->IsValid());
Second ASSERT_TRUE() is probably now removable.

[cbentzel, 2011-01-06 19:20:33]

http://codereview.chromium.org/6042009/diff/8001/webkit/fileapi/file_system_p...
File webkit/fileapi/file_system_path_manager_unittest.cc (right):

http://codereview.chromium.org/6042009/diff/8001/webkit/fileapi/file_system_p...
webkit/fileapi/file_system_path_manager_unittest.cc:169:
ASSERT_TRUE(data_dir_->IsValid());
On 2011/01/06 19:17:26, shess wrote:
> Second ASSERT_TRUE() is probably now removable.

Thanks. Done.

[Nirnimesh, 2011-01-06 19:47:58]

automation_proxy_uitest LGTM

[willchan no longer on Chromium, 2011-01-06 19:58:17]

process_singleton_* lgtm.

On Thu, Jan 6, 2011 at 4:54 AM, <cbentzel@chromium.org> wrote:

> Reviewers: Erik Kay, Aaron Boodman, ukai, Sam Kerner (Chrome), willchan,
> shess,
>
> Message:
> This change adds WARN_UNUSED_RESULT to ScopedTempDir, and adjusts the users
> of
> that to match. The users are pretty spread out, so reviewers only need to
> look
> at small pieces:
>
> erikkay: scoped_temp_dir.h and scoped_temp_dir_unittest.cc
> aa: convert_web_app_unittest.cc
> ukai: extension_websocket_apitest.cc
> skerner: sandboxed_extension_unpacket_unittest.cc
> willchan: process_singleton_linux.cc, process_singleton_uitest.cc
>
>
> Description:
> Added WARN_UNUSED_RESULT to ScopedTempDir methods.
>
> BUG=NONE
> TEST=all targets build, tests pass.
>
> Please review this at http://codereview.chromium.org/6042009/
>
> SVN Base: svn://svn.chromium.org/chrome/trunk/src
>
> Affected files:
>  M base/scoped_temp_dir.h
>  M base/scoped_temp_dir_unittest.cc
>  M chrome/browser/extensions/convert_web_app_unittest.cc
>  M chrome/browser/extensions/extension_websocket_apitest.cc
>  M chrome/browser/extensions/sandboxed_extension_unpacker_unittest.cc
>  M chrome/browser/process_singleton_linux.cc
>  M chrome/browser/process_singleton_mac_unittest.cc
>  M chrome/browser/process_singleton_uitest.cc
>  M chrome/service/service_utility_process_host.cc
>  M chrome/test/automation/automation_proxy_uitest.cc
>  M chrome/test/ui_test_utils.h
>  M chrome/test/ui_test_utils.cc
>  M chrome/worker/worker_uitest.cc
>  M webkit/fileapi/file_system_operation_unittest.cc
>  M webkit/fileapi/file_system_path_manager_unittest.cc
>  M webkit/tools/test_shell/simple_database_system.cc
>
>
> Index: base/scoped_temp_dir.h
> diff --git a/base/scoped_temp_dir.h b/base/scoped_temp_dir.h
> index
>
f44bcca8751d79d99aebe8ff6115eb6229719f63..4286d28c3fb0e98b8b9b1c38c29b796af950286f
> 100644
> --- a/base/scoped_temp_dir.h
> +++ b/base/scoped_temp_dir.h
> @@ -28,17 +28,17 @@ class ScopedTempDir {
>
>   // Creates a unique directory in TempPath, and takes ownership of it.
>   // See file_util::CreateNewTemporaryDirectory.
> -  bool CreateUniqueTempDir();
> +  bool CreateUniqueTempDir() WARN_UNUSED_RESULT;
>
>   // Creates a unique directory under a given path, and takes ownership of
> it.
> -  bool CreateUniqueTempDirUnderPath(const FilePath& path);
> +  bool CreateUniqueTempDirUnderPath(const FilePath& path)
> WARN_UNUSED_RESULT;
>
>   // Takes ownership of directory at |path|, creating it if necessary.
>   // Don't call multiple times unless Take() has been called first.
> -  bool Set(const FilePath& path);
> +  bool Set(const FilePath& path) WARN_UNUSED_RESULT;
>
>   // Deletes the temporary directory wrapped by this object.
> -  bool Delete();
> +  bool Delete() WARN_UNUSED_RESULT;
>
>   // Caller takes ownership of the temporary directory so it won't be
> destroyed
>   // when this object goes out of scope.
> Index: base/scoped_temp_dir_unittest.cc
> diff --git a/base/scoped_temp_dir_unittest.cc
> b/base/scoped_temp_dir_unittest.cc
> index
>
039a1edb39c6d8c0f72e9f2058126c3dd98f5a2c..135c2fd809870760f5ab2c61823ad6986bf00140
> 100644
> --- a/base/scoped_temp_dir_unittest.cc
> +++ b/base/scoped_temp_dir_unittest.cc
> @@ -23,7 +23,7 @@ TEST(ScopedTempDir, FullPath) {
>
>   {
>     ScopedTempDir dir;
> -    dir.Set(test_path);
> +    EXPECT_TRUE(dir.Set(test_path));
>     // Now the dir doesn't exist, so ensure that it gets created.
>     EXPECT_TRUE(file_util::DirectoryExists(test_path));
>     // When we call Release(), it shouldn't get destroyed when leaving
> scope.
> @@ -36,7 +36,7 @@ TEST(ScopedTempDir, FullPath) {
>   // Clean up.
>   {
>     ScopedTempDir dir;
> -    dir.Set(test_path);
> +    EXPECT_TRUE(dir.Set(test_path));
>   }
>   EXPECT_FALSE(file_util::DirectoryExists(test_path));
>  }
> @@ -83,7 +83,7 @@ TEST(ScopedTempDir, MultipleInvocations) {
>   EXPECT_TRUE(dir.CreateUniqueTempDir());
>   EXPECT_FALSE(dir.CreateUniqueTempDir());
>   ScopedTempDir other_dir;
> -  other_dir.Set(dir.Take());
> +  EXPECT_TRUE(other_dir.Set(dir.Take()));
>   EXPECT_TRUE(dir.CreateUniqueTempDir());
>   EXPECT_FALSE(dir.CreateUniqueTempDir());
>   EXPECT_FALSE(other_dir.CreateUniqueTempDir());
> Index: chrome/browser/extensions/convert_web_app_unittest.cc
> diff --git a/chrome/browser/extensions/convert_web_app_unittest.cc
> b/chrome/browser/extensions/convert_web_app_unittest.cc
> index
>
eab9b8f4e2b265ec479a79cc6c22dc931b9a7b59..6424ae76ad8c495d47a1783456df7baa9e4667ee
> 100644
> --- a/chrome/browser/extensions/convert_web_app_unittest.cc
> +++ b/chrome/browser/extensions/convert_web_app_unittest.cc
> @@ -110,7 +110,7 @@ TEST(ExtensionFromWebApp, Basic) {
>   ASSERT_TRUE(extension.get());
>
>   ScopedTempDir extension_dir;
> -  extension_dir.Set(extension->path());
> +  EXPECT_TRUE(extension_dir.Set(extension->path()));
>
>   EXPECT_TRUE(extension->is_app());
>   EXPECT_TRUE(extension->is_hosted_app());
> @@ -153,7 +153,7 @@ TEST(ExtensionFromWebApp, Minimal) {
>   ASSERT_TRUE(extension.get());
>
>   ScopedTempDir extension_dir;
> -  extension_dir.Set(extension->path());
> +  EXPECT_TRUE(extension_dir.Set(extension->path()));
>
>   EXPECT_TRUE(extension->is_app());
>   EXPECT_TRUE(extension->is_hosted_app());
> Index: chrome/browser/extensions/extension_websocket_apitest.cc
> diff --git a/chrome/browser/extensions/extension_websocket_apitest.cc
> b/chrome/browser/extensions/extension_websocket_apitest.cc
> index
>
227a01ac0980fa4de6464601888d40d831ba3220..f8cbebc961504f21400c3a8fc2ed3da39dcd1511
> 100644
> --- a/chrome/browser/extensions/extension_websocket_apitest.cc
> +++ b/chrome/browser/extensions/extension_websocket_apitest.cc
> @@ -20,6 +20,7 @@ IN_PROC_BROWSER_TEST_F(ExtensionApiTest, MAYBE_WebSocket)
> {
>   PathService::Get(chrome::DIR_TEST_DATA, &websocket_root_dir);
>   websocket_root_dir = websocket_root_dir.AppendASCII("layout_tests")
>       .AppendASCII("LayoutTests");
> -  ui_test_utils::TestWebSocketServer server(websocket_root_dir);
> +  ui_test_utils::TestWebSocketServer server;
> +  ASSERT_TRUE(server.Start(websocket_root_dir));
>   ASSERT_TRUE(RunExtensionTest("websocket")) << message_;
>  }
> Index: chrome/browser/extensions/sandboxed_extension_unpacker_unittest.cc
> diff --git
> a/chrome/browser/extensions/sandboxed_extension_unpacker_unittest.cc
> b/chrome/browser/extensions/sandboxed_extension_unpacker_unittest.cc
> index
>
19cd3796a895a0da9cabe9bf54ce4c7ee17cb89c..6246a6fd45ab57b25af4e9875c4baa971165707c
> 100644
> --- a/chrome/browser/extensions/sandboxed_extension_unpacker_unittest.cc
> +++ b/chrome/browser/extensions/sandboxed_extension_unpacker_unittest.cc
> @@ -100,16 +100,18 @@ class SandboxedExtensionUnpackerTest : public
> testing::Test {
>     // Hack since SandboxedExtensionUnpacker gets its background thread id
> from
>     // the Start call, but we don't call it here.
>     sandboxed_unpacker_->thread_identifier_ = BrowserThread::FILE;
> -    PrepareUnpackerEnv();
> +    EXPECT_TRUE(PrepareUnpackerEnv());
>   }
>
> -  void PrepareUnpackerEnv() {
> +  bool PrepareUnpackerEnv() {
>     sandboxed_unpacker_->extension_root_ =
>       install_dir_.AppendASCII(extension_filenames::kTempExtensionName);
>
> -    sandboxed_unpacker_->temp_dir_.Set(install_dir_);
> +    if (!sandboxed_unpacker_->temp_dir_.Set(install_dir_))
> +      return false;
>     sandboxed_unpacker_->public_key_ =
>       "ocnapchkplbmjmpfehjocmjnipfmogkh";
> +    return true;
>   }
>
>   void OnUnpackSucceeded() {
> Index: chrome/browser/process_singleton_linux.cc
> diff --git a/chrome/browser/process_singleton_linux.cc
> b/chrome/browser/process_singleton_linux.cc
> index
>
982f370bbf4498b9e30194f1ea26b4af90c51123..7c7cb472d6a026837aaf2c6de188238279b953bb
> 100644
> --- a/chrome/browser/process_singleton_linux.cc
> +++ b/chrome/browser/process_singleton_linux.cc
> @@ -951,7 +951,8 @@ bool ProcessSingleton::Create() {
>     // We've already locked things, so we can't have lost the startup race,
>     // but something doesn't like us.
>     LOG(ERROR) << "Failed to create symlinks.";
> -    socket_dir_.Delete();
> +    if (!socket_dir_.Delete())
> +      LOG(ERROR) << "Encountered a problem when deleting socket
> directory.";
>     return false;
>   }
>
> Index: chrome/browser/process_singleton_mac_unittest.cc
> diff --git a/chrome/browser/process_singleton_mac_unittest.cc
> b/chrome/browser/process_singleton_mac_unittest.cc
> index
>
bdbeff6d99d08bf9a4e2ac1d0c8e825185c4334b..978f6f971869984299b947dfcfb1cb10b620b480
> 100644
> --- a/chrome/browser/process_singleton_mac_unittest.cc
> +++ b/chrome/browser/process_singleton_mac_unittest.cc
> @@ -25,8 +25,7 @@ class ProcessSingletonMacTest : public PlatformTest {
>
>     // Put the lock in a temporary directory.  Doesn't need to be a
>     // full profile to test this code.
> -    temp_dir_.CreateUniqueTempDir();
> -    ASSERT_TRUE(temp_dir_.IsValid());
> +    ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
>     lock_path_ = temp_dir_.path().Append(chrome::kSingletonLockFilename);
>   }
>
> Index: chrome/browser/process_singleton_uitest.cc
> diff --git a/chrome/browser/process_singleton_uitest.cc
> b/chrome/browser/process_singleton_uitest.cc
> index
>
235f18129536e60ed5232ebe41d42e0e33dd4f22..d50e55f85ac5eeef02b57750c9368469085782ed
> 100644
> --- a/chrome/browser/process_singleton_uitest.cc
> +++ b/chrome/browser/process_singleton_uitest.cc
> @@ -136,7 +136,8 @@ class ProcessSingletonTest : public UITest {
>       // We use a manual reset so that all threads wake up at once when
> signaled
>       // and thus we must manually reset it for each attempt.
>       : threads_waker_(true /* manual */, false /* signaled */) {
> -    temp_profile_dir_.CreateUniqueTempDir();
> +    if (!temp_profile_dir_.CreateUniqueTempDir())
> +      ADD_FAILURE();
>   }
>
>   void SetUp() {
> Index: chrome/service/service_utility_process_host.cc
> diff --git a/chrome/service/service_utility_process_host.cc
> b/chrome/service/service_utility_process_host.cc
> index
>
859e7259f2e404d2ed596e64a3d395da6837762b..d8befd1fc3877a80bb7d260088b59b49ad791fc4
> 100644
> --- a/chrome/service/service_utility_process_host.cc
> +++ b/chrome/service/service_utility_process_host.cc
> @@ -194,7 +194,12 @@ void
> ServiceUtilityProcessHost::Client::MetafileAvailable(
>   // The metafile was created in a temp folder which needs to get deleted
> after
>   // we have processed it.
>   ScopedTempDir scratch_metafile_dir;
> -  scratch_metafile_dir.Set(metafile_path.DirName());
> +  if (!scratch_metafile_dir.Set(metafile_path.DirName())) {
> +    // This will happen if metafile_path is invalid. It could potentially
> +    // be strengthened to a CHECK.
> +    OnRenderPDFPagesToMetafileFailed();
> +    return;
> +  }
>  #if defined(OS_WIN)
>   printing::NativeMetafile metafile;
>   if (!metafile.CreateFromFile(metafile_path)) {
> Index: chrome/test/automation/automation_proxy_uitest.cc
> diff --git a/chrome/test/automation/automation_proxy_uitest.cc
> b/chrome/test/automation/automation_proxy_uitest.cc
> index
>
465b61023a1afc3f33462a0afa769b24cf2fc91f..3856cb16d2d1cf2abe6a82487bf04684f1a79d4f
> 100644
> --- a/chrome/test/automation/automation_proxy_uitest.cc
> +++ b/chrome/test/automation/automation_proxy_uitest.cc
> @@ -1569,8 +1569,10 @@ class AutomationProxySnapshotTest : public UITest {
>  protected:
>   AutomationProxySnapshotTest() {
>     dom_automation_enabled_ = true;
> -    snapshot_dir_.CreateUniqueTempDir();
> -    snapshot_path_ = snapshot_dir_.path().AppendASCII("snapshot.png");
> +    if (!snapshot_dir_.CreateUniqueTempDir())
> +      ADD_FAILURE() << "Unable to create temporary directory";
> +    else
> +      snapshot_path_ = snapshot_dir_.path().AppendASCII("snapshot.png");
>   }
>
>   // Asserts that the given png file can be read and decoded into the given
> Index: chrome/test/ui_test_utils.cc
> diff --git a/chrome/test/ui_test_utils.cc b/chrome/test/ui_test_utils.cc
> index
>
0be4bd8f7cdc87f3254ceb2fa14f5f0089889ee0..45035135bd1863a11f6256ef746f9bc7871db089
> 100644
> --- a/chrome/test/ui_test_utils.cc
> +++ b/chrome/test/ui_test_utils.cc
> @@ -707,17 +707,27 @@ void AppendToPythonPath(const FilePath& dir) {
>
>  }  // anonymous namespace
>
> -TestWebSocketServer::TestWebSocketServer(const FilePath& root_directory) {
> +TestWebSocketServer::TestWebSocketServer()
> +    : started_(false) {
> +}
> +
> +bool TestWebSocketServer::Start(const FilePath& root_directory) {
> +  if (started_)
> +    return true;
>   scoped_ptr<CommandLine> cmd_line(CreateWebSocketServerCommandLine());
>   cmd_line->AppendSwitchASCII("server", "start");
>   cmd_line->AppendSwitch("chromium");
>   cmd_line->AppendSwitch("register_cygwin");
>   cmd_line->AppendSwitchPath("root", root_directory);
> -  temp_dir_.CreateUniqueTempDir();
> +  if (!temp_dir_.CreateUniqueTempDir())
> +    return false;
>   websocket_pid_file_ = temp_dir_.path().AppendASCII("websocket.pid");
>   cmd_line->AppendSwitchPath("pidfile", websocket_pid_file_);
>   SetPythonPath();
> -  base::LaunchApp(*cmd_line.get(), true, false, NULL);
> +  if (!base::LaunchApp(*cmd_line.get(), true, false, NULL))
> +    return false;
> +  started_ = true;
> +  return true;
>  }
>
>  CommandLine* TestWebSocketServer::CreatePythonCommandLine() {
> @@ -754,6 +764,8 @@ CommandLine*
> TestWebSocketServer::CreateWebSocketServerCommandLine() {
>  }
>
>  TestWebSocketServer::~TestWebSocketServer() {
> +  if (!started_)
> +    return;
>   scoped_ptr<CommandLine> cmd_line(CreateWebSocketServerCommandLine());
>   cmd_line->AppendSwitchASCII("server", "stop");
>   cmd_line->AppendSwitch("chromium");
> Index: chrome/test/ui_test_utils.h
> diff --git a/chrome/test/ui_test_utils.h b/chrome/test/ui_test_utils.h
> index
>
5299451be35246be39bc9230e5cf3b76bc54d62c..323e3744a44fe16e5afc83816eb9effab2d68b0c
> 100644
> --- a/chrome/test/ui_test_utils.h
> +++ b/chrome/test/ui_test_utils.h
> @@ -280,12 +280,15 @@ class TimedMessageLoopRunner {
>  // server.
>  class TestWebSocketServer {
>  public:
> -  // Creates and starts a python websocket server with |root_directory|.
> -  explicit TestWebSocketServer(const FilePath& root_directory);
> +  TestWebSocketServer();
>
> -  // Destroys and stops the server.
> +  // Stops the python websocket server if it was already started.
>   ~TestWebSocketServer();
>
> +  // Starts the python websocket server using |root_directory|. Returns
> whether
> +  // the server was successfully started.
> +  bool Start(const FilePath& root_directory);
> +
>  private:
>   // Sets up PYTHONPATH to run websocket_server.py.
>   void SetPythonPath();
> @@ -296,6 +299,9 @@ class TestWebSocketServer {
>   // Creates a CommandLine for invoking the python websocker server.
>   CommandLine* CreateWebSocketServerCommandLine();
>
> +  // Has the server been started?
> +  bool started_;
> +
>   // A Scoped temporary directory for holding the python pid file.
>   ScopedTempDir temp_dir_;
>
> Index: chrome/worker/worker_uitest.cc
> diff --git a/chrome/worker/worker_uitest.cc
> b/chrome/worker/worker_uitest.cc
> index
>
3babc788cadb0a7cdd4f17622ba6d6297f8683d6..d9bfcba15b9fd3197e79620e2bbfe917dd032f57
> 100644
> --- a/chrome/worker/worker_uitest.cc
> +++ b/chrome/worker/worker_uitest.cc
> @@ -468,7 +468,8 @@ TEST_F(WorkerTest, WorkerWebSocketLayoutTests) {
>
>   FilePath websocket_root_dir(temp_test_dir_);
>   websocket_root_dir = websocket_root_dir.AppendASCII("LayoutTests");
> -  ui_test_utils::TestWebSocketServer websocket_server(websocket_root_dir);
> +  ui_test_utils::TestWebSocketServer websocket_server;
> +  ASSERT_TRUE(websocket_server.Start(websocket_root_dir));
>
>   StartHttpServer(new_http_root_dir_);
>   for (size_t i = 0; i < arraysize(kLayoutTestFiles); ++i)
> Index: webkit/fileapi/file_system_operation_unittest.cc
> diff --git a/webkit/fileapi/file_system_operation_unittest.cc
> b/webkit/fileapi/file_system_operation_unittest.cc
> index
>
dffcc33e57667492e8f3cace6fa72b9e5c79797d..7917d315758c486e873f35c03873c28abbc55539
> 100644
> --- a/webkit/fileapi/file_system_operation_unittest.cc
> +++ b/webkit/fileapi/file_system_operation_unittest.cc
> @@ -27,8 +27,7 @@ class FileSystemOperationTest : public testing::Test {
>  public:
>   FileSystemOperationTest()
>       : status_(kFileOperationStatusNotSet) {
> -    base_.CreateUniqueTempDir();
> -    EXPECT_TRUE(base_.IsValid());
> +    EXPECT_TRUE(base_.CreateUniqueTempDir());
>   }
>
>   FileSystemOperation* operation();
> Index: webkit/fileapi/file_system_path_manager_unittest.cc
> diff --git a/webkit/fileapi/file_system_path_manager_unittest.cc
> b/webkit/fileapi/file_system_path_manager_unittest.cc
> index
>
7fa3672a74c0f9316afbaf216076dcbf4f3ef479..1aee2cd03bb4a039a2a8f934c22c81941406c55b
> 100644
> --- a/webkit/fileapi/file_system_path_manager_unittest.cc
> +++ b/webkit/fileapi/file_system_path_manager_unittest.cc
> @@ -165,7 +165,7 @@ class FileSystemPathManagerTest : public testing::Test
> {
>
>   void SetUp() {
>     data_dir_.reset(new ScopedTempDir);
> -    data_dir_->CreateUniqueTempDir();
> +    ASSERT_TRUE(data_dir_->CreateUniqueTempDir());
>     ASSERT_TRUE(data_dir_->IsValid());
>     root_path_callback_status_ = false;
>     root_path_.clear();
> Index: webkit/tools/test_shell/simple_database_system.cc
> diff --git a/webkit/tools/test_shell/simple_database_system.cc
> b/webkit/tools/test_shell/simple_database_system.cc
> index
>
4c591e654026328b10eb573ce557bded439f0c52..29b78f1259225a5ba2339f7fa623b6bd215af731
> 100644
> --- a/webkit/tools/test_shell/simple_database_system.cc
> +++ b/webkit/tools/test_shell/simple_database_system.cc
> @@ -28,7 +28,7 @@ SimpleDatabaseSystem* SimpleDatabaseSystem::GetInstance()
> {
>
>  SimpleDatabaseSystem::SimpleDatabaseSystem()
>     : waiting_for_dbs_to_close_(false) {
> -  temp_dir_.CreateUniqueTempDir();
> +  CHECK(temp_dir_.CreateUniqueTempDir());
>   db_tracker_ = new DatabaseTracker(temp_dir_.path(), false);
>   db_tracker_->AddObserver(this);
>   DCHECK(!instance_);
>
>
>

[kinuko, 2011-01-07 02:11:15]

On 2011/01/06 13:01:02, cbentzel wrote:
> kinuko: file_system_operation_unittest, file_system_path_manager_unittest

file_system* LGTM.

[ukai, 2011-01-07 02:46:32]

extension_websocket_apitest: LGTM.

[sanjeevr, 2011-01-07 04:38:56]

http://codereview.chromium.org/6042009/diff/17001/chrome/service/service_util...
File chrome/service/service_utility_process_host.cc (right):

http://codereview.chromium.org/6042009/diff/17001/chrome/service/service_util...
chrome/service/service_utility_process_host.cc:197: if
(!scratch_metafile_dir.Set(metafile_path.DirName())) {
The only use to the scratch_metafile_dir is to delete the temp folder. We really
do not want to do any error checking on this. If metafile_path is invalid, the
CreateFromFile call below will fail.

[cbentzel, 2011-01-10 14:14:36]

http://codereview.chromium.org/6042009/diff/17001/chrome/service/service_util...
File chrome/service/service_utility_process_host.cc (right):

http://codereview.chromium.org/6042009/diff/17001/chrome/service/service_util...
chrome/service/service_utility_process_host.cc:197: if
(!scratch_metafile_dir.Set(metafile_path.DirName())) {
On 2011/01/07 04:38:57, sanjeevr wrote:
> The only use to the scratch_metafile_dir is to delete the temp folder. We
really
> do not want to do any error checking on this. If metafile_path is invalid, the
> CreateFromFile call below will fail.

Moved to LOG(WARNING) and fallthrough in this case.

[sanjeevr, 2011-01-10 18:20:36]

LGTM for service_utility_process_host.cc

[cbentzel, 2011-01-11 12:55:23]

Thanks everyone.

I'm still waiting for:

aa: convert_web_app_unittest.cc [although that change is trivial]
phajdan.jr: ui_test_utils, worker_uitest, extension_websocket_apitest
dumi: simple_database_system

[Paweł Hajdan Jr., 2011-01-11 13:01:09]

LGTM

Sorry for late reply. Rather minor comments.

http://codereview.chromium.org/6042009/diff/28001/chrome/browser/extensions/s...
File chrome/browser/extensions/sandboxed_extension_unpacker_unittest.cc (right):

http://codereview.chromium.org/6042009/diff/28001/chrome/browser/extensions/s...
chrome/browser/extensions/sandboxed_extension_unpacker_unittest.cc:106: bool
PrepareUnpackerEnv() {
nit: It may be useful to propagate WARN_UNUSED_RESULT here.

http://codereview.chromium.org/6042009/diff/28001/chrome/browser/process_sing...
File chrome/browser/process_singleton_uitest.cc (right):

http://codereview.chromium.org/6042009/diff/28001/chrome/browser/process_sing...
chrome/browser/process_singleton_uitest.cc:139: if
(!temp_profile_dir_.CreateUniqueTempDir())
nit: How about just EXPECT_TRUE? Should have the same effect.

http://codereview.chromium.org/6042009/diff/28001/chrome/test/ui_test_utils.cc
File chrome/test/ui_test_utils.cc (right):

http://codereview.chromium.org/6042009/diff/28001/chrome/test/ui_test_utils.c...
chrome/test/ui_test_utils.cc:711: : started_(false) {
nit: Can we fit it in the previous line?

http://codereview.chromium.org/6042009/diff/28001/chrome/test/ui_test_utils.c...
chrome/test/ui_test_utils.cc:722: if (!temp_dir_.CreateUniqueTempDir())
LOG messages would be useful for all error cases here. Unfortunately, test
servers not starting are common.

http://codereview.chromium.org/6042009/diff/28001/chrome/test/ui_test_utils.c...
chrome/test/ui_test_utils.cc:727: if (!base::LaunchApp(*cmd_line.get(), true,
false, NULL))
nit: maybe just started_ = LaunchApp; return started_; Up to you.

[cbentzel, 2011-01-11 19:56:38]

Thanks for the review. 

Still waiting from aa+dumi, but I will land without their comment since changes
in both files are trivial.

http://codereview.chromium.org/6042009/diff/28001/chrome/browser/extensions/s...
File chrome/browser/extensions/sandboxed_extension_unpacker_unittest.cc (right):

http://codereview.chromium.org/6042009/diff/28001/chrome/browser/extensions/s...
chrome/browser/extensions/sandboxed_extension_unpacker_unittest.cc:106: bool
PrepareUnpackerEnv() {
On 2011/01/11 13:01:10, Paweł Hajdan Jr. wrote:
> nit: It may be useful to propagate WARN_UNUSED_RESULT here.

I'll hold off on this - I don't have as much context about this method.

http://codereview.chromium.org/6042009/diff/28001/chrome/browser/process_sing...
File chrome/browser/process_singleton_uitest.cc (right):

http://codereview.chromium.org/6042009/diff/28001/chrome/browser/process_sing...
chrome/browser/process_singleton_uitest.cc:139: if
(!temp_profile_dir_.CreateUniqueTempDir())
On 2011/01/11 13:01:10, Paweł Hajdan Jr. wrote:
> nit: How about just EXPECT_TRUE? Should have the same effect.

Done.

http://codereview.chromium.org/6042009/diff/28001/chrome/test/ui_test_utils.cc
File chrome/test/ui_test_utils.cc (right):

http://codereview.chromium.org/6042009/diff/28001/chrome/test/ui_test_utils.c...
chrome/test/ui_test_utils.cc:711: : started_(false) {
On 2011/01/11 13:01:10, Paweł Hajdan Jr. wrote:
> nit: Can we fit it in the previous line?

Done.

http://codereview.chromium.org/6042009/diff/28001/chrome/test/ui_test_utils.c...
chrome/test/ui_test_utils.cc:722: if (!temp_dir_.CreateUniqueTempDir())
On 2011/01/11 13:01:10, Paweł Hajdan Jr. wrote:
> LOG messages would be useful for all error cases here. Unfortunately, test
> servers not starting are common.

Done.

http://codereview.chromium.org/6042009/diff/28001/chrome/test/ui_test_utils.c...
chrome/test/ui_test_utils.cc:727: if (!base::LaunchApp(*cmd_line.get(), true,
false, NULL))
On 2011/01/11 13:01:10, Paweł Hajdan Jr. wrote:
> nit: maybe just started_ = LaunchApp; return started_; Up to you.

I added a LOG in failure case, so kept the old version.

[Scott Hess - ex-Googler, 2011-01-11 20:06:43]

On 2011/01/11 19:56:38, cbentzel wrote:
> Still waiting from aa+dumi, but I will land without their comment since
changes
> in both files are trivial.

I think the simple_database_system.cc change looks reasonable, given that it's
in a test harness.  The convert_web_app_unittest.cc also looks correct, and also
innocuous since it's in test code.

[cbentzel, 2011-01-11 21:13:13]

On Tue, Jan 11, 2011 at 3:06 PM, <shess@chromium.org> wrote:

> On 2011/01/11 19:56:38, cbentzel wrote:
>
>> Still waiting from aa+dumi, but I will land without their comment since
>>
> changes
>
>> in both files are trivial.
>>
>
> I think the simple_database_system.cc change looks reasonable, given that
> it's
> in a test harness.


Yes. I wouldn't have introduced a CHECK if it was in production code.