Extension BaseFeatureProvider minor cleanups. Don't crash in release mode if we failed to parse a p…

chrome/common/extensions/features/base_feature_provider.cc

Index: chrome/common/extensions/features/base_feature_provider.cc
diff --git a/chrome/common/extensions/features/base_feature_provider.cc b/chrome/common/extensions/features/base_feature_provider.cc
index d42cb50acc90375e67930af3aeeb64b086de1bd1..05ab74f3d83c87531ee57b83549ddcfb2f5b69a0 100644
--- a/chrome/common/extensions/features/base_feature_provider.cc
+++ b/chrome/common/extensions/features/base_feature_provider.cc
@@ -41,19 +41,22 @@ struct Static {
       const std::string& debug_string,
       BaseFeatureProvider::FeatureFactory factory,
       int resource_id) {
-    std::string manifest_features =
+    const std::string& features_file =
         ResourceBundle::GetSharedInstance().GetRawDataResource(
             resource_id).as_string();
     int error_code = 0;
     std::string error_message;
     Value* value = base::JSONReader::ReadAndReturnError(
-        manifest_features, base::JSON_PARSE_RFC,
+        features_file, base::JSON_PARSE_RFC,
         &error_code, &error_message);
-    CHECK(value) << "Could not load features: " << debug_string << " "
+    scoped_ptr<DictionaryValue> dictionary_value(new DictionaryValue());
+    if (!value) {

[not at google - send to devlin, 2013/02/15 16:33:34]

link to bug

[justinlin, 2013/02/21 09:18:23]

Done.

+      LOG(ERROR) << "Could not load features: " << debug_string << " "
                  << error_message;

[not at google - send to devlin, 2013/02/15 16:33:34]

If we're going to clean up this stuff, something like

scoped_ptr<Value> value(base::JSONReader::ReadAndReturnError(...))
DCHECK(value) << ...;
scoped_ptr<DictionaryValue> value_as_dict;
if (value) {
  CHECK(value->IsType(...));
  value_as_dict = value.PassAs<DictionaryValue>();
} else {
  value_as_dict.reset(new DictionaryValue());
}
return make_scoped_ptr(new BaseFeatureProvider(*dictionary_value, factory));

[justinlin, 2013/02/21 09:18:23]

Done, except I can't seem to use PassAs<DictionaryValue>() for this particular
downcast. I don't see any doc in scoped_ptr about PassAs supporting downcasts..

../../base/memory/scoped_ptr.h:202:9: error: no matching constructor for
initialization of 'base::internal::scoped_ptr_impl<base::DictionaryValue,
base::DefaultDeleter<base::DictionaryValue> >
::Data'
      : data_(other->release(), other->get_deleter()) {
        ^     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../base/memory/scoped_ptr.h:339:40: note: in instantiation of function
template specialization 'base::internal::scoped_ptr_impl<base::DictionaryValue,
base::DefaultDeleter<base::DictionaryV
alue> >::scoped_ptr_impl<base::Value, base::DefaultDeleter<base::Value> >'
requested here
  scoped_ptr(scoped_ptr<U, V> other) : impl_(&other.impl_) {
                                       ^

[not at google - send to devlin, 2013/02/21 16:20:09]

Bummer, sorry about that.

-    CHECK(value->IsType(Value::TYPE_DICTIONARY)) << debug_string;
-    scoped_ptr<DictionaryValue> dictionary_value(
-        static_cast<DictionaryValue*>(value));
+    } else {
+      CHECK(value->IsType(Value::TYPE_DICTIONARY)) << debug_string;
+      dictionary_value.reset(static_cast<DictionaryValue*>(value));

[not at google - send to devlin, 2013/02/15 16:33:34]

(This means that the DictionaryValue created on line 52 is unnecessary)

[justinlin, 2013/02/21 09:18:23]

True. It was mostly for safety in case another "if" clause gets added and
doesn't initialize the pointer, but your way looks better.

+    }
     return scoped_ptr<BaseFeatureProvider>(
         new BaseFeatureProvider(*dictionary_value, factory));
   }
@@ -62,21 +65,21 @@ struct Static {
 bool ParseFeature(const DictionaryValue* value,
                   const std::string& name,
                   SimpleFeature* feature) {
-    feature->set_name(name);
-    feature->Parse(value);
+  feature->set_name(name);
+  feature->Parse(value);
 
-    if (feature->extension_types()->empty()) {
-      LOG(ERROR) << name << ": Simple features must specify at least one "
-                 << "value for extension_types.";
-      return false;
-    }
+  if (feature->extension_types()->empty()) {
+    LOG(ERROR) << name << ": Simple features must specify at least one "
+               << "value for extension_types.";
+    return false;
+  }
 
-    if (!feature->GetContexts()->empty()) {
-      LOG(ERROR) << name << ": Simple features do not support contexts.";
-      return false;
-    }
+  if (!feature->GetContexts()->empty()) {
+    LOG(ERROR) << name << ": Simple features do not support contexts.";
+    return false;
+  }
 
-    return true;
+  return true;
 }
 
 base::LazyInstance<Static> g_static = LAZY_INSTANCE_INITIALIZER;
@@ -84,7 +87,7 @@ base::LazyInstance<Static> g_static = LAZY_INSTANCE_INITIALIZER;
 }  // namespace
 
 BaseFeatureProvider::BaseFeatureProvider(const DictionaryValue& root,
-                                             FeatureFactory factory)
+                                         FeatureFactory factory)
     : factory_(factory ? factory :
                static_cast<FeatureFactory>(&CreateFeature<SimpleFeature>)) {
   for (DictionaryValue::Iterator iter(root); iter.HasNext(); iter.Advance()) {