Move SafeBrowsingUIManager::UnsafeResource to security_interstitials namespace

Move SafeBrowsingUIManager::UnsafeResource to security_interstitials component.

UnsafeResource is used in creating/showing SB interstitial, thus this is a preparation step of componentizing SB interstitial.

BUG=666100
Committed: https://crrev.com/792a666e12699e4e19e7e134e31759808fd0b6d0
Cr-Commit-Position: refs/heads/master@{#436136}

[Jialiu Lin, 2016-11-29 00:38:07]

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

[commit-bot: I haz the power, 2016-11-29 00:39:26]

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-11-29 00:45:39]

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

[commit-bot: I haz the power, 2016-11-29 00:45:39]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  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_...)

[Jialiu Lin, 2016-11-29 00:56:32]

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

[commit-bot: I haz the power, 2016-11-29 00:57:18]

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-11-29 01:11:01]

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

[commit-bot: I haz the power, 2016-11-29 01:11:02]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[Jialiu Lin, 2016-11-29 18:56:22]

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

[commit-bot: I haz the power, 2016-11-29 18:56:46]

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-11-29 19:05:30]

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

[commit-bot: I haz the power, 2016-11-29 19:05:31]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[Jialiu Lin, 2016-11-29 19:35:09]

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

[commit-bot: I haz the power, 2016-11-29 19:35: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, 2016-11-29 20:39:43]

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

[commit-bot: I haz the power, 2016-11-29 20:39:45]

Dry run: This issue passed the CQ dry run.

[Jialiu Lin, 2016-11-29 20:49:06]

Patchset #2 (id:20001) has been deleted

[Jialiu Lin, 2016-11-29 20:49:10]

Patchset #2 (id:40001) has been deleted

[Jialiu Lin, 2016-11-29 20:49:14]

Patchset #2 (id:60001) has been deleted

[Jialiu Lin, 2016-11-29 20:53:51]

Description was changed from

==========
Move SafeBrowsingUIManager::UnsafeResource to security_interstitials namespace

BUG=666100
==========

to

==========
Move SafeBrowsingUIManager::UnsafeResource to security_interstitials component.

UnsafeResource is used in creating/showing SB interstitial, thus this is a
preparation step of componentizing SB interstitial.


BUG=666100
==========

[Jialiu Lin, 2016-11-29 20:56:10]

jialiul@chromium.org changed reviewers:
+ estark@chromium.org, meacer@chromium.org, nparker@chromium.org

[Jialiu Lin, 2016-11-29 20:56:11]

Hi naprker@, meacer@, estark@
Please take a look at this refactoring CL. No logic change, simply move
UnsafeResource structure out of SB ui manager and into security_interstitials
component.

[estark, 2016-11-29 22:04:47]

Is UnsafeResource allowed to live in the component's core directory, given that
it depends on //content? I thought code with content dependencies had to live in
a component's content directory, even if the code is non-ios.

[estark, 2016-11-29 22:18:30]

Looks reasonable to me, modulo two questions:
1. My previous question about whether this code is allowed to live in the core
directory of the component
2. Is there a design doc for this componentization, or somewhere that the
overall plan is documented? I'm wondering how SafeBrowsingBlockingPage is going
to be componentized so that it can be reused on iOS. Since SBBP depends on
UnsafeResource which depends on //content, it seems like there's going to need
to be some further refactoring of UnsafeResource so that SBBP can be
componentized?

https://codereview.chromium.org/2540563002/diff/80001/components/security_int...
File components/security_interstitials/OWNERS (right):

https://codereview.chromium.org/2540563002/diff/80001/components/security_int...
components/security_interstitials/OWNERS:7: jialiul@chromium.org
nit: you should probably do this in a separate CL (and cc the existing OWNERS).
That way if this CL has to be reverted you won't be reverted as an owner :)

https://codereview.chromium.org/2540563002/diff/80001/components/security_int...
File components/security_interstitials/core/unsafe_resource.h (right):

https://codereview.chromium.org/2540563002/diff/80001/components/security_int...
components/security_interstitials/core/unsafe_resource.h:27: // Passed a boolean
indicating whether or not it is OK to proceed with
Does this need `git cl format`? I'd expect the struct fields to be indented.

