Distinguish between subresource filtering and dryrun matching.

Distinguish between subresource filtering and dryrun matching.

In order to report page load metrics for an experiment and control population,
we need to track page load metrics for affected pages both in cases where
filtering is active, and where filtering is in dry run mode.

To do this, we extend WebDocumentSubresourceFilter to report not only whether
filtering applied, but also whether it would have been appllied in cases where
we are in dry run mode.

While we're changing the WebDocumentSubresourceFilter API, we address the
related issue of not displaying the subresource filtering UI for filtered
preloads. This is accomplished by providing separate APIs to get the filtering
policy for a resource (a pure policy URL) vs an API to report that a resource
loaded by the document was filtered.

BUG=689047, 689049
Review-Url: https://codereview.chromium.org/2677223002
Cr-Commit-Position: refs/heads/master@{#448719}
Committed: https://chromium.googlesource.com/chromium/src/+/f3455dace4afe149e6bb4d68acb4d4a6f5e2134a

[Bryan McQuade, 2017-02-06 15:12:09]

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

[commit-bot: I haz the power, 2017-02-06 15:12:17]

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-02-06 16:28:48]

Description was changed from

==========
Distinguish between subresource filtering and dryrun matching.

BUG=689047, 689049
==========

to

==========
Distinguish between subresource filtering and dryrun matching.

In order to report page load metrics for an experiment and control population,
we need to track page load metrics for affected pages both in cases where
filtering is active, and where filtering is in dry run mode.

To do this, we extend WebDocumentSubresourceFilter to report not only whether
filtering applied, but also whether it would have been appllied in cases where
we are in dry run mode.

While we're changing the WebDocumentSubresourceFilter API, we address the
related issue of not displaying the subresource filtering UI for filtered
preloads.

BUG=689047, 689049
==========

[Bryan McQuade, 2017-02-06 16:28:56]

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

[Bryan McQuade, 2017-02-06 16:29:02]

PTAL, thanks!

[commit-bot: I haz the power, 2017-02-06 16:41:54]

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

[commit-bot: I haz the power, 2017-02-06 16:41:55]

Dry run: This issue passed the CQ dry run.

[Bryan McQuade, 2017-02-06 16:48:02]

Description was changed from

==========
Distinguish between subresource filtering and dryrun matching.

In order to report page load metrics for an experiment and control population,
we need to track page load metrics for affected pages both in cases where
filtering is active, and where filtering is in dry run mode.

To do this, we extend WebDocumentSubresourceFilter to report not only whether
filtering applied, but also whether it would have been appllied in cases where
we are in dry run mode.

While we're changing the WebDocumentSubresourceFilter API, we address the
related issue of not displaying the subresource filtering UI for filtered
preloads.

BUG=689047, 689049
==========

to

==========
Distinguish between subresource filtering and dryrun matching.

In order to report page load metrics for an experiment and control population,
we need to track page load metrics for affected pages both in cases where
filtering is active, and where filtering is in dry run mode.

To do this, we extend WebDocumentSubresourceFilter to report not only whether
filtering applied, but also whether it would have been appllied in cases where
we are in dry run mode.

While we're changing the WebDocumentSubresourceFilter API, we address the
related issue of not displaying the subresource filtering UI for filtered
preloads. This is accomplished by providing separate APIs to get the filtering
policy for a resource (a pure policy URL) vs an API to report that a resource
loaded by the document was filtered.

BUG=689047, 689049
==========

[engedy, 2017-02-06 16:56:32]

LGTM % nits and a naming question. Thanks!

https://codereview.chromium.org/2677223002/diff/1/components/subresource_filt...
File
components/subresource_filter/content/renderer/subresource_filter_agent_unittest.cc
(right):

https://codereview.chromium.org/2677223002/diff/1/components/subresource_filt...
components/subresource_filter/content/renderer/subresource_filter_agent_unittest.cc:496:
// the agent.
nit: ... nor should it cause a crash.

https://codereview.chromium.org/2677223002/diff/1/third_party/WebKit/public/p...
File third_party/WebKit/public/platform/WebDocumentSubresourceFilter.h (right):

https://codereview.chromium.org/2677223002/diff/1/third_party/WebKit/public/p...
third_party/WebKit/public/platform/WebDocumentSubresourceFilter.h:16: enum
LoadPolicy { Allow, Filter, AllowWouldFilter };
nit: I find the term `filter` somewhat ambiguous: does it mean the act of
filtering, or the query matching the filter? I'm not a native speaker, though.
What do you think about `Disallow`? 

