Allow Safe Browsing Saresource Filter to distinguish between different lists.

Allow Safe Browsing Saresource Filter to distinguish between different lists.

Safe browsing subresource filter should be able to make activation
descision based on the type of the list and metadata for the list.

BUG=609747, 641037
Committed: https://crrev.com/3851251cc17db046f32af57e003a1e86fd375db3
Cr-Commit-Position: refs/heads/master@{#414674}

[melandory, 2016-08-25 05:55:16]

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

[commit-bot: I haz the power, 2016-08-25 05:55:32]

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

[melandory, 2016-08-25 05:58:17]

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

[commit-bot: I haz the power, 2016-08-25 05:58:39]

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

[melandory, 2016-08-25 05:59:20]

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

[melandory, 2016-08-25 05:59:21]

PTAL. There is literally 0 tests for now, but it is what I have.

[commit-bot: I haz the power, 2016-08-25 07:10:54]

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

[commit-bot: I haz the power, 2016-08-25 07:10:55]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[melandory, 2016-08-25 17:33:51]

On 2016/08/25 05:59:21, melandory wrote:
> PTAL. There is literally 0 tests for now, but it is what I have.

Any comments?

[melandory, 2016-08-25 17:39:08]

Description was changed from

==========
Allow Safe Browsing Saresource Filter to distinguish between different lists.

Safe browsing subresource filter should be able to make activation
descision based on the type of the list and metadata for the list.

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

to

==========
Allow Safe Browsing Saresource Filter to distinguish between different lists.

Safe browsing subresource filter should be able to make activation
descision based on the type of the list and metadata for the list.

BUG=609747, 641037
==========

[engedy, 2016-08-25 21:32:55]

LGTM % comments, thanks for working on this!

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

https://codereview.chromium.org/2272323002/diff/20001/components/subresource_...
components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:92:
bool proceed =
We should have at least one test that verifies this matrix of ActivationList x
(threat_type, meta).

https://codereview.chromium.org/2272323002/diff/20001/components/subresource_...
File components/subresource_filter/core/browser/subresource_filter_features.cc
(right):

https://codereview.chromium.org/2272323002/diff/20001/components/subresource_...
components/subresource_filter/core/browser/subresource_filter_features.cc:58:
base::SplitString(activation_lists, ",", base::TRIM_WHITESPACE,
Let's use SplitStringPiece() here to avoid copies.  Did you check that
comma-separated values can be input on the command line?

https://codereview.chromium.org/2272323002/diff/20001/components/subresource_...
components/subresource_filter/core/browser/subresource_filter_features.cc:59:
base::SPLIT_WANT_ALL)) {
nit: SPLIT_WANT_NONEMPTY (not that it matters)

https://codereview.chromium.org/2272323002/diff/20001/components/subresource_...
File components/subresource_filter/core/browser/subresource_filter_features.h
(right):

https://codereview.chromium.org/2272323002/diff/20001/components/subresource_...
components/subresource_filter/core/browser/subresource_filter_features.h:30:
extern const char kActivationListSocialEngAdsIntestitial[];
typo: "Interstitial" (missing 'r').  In more places in the enumerator and
strings too. 

Also, for consistency with SB terminology elsewhere, can we just
s/Eng/Engineering/? I know it's long, but it's already long. :)

https://codereview.chromium.org/2272323002/diff/20001/components/subresource_...
components/subresource_filter/core/browser/subresource_filter_features.h:48:
ActivationList GetCurrentActivationList();
I'd suggest being even more conservative here. An empty list should mean none,
and more importantly, a "list containing only future values unknown to this
client" should mean none too.

In the future we will probably want this to return a bitmask. For now, fine to
just have ActivationList::NONE or something along these lines. And return the
widest list for which we find a token in the comma-separated list.

[melandory, 2016-08-25 22:51:42]

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

[commit-bot: I haz the power, 2016-08-25 22:52:31]

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

[melandory, 2016-08-25 23:38:26]

