Support optional performance measuring in SubresourceFilter.

Support optional performance measuring in SubresourceFilter.

The purpose of this CL is to provide a mechanism for enabling or
disabling SubresourceFilter's performance measuring on a per-page basis,
i.e. consistently for all DocumentSubresourceFilter instances of a page.

A decision on whether to measure performance (currently always true)
originates in ContentSubresourceFilterDriverFactory. It is distributed
to SubresourceFilterAgents first in the ActivateForProvisionalLoad
message, and eventually reaches individual DocumentSubresourceFilter
instances corresponding to each subframe document.

BUG=672519
Committed: https://crrev.com/88e76494111e68722f089ad41da81fe258ae09ab
Cr-Commit-Position: refs/heads/master@{#437837}

[pkalinnikov, 2016-12-06 16:11:35]

Balazs, please take a look.

1. Please verify the approach in general.
2. Probably I should add some tests verifying that the bit is correctly
interpreted by the renderers. E.g., if the bit is false, there should be no
performance histograms created. What do you think? Any suggestions on tests?

[pkalinnikov, 2016-12-06 16:11:49]

Description was changed from

==========
Support optional performance measuring in SubresourceFilter.

The purpose of this CL is to provide a mechanism for enabling or
disabling SubresourceFilter's performance measuring on per-page basis,
i.e., for all DocumentSubresourceFilter instances of a page.

A decision on whether to measure performance (currently always true) is
originated in ContentSubresourceFilterDriverFactory. It is distributed
piggybacked with the ActivateForProvisionalLoad message down to
SubresourceFilterAgent's, and eventually reaches individual
DocumentSubresourceFilter instances corresponding to each subdocument.

BUG=609747
==========

to

==========
Support optional performance measuring in SubresourceFilter.

The purpose of this CL is to provide a mechanism for enabling or
disabling SubresourceFilter's performance measuring on per-page basis,
i.e., for all DocumentSubresourceFilter instances of a page.

A decision on whether to measure performance (currently always true) is
originated in ContentSubresourceFilterDriverFactory. It is distributed
piggybacked with the ActivateForProvisionalLoad message down to
SubresourceFilterAgent's, and eventually reaches individual
DocumentSubresourceFilter instances corresponding to each subdocument.

BUG=609747
==========

[pkalinnikov, 2016-12-06 16:11:49]

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

[engedy, 2016-12-06 19:19:44]

LGTM % comments.

I don't know if there is any way to add an end-to-end test right now. I would
add such a test once we have the page level aggregation, which makes things
significantly easier.

https://codereview.chromium.org/2556433003/diff/1/components/subresource_filt...
File
components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc
(right):

https://codereview.chromium.org/2556433003/diff/1/components/subresource_filt...
components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:265:
ActivateForProvisionalLoad(::testing::_, ::testing::_, ::testing::_))
Maybe add an expectation that the last param is always true.

https://codereview.chromium.org/2556433003/diff/1/components/subresource_filt...
File
components/subresource_filter/content/renderer/document_subresource_filter.h
(right):

https://codereview.chromium.org/2556433003/diff/1/components/subresource_filt...
components/subresource_filter/content/renderer/document_subresource_filter.h:75:
bool measure_performance_;
nit: const bool. Maybe the activation_state can be const too?

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

https://codereview.chromium.org/2556433003/diff/1/components/subresource_filt...
components/subresource_filter/content/renderer/document_subresource_filter_unittest.cc:97:
TEST_F(DocumentSubresourceFilterTest, Enabled) {
Please add the equivalent of this unittest where `false` is passed as
|measure_performance|.

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

https://codereview.chromium.org/2556433003/diff/1/components/subresource_filt...
components/subresource_filter/content/renderer/subresource_filter_agent.cc:135:
activation_state_for_provisional_load_ = ActivationState::DISABLED;
Will need to reset |measure_performance_| to false here.

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

https://codereview.chromium.org/2556433003/diff/1/components/subresource_filt...
components/subresource_filter/content/renderer/subresource_filter_agent_unittest.cc:423:
histogram_tester.ExpectTotalCount(kEvaluationTotalWallDuration, 1);
Please add a test with the expectation from lines 423--426 and the necessary
moves to exercise them, but with performance measuring disabled.

[pkalinnikov, 2016-12-07 16:26:31]

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

[commit-bot: I haz the power, 2016-12-07 16:26:57]

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

[pkalinnikov, 2016-12-07 16:30:49]

Balazs, PTAL again.

https://codereview.chromium.org/2556433003/diff/1/components/subresource_filt...
File
components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc
(right):

https://codereview.chromium.org/2556433003/diff/1/components/subresource_filt...
components/subresource_filter/content/browser/content_subresource_filter_driver_factory_unittest.cc:265:
ActivateForProvisionalLoad(::testing::_, ::testing::_, ::testing::_))
On 2016/12/06 19:19:44, engedy wrote:
> Maybe add an expectation that the last param is always true.

Done.