Then the third option could be, e.g., just `WouldDisallow`. Maybe we could add
comment?

https://codereview.chromium.org/2677223002/diff/1/third_party/WebKit/public/p...
File third_party/WebKit/public/platform/WebLoadingBehaviorFlag.h (right):

https://codereview.chromium.org/2677223002/diff/1/third_party/WebKit/public/p...
third_party/WebKit/public/platform/WebLoadingBehaviorFlag.h:35: // policy.
nit: s/policy/rules/ for consistency.

[Bryan McQuade, 2017-02-06 17:11:38]

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

[Bryan McQuade, 2017-02-06 17:11:56]

https://codereview.chromium.org/2677223002/diff/1/components/subresource_filt...
File
components/subresource_filter/content/renderer/subresource_filter_agent_unittest.cc
(right):

https://codereview.chromium.org/2677223002/diff/1/components/subresource_filt...
components/subresource_filter/content/renderer/subresource_filter_agent_unittest.cc:496:
// the agent.
On 2017/02/06 at 16:56:32, engedy wrote:
> nit: ... nor should it cause a crash.

Done

https://codereview.chromium.org/2677223002/diff/1/third_party/WebKit/public/p...
File third_party/WebKit/public/platform/WebDocumentSubresourceFilter.h (right):

https://codereview.chromium.org/2677223002/diff/1/third_party/WebKit/public/p...
third_party/WebKit/public/platform/WebDocumentSubresourceFilter.h:16: enum
LoadPolicy { Allow, Filter, AllowWouldFilter };
On 2017/02/06 at 16:56:32, engedy wrote:
> nit: I find the term `filter` somewhat ambiguous: does it mean the act of
filtering, or the query matching the filter? I'm not a native speaker, though.
What do you think about `Disallow`? 
> 
> Then the third option could be, e.g., just `WouldDisallow`. Maybe we could add
comment?

Good suggestion, thanks! Updated to Allow/Disallow/WouldDisallow.

https://codereview.chromium.org/2677223002/diff/1/third_party/WebKit/public/p...
File third_party/WebKit/public/platform/WebLoadingBehaviorFlag.h (right):

https://codereview.chromium.org/2677223002/diff/1/third_party/WebKit/public/p...
third_party/WebKit/public/platform/WebLoadingBehaviorFlag.h:35: // policy.
On 2017/02/06 at 16:56:32, engedy wrote:
> nit: s/policy/rules/ for consistency.

Done.

[Bryan McQuade, 2017-02-06 17:11:56]

https://codereview.chromium.org/2677223002/diff/1/components/subresource_filt...
File
components/subresource_filter/content/renderer/subresource_filter_agent_unittest.cc
(right):

https://codereview.chromium.org/2677223002/diff/1/components/subresource_filt...
components/subresource_filter/content/renderer/subresource_filter_agent_unittest.cc:496:
// the agent.
On 2017/02/06 at 16:56:32, engedy wrote:
> nit: ... nor should it cause a crash.

Done

https://codereview.chromium.org/2677223002/diff/1/third_party/WebKit/public/p...
File third_party/WebKit/public/platform/WebDocumentSubresourceFilter.h (right):

https://codereview.chromium.org/2677223002/diff/1/third_party/WebKit/public/p...
third_party/WebKit/public/platform/WebDocumentSubresourceFilter.h:16: enum
LoadPolicy { Allow, Filter, AllowWouldFilter };
On 2017/02/06 at 16:56:32, engedy wrote:
> nit: I find the term `filter` somewhat ambiguous: does it mean the act of
filtering, or the query matching the filter? I'm not a native speaker, though.
What do you think about `Disallow`? 
> 
> Then the third option could be, e.g., just `WouldDisallow`. Maybe we could add
comment?

Good suggestion, thanks! Updated to Allow/Disallow/WouldDisallow.

https://codereview.chromium.org/2677223002/diff/1/third_party/WebKit/public/p...
File third_party/WebKit/public/platform/WebLoadingBehaviorFlag.h (right):

https://codereview.chromium.org/2677223002/diff/1/third_party/WebKit/public/p...
third_party/WebKit/public/platform/WebLoadingBehaviorFlag.h:35: // policy.
On 2017/02/06 at 16:56:32, engedy wrote:
> nit: s/policy/rules/ for consistency.

