Extensions: Only create Web request rules registry if Declarative Web Request is enabled.

Extensions: Only create Web request rules registry if Declarative Web Request is enabled.

Currently the Web Request rules registry required by the Declarative Web
Request API is created on all Chrome channels, even though the Declarative Web
Request API is disabled on Stable. The UMA metric
Extensions.NetworkDelayRegistryLoad shows significant delay for some web
requests which are blocked due to the loading of the Web Request rules
registry even on Stable. The mean delay is around 1.7 sec and the median delay
is around 50ms.

This CL modifies RulesRegistryService::EnsureDefaultRulesRegistriesRegistered
to ensure the Web Request rules registry is only created for the channels on
which the Declarative Web Request API is enabled. To do this,
Feature::IsAvailableToChannel is introduced.

BUG=693243
Review-Url: https://codereview.chromium.org/2705513002
Cr-Commit-Position: refs/heads/master@{#452240}
Committed: https://chromium.googlesource.com/chromium/src/+/a77b53baca5f2a4ff102592c17c8ea3f53dd5307

[karandeepb, 2017-02-16 22:41:02]

The CQ bit was checked by karandeepb@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-16 22:42:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-17 01:44:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-17 01:44:51]

Dry run: This issue passed the CQ dry run.

[karandeepb, 2017-02-17 02:02:11]

Description was changed from

==========
Extensions: Only create Web request rules registry if Declarative Web Request is
enabled.
==========

to

==========
Extensions: Only create Web request rules registry if Declarative Web Request is
enabled.

Currently the Web Request rules registry required by the Declarative Web
Request API is created on all Chrome channels, even though the Declarative Web
Request API is disabled on Stable. The UMA metric
Extensions.NetworkDelayRegistryLoad shows significant delay for some web
requests which are blocked due to the loading of the Web Request rules
registry even on Stable. The mean delay is around 1.7 sec and the median delay
is around 50ms.

This CL modifies RulesRegistryService::EnsureDefaultRulesRegistriesRegistered
to ensure the Web Request rules registry is only created for the channels on
which the Declarative Web Request API is enabled. To do this,
Feature::IsAvailableToChannel is introduced.

BUG=693243
==========

[karandeepb, 2017-02-17 02:07:56]

karandeepb@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org

[karandeepb, 2017-02-17 02:07:57]

PTAL Devlin. Can add tests for IsAvailableToChannel if you think the approach is
sound.

https://codereview.chromium.org/2705513002/diff/20001/extensions/browser/api/...
File extensions/browser/api/declarative/rules_registry_service.cc (left):

https://codereview.chromium.org/2705513002/diff/20001/extensions/browser/api/...
extensions/browser/api/declarative/rules_registry_service.cc:78:
RulesCacheDelegate* web_request_cache_delegate = NULL;
Wasn't able to find code which relied on the existence of a web request rules
registry.

https://codereview.chromium.org/2705513002/diff/20001/extensions/common/featu...
File extensions/common/features/feature.h (right):

https://codereview.chromium.org/2705513002/diff/20001/extensions/common/featu...
extensions/common/features/feature.h:143: virtual Availability
IsAvailableToChannel(
I couldn't find an existing way to check if an API is available on a channel.
There is IsAvailableToEnvironment, but I don't think it works correctly (Not
even sure what it is intended for, it has no actual callers, except tests).

[Devlin, 2017-02-17 19:58:28]

This seems reasonable to me.  A couple quick sanity tests on the new feature
method would be great, but otherwise lg.

Normally, we wouldn't want to restrict instantiation of objects on channel
(because we want coverage), but in this case, I think it's appropriate since
this is a non-trivial performance drain.  Good find!

https://codereview.chromium.org/2705513002/diff/20001/extensions/browser/api/...
File extensions/browser/api/declarative/rules_registry_service.cc (right):

https://codereview.chromium.org/2705513002/diff/20001/extensions/browser/api/...
extensions/browser/api/declarative/rules_registry_service.cc:86:
RulesCacheDelegate* web_request_cache_delegate = NULL;
nit: since we're touching this code, update to nullptr

[karandeepb, 2017-02-18 01:54:51]