melandory@chromium.org changed reviewers:
+ thestig@chromium.org

[melandory, 2016-08-25 23:38:27]

thestig@chromium.org: Please review changes in 

chrome/browser/renderer_host/safe_browsing_resource_throttle.cc

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

https://codereview.chromium.org/2272323002/diff/20001/components/subresource_...
components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:92:
bool proceed =
On 2016/08/25 21:32:54, engedy wrote:
> We should have at least one test that verifies this matrix of ActivationList x
> (threat_type, meta).

Yes, I'm trying to add at least some basic tests.

[commit-bot: I haz the power, 2016-08-25 23:45:22]

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

[commit-bot: I haz the power, 2016-08-25 23:45:23]

Dry run: This issue passed the CQ dry run.

[Lei Zhang, 2016-08-26 00:46:26]

lgtm

[melandory, 2016-08-26 02:59:41]

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

[commit-bot: I haz the power, 2016-08-26 03:00: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, 2016-08-26 03:36:50]

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

[commit-bot: I haz the power, 2016-08-26 03:36:50]

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

[melandory, 2016-08-26 05:14:17]

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

[commit-bot: I haz the power, 2016-08-26 05:14:43]

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

[melandory, 2016-08-26 05:39:08]

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

[commit-bot: I haz the power, 2016-08-26 05:39:15]

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

[melandory, 2016-08-26 06:07:01]

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

[melandory, 2016-08-26 06:07:07]

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

[commit-bot: I haz the power, 2016-08-26 06:07: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, 2016-08-26 07:40:15]

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

[commit-bot: I haz the power, 2016-08-26 07:40:16]

Dry run: This issue passed the CQ dry run.

[melandory, 2016-08-26 08:46:54]

The CQ bit was checked by melandory@chromium.org

[melandory, 2016-08-26 08:46:55]

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

[commit-bot: I haz the power, 2016-08-26 08:47:05]

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-08-26 08:51:34]

Description was changed from

==========
Allow Safe Browsing Saresource Filter to distinguish between different lists.

Safe browsing subresource filter should be able to make activation
descision based on the type of the list and metadata for the list.

BUG=609747, 641037
==========

to

==========
Allow Safe Browsing Saresource Filter to distinguish between different lists.

Safe browsing subresource filter should be able to make activation
descision based on the type of the list and metadata for the list.

BUG=609747, 641037
==========

[commit-bot: I haz the power, 2016-08-26 08:51:35]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-08-26 08:53:49]

Description was changed from

==========
Allow Safe Browsing Saresource Filter to distinguish between different lists.

Safe browsing subresource filter should be able to make activation
descision based on the type of the list and metadata for the list.

BUG=609747, 641037
==========

to

==========
Allow Safe Browsing Saresource Filter to distinguish between different lists.

Safe browsing subresource filter should be able to make activation
descision based on the type of the list and metadata for the list.

BUG=609747, 641037