[Jialiu Lin, 2016-11-30 22:49:29]

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

[commit-bot: I haz the power, 2016-11-30 22:49:58]

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

[Jialiu Lin, 2016-11-30 23:03:53]

On 2016/11/29 at 22:18:30, estark wrote:
> Looks reasonable to me, modulo two questions:
> 1. My previous question about whether this code is allowed to live in the core
directory of the component

Thanks for pointing that out. I moved the UnsafeResource code into a content
folder under security_interstitials component. 

> 2. Is there a design doc for this componentization, or somewhere that the
overall plan is documented? I'm wondering how SafeBrowsingBlockingPage is going
to be componentized so that it can be reused on iOS. Since SBBP depends on
UnsafeResource which depends on //content, it seems like there's going to need
to be some further refactoring of UnsafeResource so that SBBP can be
componentized?

This componentization is intended for WebView safebrowsing integration. Here is
a doc from felt@ describing general guidelines and procedures of componentizing
safebrowsingblockingpage
go/componentizing_sbbp

> 
>
https://codereview.chromium.org/2540563002/diff/80001/components/security_int...
> File components/security_interstitials/OWNERS (right):
> 
>
https://codereview.chromium.org/2540563002/diff/80001/components/security_int...
> components/security_interstitials/OWNERS:7: jialiul@chromium.org
> nit: you should probably do this in a separate CL (and cc the existing
OWNERS). That way if this CL has to be reverted you won't be reverted as an
owner :)
> 
>
https://codereview.chromium.org/2540563002/diff/80001/components/security_int...
> File components/security_interstitials/core/unsafe_resource.h (right):
> 
>
https://codereview.chromium.org/2540563002/diff/80001/components/security_int...
> components/security_interstitials/core/unsafe_resource.h:27: // Passed a
boolean indicating whether or not it is OK to proceed with
> Does this need `git cl format`? I'd expect the struct fields to be indented.

[Jialiu Lin, 2016-11-30 23:04:00]

https://codereview.chromium.org/2540563002/diff/80001/components/security_int...
File components/security_interstitials/OWNERS (right):

https://codereview.chromium.org/2540563002/diff/80001/components/security_int...
components/security_interstitials/OWNERS:7: jialiul@chromium.org
On 2016/11/29 at 22:18:30, estark wrote:
> nit: you should probably do this in a separate CL (and cc the existing
OWNERS). That way if this CL has to be reverted you won't be reverted as an
owner :)

Sure. I'll move this to a separate CL.

https://codereview.chromium.org/2540563002/diff/80001/components/security_int...
File components/security_interstitials/core/unsafe_resource.h (right):

https://codereview.chromium.org/2540563002/diff/80001/components/security_int...
components/security_interstitials/core/unsafe_resource.h:27: // Passed a boolean
indicating whether or not it is OK to proceed with
On 2016/11/29 at 22:18:30, estark wrote:
> Does this need `git cl format`? I'd expect the struct fields to be indented.

done.

[estark, 2016-12-01 00:29:27]

On 2016/11/30 23:03:53, Jialiu Lin wrote:
> On 2016/11/29 at 22:18:30, estark wrote:
> > Looks reasonable to me, modulo two questions:
> > 1. My previous question about whether this code is allowed to live in the
core
> directory of the component
> 
> Thanks for pointing that out. I moved the UnsafeResource code into a content
> folder under security_interstitials component. 
> 
> > 2. Is there a design doc for this componentization, or somewhere that the
> overall plan is documented? I'm wondering how SafeBrowsingBlockingPage is
going
> to be componentized so that it can be reused on iOS. Since SBBP depends on
> UnsafeResource which depends on //content, it seems like there's going to need
> to be some further refactoring of UnsafeResource so that SBBP can be
> componentized?
> 
> This componentization is intended for WebView safebrowsing integration. Here
is
> a doc from felt@ describing general guidelines and procedures of
componentizing
> safebrowsingblockingpage
> go/componentizing_sbbp
> 

