Componentize safe_browsing [1]: create component, move messages, constants and switches.

Componentize safe_browsing [1]: create component, move messages, constants and switches.

Rationale:
Current safe browsing code lives in the chrome/ layer, but we
want to make it available in WebView as well (at least the
relevant parts of it). Hence this effort of making a safe
browsing component.

In this patch:
- move IPCs (spellcheck_messages) + add dedicated generator
- isolate and move safe browsing specific constants
- isolate and move safe browsing specific switches
- remove a few unused includes

Note:
To reduce the number of components and keep everything
related in one place the idea is to move the code currently
living in components/safe_browsing_db/ to this safe_browsing/
component in the future.

BUG=488675
TBR=brettw@chromium.org

Committed: https://crrev.com/f30c94f1fa39ad0467a31ff99e7acc02142e6fa6
Cr-Commit-Position: refs/heads/master@{#435635}

[timvolodine, 2016-10-26 16:49:09]

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

[commit-bot: I haz the power, 2016-10-26 16:49:38]

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-10-26 16:53:41]

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

[commit-bot: I haz the power, 2016-10-26 16:53:43]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  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_...)

[timvolodine, 2016-10-26 16:54:58]

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

[commit-bot: I haz the power, 2016-10-26 16:55:16]

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

[timvolodine, 2016-10-26 16:57:02]

Description was changed from

==========
Componentize safe_browsing [1]: create component and move messages, constants
and switches.

Rationale:
Current safe browsing code lives in the chrome/ layer, but we
want to make it available in WebView as well (at least the
relevant parts of it). Hence this effort of making a safe
browsing component.

In this patch:
- move IPCs (spellcheck_messages)
- isolate and move safe browsing specific constants
- update a few unused includes
- isolate and move safe browsing specific switches

BUG=
==========

to

==========
Componentize safe_browsing [1]: create component and move messages, constants
and switches.

Rationale:
Current safe browsing code lives in the chrome/ layer, but we
want to make it available in WebView as well (at least the
relevant parts of it). Hence this effort of making a safe
browsing component.

In this patch:
- move IPCs (spellcheck_messages)
- isolate and move safe browsing specific constants
- isolate and move safe browsing specific switches
- remove a few unused includes

BUG=
==========

[commit-bot: I haz the power, 2016-10-26 16:59:25]

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

[commit-bot: I haz the power, 2016-10-26 16:59:26]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  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_...)

[timvolodine, 2016-10-26 17:07:48]

Description was changed from

==========
Componentize safe_browsing [1]: create component and move messages, constants
and switches.

Rationale:
Current safe browsing code lives in the chrome/ layer, but we
want to make it available in WebView as well (at least the
relevant parts of it). Hence this effort of making a safe
browsing component.

In this patch:
- move IPCs (spellcheck_messages)
- isolate and move safe browsing specific constants
- isolate and move safe browsing specific switches
- remove a few unused includes

BUG=
==========

to

==========
Componentize safe_browsing [1]: create component, move messages, constants and
switches.

Rationale:
Current safe browsing code lives in the chrome/ layer, but we
want to make it available in WebView as well (at least the
relevant parts of it). Hence this effort of making a safe
browsing component.

In this patch:
- move IPCs (spellcheck_messages)
- isolate and move safe browsing specific constants
- isolate and move safe browsing specific switches
- remove a few unused includes

BUG=
==========

[timvolodine, 2016-10-26 17:08:54]

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

[commit-bot: I haz the power, 2016-10-26 17:09:14]

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-10-26 17:19:23]

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

[commit-bot: I haz the power, 2016-10-26 17:19:24]

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...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[timvolodine, 2016-10-26 17:32:51]

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

[commit-bot: I haz the power, 2016-10-26 17:33:21]

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

[timvolodine, 2016-10-26 17:37:15]

Description was changed from

==========
Componentize safe_browsing [1]: create component, move messages, constants and
switches.

Rationale:
Current safe browsing code lives in the chrome/ layer, but we
want to make it available in WebView as well (at least the
relevant parts of it). Hence this effort of making a safe
browsing component.

In this patch:
- move IPCs (spellcheck_messages)
- isolate and move safe browsing specific constants
- isolate and move safe browsing specific switches
- remove a few unused includes

BUG=
==========

to

==========
Componentize safe_browsing [1]: create component, move messages, constants and
switches.

Rationale:
Current safe browsing code lives in the chrome/ layer, but we
want to make it available in WebView as well (at least the
relevant parts of it). Hence this effort of making a safe
browsing component.

In this patch:
- move IPCs (spellcheck_messages) + add dedicated generator
- isolate and move safe browsing specific constants
- isolate and move safe browsing specific switches
- remove a few unused includes

BUG=
==========