Committed: https://crrev.com/3851251cc17db046f32af57e003a1e86fd375db3
Cr-Commit-Position: refs/heads/master@{#414674}
==========

[commit-bot: I haz the power, 2016-08-26 08:53:51]

Patchset 7 (id:??) landed as
https://crrev.com/3851251cc17db046f32af57e003a1e86fd375db3
Cr-Commit-Position: refs/heads/master@{#414674}

[melandory, 2016-08-26 15:06:10]

https://codereview.chromium.org/2272323002/diff/20001/components/subresource_...
File components/subresource_filter/core/browser/subresource_filter_features.cc
(right):

https://codereview.chromium.org/2272323002/diff/20001/components/subresource_...
components/subresource_filter/core/browser/subresource_filter_features.cc:58:
base::SplitString(activation_lists, ",", base::TRIM_WHITESPACE,
On 2016/08/25 21:32:54, engedy wrote:
> Let's use SplitStringPiece() here to avoid copies.  Did you check that
> comma-separated values can be input on the command line?

Done.

https://codereview.chromium.org/2272323002/diff/20001/components/subresource_...
components/subresource_filter/core/browser/subresource_filter_features.cc:59:
base::SPLIT_WANT_ALL)) {
On 2016/08/25 21:32:54, engedy wrote:
> nit: SPLIT_WANT_NONEMPTY (not that it matters)

Done.

https://codereview.chromium.org/2272323002/diff/20001/components/subresource_...
File components/subresource_filter/core/browser/subresource_filter_features.h
(right):

https://codereview.chromium.org/2272323002/diff/20001/components/subresource_...
components/subresource_filter/core/browser/subresource_filter_features.h:30:
extern const char kActivationListSocialEngAdsIntestitial[];
On 2016/08/25 21:32:54, engedy wrote:
> typo: "Interstitial" (missing 'r').  In more places in the enumerator and
> strings too. 
> 
> Also, for consistency with SB terminology elsewhere, can we just
> s/Eng/Engineering/? I know it's long, but it's already long. :)  

Done.

https://codereview.chromium.org/2272323002/diff/20001/components/subresource_...
components/subresource_filter/core/browser/subresource_filter_features.h:48:
ActivationList GetCurrentActivationList();
On 2016/08/25 21:32:54, engedy wrote:
> I'd suggest being even more conservative here. An empty list should mean none,
> and more importantly, a "list containing only future values unknown to this
> client" should mean none too.
> 
> In the future we will probably want this to return a bitmask. For now, fine to
> just have ActivationList::NONE or something along these lines. And return the
> widest list for which we find a token in the comma-separated list.

Done.

[engedy, 2016-08-26 17:37:18]

Thank you for the rigorous tests!  Two important things below.

https://codereview.chromium.org/2272323002/diff/120001/components/subresource...
File
components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc
(right):

https://codereview.chromium.org/2272323002/diff/120001/components/subresource...
components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:100:
(threat_type == safe_browsing::SB_THREAT_TYPE_CLIENT_SIDE_PHISHING_URL);
Could you please double check with Noe if this should be
SB_THREAT_TYPE_CLIENT_SIDE_PHISHING_URL or SB_THREAT_TYPE_URL_PHISHING or both?

https://codereview.chromium.org/2272323002/diff/120001/components/subresource...
File components/subresource_filter/core/browser/subresource_filter_features.cc
(right):

https://codereview.chromium.org/2272323002/diff/120001/components/subresource...
components/subresource_filter/core/browser/subresource_filter_features.cc:30:
"social_eng_ads_intertitial";
Watch out, typo still hiding in this literal.  We should probably change `eng`
to `engineering` too.

[melandory, 2016-08-26 18:20:52]

On 2016/08/26 17:37:18, engedy wrote:
> Thank you for the rigorous tests!  Two important things below.
> 
>
https://codereview.chromium.org/2272323002/diff/120001/components/subresource...
> File
>
components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc
> (right):
> 
>
https://codereview.chromium.org/2272323002/diff/120001/components/subresource...
>
components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:100:
> (threat_type == safe_browsing::SB_THREAT_TYPE_CLIENT_SIDE_PHISHING_URL);
> Could you please double check with Noe if this should be
> SB_THREAT_TYPE_CLIENT_SIDE_PHISHING_URL or SB_THREAT_TYPE_URL_PHISHING or
both?
Yeah, I decided to do my best guess and wait util morning to fix or confirm. So
fix will arrive shortly.
> 
>
https://codereview.chromium.org/2272323002/diff/120001/components/subresource...
> File components/subresource_filter/core/browser/subresource_filter_features.cc
> (right):
> 
>
https://codereview.chromium.org/2272323002/diff/120001/components/subresource...
> components/subresource_filter/core/browser/subresource_filter_features.cc:30:
> "social_eng_ads_intertitial";
> Watch out, typo still hiding in this literal.  We should probably change `eng`
> to `engineering` too.
Yeah! I tried to find it in the evening, but no luck.