Done.

[commit-bot: I haz the power, 2017-02-06 17:12:02]

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

[engedy, 2017-02-06 17:20:12]

Still LGTM % one final nit.

Also, in a follow-up CL, do you think you couod update histogram
SubresourceFilter.DocumentLoad.NumSubresourceLoads.Disallowed to record the
number of times that method was called instead?

https://codereview.chromium.org/2677223002/diff/20001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/WebDocumentSubresourceFilter.h (right):

https://codereview.chromium.org/2677223002/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebDocumentSubresourceFilter.h:20: virtual
void reportFilteredLoad() = 0;
nit: `reportDisallowedLoad` for consistency.

Also, we should add a comment that this should not be called for preloads, and
maybe indicate that in the name too? Not sure what the opposite of `preload` is?
 `Real` or `actual` load?

[engedy, 2017-02-06 17:20:27]

could*

[Bryan McQuade, 2017-02-06 17:31:58]

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

[commit-bot: I haz the power, 2017-02-06 17:33:27]

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-02-06 17:53:35]

https://codereview.chromium.org/2677223002/diff/20001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/WebDocumentSubresourceFilter.h (right):

https://codereview.chromium.org/2677223002/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebDocumentSubresourceFilter.h:20: virtual
void reportFilteredLoad() = 0;
On 2017/02/06 at 17:20:12, engedy wrote:
> nit: `reportDisallowedLoad` for consistency.
> 
> Also, we should add a comment that this should not be called for preloads, and
maybe indicate that in the name too? Not sure what the opposite of `preload` is?
 `Real` or `actual` load?

Done, thanks!

[Bryan McQuade, 2017-02-06 17:56:01]

bmcquade@chromium.org changed reviewers:
+ japhet@chromium.org, rbyers@chromium.org

[Bryan McQuade, 2017-02-06 17:57:20]

japhet, PTAL for third_party/WebKit changes
rbyers, PTAL for test_runner changes (just API updates to mock classes)

engedy has already reviewed the subresource filtering aspects of this change.
thanks!

[Nate Chapin, 2017-02-06 18:04:02]

lgtm

[commit-bot: I haz the power, 2017-02-06 19:06:36]

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

[commit-bot: I haz the power, 2017-02-06 19:06:38]

Dry run: This issue passed the CQ dry run.

[Rick Byers, 2017-02-06 20:33:06]

On 2017/02/06 17:57:20, Bryan McQuade wrote:
> japhet, PTAL for third_party/WebKit changes
> rbyers, PTAL for test_runner changes (just API updates to mock classes)
test_runner LGTM

> 
> engedy has already reviewed the subresource filtering aspects of this change.
> thanks!

[engedy, 2017-02-07 12:05:33]

Description was changed from

==========
Distinguish between subresource filtering and dryrun matching.

In order to report page load metrics for an experiment and control population,
we need to track page load metrics for affected pages both in cases where
filtering is active, and where filtering is in dry run mode.

To do this, we extend WebDocumentSubresourceFilter to report not only whether
filtering applied, but also whether it would have been appllied in cases where
we are in dry run mode.

While we're changing the WebDocumentSubresourceFilter API, we address the
related issue of not displaying the subresource filtering UI for filtered
preloads. This is accomplished by providing separate APIs to get the filtering
policy for a resource (a pure policy URL) vs an API to report that a resource
loaded by the document was filtered.

BUG=689047, 689049
==========

to

==========
Distinguish between subresource filtering and dryrun matching.

In order to report page load metrics for an experiment and control population,
we need to track page load metrics for affected pages both in cases where
filtering is active, and where filtering is in dry run mode.

To do this, we extend WebDocumentSubresourceFilter to report not only whether
filtering applied, but also whether it would have been appllied in cases where
we are in dry run mode.

While we're changing the WebDocumentSubresourceFilter API, we address the
related issue of not displaying the subresource filtering UI for filtered
preloads. This is accomplished by providing separate APIs to get the filtering
policy for a resource (a pure policy URL) vs an API to report that a resource
loaded by the document was filtered.

BUG=689047, 689049
==========

[Bryan McQuade, 2017-02-07 12:34:28]

The CQ bit was checked by bmcquade@chromium.org