https://codereview.chromium.org/2556433003/diff/1/components/subresource_filt...
File
components/subresource_filter/content/renderer/document_subresource_filter.h
(right):

https://codereview.chromium.org/2556433003/diff/1/components/subresource_filt...
components/subresource_filter/content/renderer/document_subresource_filter.h:75:
bool measure_performance_;
On 2016/12/06 19:19:44, engedy wrote:
> nit: const bool. Maybe the activation_state can be const too?

Done.

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

https://codereview.chromium.org/2556433003/diff/1/components/subresource_filt...
components/subresource_filter/content/renderer/document_subresource_filter_unittest.cc:97:
TEST_F(DocumentSubresourceFilterTest, Enabled) {
On 2016/12/06 19:19:44, engedy wrote:
> Please add the equivalent of this unittest where `false` is passed as
> |measure_performance|.

Done.

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

https://codereview.chromium.org/2556433003/diff/1/components/subresource_filt...
components/subresource_filter/content/renderer/subresource_filter_agent.cc:135:
activation_state_for_provisional_load_ = ActivationState::DISABLED;
On 2016/12/06 19:19:44, engedy wrote:
> Will need to reset |measure_performance_| to false here.

Done.

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

https://codereview.chromium.org/2556433003/diff/1/components/subresource_filt...
components/subresource_filter/content/renderer/subresource_filter_agent_unittest.cc:423:
histogram_tester.ExpectTotalCount(kEvaluationTotalWallDuration, 1);
On 2016/12/06 19:19:44, engedy wrote:
> Please add a test with the expectation from lines 423--426 and the necessary
> moves to exercise them, but with performance measuring disabled.

Done.

[commit-bot: I haz the power, 2016-12-07 16:39:17]

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

[commit-bot: I haz the power, 2016-12-07 16:39:18]

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

[engedy, 2016-12-07 16:42:57]

Description was changed from

==========
Support optional performance measuring in SubresourceFilter.

The purpose of this CL is to provide a mechanism for enabling or
disabling SubresourceFilter's performance measuring on per-page basis,
i.e., for all DocumentSubresourceFilter instances of a page.

A decision on whether to measure performance (currently always true) is
originated in ContentSubresourceFilterDriverFactory. It is distributed
piggybacked with the ActivateForProvisionalLoad message down to
SubresourceFilterAgent's, and eventually reaches individual
DocumentSubresourceFilter instances corresponding to each subdocument.

BUG=609747
==========

to

==========
Support optional performance measuring in SubresourceFilter.

The purpose of this CL is to provide a mechanism for enabling or
disabling SubresourceFilter's performance measuring on a per-page basis,
i.e. consistently for all DocumentSubresourceFilter instances of a page.

A decision on whether to measure performance (currently always true)
originates in ContentSubresourceFilterDriverFactory. It is distributed
to SubresourceFilterAgents first in the ActivateForProvisionalLoad
message, and eventually reaches individual DocumentSubresourceFilter
instances corresponding to each subframe document.

BUG=609747
==========

[engedy, 2016-12-07 16:46:51]

LGTM.

Note that you'll need sign-off on IPC changes from a reviewer in
ipc/SECURITY_OWNERS.

[pkalinnikov, 2016-12-07 16:55:55]

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

[commit-bot: I haz the power, 2016-12-07 16:56:11]

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

[pkalinnikov, 2016-12-07 17:07:37]

pkalinnikov@chromium.org changed reviewers:
+ palmer@chromium.org

[pkalinnikov, 2016-12-07 17:07:38]

Hi Chris,

Adding you because you are in "ipc/SECURITY_OWNERS". Can you please review
message change in the "subresource_filter_messages.h" file?

[commit-bot: I haz the power, 2016-12-07 18:05:03]

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

[commit-bot: I haz the power, 2016-12-07 18:05:04]

Dry run: This issue passed the CQ dry run.

[palmer, 2016-12-07 20:36:11]

IPC LGTM.

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

https://codereview.chromium.org/2556433003/diff/40001/components/subresource_...
components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:126:
// the |measure_performance| bit is set.
Nit: It's good to associate a bug with a TODO.

[pkalinnikov, 2016-12-08 16:36:09]

Description was changed from

==========
Support optional performance measuring in SubresourceFilter.

The purpose of this CL is to provide a mechanism for enabling or
disabling SubresourceFilter's performance measuring on a per-page basis,
i.e. consistently for all DocumentSubresourceFilter instances of a page.

A decision on whether to measure performance (currently always true)
originates in ContentSubresourceFilterDriverFactory. It is distributed
to SubresourceFilterAgents first in the ActivateForProvisionalLoad
message, and eventually reaches individual DocumentSubresourceFilter
instances corresponding to each subframe document.

BUG=609747
==========

to

==========
Support optional performance measuring in SubresourceFilter.

The purpose of this CL is to provide a mechanism for enabling or
disabling SubresourceFilter's performance measuring on a per-page basis,
i.e. consistently for all DocumentSubresourceFilter instances of a page.

A decision on whether to measure performance (currently always true)
originates in ContentSubresourceFilterDriverFactory. It is distributed
to SubresourceFilterAgents first in the ActivateForProvisionalLoad
message, and eventually reaches individual DocumentSubresourceFilter
instances corresponding to each subframe document.

BUG=672519
==========

[pkalinnikov, 2016-12-08 16:40:44]

Addressed all comments, submitting.

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

https://codereview.chromium.org/2556433003/diff/40001/components/subresource_...
components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:126:
// the |measure_performance| bit is set.
On 2016/12/07 20:36:11, palmer wrote:
> Nit: It's good to associate a bug with a TODO.

Done.

[pkalinnikov, 2016-12-08 16:40:51]

The CQ bit was checked by pkalinnikov@chromium.org

[pkalinnikov, 2016-12-08 16:40:51]

The patchset sent to the CQ was uploaded after l-g-t-m from engedy@chromium.org,
palmer@chromium.org
Link to the patchset: https://codereview.chromium.org/2556433003/#ps60001
(title: "Link TODO to a bug.")

[commit-bot: I haz the power, 2016-12-08 16:41:09]

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, 2016-12-08 16:50:26]

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

