Dedupe code in SetIdleInstallInfo(), FinishIdleInstallInfo() and OnExtensionInstalled().

chrome/browser/extensions/extension_prefs.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 "chrome/browser/extensions/extension_prefs.h"
 
 #include "base/command_line.h"
 #include "base/prefs/pref_notifier.h"
 #include "base/string_number_conversions.h"
 #include "base/string_util.h"
@@ skipping 101 matching lines @@
 
 // A bit we use to keep track of whether we need to do an "active" ping.
 const char kActiveBit[] = "active_bit";
 
 // Path for settings specific to blacklist update.
 const char kExtensionsBlacklistUpdate[] = "extensions.blacklistupdate";
 
 // Path for the idle install info dictionary preference.
 const char kIdleInstallInfo[] = "idle_install_info";
 
+// Path for the suggested page ordinal of a delayed extension install.
+const char kPrefSuggestedPageOrdinal[] = "suggested_page_ordinal";
+
 // A preference that, if true, will allow this extension to run in incognito
 // mode.
 const char kPrefIncognitoEnabled[] = "incognito";
 
 // A preference to control whether an extension is allowed to inject script in
 // pages with file URLs.
 const char kPrefAllowFileAccess[] = "newAllowFileAccess";
 // TODO(jstritar): As part of fixing http://crbug.com/91577, we revoked all
 // extension file access by renaming the pref. We should eventually clean up
 // the old flag and possibly go back to that name.
