Make runtime.reload() work with component extensions.

extensions/browser/extension_prefs.cc

 // Copyright 2014 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 "extensions/browser/extension_prefs.h"
 
 #include <iterator>
 
 #include "base/command_line.h"
 #include "base/prefs/pref_notifier.h"
@@ skipping 1319 matching lines @@
   }
 }
 
 scoped_ptr<ExtensionInfo> ExtensionPrefs::GetInstalledInfoHelper(
     const std::string& extension_id,
     const base::DictionaryValue* extension) const {
   int location_value;
   if (!extension->GetInteger(kPrefLocation, &location_value))
     return scoped_ptr<ExtensionInfo>();
 
   base::FilePath::StringType path;

[tapted, 2014/08/13 00:25:07]

nit: `path` isn't used until the very end of the function, so I'd move lines
1340-1349 down to the bottom

[Anand Mistry (off Chromium), 2014/08/13 01:53:15]

Done.

   if (!extension->GetString(kPrefPath, &path))
     return scoped_ptr<ExtensionInfo>();
 
   // Make path absolute. Unpacked extensions will already have absolute paths,
   // otherwise make it so.
   Manifest::Location location = static_cast<Manifest::Location>(location_value);
-#if !defined(OS_CHROMEOS)
-  if (!Manifest::IsUnpackedLocation(location)) {
-    DCHECK(location == Manifest::COMPONENT ||
-           !base::FilePath(path).IsAbsolute());
-#else
-  // On Chrome OS some extensions can be installed to shared location and
-  // thus use absolute paths in prefs.
+  // Most (but not all) extension types don't have absolute paths.
   if (!base::FilePath(path).IsAbsolute()) {

[tapted, 2014/08/13 00:25:07]

nit: no curlies

[Anand Mistry (off Chromium), 2014/08/13 01:53:15]

Yuk! I prefer braces.

-#endif  // !defined(OS_CHROMEOS)
     path = install_directory_.Append(path).value();
   }
 
+  if (location == Manifest::COMPONENT) {
+    // Component extensions are ignored. Replicates the state before this
+    // change.
+    // TODO(amistry): Is this what we want?

[tapted, 2014/08/13 00:25:07]

yep! But I think the sentiment we want is: Component extensions may have data
saved in preferences, but they are already loaded at this point (by
ComponentLoader) and shouldn't be populated into the result of
GetInstalledExtensionsInfo, otherwise InstalledLoader would also want to load
them. Exiting here also skips the path validation, which doesn't make sense for
component extensions since they are loaded from the resource bundle.

(.. maybe a shorter version of that)

[Anand Mistry (off Chromium), 2014/08/13 01:53:15]

Done.

+    return scoped_ptr<ExtensionInfo>();
+  }
+
   // Only the following extension types have data saved in the preferences.
   if (location != Manifest::INTERNAL &&
       !Manifest::IsUnpackedLocation(location) &&
       !Manifest::IsExternalLocation(location)) {
     NOTREACHED();
     return scoped_ptr<ExtensionInfo>();
   }
 
   const base::DictionaryValue* manifest = NULL;
   if (!Manifest::IsUnpackedLocation(location) &&
       !extension->GetDictionary(kPrefManifest, &manifest)) {
     LOG(WARNING) << "Missing manifest for extension " << extension_id;
     // Just a warning for now.
   }
 
   return scoped_ptr<ExtensionInfo>(new ExtensionInfo(
       manifest, extension_id, base::FilePath(path), location));
 }
 
 scoped_ptr<ExtensionInfo> ExtensionPrefs::GetInstalledExtensionInfo(
     const std::string& extension_id) const {
   const base::DictionaryValue* ext = NULL;
   const base::DictionaryValue* extensions =
       prefs_->GetDictionary(pref_names::kExtensions);
   if (!extensions ||
       !extensions->GetDictionaryWithoutPathExpansion(extension_id, &ext))
     return scoped_ptr<ExtensionInfo>();
   int state_value;
-  if (!ext->GetInteger(kPrefState, &state_value) ||
-      state_value == Extension::ENABLED_COMPONENT) {
+  if (!ext->GetInteger(kPrefState, &state_value)) {

[tapted, 2014/08/13 00:25:07]

I'd probably combine this with the statement below, and drop the comment about
component extensions. i.e.

if (ext->GetInteger(kPrefState, &state_value) && state_value ==
..EXTERNAL_EXTENSION_UNINSTALLED) {
  LOG(..);
  return;
}

i.e. rely just on the check in GetInstalledInfoHelper, which is more robust

[Anand Mistry (off Chromium), 2014/08/13 01:53:15]

Done.

     // Old preferences files may not have kPrefState for component extensions.
     return scoped_ptr<ExtensionInfo>();
   }
 
   if (state_value == Extension::EXTERNAL_EXTENSION_UNINSTALLED) {
     LOG(WARNING) << "External extension with id " << extension_id
                  << " has been uninstalled by the user";
     return scoped_ptr<ExtensionInfo>();
   }
 
@@ skipping 655 matching lines @@
 void ExtensionPrefs::PopulateExtensionInfoPrefs(
     const Extension* extension,
     const base::Time install_time,
     Extension::State initial_state,
     int install_flags,
     const std::string& install_parameter,
     base::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)
+  if (extension->manifest()->location() != Manifest::COMPONENT)

[tapted, 2014/08/13 00:25:07]

This extra check can probably be removed (i.e. write the pref unconditionally).
_really_ old versions of Chrome will still crash if this is encountered, but for
~20 months they will have had that early exit case. And we don't usually cater
for old versions of chome this way - but when this first went in crash on
startup was guaranteed otherwise if you try to go e.g. Beta->Stable with the
same profile. Should be safer now.

[Anand Mistry (off Chromium), 2014/08/13 01:53:15]

Done.

[tapted, 2014/08/13 03:06:55]

so yeah - looks like this can't tickle a crash out of stable, so should be fine.
cool.

     extension_dict->Set(kPrefState, new base::FundamentalValue(initial_state));
 
   extension_dict->Set(kPrefLocation,
                       new base::FundamentalValue(extension->location()));
   extension_dict->Set(kPrefCreationFlags,
                       new base::FundamentalValue(extension->creation_flags()));
   extension_dict->Set(kPrefFromWebStore,
                       new base::FundamentalValue(extension->from_webstore()));
   extension_dict->Set(kPrefFromBookmark,
                       new base::FundamentalValue(extension->from_bookmark()));
@@ skipping 126 matching lines @@
   extension_pref_value_map_->RegisterExtension(
       extension_id, install_time, is_enabled, is_incognito_enabled);
 
   FOR_EACH_OBSERVER(
       ExtensionPrefsObserver,
       observer_list_,
       OnExtensionRegistered(extension_id, install_time, is_enabled));
 }
 
 }  // namespace extensions