[commit-bot: I haz the power, 2016-10-26 17:48:47]

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

[commit-bot: I haz the power, 2016-10-26 17:48:48]

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

[timvolodine, 2016-10-27 12:48:37]

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

[commit-bot: I haz the power, 2016-10-27 12:48:45]

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-10-27 13:04:48]

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

[commit-bot: I haz the power, 2016-10-27 13:04: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_...)

[timvolodine, 2016-10-27 13:12:36]

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

[commit-bot: I haz the power, 2016-10-27 13:12:58]

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-10-27 14:10:03]

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

[commit-bot: I haz the power, 2016-10-27 14:10:04]

Dry run: This issue passed the CQ dry run.

[timvolodine, 2016-10-28 15:52:41]

timvolodine@chromium.org changed reviewers:
+ nparker@chromium.org

[Nathan Parker, 2016-10-28 17:40:10]

nparker@chromium.org changed reviewers:
+ vakh@chromium.org

[Nathan Parker, 2016-10-28 17:55:07]

Thanks Tim.  Overall LG, though I'd like to understand the plan for what goes in
safe_browsing/ and what goes in safe_browsing_db/ components (we can discuss in
the email I just sent).  It'd be good to have a doc describing this if there
isn't already.

https://codereview.chromium.org/2450993003/diff/100001/chrome/renderer/BUILD.gn
File chrome/renderer/BUILD.gn (right):

https://codereview.chromium.org/2450993003/diff/100001/chrome/renderer/BUILD....
chrome/renderer/BUILD.gn:211: "//components/safe_browsing/common:common",
If this code is needed on Android, then it should get added for
safe_browsing_mode == 2 (or !=0 really)

https://codereview.chromium.org/2450993003/diff/100001/components/safe_browsi...
File components/safe_browsing/common/BUILD.gn (right):

