Load extensions features file separately when needed

Load extensions features file separately when needed

The JSON file for each feature type is now lazily loaded and
parsed separately.

BUG=55316
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=193532

[not at google - send to devlin, 2013-04-10 02:06:22]

nice change.

https://codereview.chromium.org/13997002/diff/2001/chrome/common/extensions/f...
File chrome/common/extensions/features/base_feature_provider.cc (right):

https://codereview.chromium.org/13997002/diff/2001/chrome/common/extensions/f...
chrome/common/extensions/features/base_feature_provider.cc:25: struct
LazyFeatureProvider : public FeatureProvider {
I don't think this needs to be a struct. The member variables should be private
(and be name_, factory_, etc too).

https://codereview.chromium.org/13997002/diff/2001/chrome/common/extensions/f...
chrome/common/extensions/features/base_feature_provider.cc:36:
features.reset(LoadProvider(name, factory, resource_id).release());
shouldn't need the release/reset here. features = LoadProvider(...) should work.

https://codereview.chromium.org/13997002/diff/2001/chrome/common/extensions/f...
chrome/common/extensions/features/base_feature_provider.cc:78:
lazy_feature_providers["api"] = linked_ptr<LazyFeatureProvider>(
make_linked_ptr?

https://codereview.chromium.org/13997002/diff/2001/chrome/common/extensions/f...
chrome/common/extensions/features/base_feature_provider.cc:94: return
lazy_feature_providers[name].get();
use find, it = lazy_feature_providers.find(..); CHECK(it != end()); return
it->second;

https://codereview.chromium.org/13997002/diff/2001/chrome/common/extensions/f...
File chrome/common/extensions/features/base_feature_provider.h (right):

https://codereview.chromium.org/13997002/diff/2001/chrome/common/extensions/f...
chrome/common/extensions/features/base_feature_provider.h:33: static
FeatureProvider* LazyGetFeaturesByName(const std::string& name);
Is there really a difference? Can you combine these into a single method (also
I'd call it GetByName) and make that the lazy version?

[cduvall, 2013-04-10 02:26:12]

https://codereview.chromium.org/13997002/diff/2001/chrome/common/extensions/f...
File chrome/common/extensions/features/base_feature_provider.cc (right):

https://codereview.chromium.org/13997002/diff/2001/chrome/common/extensions/f...
chrome/common/extensions/features/base_feature_provider.cc:25: struct
LazyFeatureProvider : public FeatureProvider {
On 2013/04/10 02:06:22, kalman wrote:
> I don't think this needs to be a struct. The member variables should be
private
> (and be name_, factory_, etc too).

Done.

https://codereview.chromium.org/13997002/diff/2001/chrome/common/extensions/f...
chrome/common/extensions/features/base_feature_provider.cc:36:
features.reset(LoadProvider(name, factory, resource_id).release());
On 2013/04/10 02:06:22, kalman wrote:
> shouldn't need the release/reset here. features = LoadProvider(...) should
work.

Done.

https://codereview.chromium.org/13997002/diff/2001/chrome/common/extensions/f...
chrome/common/extensions/features/base_feature_provider.cc:78:
lazy_feature_providers["api"] = linked_ptr<LazyFeatureProvider>(
On 2013/04/10 02:06:22, kalman wrote:
> make_linked_ptr?

Done.

https://codereview.chromium.org/13997002/diff/2001/chrome/common/extensions/f...
chrome/common/extensions/features/base_feature_provider.cc:94: return
lazy_feature_providers[name].get();
On 2013/04/10 02:06:22, kalman wrote:
> use find, it = lazy_feature_providers.find(..); CHECK(it != end()); return
> it->second;

Done.

https://codereview.chromium.org/13997002/diff/2001/chrome/common/extensions/f...
File chrome/common/extensions/features/base_feature_provider.h (right):

https://codereview.chromium.org/13997002/diff/2001/chrome/common/extensions/f...
chrome/common/extensions/features/base_feature_provider.h:33: static
FeatureProvider* LazyGetFeaturesByName(const std::string& name);
On 2013/04/10 02:06:22, kalman wrote:
> Is there really a difference? Can you combine these into a single method (also
> I'd call it GetByName) and make that the lazy version?

They're both needed because GetFeaturesByName() returns a BaseFeatureProvider,
which is used in some places for BaseFeatureProvider::GetAllFeatureNames().
LazyGetFeaturesByName() just returns a standard FeatureProvider, which just has
GetFeature().

I renamed to GetByName() and GetBaseFeatureProviderByName(), which is a little
verbose (any better ideas?)

[not at google - send to devlin, 2013-04-10 02:42:13]

lgtm if you can delete GetBaseFeatureProviderByName :)

https://codereview.chromium.org/13997002/diff/2001/chrome/common/extensions/f...
File chrome/common/extensions/features/base_feature_provider.h (right):

https://codereview.chromium.org/13997002/diff/2001/chrome/common/extensions/f...
chrome/common/extensions/features/base_feature_provider.h:33: static
FeatureProvider* LazyGetFeaturesByName(const std::string& name);
On 2013/04/10 02:26:12, cduvall wrote:
> On 2013/04/10 02:06:22, kalman wrote:
> > Is there really a difference? Can you combine these into a single method
(also
> > I'd call it GetByName) and make that the lazy version?
> 
> They're both needed because GetFeaturesByName() returns a BaseFeatureProvider,
> which is used in some places for BaseFeatureProvider::GetAllFeatureNames().
> LazyGetFeaturesByName() just returns a standard FeatureProvider, which just
has
> GetFeature().
> 
> I renamed to GetByName() and GetBaseFeatureProviderByName(), which is a little
> verbose (any better ideas?)

Ah ok. Perhaps we should pull GetAllFeatureNames into the FeatureProvider
interface then.

[cduvall, 2013-04-10 02:57:56]

https://codereview.chromium.org/13997002/diff/2001/chrome/common/extensions/f...
File chrome/common/extensions/features/base_feature_provider.h (right):

https://codereview.chromium.org/13997002/diff/2001/chrome/common/extensions/f...
chrome/common/extensions/features/base_feature_provider.h:33: static
FeatureProvider* LazyGetFeaturesByName(const std::string& name);
On 2013/04/10 02:42:13, kalman wrote:
> On 2013/04/10 02:26:12, cduvall wrote:
> > On 2013/04/10 02:06:22, kalman wrote:
> > > Is there really a difference? Can you combine these into a single method
> (also
> > > I'd call it GetByName) and make that the lazy version?
> > 
> > They're both needed because GetFeaturesByName() returns a
BaseFeatureProvider,
> > which is used in some places for BaseFeatureProvider::GetAllFeatureNames().
> > LazyGetFeaturesByName() just returns a standard FeatureProvider, which just
> has
> > GetFeature().
> > 
> > I renamed to GetByName() and GetBaseFeatureProviderByName(), which is a
little
> > verbose (any better ideas?)
> 
> Ah ok. Perhaps we should pull GetAllFeatureNames into the FeatureProvider
> interface then.

Done, had to take off the const, because it wouldn't work for the
LazyFeatureProvider.

[commit-bot: I haz the power, 2013-04-10 02:58:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cduvall@chromium.org/13997002/20001

[commit-bot: I haz the power, 2013-04-10 02:58:45]

Failed to apply patch for
chrome/common/extensions/features/base_feature_provider_unittest.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file
chrome/common/extensions/features/base_feature_provider_unittest.cc
  Hunk #1 FAILED at 10.
  Hunk #2 FAILED at 19.
  Hunk #3 FAILED at 61.
  3 out of 3 hunks FAILED -- saving rejects to file
chrome/common/extensions/features/base_feature_provider_unittest.cc.rej

Patch:       chrome/common/extensions/features/base_feature_provider_unittest.cc
Index: chrome/common/extensions/features/base_feature_provider_unittest.cc
diff --git a/chrome/common/extensions/features/base_feature_provider_unittest.cc
b/chrome/common/extensions/features/base_feature_provider_unittest.cc
index
df876dc88c308106d9fd77d668e180aba12f118d..7d71c5b4fbdfc05baba6e2d5e400593f235b85d3
100644
--- a/chrome/common/extensions/features/base_feature_provider_unittest.cc
+++ b/chrome/common/extensions/features/base_feature_provider_unittest.cc
@@ -10,6 +10,7 @@
 
 using chrome::VersionInfo;
 using extensions::BaseFeatureProvider;
+using extensions::FeatureProvider;
 using extensions::DictionaryBuilder;
 using extensions::Extension;
 using extensions::Feature;
@@ -19,8 +20,7 @@ using extensions::PermissionFeature;
 using extensions::SimpleFeature;
 
 TEST(BaseFeatureProvider, ManifestFeatures) {
-  BaseFeatureProvider* provider =
-      BaseFeatureProvider::GetManifestFeatures();
+  FeatureProvider* provider = BaseFeatureProvider::GetByName("manifest");
   SimpleFeature* feature =
       static_cast<SimpleFeature*>(provider->GetFeature("description"));
   ASSERT_TRUE(feature);
@@ -61,8 +61,7 @@ TEST(BaseFeatureProvider, ManifestFeatures) {
 }
 
 TEST(BaseFeatureProvider, PermissionFeatures) {
-  BaseFeatureProvider* provider =
-      BaseFeatureProvider::GetPermissionFeatures();
+  FeatureProvider* provider = BaseFeatureProvider::GetByName("permission");
   SimpleFeature* feature =
       static_cast<SimpleFeature*>(provider->GetFeature("contextMenus"));
   ASSERT_TRUE(feature);

[commit-bot: I haz the power, 2013-04-10 23:13:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cduvall@chromium.org/13997002/25001

[commit-bot: I haz the power, 2013-04-11 01:34:00]

Change committed as 193532