Lazily load API schemas from resource files and convert all APIs to features

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 285426d147d108f684711b6ef13a51a177ddc7e8..d09b4616b70bfa8bc704e2b49df591cafd85ee9c 100644
--- a/chrome/common/extensions/features/base_feature_provider.cc
+++ b/chrome/common/extensions/features/base_feature_provider.cc
@@ -4,8 +4,12 @@
 
 #include "chrome/common/extensions/features/base_feature_provider.h"
 
+#include <stack>
+
 #include "base/json/json_reader.h"
 #include "base/lazy_instance.h"
+#include "base/strings/string_split.h"
+#include "base/strings/string_util.h"
 #include "chrome/common/extensions/features/api_feature.h"
 #include "chrome/common/extensions/features/complex_feature.h"
 #include "chrome/common/extensions/features/manifest_feature.h"
@@ -36,7 +40,7 @@ class LazyFeatureProvider : public FeatureProvider {
     return GetBaseFeatureProvider()->GetFeature(name);
   }
 
-  virtual std::set<std::string> GetAllFeatureNames() OVERRIDE {
+  virtual const std::vector<std::string>& GetAllFeatureNames() OVERRIDE {
     return GetBaseFeatureProvider()->GetAllFeatureNames();
   }
 
@@ -125,9 +129,43 @@ BaseFeatureProvider::BaseFeatureProvider(const DictionaryValue& root,
     if (iter.value().GetType() == Value::TYPE_DICTIONARY) {
       linked_ptr<SimpleFeature> feature((*factory_)());
 
-      if (!ParseFeature(static_cast<const DictionaryValue*>(&iter.value()),
-                        iter.key(),
-                        feature.get()))
+      std::vector<std::string> split;
+      base::SplitString(iter.key(), '.', &split);
+
+      // Push parent features on the stack, starting with the current feature.
+      // If one of the features has "noparent" set, stop pushing features on
+      // the stack. The features will then be parsed in order, starting with
+      // the farthest parent that is either top level or has "noparent" set.
+      std::stack<std::pair<std::string, const DictionaryValue*> > parse_stack;
+      while (!split.empty()) {
+        std::string parent_name = JoinString(split, '.');
+        split.pop_back();
+        if (root.HasKey(parent_name)) {
+          const DictionaryValue* parent = NULL;
+          CHECK(root.GetDictionaryWithoutPathExpansion(parent_name, &parent));
+          parse_stack.push(std::make_pair(parent_name, parent));
+          bool no_parent = false;
+          parent->GetBoolean("noparent", &no_parent);
+          if (no_parent)
+            break;
+        }
+      }
+
+      CHECK(!parse_stack.empty());
+      // Parse all parent features.
+      feature->SetHasParent(parse_stack.size() > 1);

[cduvall, 2013/06/12 01:22:19]

Instead of using SetHasParent() here, and then using HasParent() in other
places, I could figure it out algorithmically (like in dispatcher.cc). I think
this makes it simpler though.

[not at google - send to devlin, 2013/06/12 22:34:18]

The only thing I'm worried about is that the parent logic might need to diverge
depending on what features end up being available.

E.g. if we have chrome.foo and chrome.foo.bar then chrome.foo.bar only has a
parent if chrome.foo is also available. We have this with chrome.app and I guess
that's the point, but is it moot anyway now that we only set up stub bindings in
things that are available to the context? I can't remember.

I guess what I'm saying is, would it work to add this GetParent thing to the
FeatureProvider itself. Then the logic in dispatcher.cc could change from

  632     Feature* feature = feature_provider->GetFeature(api_name);
  633     DCHECK(feature);
  634     if (feature->HasParent() || feature->IsInternal())
  635       continue;
  636 

to

Feature* feature = ...;
DCHECK(feature);
if (feature->IsInternal() || !context->IsAnyFeatureAvailableToContext(api_name))
  continue;

// ... explain
Feature* parent_feature = feature_provider->GetParent(feature);
if (parent_feature && parent_feature->IsAvailable(...)) {
  CHECK(!feature->no_parent())
  continue;
}

something like that anyway. I think I might be getting a bit mixed up here,
sorry, but in any case we should be holding onto the noparent thing not the
other way around? Let's continue this in IRC.

[cduvall, 2013/06/13 01:02:28]

Done.

+      bool parse_error = false;
+      while (!parse_stack.empty()) {
+        if (!ParseFeature(parse_stack.top().second,
+                          parse_stack.top().first,
+                          feature.get())) {
+          parse_error = true;
+          break;
+        }
+        parse_stack.pop();
+      }
+
+      if (parse_error)
         continue;
 
       features_[iter.key()] = feature;
@@ -176,13 +214,14 @@ FeatureProvider* BaseFeatureProvider::GetByName(
   return g_static.Get().LazyGetFeatures(name);
 }
 
-std::set<std::string> BaseFeatureProvider::GetAllFeatureNames() {
-  std::set<std::string> result;
-  for (FeatureMap::const_iterator iter = features_.begin();
-       iter != features_.end(); ++iter) {
-    result.insert(iter->first);
+const std::vector<std::string>& BaseFeatureProvider::GetAllFeatureNames() {
+  if (feature_names_.empty()) {
+    for (FeatureMap::const_iterator iter = features_.begin();
+         iter != features_.end(); ++iter) {
+      feature_names_.push_back(iter->first);
+    }
   }
-  return result;
+  return feature_names_;
 }
 
 Feature* BaseFeatureProvider::GetFeature(const std::string& name) {
@@ -193,4 +232,4 @@ Feature* BaseFeatureProvider::GetFeature(const std::string& name) {
     return NULL;
 }
 
-}  // namespace
+} // namespace extensions