Install CRX updates one at a time.

chrome/browser/extensions/crx_installer.cc

 // Copyright (c) 2011 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/extensions/crx_installer.h"
 
 #include <map>
 #include <set>
 
 #include "base/file_util.h"
@@ skipping 95 matching lines @@
   std::set<std::string>& ids = g_whitelisted_install_data.Get().ids;
   if (ContainsKey(ids, id)) {
     ids.erase(id);
     return true;
   }
   return false;
 }
 
 CrxInstaller::CrxInstaller(base::WeakPtr<ExtensionService> frontend_weak,
                            ExtensionInstallUI* client)
-    : install_directory_(frontend_weak->install_directory()),
-      install_source_(Extension::INTERNAL),
-      extensions_enabled_(frontend_weak->extensions_enabled()),
+    : install_source_(Extension::INTERNAL),
+      extensions_enabled_(false),  // Will be set again in body of this method.
       delete_source_(false),
       is_gallery_install_(false),
       create_app_shortcut_(false),
       frontend_weak_(frontend_weak),
       client_(client),
       apps_require_extension_mime_type_(false),
       allow_silent_install_(false),
       install_cause_(extension_misc::INSTALL_CAUSE_UNSET) {
+
+  // |frontend_weak| can be NULL at this point in unit tests.

[Matt Perry, 2011/05/31 21:34:25]

Can we instead pass in a mock ExtensionService in the test? Allowing NULL for
unit tests is problematic. It can become confusing when required interfaces can
be NULL.

[Sam Kerner (Chrome), 2011/05/31 23:16:15]

There is ExtensionServiceInterface, which allows easy mocking.  Sadly,
CrxInstaller uses many methods not in that interface, and adding them all makes
the interface a lot bigger and harder to reason about.  I started down this
road, and gave up.

I could make the mock service used in the test inherit from the real extension
service.  This brings in a bunch of behavior the existing tests don't want.

I created an extension service on the testing profile, and used it to generate
some CrxInstallers.  Any action the CrxInstallers take is on the testing
profile's extensions service, but I don't want them to take an action.

[Matt Perry, 2011/05/31 23:41:38]

But I'm confused.. The rest of the code does not NULL-check frontend_weak. That
seems to indicate it's not actually getting used.

Also, we don't set install_directory_ or extensions_enabled_. What is the
behavior of CrxInstaller if those are unset?

[Sam Kerner (Chrome), 2011/06/01 00:15:11]

Not sure what you mean:  There are checks for frontend_weak before every user
other than the constructor.  They are in the start of
CrxInstaller::ConfirmInstall() and CrxInstaller::ReportSuccessFromUIThread(). 
No check is done in the constructor because construction is done by
ExtensionService::MakeCrxInstaller(), and if you can get there the extension
service is not null.
I reverted all changes to this file after the first round of reviews, so there
is no case where these two variables will be unset.  We will dereference a null
pointer while setting them if frontend_weak is null, but that can't happen in
the constructor.

[Matt Perry, 2011/06/01 00:20:20]

Ah, of course, sorry. That makes sense.
I didn't realize you reverted the changes. Carry on, then!

+  if (!frontend_weak.get())
+    return;
+
+  install_directory_ = frontend_weak->install_directory();
+  extensions_enabled_ = frontend_weak->extensions_enabled();
 }
 
 CrxInstaller::~CrxInstaller() {
   // Delete the temp directory and crx file as necessary. Note that the
   // destructor might be called on any thread, so we post a task to the file
   // thread to make sure the delete happens there.
   if (!temp_dir_.value().empty()) {
     if (!BrowserThread::PostTask(
             BrowserThread::FILE, FROM_HERE,
             NewRunnableFunction(
@@ skipping 423 matching lines @@
   // Some users (such as the download shelf) need to know when a
   // CRXInstaller is done.  Listening for the EXTENSION_* events
   // is problematic because they don't know anything about the
   // extension before it is unpacked, so they can not filter based
   // on the extension.
   NotificationService::current()->Notify(
       NotificationType::CRX_INSTALLER_DONE,
       Source<CrxInstaller>(this),
       NotificationService::NoDetails());
 }