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

 // 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/common/extensions/features/base_feature_provider.h"
 
 #include "base/json/json_reader.h"
 #include "base/lazy_instance.h"
 #include "chrome/common/extensions/features/complex_feature.h"
 #include "chrome/common/extensions/features/manifest_feature.h"
@@ skipping 23 matching lines @@
   }
 
   scoped_ptr<BaseFeatureProvider> manifest_features;
   scoped_ptr<BaseFeatureProvider> permission_features;
 
  private:
   scoped_ptr<BaseFeatureProvider> LoadProvider(
       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));
   }
 };
 
 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;
 
 }  // 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()) {
     if (iter.value().GetType() == Value::TYPE_DICTIONARY) {
       linked_ptr<SimpleFeature> feature((*factory_)());
 
       if (!ParseFeature(static_cast<const DictionaryValue*>(&iter.value()),
                         iter.key(),
                         feature.get()))
         continue;
@@ skipping 59 matching lines @@
 
 Feature* BaseFeatureProvider::GetFeature(const std::string& name) {
   FeatureMap::iterator iter = features_.find(name);
   if (iter != features_.end())
     return iter->second.get();
   else
     return NULL;
 }
 
 }  // namespace