[Bryan McQuade, 2017-02-07 12:34:28]

The patchset sent to the CQ was uploaded after l-g-t-m from engedy@chromium.org
Link to the patchset: https://codereview.chromium.org/2677223002/#ps40001
(title: "address comments")

[commit-bot: I haz the power, 2017-02-07 12:34:42]

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-07 12:43:52]

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

[commit-bot: I haz the power, 2017-02-07 12:43:54]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Bryan McQuade, 2017-02-07 12:54:12]

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

[commit-bot: I haz the power, 2017-02-07 12:54:32]

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-07 13:10:14]

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

[commit-bot: I haz the power, 2017-02-07 13:10:15]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Bryan McQuade, 2017-02-07 13:28:39]

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

[commit-bot: I haz the power, 2017-02-07 13:28:53]

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-07 13:41:39]

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

[commit-bot: I haz the power, 2017-02-07 13:41:40]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Bryan McQuade, 2017-02-07 15:45:42]

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

[commit-bot: I haz the power, 2017-02-07 15:45:57]

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-07 17:18:03]

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

[commit-bot: I haz the power, 2017-02-07 17:18:04]

Dry run: This issue passed the CQ dry run.

[Bryan McQuade, 2017-02-07 17:21:30]

The CQ bit was checked by bmcquade@chromium.org

[Bryan McQuade, 2017-02-07 17:21:30]

The patchset sent to the CQ was uploaded after l-g-t-m from japhet@chromium.org,
engedy@chromium.org, rbyers@chromium.org
Link to the patchset: https://codereview.chromium.org/2677223002/#ps100001
(title: "rebase")

[commit-bot: I haz the power, 2017-02-07 17:21:48]

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-07 17:28:05]

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

[commit-bot: I haz the power, 2017-02-07 17:28:06]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Bryan McQuade, 2017-02-07 18:13:52]

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

[commit-bot: I haz the power, 2017-02-07 18:14:07]

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-07 20:11:54]

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

[commit-bot: I haz the power, 2017-02-07 20:11:55]

Dry run: This issue passed the CQ dry run.

[Bryan McQuade, 2017-02-07 20:19:23]

The CQ bit was checked by bmcquade@chromium.org

[Bryan McQuade, 2017-02-07 20:19:24]

The patchset sent to the CQ was uploaded after l-g-t-m from japhet@chromium.org,
engedy@chromium.org, rbyers@chromium.org
Link to the patchset: https://codereview.chromium.org/2677223002/#ps120001
(title: "rebase")

[commit-bot: I haz the power, 2017-02-07 20:20:25]

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-07 20:25:40]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1486498763579230,
"parent_rev": "3b772e23dc0971200d2dcf38f6305091e4d01813", "commit_rev":
"f3455dace4afe149e6bb4d68acb4d4a6f5e2134a"}

[commit-bot: I haz the power, 2017-02-07 20:26:38]

Description was changed from

==========
Distinguish between subresource filtering and dryrun matching.

In order to report page load metrics for an experiment and control population,
we need to track page load metrics for affected pages both in cases where
filtering is active, and where filtering is in dry run mode.

To do this, we extend WebDocumentSubresourceFilter to report not only whether
filtering applied, but also whether it would have been appllied in cases where
we are in dry run mode.

While we're changing the WebDocumentSubresourceFilter API, we address the
related issue of not displaying the subresource filtering UI for filtered
preloads. This is accomplished by providing separate APIs to get the filtering
policy for a resource (a pure policy URL) vs an API to report that a resource
loaded by the document was filtered.

BUG=689047, 689049
==========

to

==========
Distinguish between subresource filtering and dryrun matching.

In order to report page load metrics for an experiment and control population,
we need to track page load metrics for affected pages both in cases where
filtering is active, and where filtering is in dry run mode.

To do this, we extend WebDocumentSubresourceFilter to report not only whether
filtering applied, but also whether it would have been appllied in cases where
we are in dry run mode.

While we're changing the WebDocumentSubresourceFilter API, we address the
related issue of not displaying the subresource filtering UI for filtered
preloads. This is accomplished by providing separate APIs to get the filtering
policy for a resource (a pure policy URL) vs an API to report that a resource
loaded by the document was filtered.

BUG=689047, 689049

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

[commit-bot: I haz the power, 2017-02-07 20:26:42]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/f3455dace4afe149e6bb4d68acb4...