https://codereview.chromium.org/2450993003/diff/100001/components/safe_browsi...
components/safe_browsing/common/BUILD.gn:5: source_set("common") {
Can you comment on what will use this source set?  Is it all platforms that use
any part of SB?

https://codereview.chromium.org/2450993003/diff/100001/components/safe_browsi...
File components/safe_browsing/common/safebrowsing_message_generator.h (right):

https://codereview.chromium.org/2450993003/diff/100001/components/safe_browsi...
components/safe_browsing/common/safebrowsing_message_generator.h:2: // Use of
this source code is governed by a BSD-style license that can be
Do we need this file at all?  Could refs to it just use the component .h? I'm
not familiar with how this one is used.

[sgurun-gerrit only, 2016-11-04 02:25:36]

On 2016/10/28 17:55:07, Nathan Parker wrote:
> Thanks Tim.  Overall LG, though I'd like to understand the plan for what goes
in
> safe_browsing/ and what goes in safe_browsing_db/ components (we can discuss
in
> the email I just sent).  It'd be good to have a doc describing this if there
> isn't already.

Is there actually a discussion going on about this? any update?

> 
>
https://codereview.chromium.org/2450993003/diff/100001/chrome/renderer/BUILD.gn
> File chrome/renderer/BUILD.gn (right):
> 
>
https://codereview.chromium.org/2450993003/diff/100001/chrome/renderer/BUILD....
> chrome/renderer/BUILD.gn:211: "//components/safe_browsing/common:common",
> If this code is needed on Android, then it should get added for
> safe_browsing_mode == 2 (or !=0 really)
> 
>
https://codereview.chromium.org/2450993003/diff/100001/components/safe_browsi...
> File components/safe_browsing/common/BUILD.gn (right):
> 
>
https://codereview.chromium.org/2450993003/diff/100001/components/safe_browsi...
> components/safe_browsing/common/BUILD.gn:5: source_set("common") {
> Can you comment on what will use this source set?  Is it all platforms that
use
> any part of SB?
> 
>
https://codereview.chromium.org/2450993003/diff/100001/components/safe_browsi...
> File components/safe_browsing/common/safebrowsing_message_generator.h (right):
> 
>
https://codereview.chromium.org/2450993003/diff/100001/components/safe_browsi...
> components/safe_browsing/common/safebrowsing_message_generator.h:2: // Use of
> this source code is governed by a BSD-style license that can be
> Do we need this file at all?  Could refs to it just use the component .h? I'm
> not familiar with how this one is used.

[timvolodine, 2016-11-04 15:44:22]

On 2016/11/04 02:25:36, sgurun wrote:
> On 2016/10/28 17:55:07, Nathan Parker wrote:
> > Thanks Tim.  Overall LG, though I'd like to understand the plan for what
goes
> in
> > safe_browsing/ and what goes in safe_browsing_db/ components (we can discuss
> in
> > the email I just sent).  It'd be good to have a doc describing this if there
> > isn't already.
> 
> Is there actually a discussion going on about this? any update?

regarding safe_browsing_db/ I've posted a reply on the email thread..

> 
> > 
> >
>
https://codereview.chromium.org/2450993003/diff/100001/chrome/renderer/BUILD.gn
> > File chrome/renderer/BUILD.gn (right):
> > 
> >
>
https://codereview.chromium.org/2450993003/diff/100001/chrome/renderer/BUILD....
> > chrome/renderer/BUILD.gn:211: "//components/safe_browsing/common:common",
> > If this code is needed on Android, then it should get added for
> > safe_browsing_mode == 2 (or !=0 really)
> > 
> >
>
https://codereview.chromium.org/2450993003/diff/100001/components/safe_browsi...
> > File components/safe_browsing/common/BUILD.gn (right):
> > 
> >
>
https://codereview.chromium.org/2450993003/diff/100001/components/safe_browsi...
> > components/safe_browsing/common/BUILD.gn:5: source_set("common") {
> > Can you comment on what will use this source set?  Is it all platforms that
> use
> > any part of SB?
> > 
> >
>
https://codereview.chromium.org/2450993003/diff/100001/components/safe_browsi...
> > File components/safe_browsing/common/safebrowsing_message_generator.h
(right):
> > 
> >
>
https://codereview.chromium.org/2450993003/diff/100001/components/safe_browsi...
> > components/safe_browsing/common/safebrowsing_message_generator.h:2: // Use
of
> > this source code is governed by a BSD-style license that can be
> > Do we need this file at all?  Could refs to it just use the component .h?
I'm
> > not familiar with how this one is used.

[timvolodine, 2016-11-04 15:45:14]

On 2016/10/28 17:55:07, Nathan Parker wrote:
> Thanks Tim.  Overall LG, though I'd like to understand the plan for what goes
in
> safe_browsing/ and what goes in safe_browsing_db/ components (we can discuss
in
> the email I just sent).  It'd be good to have a doc describing this if there
> isn't already.
> 
>
https://codereview.chromium.org/2450993003/diff/100001/chrome/renderer/BUILD.gn
> File chrome/renderer/BUILD.gn (right):
> 
>
https://codereview.chromium.org/2450993003/diff/100001/chrome/renderer/BUILD....
> chrome/renderer/BUILD.gn:211: "//components/safe_browsing/common:common",
> If this code is needed on Android, then it should get added for
> safe_browsing_mode == 2 (or !=0 really)
> 
>
https://codereview.chromium.org/2450993003/diff/100001/components/safe_browsi...
> File components/safe_browsing/common/BUILD.gn (right):
> 
>
https://codereview.chromium.org/2450993003/diff/100001/components/safe_browsi...
> components/safe_browsing/common/BUILD.gn:5: source_set("common") {
> Can you comment on what will use this source set?  Is it all platforms that
use
> any part of SB?
> 
>
https://codereview.chromium.org/2450993003/diff/100001/components/safe_browsi...
> File components/safe_browsing/common/safebrowsing_message_generator.h (right):
> 
>
https://codereview.chromium.org/2450993003/diff/100001/components/safe_browsi...
> components/safe_browsing/common/safebrowsing_message_generator.h:2: // Use of
> this source code is governed by a BSD-style license that can be
> Do we need this file at all?  Could refs to it just use the component .h? I'm
> not familiar with how this one is used.

Nathan thanks for the comments! I'll address them asap )

[timvolodine, 2016-11-04 17:53:02]

replies below, ptal.

https://codereview.chromium.org/2450993003/diff/100001/chrome/renderer/BUILD.gn
File chrome/renderer/BUILD.gn (right):

https://codereview.chromium.org/2450993003/diff/100001/chrome/renderer/BUILD....
chrome/renderer/BUILD.gn:211: "//components/safe_browsing/common:common",
On 2016/10/28 17:55:07, Nathan Parker wrote:
> If this code is needed on Android, then it should get added for
> safe_browsing_mode == 2 (or !=0 really)

yes correct thanks, the thread_dom_details.cc actually depends on it so need to
be inside the !=0.
Done.

https://codereview.chromium.org/2450993003/diff/100001/components/safe_browsi...
File components/safe_browsing/common/BUILD.gn (right):

https://codereview.chromium.org/2450993003/diff/100001/components/safe_browsi...
components/safe_browsing/common/BUILD.gn:5: source_set("common") {
On 2016/10/28 17:55:07, Nathan Parker wrote:
> Can you comment on what will use this source set?  Is it all platforms that
use
> any part of SB?

yes currently all who use the files listed, in the future more 'common' files
can go here

https://codereview.chromium.org/2450993003/diff/100001/components/safe_browsi...
File components/safe_browsing/common/safebrowsing_message_generator.h (right):

https://codereview.chromium.org/2450993003/diff/100001/components/safe_browsi...
components/safe_browsing/common/safebrowsing_message_generator.h:2: // Use of
this source code is governed by a BSD-style license that can be
On 2016/10/28 17:55:07, Nathan Parker wrote:
> Do we need this file at all?  Could refs to it just use the component .h? I'm
> not familiar with how this one is used.

yes for components we usually have own generators, this is needed because we are
moving the messages out of chrome/common/common_message_generator.h

[timvolodine, 2016-11-10 15:37:30]

On 2016/11/04 17:53:02, timvolodine wrote:
> replies below, ptal.
> 
>
https://codereview.chromium.org/2450993003/diff/100001/chrome/renderer/BUILD.gn
> File chrome/renderer/BUILD.gn (right):
> 
>
https://codereview.chromium.org/2450993003/diff/100001/chrome/renderer/BUILD....
> chrome/renderer/BUILD.gn:211: "//components/safe_browsing/common:common",
> On 2016/10/28 17:55:07, Nathan Parker wrote:
> > If this code is needed on Android, then it should get added for
> > safe_browsing_mode == 2 (or !=0 really)
> 
> yes correct thanks, the thread_dom_details.cc actually depends on it so need
to
> be inside the !=0.
> Done.
> 
>
https://codereview.chromium.org/2450993003/diff/100001/components/safe_browsi...
> File components/safe_browsing/common/BUILD.gn (right):
> 
>
https://codereview.chromium.org/2450993003/diff/100001/components/safe_browsi...
> components/safe_browsing/common/BUILD.gn:5: source_set("common") {
> On 2016/10/28 17:55:07, Nathan Parker wrote:
> > Can you comment on what will use this source set?  Is it all platforms that
> use
> > any part of SB?
> 
> yes currently all who use the files listed, in the future more 'common' files
> can go here
> 
>
https://codereview.chromium.org/2450993003/diff/100001/components/safe_browsi...
> File components/safe_browsing/common/safebrowsing_message_generator.h (right):
> 
>
https://codereview.chromium.org/2450993003/diff/100001/components/safe_browsi...
> components/safe_browsing/common/safebrowsing_message_generator.h:2: // Use of
> this source code is governed by a BSD-style license that can be
> On 2016/10/28 17:55:07, Nathan Parker wrote:
> > Do we need this file at all?  Could refs to it just use the component .h?
I'm
> > not familiar with how this one is used.
> 
> yes for components we usually have own generators, this is needed because we
are
> moving the messages out of chrome/common/common_message_generator.h

friendly ping :)

Nathan, does it look ok to you?

[Nathan Parker, 2016-11-10 17:25:08]

Blah sorry, this fell off my radar. I will aim to reply by midday today.

[Nathan Parker, 2016-11-10 18:57:46]

So looking through the past discussions and existing code, I don't think there
is any advantage to having two safe_browsing* components. Let's continue to
create this new one and later move safe_browsing_db/* to here.

[The past concern with the layered security_interstitial/ component needing the
SBThreatType can be solved with a new (simpler) enum that's defined in that
component, and we can translate to it in safe_browsing/ component. So we'd have
components/safe_browsing and chrome/ depend on
components/security_interstitials]

I would still like to see a plan for what's moving and what  requires a
delegate, etc.  That procedure is described here:
https://www.chromium.org/developers/design-documents/cookbook. You could start
with the design from 2015
(https://docs.google.com/document/d/1Zilzf4dRugaoDnH2kZiXO0NVWiURohuX-Hz7TRR8q...)
and cut out about half of it, and double check the assumptions there.

https://codereview.chromium.org/2450993003/diff/140001/components/safe_browsi...
File components/safe_browsing/common/safebrowsing_constants.cc (right):

https://codereview.chromium.org/2450993003/diff/140001/components/safe_browsi...
components/safe_browsing/common/safebrowsing_constants.cc:10:
FILE_PATH_LITERAL("Safe Browsing");
This won't be needed by WebView directly.  If it's easier to move some of these
rather than disentangle them, that's probably ok, but it would be cleaner to
leave the embedder data (file paths, switches) in /chrome/.

https://codereview.chromium.org/2450993003/diff/140001/components/safe_browsi...
File components/safe_browsing/common/safebrowsing_switches.h (right):

https://codereview.chromium.org/2450993003/diff/140001/components/safe_browsi...
components/safe_browsing/common/safebrowsing_switches.h:11: extern const char
kSbDisableAutoUpdate[];
FYI none of these switches are relevant to WebView -- they will only be used by
chrome.  Is there a need to move them?

[timvolodine, 2016-11-18 14:07:11]

Thanks for the comments! replies below

https://codereview.chromium.org/2450993003/diff/140001/components/safe_browsi...
File components/safe_browsing/common/safebrowsing_constants.cc (right):

https://codereview.chromium.org/2450993003/diff/140001/components/safe_browsi...
components/safe_browsing/common/safebrowsing_constants.cc:10:
FILE_PATH_LITERAL("Safe Browsing");
On 2016/11/10 18:57:46, Nathan Parker wrote:
> This won't be needed by WebView directly.  If it's easier to move some of
these
> rather than disentangle them, that's probably ok, but it would be cleaner to
> leave the embedder data (file paths, switches) in /chrome/. 

safe_browsing_service uses that constant, and we'll need to move that file to
component as well because safe_browsing_resource_throttle depends on it.

https://codereview.chromium.org/2450993003/diff/140001/components/safe_browsi...
File components/safe_browsing/common/safebrowsing_switches.h (right):

https://codereview.chromium.org/2450993003/diff/140001/components/safe_browsi...
components/safe_browsing/common/safebrowsing_switches.h:11: extern const char
kSbDisableAutoUpdate[];
On 2016/11/10 18:57:46, Nathan Parker wrote:
> FYI none of these switches are relevant to WebView -- they will only be used
by
> chrome.  Is there a need to move them?

kSbDisableAutoUpdate is used in safe_browsing_service.cc so need to move it out.
it's possible other switches may not necessarily need moving, however it's good
to have them in one place for component's sake.

[timvolodine, 2016-11-18 14:11:10]

On 2016/11/10 18:57:46, Nathan Parker wrote:
> So looking through the past discussions and existing code, I don't think there
> is any advantage to having two safe_browsing* components. Let's continue to
> create this new one and later move safe_browsing_db/* to here.

sounds good :)

> 
> [The past concern with the layered security_interstitial/ component needing
the
> SBThreatType can be solved with a new (simpler) enum that's defined in that
> component, and we can translate to it in safe_browsing/ component. So we'd
have
> components/safe_browsing and chrome/ depend on
> components/security_interstitials]
> 
> I would still like to see a plan for what's moving and what  requires a
> delegate, etc.  That procedure is described here:
> https://www.chromium.org/developers/design-documents/cookbook. You could start
> with the design from 2015
>
(https://docs.google.com/document/d/1Zilzf4dRugaoDnH2kZiXO0NVWiURohuX-Hz7TRR8q...)
> and cut out about half of it, and double check the assumptions there.
> 

My understanding is that we need safe_browsing_resource_throttle +all its
relevant dependencies in a component to be usable for webview. This first patch
is based on this assumption + dependency analysis. The structure of component
will be /browser, /common, /renderer, probably also android/.

During past componentizations I found that working in steps and basically
hands-on refactoring made the process go faster and avoids missing any
issues/dependencies. Of course I'll write explanation/design as we progress for
things where there are questions, or where there are choices to be made.

[Nathan Parker, 2016-11-18 17:30:03]

lgtm

By just pulling on dependencies and moving code as you go, it seems to me you
could easily end up in a spot where you have to add an sub-optimal
interface/delegate to split chrome-vs-component.

I think the structure of the component is fine, and I'll LGTM this.

Some questions: Are you planning to move all of SafeBrowsingService and
ServicesDelegateImpl? How about the windows-only incident_reporting code?  Some
of that is fairly tightly coupled to Chrome. The DownloadProtectionService isn't
currently used on Android, but it will be soon.

https://codereview.chromium.org/2450993003/diff/140001/components/safe_browsi...
File components/safe_browsing/common/safebrowsing_constants.cc (right):

https://codereview.chromium.org/2450993003/diff/140001/components/safe_browsi...
components/safe_browsing/common/safebrowsing_constants.cc:10:
FILE_PATH_LITERAL("Safe Browsing");
On 2016/11/18 14:07:11, timvolodine wrote:
> On 2016/11/10 18:57:46, Nathan Parker wrote:
> > This won't be needed by WebView directly.  If it's easier to move some of
> these
> > rather than disentangle them, that's probably ok, but it would be cleaner to
> > leave the embedder data (file paths, switches) in /chrome/. 
> 
> safe_browsing_service uses that constant, and we'll need to move that file to
> component as well because safe_browsing_resource_throttle depends on it.

Digging into this, it's used by the SafeBrowisingServiceImpl (which I assume
does not need componentizing), but then also by the safe_browsing_cookie_store. 
The latter _may_ be used by the PingManager (which does need moving).  Some safe
browsing code that shares that safe_browsing_request_context doesn't set cookies
so it wouldn't need this, but we can tease that apart another time.  I think
this is fine.

[vakh (use Gerrit instead), 2016-11-18 22:16:18]

lgtm. thanks for doing this dirty work.

[timvolodine, 2016-11-21 16:36:37]

Description was changed from

==========
Componentize safe_browsing [1]: create component, move messages, constants and
switches.

Rationale:
Current safe browsing code lives in the chrome/ layer, but we
want to make it available in WebView as well (at least the
relevant parts of it). Hence this effort of making a safe
browsing component.

In this patch:
- move IPCs (spellcheck_messages) + add dedicated generator
- isolate and move safe browsing specific constants
- isolate and move safe browsing specific switches
- remove a few unused includes

BUG=
==========

to

==========
Componentize safe_browsing [1]: create component, move messages, constants and
switches.

Rationale:
Current safe browsing code lives in the chrome/ layer, but we
want to make it available in WebView as well (at least the
relevant parts of it). Hence this effort of making a safe
browsing component.

In this patch:
- move IPCs (spellcheck_messages) + add dedicated generator
- isolate and move safe browsing specific constants
- isolate and move safe browsing specific switches
- remove a few unused includes

BUG=488675
==========

[timvolodine, 2016-11-21 16:57:54]

On 2016/11/18 17:30:03, Nathan Parker wrote:
> lgtm
> 
> By just pulling on dependencies and moving code as you go, it seems to me you
> could easily end up in a spot where you have to add an sub-optimal
> interface/delegate to split chrome-vs-component.
> 
> I think the structure of the component is fine, and I'll LGTM this.
> 
> Some questions: Are you planning to move all of SafeBrowsingService and
> ServicesDelegateImpl? How about the windows-only incident_reporting code? 
Some
> of that is fairly tightly coupled to Chrome. The DownloadProtectionService
isn't
> currently used on Android, but it will be soon.

My original thought was to move the safebrowsingservice completely, but we would
likely need to have delegate to separate chrome-specific parts to be able to do
this. Don't know the details yet but potentially we could reuse the existing
delegate structure somewhat.
Windows-only code seems like low-priority to componentize, if possible maybe do
it rather as a follow-up at a later stage or so.
Good to know regarding DownloadProtectionService, this is the kind of things I
need some advice on..

> 
>
https://codereview.chromium.org/2450993003/diff/140001/components/safe_browsi...
> File components/safe_browsing/common/safebrowsing_constants.cc (right):
> 
>
https://codereview.chromium.org/2450993003/diff/140001/components/safe_browsi...
> components/safe_browsing/common/safebrowsing_constants.cc:10:
> FILE_PATH_LITERAL("Safe Browsing");
> On 2016/11/18 14:07:11, timvolodine wrote:
> > On 2016/11/10 18:57:46, Nathan Parker wrote:
> > > This won't be needed by WebView directly.  If it's easier to move some of
> > these
> > > rather than disentangle them, that's probably ok, but it would be cleaner
to
> > > leave the embedder data (file paths, switches) in /chrome/. 
> > 
> > safe_browsing_service uses that constant, and we'll need to move that file
to
> > component as well because safe_browsing_resource_throttle depends on it.
> 
> Digging into this, it's used by the SafeBrowisingServiceImpl (which I assume
> does not need componentizing), but then also by the
safe_browsing_cookie_store. 
> The latter _may_ be used by the PingManager (which does need moving).  Some
safe
> browsing code that shares that safe_browsing_request_context doesn't set
cookies
> so it wouldn't need this, but we can tease that apart another time.  I think
> this is fine.

[timvolodine, 2016-11-21 17:00:07]

timvolodine@chromium.org changed reviewers:
+ blundell@chromium.org, thakis@chromium.org, tsepez@chromium.org

[timvolodine, 2016-11-21 17:00:07]

+OWNERS for the remaining bits

+tsepez@: for safebrowsing_messages.h
+thakis@: for chrome/common, chrome/test
+blundell@: for components/

[Tom Sepez, 2016-11-21 18:26:39]

LGTM.  Also make sure to confirm that target ipc_fuzzer_all builds locally, bots
may skip that one to save time :(.

[blundell, 2016-11-22 16:48:47]

Tim, what do you need me to review?

[timvolodine, 2016-11-23 13:43:28]

On 2016/11/22 16:48:47, blundell wrote:
> Tim, what do you need me to review?

blundell@: basically hopefully rs for components/ since you are components/
owner.

[blundell, 2016-11-23 16:17:06]

Apologies for reviving this issue, but looking through the messages on this CL
wasn't sufficient for me to understand the plan wrt //components/safe_browsing,
//components/safe_browsing_db, and //components/security_interstitials. Could
you add a description of that plan to the CL description? Thanks!

[timvolodine, 2016-11-23 16:40:08]

Description was changed from

==========
Componentize safe_browsing [1]: create component, move messages, constants and
switches.

Rationale:
Current safe browsing code lives in the chrome/ layer, but we
want to make it available in WebView as well (at least the
relevant parts of it). Hence this effort of making a safe
browsing component.

In this patch:
- move IPCs (spellcheck_messages) + add dedicated generator
- isolate and move safe browsing specific constants
- isolate and move safe browsing specific switches
- remove a few unused includes

BUG=488675
==========

to

==========
Componentize safe_browsing [1]: create component, move messages, constants and
switches.

Rationale:
Current safe browsing code lives in the chrome/ layer, but we
want to make it available in WebView as well (at least the
relevant parts of it). Hence this effort of making a safe
browsing component.

In this patch:
- move IPCs (spellcheck_messages) + add dedicated generator
- isolate and move safe browsing specific constants
- isolate and move safe browsing specific switches
- remove a few unused includes

Note:
To reduce the number of components and keep everything
related in one place the idea is to move the code currently
living in components/safe_browsing_db/ to this safe_browsing/
component in the future.

BUG=488675
==========

[timvolodine, 2016-11-23 16:40:14]

On 2016/11/23 16:17:06, blundell wrote:
> Apologies for reviving this issue, but looking through the messages on this CL
> wasn't sufficient for me to understand the plan wrt
//components/safe_browsing,
> //components/safe_browsing_db, and //components/security_interstitials. Could
> you add a description of that plan to the CL description? Thanks!

safe_browsing_db will be potentially integrated into this component (currently
there is parallel work happening AFAIK).
security_interstitials is a separate effort that has happened before (AFAIK). I
have added a note to the description.

[Nico, 2016-11-24 00:42:43]

chrome/common and chrome/test lgtm (but you'll need blundell's approval to for
components/)

[blundell, 2016-11-24 15:23:01]

new component LGTM

As we discussed offline, I'd like to see the safe_browsing and safe_browsing_db
situation resolved -- ideally they should be folded into one component. Tim is
going to file a bug for that. Tim/Nathan, can you please make sure that that
happens so we don't have long-term weirdness with the existence of the two
separate components? Thanks!

[timvolodine, 2016-11-24 15:39:20]

On 2016/11/24 15:23:01, blundell wrote:
> new component LGTM
> 
> As we discussed offline, I'd like to see the safe_browsing and
safe_browsing_db
> situation resolved -- ideally they should be folded into one component. Tim is
> going to file a bug for that. Tim/Nathan, can you please make sure that that
> happens so we don't have long-term weirdness with the existence of the two
> separate components? Thanks!

Thanks Colin, filed crbug.com/668515 will follow-up there on the
safe_browsing_db situation.

[timvolodine, 2016-12-01 15:46:51]

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

[commit-bot: I haz the power, 2016-12-01 15:47:06]

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-01 16:12:02]

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

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

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

[timvolodine, 2016-12-01 16:14:23]

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

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

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

[Tom Sepez, 2016-12-01 16:40:02]

Messages LGTM, but be sure to manually build target ipc_fuzzer_all (not always
picked up by the bots due to performance concerns) when moving things around.

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

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

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

Dry run: This issue passed the CQ dry run.

[timvolodine, 2016-12-01 17:11:11]

On 2016/12/01 16:40:02, Tom Sepez wrote:
> Messages LGTM, but be sure to manually build target ipc_fuzzer_all (not always
> picked up by the bots due to performance concerns) when moving things around.

yes thanks Tom, built ipc_fuzzer_all locally -- no issues

[timvolodine, 2016-12-01 17:11:58]

Thanks for the reviews everybody!

[timvolodine, 2016-12-01 17:12:12]

The CQ bit was checked by timvolodine@chromium.org

[timvolodine, 2016-12-01 17:12:13]

The patchset sent to the CQ was uploaded after l-g-t-m from
nparker@chromium.org, vakh@chromium.org, thakis@chromium.org,
blundell@chromium.org
Link to the patchset: https://codereview.chromium.org/2450993003/#ps180001
(title: "fix compile")

[commit-bot: I haz the power, 2016-12-01 17:12:37]

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-01 17:20:39]

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

[commit-bot: I haz the power, 2016-12-01 17:20:41]

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

[timvolodine, 2016-12-01 17:22:14]

timvolodine@chromium.org changed reviewers:
+ brettw@chromium.org

[timvolodine, 2016-12-01 17:22:15]

+brettw@ : apparently for components/safe_browsing/common/DEPS +url dependency

[timvolodine, 2016-12-01 17:22:49]

Description was changed from

==========
Componentize safe_browsing [1]: create component, move messages, constants and
switches.

Rationale:
Current safe browsing code lives in the chrome/ layer, but we
want to make it available in WebView as well (at least the
relevant parts of it). Hence this effort of making a safe
browsing component.

In this patch:
- move IPCs (spellcheck_messages) + add dedicated generator
- isolate and move safe browsing specific constants
- isolate and move safe browsing specific switches
- remove a few unused includes

Note:
To reduce the number of components and keep everything
related in one place the idea is to move the code currently
living in components/safe_browsing_db/ to this safe_browsing/
component in the future.

BUG=488675
==========

to

==========
Componentize safe_browsing [1]: create component, move messages, constants and
switches.

Rationale:
Current safe browsing code lives in the chrome/ layer, but we
want to make it available in WebView as well (at least the
relevant parts of it). Hence this effort of making a safe
browsing component.

In this patch:
- move IPCs (spellcheck_messages) + add dedicated generator
- isolate and move safe browsing specific constants
- isolate and move safe browsing specific switches
- remove a few unused includes

Note:
To reduce the number of components and keep everything
related in one place the idea is to move the code currently
living in components/safe_browsing_db/ to this safe_browsing/
component in the future.

BUG=488675
TBR=brettw@chromium.org
==========

[timvolodine, 2016-12-01 17:24:13]

On 2016/12/01 17:22:15, timvolodine wrote:
> +brettw@ : apparently for components/safe_browsing/common/DEPS +url dependency
(actually TBR because presently OOO)

[timvolodine, 2016-12-01 17:24:25]

The CQ bit was checked by timvolodine@chromium.org

[commit-bot: I haz the power, 2016-12-01 17:24:47]

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

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1480613065713060,
"parent_rev": "28e6fbbc64bb81d6ff5f9e6f1667cf1ff3de3b54", "commit_rev":
"41bbb17b9364aad4da961c682096b72e13e76513"}

[commit-bot: I haz the power, 2016-12-01 17:30:05]

Description was changed from

==========
Componentize safe_browsing [1]: create component, move messages, constants and
switches.

Rationale:
Current safe browsing code lives in the chrome/ layer, but we
want to make it available in WebView as well (at least the
relevant parts of it). Hence this effort of making a safe
browsing component.

In this patch:
- move IPCs (spellcheck_messages) + add dedicated generator
- isolate and move safe browsing specific constants
- isolate and move safe browsing specific switches
- remove a few unused includes

Note:
To reduce the number of components and keep everything
related in one place the idea is to move the code currently
living in components/safe_browsing_db/ to this safe_browsing/
component in the future.

BUG=488675
TBR=brettw@chromium.org
==========

to

==========
Componentize safe_browsing [1]: create component, move messages, constants and
switches.

Rationale:
Current safe browsing code lives in the chrome/ layer, but we
want to make it available in WebView as well (at least the
relevant parts of it). Hence this effort of making a safe
browsing component.

In this patch:
- move IPCs (spellcheck_messages) + add dedicated generator
- isolate and move safe browsing specific constants
- isolate and move safe browsing specific switches
- remove a few unused includes

Note:
To reduce the number of components and keep everything
related in one place the idea is to move the code currently
living in components/safe_browsing_db/ to this safe_browsing/
component in the future.

BUG=488675
TBR=brettw@chromium.org
==========

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

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-12-01 17:32:13]

Description was changed from

==========
Componentize safe_browsing [1]: create component, move messages, constants and
switches.

Rationale:
Current safe browsing code lives in the chrome/ layer, but we
want to make it available in WebView as well (at least the
relevant parts of it). Hence this effort of making a safe
browsing component.

In this patch:
- move IPCs (spellcheck_messages) + add dedicated generator
- isolate and move safe browsing specific constants
- isolate and move safe browsing specific switches
- remove a few unused includes

Note:
To reduce the number of components and keep everything
related in one place the idea is to move the code currently
living in components/safe_browsing_db/ to this safe_browsing/
component in the future.

BUG=488675
TBR=brettw@chromium.org
==========

to

==========
Componentize safe_browsing [1]: create component, move messages, constants and
switches.

Rationale:
Current safe browsing code lives in the chrome/ layer, but we
want to make it available in WebView as well (at least the
relevant parts of it). Hence this effort of making a safe
browsing component.

In this patch:
- move IPCs (spellcheck_messages) + add dedicated generator
- isolate and move safe browsing specific constants
- isolate and move safe browsing specific switches
- remove a few unused includes

Note:
To reduce the number of components and keep everything
related in one place the idea is to move the code currently
living in components/safe_browsing_db/ to this safe_browsing/
component in the future.

BUG=488675
TBR=brettw@chromium.org

Committed: https://crrev.com/f30c94f1fa39ad0467a31ff99e7acc02142e6fa6
Cr-Commit-Position: refs/heads/master@{#435635}
==========

[commit-bot: I haz the power, 2016-12-01 17:32:15]

Patchset 10 (id:??) landed as
https://crrev.com/f30c94f1fa39ad0467a31ff99e7acc02142e6fa6
Cr-Commit-Position: refs/heads/master@{#435635}