Miscellaneous tiny cleanups done while converting files to use ScopedCOMInitializer, pulled out sep…

net/url_request/url_request_unittest.cc

 // Copyright (c) 2012 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 "build/build_config.h"
 
 #if defined(OS_WIN)
 #include <windows.h>
 #include <shlobj.h>
 #endif
@@ skipping 684 matching lines @@
 TEST_F(URLRequestTest, ResolveShortcutTest) {
   FilePath app_path;
   PathService::Get(base::DIR_SOURCE_ROOT, &app_path);
   app_path = app_path.AppendASCII("net");
   app_path = app_path.AppendASCII("data");
   app_path = app_path.AppendASCII("url_request_unittest");
   app_path = app_path.AppendASCII("with-headers.html");
 
   std::wstring lnk_path = app_path.value() + L".lnk";
 
-  HRESULT result;
+  CoInitialize(NULL);

[Ryan Sleevi, 2012/10/01 14:21:01]

According to the e-mail that went out, there shouldn't be explicit calls to
CoInitialize, and that functions should rely on the thread being configured
appropriately.

It seems like this should either be ScopedCOMInitializer (in order to ensure
that decrement of the COM initialization refcount on test failure) or, and this
seems entirely more appropriate, that net_unittests should be calling
CoInitialize in the overall test setup ( net/base/run_all_unittests.cc )

[Peter Kasting, 2012/10/01 18:18:55]

I am in fact going to change this to ScopedCOMInitializer -- that's why I was
touching this code in the first place.  I've just split the changes into pieces
to make them more reviewable.

I'm not opposed to the idea of moving the initialization to happening in
net/base, but I don't know if it's necessary.  This seems to be the only place
in all of net/ that wants COM initialized.  Do you still want to move the
initialization?

[Ryan Sleevi, 2012/10/01 20:54:43]

For some reason, I thought there were more places in net/ where we were doing
crap like this with shortcuts.

I think it's fine to just change this call, and in a future CL, so LGTM with no
nits :)

+
+  // Temporarily create a shortcut for test
   IShellLink* shell = NULL;
+  ASSERT_TRUE(SUCCEEDED(CoCreateInstance(
+      CLSID_ShellLink, NULL, CLSCTX_INPROC_SERVER, IID_IShellLink,
+      reinterpret_cast<LPVOID*>(&shell))));
   IPersistFile* persist = NULL;
-
-  CoInitialize(NULL);
-  // Temporarily create a shortcut for test
-  result = CoCreateInstance(CLSID_ShellLink, NULL,
-                            CLSCTX_INPROC_SERVER, IID_IShellLink,
-                            reinterpret_cast<LPVOID*>(&shell));
-  ASSERT_TRUE(SUCCEEDED(result));
-  result = shell->QueryInterface(IID_IPersistFile,
-                                reinterpret_cast<LPVOID*>(&persist));
-  ASSERT_TRUE(SUCCEEDED(result));
-  result = shell->SetPath(app_path.value().c_str());
-  EXPECT_TRUE(SUCCEEDED(result));
-  result = shell->SetDescription(L"ResolveShortcutTest");
-  EXPECT_TRUE(SUCCEEDED(result));
-  result = persist->Save(lnk_path.c_str(), TRUE);
-  EXPECT_TRUE(SUCCEEDED(result));
+  ASSERT_TRUE(SUCCEEDED(shell->QueryInterface(
+      IID_IPersistFile, reinterpret_cast<LPVOID*>(&persist))));
+  EXPECT_TRUE(SUCCEEDED(shell->SetPath(app_path.value().c_str())));
+  EXPECT_TRUE(SUCCEEDED(shell->SetDescription(L"ResolveShortcutTest")));
+  EXPECT_TRUE(SUCCEEDED(persist->Save(lnk_path.c_str(), TRUE)));
   if (persist)
     persist->Release();
   if (shell)
     shell->Release();
 
   TestDelegate d;
   {
     URLRequest r(FilePathToFileURL(FilePath(lnk_path)), &d, &default_context_);
 
     r.Start();
@@ skipping 4057 matching lines @@
 
     EXPECT_FALSE(r.is_pending());
     EXPECT_EQ(1, d->response_started_count());
     EXPECT_FALSE(d->received_data_before_response());
     EXPECT_EQ(d->bytes_received(), static_cast<int>(file_size));
   }
 }
 #endif  // !defined(DISABLE_FTP_SUPPORT)
 
 }  // namespace net