[commit-bot: I haz the power, 2016-12-08 16:50:27]

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...)

[pkalinnikov, 2016-12-08 17:33:41]

pkalinnikov@chromium.org changed reviewers:
+ mattm@chromium.org

[pkalinnikov, 2016-12-08 17:33:42]

Hey Matt,

Please review the "safe_browsing_service_browsertest.cc" file. Thanks.

[mattm, 2016-12-08 22:31:10]

safe_browsing lgtm

[pkalinnikov, 2016-12-12 09:00:56]

The CQ bit was checked by pkalinnikov@chromium.org

[commit-bot: I haz the power, 2016-12-12 09:01:16]

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, 2016-12-12 09:41:49]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1481533256479600,
"parent_rev": "e1cd1075dc887838798512bc95d9fe1925a5a5d5", "commit_rev":
"331e862a1d51d88829e8c8e4e87b871fb9b9f3ad"}

[commit-bot: I haz the power, 2016-12-12 09:42:18]

Description was changed from

==========
Support optional performance measuring in SubresourceFilter.

The purpose of this CL is to provide a mechanism for enabling or
disabling SubresourceFilter's performance measuring on a per-page basis,
i.e. consistently for all DocumentSubresourceFilter instances of a page.

A decision on whether to measure performance (currently always true)
originates in ContentSubresourceFilterDriverFactory. It is distributed
to SubresourceFilterAgents first in the ActivateForProvisionalLoad
message, and eventually reaches individual DocumentSubresourceFilter
instances corresponding to each subframe document.

BUG=672519
==========

to

==========
Support optional performance measuring in SubresourceFilter.

The purpose of this CL is to provide a mechanism for enabling or
disabling SubresourceFilter's performance measuring on a per-page basis,
i.e. consistently for all DocumentSubresourceFilter instances of a page.

A decision on whether to measure performance (currently always true)
originates in ContentSubresourceFilterDriverFactory. It is distributed
to SubresourceFilterAgents first in the ActivateForProvisionalLoad
message, and eventually reaches individual DocumentSubresourceFilter
instances corresponding to each subframe document.

BUG=672519

Review-Url: https://codereview.chromium.org/2556433003
==========

[commit-bot: I haz the power, 2016-12-12 09:42:19]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-12-12 15:11:11]

Description was changed from

==========
Support optional performance measuring in SubresourceFilter.

The purpose of this CL is to provide a mechanism for enabling or
disabling SubresourceFilter's performance measuring on a per-page basis,
i.e. consistently for all DocumentSubresourceFilter instances of a page.

A decision on whether to measure performance (currently always true)
originates in ContentSubresourceFilterDriverFactory. It is distributed
to SubresourceFilterAgents first in the ActivateForProvisionalLoad
message, and eventually reaches individual DocumentSubresourceFilter
instances corresponding to each subframe document.

BUG=672519

Review-Url: https://codereview.chromium.org/2556433003
==========

to

==========
Support optional performance measuring in SubresourceFilter.

The purpose of this CL is to provide a mechanism for enabling or
disabling SubresourceFilter's performance measuring on a per-page basis,
i.e. consistently for all DocumentSubresourceFilter instances of a page.

A decision on whether to measure performance (currently always true)
originates in ContentSubresourceFilterDriverFactory. It is distributed
to SubresourceFilterAgents first in the ActivateForProvisionalLoad
message, and eventually reaches individual DocumentSubresourceFilter
instances corresponding to each subframe document.

BUG=672519

Committed: https://crrev.com/88e76494111e68722f089ad41da81fe258ae09ab
Cr-Commit-Position: refs/heads/master@{#437837}
==========

[commit-bot: I haz the power, 2016-12-12 15:11:12]

Patchset 4 (id:??) landed as
https://crrev.com/88e76494111e68722f089ad41da81fe258ae09ab
Cr-Commit-Position: refs/heads/master@{#437837}