The CQ bit was checked by karandeepb@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-18 01:55:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[karandeepb, 2017-02-18 02:00:30]

PTAL Devlin.

https://codereview.chromium.org/2705513002/diff/20001/extensions/browser/api/...
File extensions/browser/api/declarative/rules_registry_service.cc (right):

https://codereview.chromium.org/2705513002/diff/20001/extensions/browser/api/...
extensions/browser/api/declarative/rules_registry_service.cc:86:
RulesCacheDelegate* web_request_cache_delegate = NULL;
On 2017/02/17 19:58:28, Devlin wrote:
> nit: since we're touching this code, update to nullptr

Done.

https://codereview.chromium.org/2705513002/diff/40001/extensions/common/featu...
File extensions/common/features/simple_feature_unittest.cc (left):

https://codereview.chromium.org/2705513002/diff/40001/extensions/common/featu...
extensions/common/features/simple_feature_unittest.cc:766:
TEST_F(SimpleFeatureTest, SimpleFeatureAvailability) {
Deleted this test (Diff isn't clear on that). It wasn't actually testing Simple
feature availability and was testing the same things as the next test.

[commit-bot: I haz the power, 2017-02-18 03:58:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-18 03:58:20]

Dry run: Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)

[Devlin, 2017-02-18 07:50:07]

lgtm

https://codereview.chromium.org/2705513002/diff/40001/extensions/common/featu...
File extensions/common/features/simple_feature_unittest.cc (right):

https://codereview.chromium.org/2705513002/diff/40001/extensions/common/featu...
extensions/common/features/simple_feature_unittest.cc:768: // Rule:
"legacy_packaged_app", channel stable.
nit: update this comment -> channel beta

[karandeepb, 2017-02-21 18:47:08]

https://codereview.chromium.org/2705513002/diff/40001/extensions/common/featu...
File extensions/common/features/simple_feature_unittest.cc (right):

https://codereview.chromium.org/2705513002/diff/40001/extensions/common/featu...
extensions/common/features/simple_feature_unittest.cc:768: // Rule:
"legacy_packaged_app", channel stable.
On 2017/02/18 07:50:07, Devlin wrote:
> nit: update this comment -> channel beta

Done.

[karandeepb, 2017-02-21 18:47:13]

The CQ bit was checked by karandeepb@chromium.org

[karandeepb, 2017-02-21 18:47:13]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/2705513002/#ps60001
(title: "Correct comment.")

[commit-bot: I haz the power, 2017-02-21 18:48:24]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-21 20:52:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-21 20:52:09]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)

[karandeepb, 2017-02-21 21:32:19]

The CQ bit was checked by karandeepb@chromium.org

[commit-bot: I haz the power, 2017-02-21 21:34:20]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-21 23:39:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-21 23:39:02]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)

[karandeepb, 2017-02-22 18:47:03]

The CQ bit was checked by karandeepb@chromium.org

[commit-bot: I haz the power, 2017-02-22 18:48:03]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-22 22:37:36]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1487789223821140,
"parent_rev": "51d9be7fe7e02868cce6ce9794f973e4a3279cd1", "commit_rev":
"a77b53baca5f2a4ff102592c17c8ea3f53dd5307"}

[commit-bot: I haz the power, 2017-02-22 22:38:12]

Description was changed from

==========
Extensions: Only create Web request rules registry if Declarative Web Request is
enabled.

Currently the Web Request rules registry required by the Declarative Web
Request API is created on all Chrome channels, even though the Declarative Web
Request API is disabled on Stable. The UMA metric
Extensions.NetworkDelayRegistryLoad shows significant delay for some web
requests which are blocked due to the loading of the Web Request rules
registry even on Stable. The mean delay is around 1.7 sec and the median delay
is around 50ms.

This CL modifies RulesRegistryService::EnsureDefaultRulesRegistriesRegistered
to ensure the Web Request rules registry is only created for the channels on
which the Declarative Web Request API is enabled. To do this,
Feature::IsAvailableToChannel is introduced.

BUG=693243
==========

to

==========
Extensions: Only create Web request rules registry if Declarative Web Request is
enabled.