@@ skipping 1309 matching lines @@
 }
 
 void ExtensionPrefs::SetActionBoxOrder(const ExtensionIdList& extension_ids) {
   SetExtensionPrefFromVector(kExtensionActionBox, extension_ids);
 }
 
 void ExtensionPrefs::OnExtensionInstalled(
     const Extension* extension,
     Extension::State initial_state,
     const syncer::StringOrdinal& page_ordinal) {
-  const std::string& id = extension->id();
-  CHECK(Extension::IdIsValid(id));
-  ScopedExtensionPrefUpdate update(prefs_, id);
+  ScopedExtensionPrefUpdate update(prefs_, extension->id());
   DictionaryValue* extension_dict = update.Get();
   const base::Time install_time = time_provider_->GetCurrentTime();
-
-  // Leave the state blank for component extensions so that old chrome versions
-  // loading new profiles do not fail in GetInstalledExtensionInfo. Older
-  // Chrome versions would only check for an omitted state.
-  if (initial_state != Extension::ENABLED_COMPONENT)
-    extension_dict->Set(kPrefState, Value::CreateIntegerValue(initial_state));
-
-  extension_dict->Set(kPrefLocation,
-                      Value::CreateIntegerValue(extension->location()));
-  extension_dict->Set(kPrefCreationFlags,
-                      Value::CreateIntegerValue(extension->creation_flags()));
-  extension_dict->Set(kPrefFromWebStore,
-                      Value::CreateBooleanValue(extension->from_webstore()));
-  extension_dict->Set(kPrefFromBookmark,
-                      Value::CreateBooleanValue(extension->from_bookmark()));
-  extension_dict->Set(kPrefWasInstalledByDefault,
-      Value::CreateBooleanValue(extension->was_installed_by_default()));
-  extension_dict->Set(kPrefInstallTime,
-                      Value::CreateStringValue(
-                          base::Int64ToString(install_time.ToInternalValue())));
-  extension_dict->Set(kPrefPreferences, new DictionaryValue());
-  extension_dict->Set(kPrefIncognitoPreferences, new DictionaryValue());
-  extension_dict->Set(kPrefRegularOnlyPreferences, new DictionaryValue());
-  extension_dict->Set(kPrefContentSettings, new ListValue());
-  extension_dict->Set(kPrefIncognitoContentSettings, new ListValue());
-
-  FilePath::StringType path = MakePathRelative(install_directory_,
-      extension->path());
-  extension_dict->Set(kPrefPath, Value::CreateStringValue(path));
-  // We store prefs about LOAD extensions, but don't cache their manifest
-  // since it may change on disk.
-  if (extension->location() != Extension::LOAD) {
-    extension_dict->Set(kPrefManifest,
-                        extension->manifest()->value()->DeepCopy());
-  }
-
-  // Clear state that may be registered from a previous install.
-  extension_dict->Remove(kRegisteredEvents, NULL);
-
-  if (extension->RequiresSortOrdinal())
-    extension_sorting_->EnsureValidOrdinals(extension->id(), page_ordinal);
-
-  extension_pref_value_map_->RegisterExtension(
-      id, install_time, initial_state == Extension::ENABLED);
-  content_settings_store_->RegisterExtension(
-      id, install_time, initial_state == Extension::ENABLED);
+  PopulateExtensionInfoPrefs(extension, install_time, initial_state,
+                             extension_dict);
+  FinishExtensionInfoPrefs(extension->id(), install_time,
+                           extension->RequiresSortOrdinal(),
+                           page_ordinal, extension_dict);
 }
 
 void ExtensionPrefs::OnExtensionUninstalled(const std::string& extension_id,
                                             const Extension::Location& location,
                                             bool external_uninstall) {
   extension_sorting_->ClearOrdinals(extension_id);
 
   // For external extensions, we save a preference reminding ourself not to try
   // and install the extension anymore (except when |external_uninstall| is
   // true, which signifies that the registry key was deleted or the pref file
@@ skipping 198 matching lines @@
     scoped_ptr<ExtensionInfo> info = GetInstalledExtensionInfo(*extension_id);
     if (info)
       extensions_info->push_back(linked_ptr<ExtensionInfo>(info.release()));
   }
 
   return extensions_info.Pass();
 }
 
 void ExtensionPrefs::SetIdleInstallInfo(
     const Extension* extension,
-    Extension::State initial_state) {
-  const base::Time install_time = time_provider_->GetCurrentTime();
+    Extension::State initial_state,
+    const syncer::StringOrdinal& page_ordinal) {
   DictionaryValue* extension_dict = new DictionaryValue();
-  extension_dict->Set(kPrefState, Value::CreateIntegerValue(initial_state));
-  extension_dict->Set(kPrefLocation,
-                      Value::CreateIntegerValue(extension->location()));
-  extension_dict->Set(kPrefCreationFlags,
-                      Value::CreateIntegerValue(extension->creation_flags()));
-  extension_dict->Set(kPrefFromWebStore,
-                      Value::CreateBooleanValue(extension->from_webstore()));
-  extension_dict->Set(kPrefFromBookmark,
-                      Value::CreateBooleanValue(extension->from_bookmark()));
-  extension_dict->Set(kPrefWasInstalledByDefault,
-      Value::CreateBooleanValue(extension->was_installed_by_default()));
-  extension_dict->Set(kPrefInstallTime,
-                      Value::CreateStringValue(
-                          base::Int64ToString(install_time.ToInternalValue())));
+  PopulateExtensionInfoPrefs(extension, time_provider_->GetCurrentTime(),
+                             initial_state, extension_dict);
 
-  FilePath::StringType path = MakePathRelative(install_directory_,
-      extension->path());
-  extension_dict->Set(kPrefPath, Value::CreateStringValue(path));
-  extension_dict->Set(kPrefManifest,
-                      extension->manifest()->value()->DeepCopy());
+  // Add transient data that is needed by FinishIdleInstallInfo(), but
+  // should not be in the final extension prefs. All entries here should have
+  // a corresponding Remove() call in FinishIdleInstallInfo().
+  if (extension->RequiresSortOrdinal()) {
+    extension_dict->SetString(kPrefSuggestedPageOrdinal,
+                              page_ordinal.ToInternalValue());
+  }
 
-  UpdateExtensionPref(extension->id(), kIdleInstallInfo, extension_dict);
+  // RESOLVE BEFORE COMMIT: Why did this used to use UpdateExtensionPref

[Marijn Kruisselbrink, 2012/12/07 17:29:49]

Why not? Most code seems to use UpdateExtensionPref instead of using
ScopedExtensionPrefUpdate directly.

[awong, 2012/12/07 22:02:25]

Okay...I'll move back to UpdateExtensionPref().  It's just that all the other
XXIdleInstallInfo() functions use the ScopedExtensinPrefUpdate method.  Glad to
know there's nothing special.

[Marijn Kruisselbrink, 2012/12/07 22:15:09]

Ah, yeah, RemoveIdleInstallInfo I didn't touch, and apparently the old unused
version used ScopedExtensionPrefUpdate, and FinishIdleInstallInfo was mostly
just copied from OnExtensionInstalled (and there it makes sense to use
ScopedExtensionPrefUpdate directly to make sure there's only one notification
rather than one for each property that is modified).

+  // directly?
+  ScopedExtensionPrefUpdate update(prefs_, extension->id());
+  update->Set(kIdleInstallInfo, extension_dict);
 }
 
 bool ExtensionPrefs::RemoveIdleInstallInfo(const std::string& extension_id) {
   if (!GetExtensionPref(extension_id))
     return false;
   ScopedExtensionPrefUpdate update(prefs_, extension_id);
   bool result = update->Remove(kIdleInstallInfo, NULL);
   return result;
 }
 
 bool ExtensionPrefs::FinishIdleInstallInfo(const std::string& extension_id) {
   CHECK(Extension::IdIsValid(extension_id));
   ScopedExtensionPrefUpdate update(prefs_, extension_id);
   DictionaryValue* extension_dict = update.Get();
-  DictionaryValue* update_dict;
-  if (!extension_dict->GetDictionary(kIdleInstallInfo, &update_dict))
+  DictionaryValue* pending_install_dict = NULL;
+  if (!extension_dict->GetDictionary(kIdleInstallInfo, &pending_install_dict))
     return false;
 
+  // Retrieve and clear transient values populated by SetIdleInstallInfo(). Also
+  // do any other data cleanup that makes sense.
+  std::string serialized_ordinal;
+  syncer::StringOrdinal suggested_page_ordinal;
+  bool needs_sort_ordinal = false;
+  if (pending_install_dict->GetString(kPrefSuggestedPageOrdinal,
+                                      &serialized_ordinal)) {
+    suggested_page_ordinal = syncer::StringOrdinal(serialized_ordinal);
+    needs_sort_ordinal = true;
+    pending_install_dict->Remove(kPrefSuggestedPageOrdinal, NULL);
+  }
+
   const base::Time install_time = time_provider_->GetCurrentTime();
-  extension_dict->MergeDictionary(update_dict);
-  extension_dict->Set(kPrefInstallTime,
-                      Value::CreateStringValue(
-                          base::Int64ToString(install_time.ToInternalValue())));
-  extension_dict->Set(kPrefPreferences, new DictionaryValue());
-  extension_dict->Set(kPrefIncognitoPreferences, new DictionaryValue());
-  extension_dict->Set(kPrefRegularOnlyPreferences, new DictionaryValue());
-  extension_dict->Set(kPrefContentSettings, new ListValue());
-  extension_dict->Set(kPrefIncognitoContentSettings, new ListValue());
+  pending_install_dict->Set(
+      kPrefInstallTime,
+      Value::CreateStringValue(
+          base::Int64ToString(install_time.ToInternalValue())));
 
-  // Clear state that may be registered from a previous install.
-  extension_dict->Remove(kRegisteredEvents, NULL);
-
-  // Remove pending update information
-  extension_dict->Remove(kIdleInstallInfo, NULL);
-
-  int initial_state;
-  if (extension_dict->GetInteger(kPrefState, &initial_state)) {
-    extension_pref_value_map_->RegisterExtension(
-        extension_id, install_time, initial_state == Extension::ENABLED);
-    content_settings_store_->RegisterExtension(
-        extension_id, install_time, initial_state == Extension::ENABLED);
-  }
+  // Commit the delayed install data.
+  extension_dict->MergeDictionary(pending_install_dict);
+  FinishExtensionInfoPrefs(extension_id, install_time, needs_sort_ordinal,
+                           suggested_page_ordinal, extension_dict);
   return true;
 }
 
 scoped_ptr<ExtensionInfo> ExtensionPrefs::GetIdleInstallInfo(
     const std::string& extension_id) const {
   const DictionaryValue* extension_prefs = GetExtensionPref(extension_id);
   if (!extension_prefs)
     return scoped_ptr<ExtensionInfo>();
 
   const DictionaryValue* ext = NULL;
@@ skipping 511 matching lines @@
     const char* pref,
     const ExtensionIdList& strings) {
   ListPrefUpdate update(prefs_, pref);
   ListValue* list_of_values = update.Get();
   list_of_values->Clear();
   for (ExtensionIdList::const_iterator iter = strings.begin();
        iter != strings.end(); ++iter)
     list_of_values->Append(new StringValue(*iter));
 }
 