Thanks, that makes sense. As discussed in person, I was confused and thought we
were trying to reuse this code on iOS, not webview, so it sounds like putting it
in the content layer of the component makes sense.

> > 
> >
>
https://codereview.chromium.org/2540563002/diff/80001/components/security_int...
> > File components/security_interstitials/OWNERS (right):
> > 
> >
>
https://codereview.chromium.org/2540563002/diff/80001/components/security_int...
> > components/security_interstitials/OWNERS:7: mailto:jialiul@chromium.org
> > nit: you should probably do this in a separate CL (and cc the existing
> OWNERS). That way if this CL has to be reverted you won't be reverted as an
> owner :)
> > 
> >
>
https://codereview.chromium.org/2540563002/diff/80001/components/security_int...
> > File components/security_interstitials/core/unsafe_resource.h (right):
> > 
> >
>
https://codereview.chromium.org/2540563002/diff/80001/components/security_int...
> > components/security_interstitials/core/unsafe_resource.h:27: // Passed a
> boolean indicating whether or not it is OK to proceed with
> > Does this need `git cl format`? I'd expect the struct fields to be indented.

[estark, 2016-12-01 00:29:56]

lgtm with a few nits, thanks!

https://codereview.chromium.org/2540563002/diff/120001/components/security_in...
File components/security_interstitials/DEPS (right):

https://codereview.chromium.org/2540563002/diff/120001/components/security_in...
components/security_interstitials/DEPS:10: "+content/public/browser",
content DEPS should go in components/security_interstitials/content/DEPS

https://codereview.chromium.org/2540563002/diff/120001/components/security_in...
File components/security_interstitials/content/BUILD.gn (right):

https://codereview.chromium.org/2540563002/diff/120001/components/security_in...
components/security_interstitials/content/BUILD.gn:1: # Copyright 2015 The
Chromium Authors. All rights reserved.
nit: 2016

