[subresource_filter] Decide UNSUPPORTED_SCHEME only if we'd otherwise activate

[subresource_filter] Decide UNSUPPORTED_SCHEME only if we'd otherwise activate

Right now the subresource filter reports UNSUPPORTED_SCHEME even if the
feature is disabled and the navigation would never have activated. This
patch aligns things closer to how they were before [1] landed, so that
UNSUPPORTED_SCHEME is not logged if the matched configuration's
activation level is DISABLED or there is no matched configuration.

BUG=todo

Review-Url: https://codereview.chromium.org/2888163002
Cr-Commit-Position: refs/heads/master@{#473093}
Committed: https://chromium.googlesource.com/chromium/src/+/a30d68388644568f7930aa57cdd2fa015816aeb9

[Charlie Harrison, 2017-05-18 18:28:12]

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

[commit-bot: I haz the power, 2017-05-18 18:29:35]

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-05-18 18:34:47]

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

[commit-bot: I haz the power, 2017-05-18 18:34:48]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Charlie Harrison, 2017-05-18 19:25:02]

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

[Charlie Harrison, 2017-05-18 19:25:53]

csharrison@chromium.org changed reviewers:
+ bmcquade@chromium.org

[Charlie Harrison, 2017-05-18 19:25:54]

Bryan, PTAL?

[commit-bot: I haz the power, 2017-05-18 19:26:11]

Dry run: CQ is trying da patch.

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

[Bryan McQuade, 2017-05-18 19:32:01]

Thanks! LGTM. Just a couple comments.

https://codereview.chromium.org/2888163002/diff/20001/components/subresource_...
File
components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc
(right):

https://codereview.chromium.org/2888163002/diff/20001/components/subresource_...
components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:182:
activation_options_ = activation_options;
just to double check, if activation level is disabled, we don't want to set
activation_options_ to Configuration::ActivationOptions()?

https://codereview.chromium.org/2888163002/diff/20001/components/subresource_...
components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:195:
DCHECK(!scheme_is_http_or_https || url.SchemeIsHTTPOrHTTPS());
this might benefit from a comment - we're expecting that either
scheme_is_http_or_https is false, or the url's scheme is http/https. for someone
unfamiliar with the code it's not totally clear why we expect this.

[Charlie Harrison, 2017-05-18 19:39:12]

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

[Charlie Harrison, 2017-05-18 19:39:20]

https://codereview.chromium.org/2888163002/diff/20001/components/subresource_...
File
components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc
(right):

https://codereview.chromium.org/2888163002/diff/20001/components/subresource_...
components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:182:
activation_options_ = activation_options;
On 2017/05/18 19:32:01, Bryan McQuade wrote:
> just to double check, if activation level is disabled, we don't want to set
> activation_options_ to Configuration::ActivationOptions()?

It's pretty much the same end result, % a few booleans like
performance_measurement_rate, etc.

https://codereview.chromium.org/2888163002/diff/20001/components/subresource_...
components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:195:
DCHECK(!scheme_is_http_or_https || url.SchemeIsHTTPOrHTTPS());
On 2017/05/18 19:32:01, Bryan McQuade wrote:
> this might benefit from a comment - we're expecting that either
> scheme_is_http_or_https is false, or the url's scheme is http/https. for
someone
> unfamiliar with the code it's not totally clear why we expect this.

It is an implication, So scheme_is_http_or_https implies
url.SchemeIsHTTPOrHTTPS(). Added a comment.

[commit-bot: I haz the power, 2017-05-18 19:40:35]

Dry run: CQ is trying da patch.

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

[Charlie Harrison, 2017-05-18 21:43:02]

The CQ bit was unchecked by csharrison@chromium.org

[Charlie Harrison, 2017-05-18 21:43:02]

The CQ bit was checked by csharrison@chromium.org

[Charlie Harrison, 2017-05-18 21:43:03]

The patchset sent to the CQ was uploaded after l-g-t-m from
bmcquade@chromium.org
Link to the patchset: https://codereview.chromium.org/2888163002/#ps40001
(title: "Add comment")

[commit-bot: I haz the power, 2017-05-18 21:44:03]

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

[commit-bot: I haz the power, 2017-05-18 21:44:04]

This CL has an open dependency (Issue 2889913002 Patch 40001). Please resolve
the dependency and try again.
If you are sure that there is no real dependency, please use one of the options
listed in https://goo.gl/9Es4OR to land the CL.

[Charlie Harrison, 2017-05-19 04:22:22]

The CQ bit was checked by csharrison@chromium.org

[commit-bot: I haz the power, 2017-05-19 04:23:06]

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-05-19 05:14:00]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1495167742827990,
"parent_rev": "f4166640f3a4119cdb4b33e2f16ebdd28ef6ff7f", "commit_rev":
"a30d68388644568f7930aa57cdd2fa015816aeb9"}

[commit-bot: I haz the power, 2017-05-19 05:14:19]

Description was changed from

==========
[subresource_filter] Decide UNSUPPORTED_SCHEME only if we'd otherwise activate

Right now the subresource filter reports UNSUPPORTED_SCHEME even if the
feature is disabled and the navigation would never have activated. This
patch aligns things closer to how they were before [1] landed, so that
UNSUPPORTED_SCHEME is not logged if the matched configuration's
activation level is DISABLED or there is no matched configuration.

BUG=todo
==========

to

==========
[subresource_filter] Decide UNSUPPORTED_SCHEME only if we'd otherwise activate

Right now the subresource filter reports UNSUPPORTED_SCHEME even if the
feature is disabled and the navigation would never have activated. This
patch aligns things closer to how they were before [1] landed, so that
UNSUPPORTED_SCHEME is not logged if the matched configuration's
activation level is DISABLED or there is no matched configuration.

BUG=todo

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

[commit-bot: I haz the power, 2017-05-19 05:14:20]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/a30d68388644568f7930aa57cdd2...

[Marc Treib, 2017-05-19 10:56:43]

A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.chromium.org/2895593002/ by treib@chromium.org.

The reason for reverting is: Depends on
https://codereview.chromium.org/2889913002 which is being reverted..