+void ExtensionPrefs::PopulateExtensionInfoPrefs(
+    const Extension* extension,
+    const base::Time install_time,
+    Extension::State initial_state,
+    DictionaryValue* extension_dict) {
+  // Leave the state blank for component extensions so that old chrome versions
+  // loading new profiles do not fail in GetInstalledExtensionInfo. Older
+  // Chrome versions would only check for an omitted state.
+  if (initial_state != Extension::ENABLED_COMPONENT)
+    extension_dict->Set(kPrefState, Value::CreateIntegerValue(initial_state));
+
+  extension_dict->Set(kPrefLocation,
+                      Value::CreateIntegerValue(extension->location()));
+  extension_dict->Set(kPrefCreationFlags,
+                      Value::CreateIntegerValue(extension->creation_flags()));
+  extension_dict->Set(kPrefFromWebStore,
+                      Value::CreateBooleanValue(extension->from_webstore()));
+  extension_dict->Set(kPrefFromBookmark,
+                      Value::CreateBooleanValue(extension->from_bookmark()));
+  extension_dict->Set(kPrefWasInstalledByDefault,
+      Value::CreateBooleanValue(extension->was_installed_by_default()));
+  extension_dict->Set(kPrefInstallTime,
+                      Value::CreateStringValue(
+                          base::Int64ToString(install_time.ToInternalValue())));
+
+  FilePath::StringType path = MakePathRelative(install_directory_,
+      extension->path());
+  extension_dict->Set(kPrefPath, Value::CreateStringValue(path));
+  // We store prefs about LOAD extensions, but don't cache their manifest
+  // since it may change on disk.
+  if (extension->location() != Extension::LOAD) {
+    // RESOLVE ME BEFORE COMMIT: We did not preivously avoid this for LOAD extensions in

[Marijn Kruisselbrink, 2012/12/07 17:29:49]

Yes, I agree. No reason to not avoid this even in the delayed install case.

[awong, 2012/12/07 22:02:25]

Done.

+    // SetIdleInstallInfo(). Having this condition still seems sensible even
+    // if the extension was delayed.
+    extension_dict->Set(kPrefManifest,
+                        extension->manifest()->value()->DeepCopy());
+  }
+
+  // RESOLVE ME BEFORE COMMIT: This could be created post commit, but it feels

[Marijn Kruisselbrink, 2012/12/07 17:29:49]

I'm not sure if this actually works though. In FinishIdleInfo we use
DictionaryValue::MergeDictionary to get the delayed info in the correct
location, but MergeDictionary recursively merges dictionaries, so setting some
of these values to empty dictionaries won't actually result in the dictionaries
being erased.

[awong, 2012/12/07 22:02:25]

Ah! Good catch. Moved into FinishExtensionInfoPrefs.

+  // cleaner to just keep all this in one spot.  Do we actually care enough to
+  // save these 5 empty preferences from being committed to disk?
+  extension_dict->Set(kPrefPreferences, new DictionaryValue());
+  extension_dict->Set(kPrefIncognitoPreferences, new DictionaryValue());
+  extension_dict->Set(kPrefRegularOnlyPreferences, new DictionaryValue());
+  extension_dict->Set(kPrefContentSettings, new ListValue());
+  extension_dict->Set(kPrefIncognitoContentSettings, new ListValue());
+}
+
+void ExtensionPrefs::FinishExtensionInfoPrefs(
+    const std::string& extension_id,
+    const base::Time install_time,
+    bool needs_sort_ordinal,
+    const syncer::StringOrdinal& suggested_page_ordinal,
+    DictionaryValue* extension_dict) {
+  // If this point has been reached, any pending installs should be considered
+  // out of date.
+  extension_dict->Remove(kIdleInstallInfo, NULL);
+
+  // Clear state that may be registered from a previous install.
+  extension_dict->Remove(kRegisteredEvents, NULL);
+
+  // FYI, all code below here races on sudden shutdown because
+  // |extension_dict|, |extension_sorting_|, |extension_pref_value_map_|,
+  // and |content_settings_store_| are updated non-transactionally. This is
+  // probably not fixable without nested transactional updates to pref
+  // dictionaries.
+  if (needs_sort_ordinal) {
+    extension_sorting_->EnsureValidOrdinals(extension_id,
+                                            suggested_page_ordinal);
+  }
+
+  int initial_state;
+  if (extension_dict->GetInteger(kPrefState, &initial_state)) {
+  /* RESOLVE ME BEFORE COMMIT: This conditional seems impossible to fail since

[Marijn Kruisselbrink, 2012/12/07 17:29:49]

Isn't initial_state not set when its value would be
Extension::ENABLED_COMPONENT? So it seems like this condition can fail
(apparently that is for supporting new prefs with old chromes... see the comment
in PopulateExtensionInfoPrefs)

[awong, 2012/12/07 22:02:25]

Oh interesting. Yeah, that came in via a last minute merge.  So is skipping the
RegisterExtension() calls in extension_pref_value_map_ and
content_settings_store_ the right behavior?

[Marijn Kruisselbrink, 2012/12/07 22:15:09]

It probably isn't right to skip these calls. OnExtensionInstalled always called
these methods.

[awong, 2012/12/07 22:56:53]

Added some logic that I *think* has the correct behavior.

+   * SetIdleInstallInfo() sets it. Should we NOTREACHED() the failure of this
+   * or are we supporting old prefs somehow? If we're supporting old prefs,
+   * should we pick a default initial_state?
+   */
+    extension_pref_value_map_->RegisterExtension(
+        extension_id, install_time, initial_state == Extension::ENABLED);
+    content_settings_store_->RegisterExtension(
+        extension_id, install_time, initial_state == Extension::ENABLED);
+  }
+}
+
 }  // namespace extensions