Currently the Web Request rules registry required by the Declarative Web
Request API is created on all Chrome channels, even though the Declarative Web
Request API is disabled on Stable. The UMA metric
Extensions.NetworkDelayRegistryLoad shows significant delay for some web
requests which are blocked due to the loading of the Web Request rules
registry even on Stable. The mean delay is around 1.7 sec and the median delay
is around 50ms.

This CL modifies RulesRegistryService::EnsureDefaultRulesRegistriesRegistered
to ensure the Web Request rules registry is only created for the channels on
which the Declarative Web Request API is enabled. To do this,
Feature::IsAvailableToChannel is introduced.

BUG=693243

Review-Url: https://codereview.chromium.org/2705513002
Cr-Commit-Position: refs/heads/master@{#452240}
Committed:
https://chromium.googlesource.com/chromium/src/+/a77b53baca5f2a4ff102592c17c8...
==========

[commit-bot: I haz the power, 2017-02-22 22:38:13]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/a77b53baca5f2a4ff102592c17c8...

[Devlin, 2017-03-16 20:21:22]

On 2017/02/22 22:38:13, commit-bot: I haz the power wrote:
> Committed patchset #4 (id:60001) as
>
https://chromium.googlesource.com/chromium/src/+/a77b53baca5f2a4ff102592c17c8...

It looks like we may have been a bit too enthusiastic here.  Trying this on
pseudo-stable results in quite a few test failures:
https://codereview.chromium.org/2753073003/

[karandeepb, 2017-03-16 20:49:30]

On 2017/03/16 20:21:22, Devlin wrote:
> On 2017/02/22 22:38:13, commit-bot: I haz the power wrote:
> > Committed patchset #4 (id:60001) as
> >
>
https://chromium.googlesource.com/chromium/src/+/a77b53baca5f2a4ff102592c17c8...
> 
> It looks like we may have been a bit too enthusiastic here.  Trying this on
> pseudo-stable results in quite a few test failures:
> https://codereview.chromium.org/2753073003/

Thanks for catching this. Hadn't thought about this! Will investigate.

[Devlin, 2017-03-16 20:51:05]

On 2017/03/16 20:49:30, karandeepb wrote:
> On 2017/03/16 20:21:22, Devlin wrote:
> > On 2017/02/22 22:38:13, commit-bot: I haz the power wrote:
> > > Committed patchset #4 (id:60001) as
> > >
> >
>
https://chromium.googlesource.com/chromium/src/+/a77b53baca5f2a4ff102592c17c8...
> > 
> > It looks like we may have been a bit too enthusiastic here.  Trying this on
> > pseudo-stable results in quite a few test failures:
> > https://codereview.chromium.org/2753073003/
> 
> Thanks for catching this. Hadn't thought about this! Will investigate.

Thanks, and sorry for not thinking of it earlier.

[karandeepb, 2017-03-16 21:02:40]

On 2017/03/16 20:51:05, Devlin wrote:
> On 2017/03/16 20:49:30, karandeepb wrote:
> > On 2017/03/16 20:21:22, Devlin wrote:
> > > On 2017/02/22 22:38:13, commit-bot: I haz the power wrote:
> > > > Committed patchset #4 (id:60001) as
> > > >
> > >
> >
>
https://chromium.googlesource.com/chromium/src/+/a77b53baca5f2a4ff102592c17c8...
> > > 
> > > It looks like we may have been a bit too enthusiastic here.  Trying this
on
> > > pseudo-stable results in quite a few test failures:
> > > https://codereview.chromium.org/2753073003/
> > 
> > Thanks for catching this. Hadn't thought about this! Will investigate.
> 
> Thanks, and sorry for not thinking of it earlier.

It was my CL :) Although I think the actual # of test failures will be less,
since with the CL you pointed out, there is bound to be a channel mismatch. See
https://cs.chromium.org/chromium/src/chrome/browser/extensions/extension_brow....

[karandeepb, 2017-03-23 18:20:30]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/2773593003/ by karandeepb@chromium.org.

The reason for reverting is: Declarative Web Request is enabled on Stable
through <webview>. Hence we can't disable creation of rules registry on Stable..