https://codereview.chromium.org/2540563002/diff/120001/components/security_in...
components/security_interstitials/content/BUILD.gn:18: if (is_ios) {
I thiiiiiink this is unnecessary as content/ layers of components shouldn't get
built on iOS.

[Jialiu Lin, 2016-12-01 00:47:10]

Patchset #5 (id:140001) has been deleted

[meacer, 2016-12-01 00:55:08]

Lgtm

https://codereview.chromium.org/2540563002/diff/160001/components/security_in...
File components/security_interstitials/content/unsafe_resource.cc (right):

https://codereview.chromium.org/2540563002/diff/160001/components/security_in...
components/security_interstitials/content/unsafe_resource.cc:36:
UnsafeResource::~UnsafeResource() { }
nit: extra space between braces

https://codereview.chromium.org/2540563002/diff/160001/components/security_in...
File components/security_interstitials/content/unsafe_resource.h (right):

https://codereview.chromium.org/2540563002/diff/160001/components/security_in...
components/security_interstitials/content/unsafe_resource.h:32:
UnsafeResource(const UnsafeResource& other);
explicit for single parameter constructor.

[Jialiu Lin, 2016-12-01 00:55:09]

jialiul@chromium.org changed reviewers:
+ csharrison@chromium.org, sgurun@chromium.org

[Jialiu Lin, 2016-12-01 00:55:10]

Thanks estark@! 

+ sgurun@ for c/b/loader/data_reduction_proxy_resource_throttle_android/*
+ csharrison@ for other files in c/b/loader/

https://codereview.chromium.org/2540563002/diff/120001/components/security_in...
File components/security_interstitials/DEPS (right):

https://codereview.chromium.org/2540563002/diff/120001/components/security_in...
components/security_interstitials/DEPS:10: "+content/public/browser",
On 2016/12/01 at 00:29:56, estark wrote:
> content DEPS should go in components/security_interstitials/content/DEPS

Done

https://codereview.chromium.org/2540563002/diff/120001/components/security_in...
File components/security_interstitials/content/BUILD.gn (right):

https://codereview.chromium.org/2540563002/diff/120001/components/security_in...
components/security_interstitials/content/BUILD.gn:1: # Copyright 2015 The
Chromium Authors. All rights reserved.
On 2016/12/01 at 00:29:56, estark wrote:
> nit: 2016

Thanks for catching this!

https://codereview.chromium.org/2540563002/diff/120001/components/security_in...
components/security_interstitials/content/BUILD.gn:18: if (is_ios) {
On 2016/12/01 at 00:29:56, estark wrote:
> I thiiiiiink this is unnecessary as content/ layers of components shouldn't
get built on iOS.

You're right. Since content is separated from core, I should remove this "if"
and only add components/security_interstitials/content deps when safe browsing
is enabled (inchrome/browser/BUILD.gn).

[Jialiu Lin, 2016-12-01 00:55:22]

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

[commit-bot: I haz the power, 2016-12-01 00:55:57]

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

[Jialiu Lin, 2016-12-01 01:04:37]

jialiul@chromium.org changed reviewers:
+ avi@chromium.org

[Jialiu Lin, 2016-12-01 01:04:38]

Thanks, meacer@! 

+avi@ because of deps change '+content/public/browser' in
components/security_interstitial/content/DEPS

https://codereview.chromium.org/2540563002/diff/160001/components/security_in...
File components/security_interstitials/content/unsafe_resource.cc (right):

https://codereview.chromium.org/2540563002/diff/160001/components/security_in...
components/security_interstitials/content/unsafe_resource.cc:36:
UnsafeResource::~UnsafeResource() { }
On 2016/12/01 at 00:55:08, Mustafa Emre Acer wrote:
> nit: extra space between braces

Nice catch!

https://codereview.chromium.org/2540563002/diff/160001/components/security_in...
File components/security_interstitials/content/unsafe_resource.h (right):

https://codereview.chromium.org/2540563002/diff/160001/components/security_in...
components/security_interstitials/content/unsafe_resource.h:32:
UnsafeResource(const UnsafeResource& other);
On 2016/12/01 at 00:55:08, Mustafa Emre Acer wrote:
> explicit for single parameter constructor.
done

[sgurun-gerrit only, 2016-12-01 01:47:28]

On 2016/12/01 01:04:38, Jialiu Lin wrote:
> Thanks, meacer@! 
> 
> +avi@ because of deps change '+content/public/browser' in
> components/security_interstitial/content/DEPS
> 
>
https://codereview.chromium.org/2540563002/diff/160001/components/security_in...
> File components/security_interstitials/content/unsafe_resource.cc (right):
> 
>
https://codereview.chromium.org/2540563002/diff/160001/components/security_in...
> components/security_interstitials/content/unsafe_resource.cc:36:
> UnsafeResource::~UnsafeResource() { }
> On 2016/12/01 at 00:55:08, Mustafa Emre Acer wrote:
> > nit: extra space between braces
> 
> Nice catch!
> 
>
https://codereview.chromium.org/2540563002/diff/160001/components/security_in...
> File components/security_interstitials/content/unsafe_resource.h (right):
> 
>
https://codereview.chromium.org/2540563002/diff/160001/components/security_in...
> components/security_interstitials/content/unsafe_resource.h:32:
> UnsafeResource(const UnsafeResource& other);
> On 2016/12/01 at 00:55:08, Mustafa Emre Acer wrote:
> > explicit for single parameter constructor.
> done
data_reduction_proxy_resource_throttle_android lgtm

[Avi (use Gerrit), 2016-12-01 15:57:06]

On 2016/12/01 01:04:38, Jialiu Lin wrote:
> +avi@ because of deps change '+content/public/browser' in
> components/security_interstitial/content/DEPS

If you're OK with that component depending on content, I'm OK with it.

LGTM

[Jialiu Lin, 2016-12-02 17:19:06]

The CQ bit was checked by jialiul@chromium.org

[Jialiu Lin, 2016-12-02 17:19:07]

The patchset sent to the CQ was uploaded after l-g-t-m from estark@chromium.org,
meacer@chromium.org
Link to the patchset: https://codereview.chromium.org/2540563002/#ps180001
(title: "meacer's comments")

[commit-bot: I haz the power, 2016-12-02 17:19:30]

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-02 17:26:29]

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

[commit-bot: I haz the power, 2016-12-02 17:26:30]

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

[Charlie Harrison, 2016-12-02 21:58:09]

LGTM sorry for the delay, missed this one.

[Jialiu Lin, 2016-12-02 22:23:38]

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

[commit-bot: I haz the power, 2016-12-02 22:24:22]

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-12-02 23:29:38]

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

[commit-bot: I haz the power, 2016-12-02 23:29:39]

Dry run: This issue passed the CQ dry run.

[Jialiu Lin, 2016-12-02 23:30:20]

The CQ bit was checked by jialiul@chromium.org

[Jialiu Lin, 2016-12-02 23:30:21]

The patchset sent to the CQ was uploaded after l-g-t-m from sgurun@chromium.org,
avi@chromium.org, csharrison@chromium.org, estark@chromium.org,
meacer@chromium.org
Link to the patchset: https://codereview.chromium.org/2540563002/#ps200001
(title: "make bots happy")

[commit-bot: I haz the power, 2016-12-02 23:31:10]

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-02 23:39:12]

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

[commit-bot: I haz the power, 2016-12-02 23:39:14]

Failed to apply patch for chrome/browser/BUILD.gn:
While running git apply --index -p1;
  error: patch failed: chrome/browser/BUILD.gn:2006
  error: chrome/browser/BUILD.gn: patch does not apply

Patch:       chrome/browser/BUILD.gn
Index: chrome/browser/BUILD.gn
diff --git a/chrome/browser/BUILD.gn b/chrome/browser/BUILD.gn
index
cb48ee8cffd6f7fd2394fc156e9f2f23ff36d27d..188183f88378ee09020b67d69068cdbc88799101
100644
--- a/chrome/browser/BUILD.gn
+++ b/chrome/browser/BUILD.gn
@@ -2006,6 +2006,7 @@ split_static_library("browser") {
       "//chrome/browser/safe_browsing:chunk_proto",
       "//chrome/common/safe_browsing:proto",
       "//components/safe_browsing_db:metadata_proto",
+      "//components/security_interstitials/content",
     ]
     if (safe_browsing_mode == 1) {
       # "Safe Browsing Full" files in addition to the "basic" ones to use for

[Jialiu Lin, 2016-12-03 00:27:36]

The CQ bit was checked by jialiul@chromium.org

[Jialiu Lin, 2016-12-03 00:27:37]

The patchset sent to the CQ was uploaded after l-g-t-m from sgurun@chromium.org,
avi@chromium.org, csharrison@chromium.org, estark@chromium.org,
meacer@chromium.org
Link to the patchset: https://codereview.chromium.org/2540563002/#ps220001
(title: "rebase update")

[commit-bot: I haz the power, 2016-12-03 00:28:10]

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-03 01:44:15]

CQ is committing da patch.

Bot data: {"patchset_id": 220001, "attempt_start_ts": 1480724856857150,
"parent_rev": "9a747a30f532fb92135126fb1b7293b8af760c3a", "commit_rev":
"2aa013d229401436cf553dd68f98d21d4be0a53d"}

[commit-bot: I haz the power, 2016-12-03 01:44:49]

Committed patchset #8 (id:220001)

[commit-bot: I haz the power, 2016-12-03 01:47:26]

Description was changed from

==========
Move SafeBrowsingUIManager::UnsafeResource to security_interstitials component.

UnsafeResource is used in creating/showing SB interstitial, thus this is a
preparation step of componentizing SB interstitial.


BUG=666100
==========

to

==========
Move SafeBrowsingUIManager::UnsafeResource to security_interstitials component.

UnsafeResource is used in creating/showing SB interstitial, thus this is a
preparation step of componentizing SB interstitial.

BUG=666100

Committed: https://crrev.com/792a666e12699e4e19e7e134e31759808fd0b6d0
Cr-Commit-Position: refs/heads/master@{#436136}
==========

[commit-bot: I haz the power, 2016-12-03 01:47:27]

Patchset 8 (id:??) landed as
https://crrev.com/792a666e12699e4e19e7e134e31759808fd0b6d0
Cr-Commit-Position: refs/heads/master@{#436136}