Componentize safe_browsing [+1]: move protos to component

Componentize safe_browsing [+1]: move protos to component

Move the relevant chrome/common/safe_browsing/ proto files to
components/safe_browsing/common/. Update build files and
dependencies.

Also moves csd.proto to components/safe_browsing/common (after
crrev.com/2733343002) + update all references.

NOTE:
This patch is needed to move the safebrowsing renderer/ files
to component later, so needs to land before
https://codereview.chromium.org/2667343006/

BUG=700351
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

[timvolodine, 2017-03-10 12:31:31]

Description was changed from

==========
Componentize safe_browsing [+1]: move protos to component

Move chrome/common/safe_browsing/ proto files to
components/safe_browsing/common/.

NOTE:
This patch is needed to move the renderer files to components
later, so needs to land before
https://codereview.chromium.org/2667343006/

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

to

==========
Componentize safe_browsing [+1]: move protos to component

Move chrome/common/safe_browsing/ proto files to
components/safe_browsing/common/.

NOTE:
This patch is needed to move the renderer files to components
later, so needs to land before
https://codereview.chromium.org/2667343006/

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[timvolodine, 2017-03-10 12:34:30]

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

[timvolodine, 2017-03-10 12:37:14]

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

[commit-bot: I haz the power, 2017-03-10 12:37:24]

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

[commit-bot: I haz the power, 2017-03-10 12:41:03]

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

[commit-bot: I haz the power, 2017-03-10 12:41:04]

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

[Nathan Parker, 2017-03-10 17:21:30]

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

[Nathan Parker, 2017-03-10 17:21:31]

I think Jialiul's https://codereview.chromium.org/2733343002/ already moves the
protos needed for reporting.  Tim what bug is this attached to? Let's sync up
today to ensure we're not duplicating work!

[Jialiu Lin, 2017-03-10 17:27:13]

On 2017/03/10 17:21:31, Nathan Parker wrote:
> I think Jialiul's https://codereview.chromium.org/2733343002/ already moves
the
> protos needed for reporting.  Tim what bug is this attached to? Let's sync up
> today to ensure we're not duplicating work!

My CL landed yesterday. I only moved csd.proto, since it is also needed for
another project.

[timvolodine, 2017-03-10 18:11:56]

On 2017/03/10 17:27:13, Jialiu Lin wrote:
> On 2017/03/10 17:21:31, Nathan Parker wrote:
> > I think Jialiul's https://codereview.chromium.org/2733343002/ already moves
> the
> > protos needed for reporting.  Tim what bug is this attached to? Let's sync
up
> > today to ensure we're not duplicating work!
> 
> My CL landed yesterday. I only moved csd.proto, since it is also needed for
> another project.

oh I see, I am not cc'd on that bug so I didn't know this happened. I was
assuming I was working on that part since:
https://codereview.chromium.org/2667343006/ and also mentioned in the roadmap.
Please cc me in the future to avoid this situation.

jialiul@, nparker@:
For the renderer/ side patch we need more than only csd.proto. Looking at the
bug jialiul's patch has been reverted. Probably the way forward is to reland and
I will rebase this patch on top.

[timvolodine, 2017-03-10 18:13:01]

+cc:sgurun@ FYI

[timvolodine, 2017-03-10 18:22:31]

Description was changed from

==========
Componentize safe_browsing [+1]: move protos to component

Move chrome/common/safe_browsing/ proto files to
components/safe_browsing/common/.

NOTE:
This patch is needed to move the renderer files to components
later, so needs to land before
https://codereview.chromium.org/2667343006/

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Componentize safe_browsing [+1]: move protos to component

Move chrome/common/safe_browsing/ proto files to
components/safe_browsing/common/.

NOTE:
This patch is needed to move the renderer files to components
later, so needs to land before
https://codereview.chromium.org/2667343006/

BUG=700351
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[timvolodine, 2017-03-10 18:24:27]

On 2017/03/10 17:21:31, Nathan Parker wrote:
> I think Jialiul's https://codereview.chromium.org/2733343002/ already moves
the
> protos needed for reporting.  Tim what bug is this attached to? Let's sync up
> today to ensure we're not duplicating work!

this is crbug.com/700351, looks like need syncing with crbug.com/688629 (which I
was cc'd on) ;)

[timvolodine, 2017-03-10 18:38:15]

On 2017/03/10 18:24:27, timvolodine wrote:
> On 2017/03/10 17:21:31, Nathan Parker wrote:
> > I think Jialiul's https://codereview.chromium.org/2733343002/ already moves
> the
> > protos needed for reporting.  Tim what bug is this attached to? Let's sync
up
> > today to ensure we're not duplicating work!
> 
> this is crbug.com/700351, looks like need syncing with crbug.com/688629 (which
I
> was cc'd on) ;)

err correction: was -> wasn't

[Jialiu Lin, 2017-03-10 19:27:02]

On 2017/03/10 18:11:56, timvolodine wrote:
> On 2017/03/10 17:27:13, Jialiu Lin wrote:
> > On 2017/03/10 17:21:31, Nathan Parker wrote:
> > > I think Jialiul's https://codereview.chromium.org/2733343002/ already
moves
> > the
> > > protos needed for reporting.  Tim what bug is this attached to? Let's sync
> up
> > > today to ensure we're not duplicating work!
> > 
> > My CL landed yesterday. I only moved csd.proto, since it is also needed for
> > another project.
> 
> oh I see, I am not cc'd on that bug so I didn't know this happened. I was
> assuming I was working on that part since:
> https://codereview.chromium.org/2667343006/ and also mentioned in the roadmap.
> Please cc me in the future to avoid this situation.
> 
> jialiul@, nparker@:
> For the renderer/ side patch we need more than only csd.proto. Looking at the
> bug jialiul's patch has been reverted. Probably the way forward is to reland
and
> I will rebase this patch on top.
Hi timvolodine@,
Really sorry about this. I just added myself as reviewer to all the CLs in
crbug.com/700351, so I can keep track of them. 
I re-landed https://codereview.chromium.org/2733343002/ yesterday.
Also I commented on https://crbug.com/700351 to explain why some file path under

c/b/safe_browsing/ don't need to be moved. Please feel free to ping me if you
have
any questions or concerns.

[timvolodine, 2017-03-10 19:37:12]

On 2017/03/10 19:27:02, Jialiu Lin wrote:
> On 2017/03/10 18:11:56, timvolodine wrote:
> > On 2017/03/10 17:27:13, Jialiu Lin wrote:
> > > On 2017/03/10 17:21:31, Nathan Parker wrote:
> > > > I think Jialiul's https://codereview.chromium.org/2733343002/ already
> moves
> > > the
> > > > protos needed for reporting.  Tim what bug is this attached to? Let's
sync
> > up
> > > > today to ensure we're not duplicating work!
> > > 
> > > My CL landed yesterday. I only moved csd.proto, since it is also needed
for
> > > another project.
> > 
> > oh I see, I am not cc'd on that bug so I didn't know this happened. I was
> > assuming I was working on that part since:
> > https://codereview.chromium.org/2667343006/ and also mentioned in the
roadmap.
> > Please cc me in the future to avoid this situation.
> > 
> > jialiul@, nparker@:
> > For the renderer/ side patch we need more than only csd.proto. Looking at
the
> > bug jialiul's patch has been reverted. Probably the way forward is to reland
> and
> > I will rebase this patch on top.
> Hi timvolodine@,
> Really sorry about this. I just added myself as reviewer to all the CLs in
> crbug.com/700351, so I can keep track of them. 
> I re-landed https://codereview.chromium.org/2733343002/ yesterday.
> Also I commented on https://crbug.com/700351 to explain why some file path
under
> 
> c/b/safe_browsing/ don't need to be moved. Please feel free to ping me if you
> have
> any questions or concerns.

no worries, thanks for the follow-up. I'll rebase this patch, so that we can
cleanly land the renderer/ side. Thanks for the comments on crbug.com/700351,
sometimes I think it's better to move all files (if not too complicated) for the
sake of having them in one place, which is easier to manage..

[timvolodine, 2017-03-13 15:47:57]

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

[commit-bot: I haz the power, 2017-03-13 15:48:16]

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

[timvolodine, 2017-03-13 15:50:23]

Description was changed from

==========
Componentize safe_browsing [+1]: move protos to component

Move chrome/common/safe_browsing/ proto files to
components/safe_browsing/common/.

NOTE:
This patch is needed to move the renderer files to components
later, so needs to land before
https://codereview.chromium.org/2667343006/

BUG=700351
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Componentize safe_browsing [+1]: move protos to component

Move the relevant chrome/common/safe_browsing/ proto files
to components/safe_browsing/common/.

Also moves csd.proto to components/safe_browsing/common
after crrev.com/2733343002.

NOTE:
This patch is needed to move the renderer files to components
later, so needs to land before
https://codereview.chromium.org/2667343006/

BUG=700351
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[commit-bot: I haz the power, 2017-03-13 15:52:55]

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

[commit-bot: I haz the power, 2017-03-13 15:52:57]

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_...)
  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_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[timvolodine, 2017-03-13 16:18:26]

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

[commit-bot: I haz the power, 2017-03-13 16:19:01]

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

[commit-bot: I haz the power, 2017-03-13 16:24:02]

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

[commit-bot: I haz the power, 2017-03-13 16:24:04]

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_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_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, 2017-03-13 16:41:29]

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

[commit-bot: I haz the power, 2017-03-13 16:41:59]

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

[commit-bot: I haz the power, 2017-03-13 16:48:14]

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

[commit-bot: I haz the power, 2017-03-13 16:48:15]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_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_...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[timvolodine, 2017-03-13 16:56:17]

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

[commit-bot: I haz the power, 2017-03-13 16:56:42]

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

[timvolodine, 2017-03-13 17:57:40]

Description was changed from

==========
Componentize safe_browsing [+1]: move protos to component

Move the relevant chrome/common/safe_browsing/ proto files
to components/safe_browsing/common/.

Also moves csd.proto to components/safe_browsing/common
after crrev.com/2733343002.

NOTE:
This patch is needed to move the renderer files to components
later, so needs to land before
https://codereview.chromium.org/2667343006/

BUG=700351
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Componentize safe_browsing [+1]: move protos to component

Move the relevant chrome/common/safe_browsing/ proto files to
components/safe_browsing/common/. Update build files and
dependencies.

Also moves csd.proto to components/safe_browsing/common (after
crrev.com/2733343002) + update all references.

NOTE:
This patch is needed to move the safebrowsing renderer/ files
to component later, so needs to land before
https://codereview.chromium.org/2667343006/

BUG=700351
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[timvolodine, 2017-03-13 18:00:35]

Rebased the patch, moved the protos to common/. Bots seem to be happy. Please
take a look.

[commit-bot: I haz the power, 2017-03-13 18:14:33]

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

[commit-bot: I haz the power, 2017-03-13 18:14:34]

Dry run: This issue passed the CQ dry run.

[Jialiu Lin, 2017-03-13 18:31:12]

Thanks for compiling this big CL! That's a lot of effort.

I'm OK with moving csd.proto to components/safe_browsing/common. 
For other protos, I don't feel strongly about moving them since their calling
functions are not needed to be componentized.

For example, client_model.proto is only used by Client Phishing Detection, which
is only triggered on Desktop Chrome and will be deprecated soon.
permission_report.proto is used by permission reporting functionality, which is
called inside chrome/browser/permissions/ folder  (WebView does not have
'permission', right?).
Similarly, crx_info.proto needed by chrome/browser/extensions/, which is not
relevant to WebView either.

nparker@, vahk@, WDYT?

[timvolodine, 2017-03-13 18:35:50]

On 2017/03/13 18:31:12, Jialiu Lin wrote:
> Thanks for compiling this big CL! That's a lot of effort.
> 
> I'm OK with moving csd.proto to components/safe_browsing/common. 
> For other protos, I don't feel strongly about moving them since their calling
> functions are not needed to be componentized.
> 
> For example, client_model.proto is only used by Client Phishing Detection,
which
> is only triggered on Desktop Chrome and will be deprecated soon.
> permission_report.proto is used by permission reporting functionality, which
is
> called inside chrome/browser/permissions/ folder  (WebView does not have
> 'permission', right?).
> Similarly, crx_info.proto needed by chrome/browser/extensions/, which is not
> relevant to WebView either.
> 
> nparker@, vahk@, WDYT?

I think the protos are best kept in one place as much as possible + that allows
us to subsequently move the whole renderer/ side of safe_browsing which is much
cleaner.

[Jialiu Lin, 2017-03-13 18:40:01]

On 2017/03/13 18:35:50, timvolodine wrote:
> On 2017/03/13 18:31:12, Jialiu Lin wrote:
> > Thanks for compiling this big CL! That's a lot of effort.
> > 
> > I'm OK with moving csd.proto to components/safe_browsing/common. 
> > For other protos, I don't feel strongly about moving them since their
calling
> > functions are not needed to be componentized.
> > 
> > For example, client_model.proto is only used by Client Phishing Detection,
> which
> > is only triggered on Desktop Chrome and will be deprecated soon.
> > permission_report.proto is used by permission reporting functionality, which
> is
> > called inside chrome/browser/permissions/ folder  (WebView does not have
> > 'permission', right?).
> > Similarly, crx_info.proto needed by chrome/browser/extensions/, which is not
> > relevant to WebView either.
> > 
> > nparker@, vahk@, WDYT?
> 
> I think the protos are best kept in one place as much as possible + that
allows
> us to subsequently move the whole renderer/ side of safe_browsing which is
much
> cleaner.

Inside chrome/renderer/safe_browsing folder, you probably only need to move
threat_dom_details* (threat_dom_details* only depend on csd.proto).
All the other files are related to client phishing detection,
which will be deprecated very soon.

[Jialiu Lin, 2017-03-13 18:57:54]

BTW, all trybots do incremental build. This masks the compilation issue caused
by this CL.
On your local machine, before you do ninja build, please manually remove
out/Default/gen/components/safe_browsing/csd.pb.*. You'll see a number of
compilation errors.

https://codereview.chromium.org/2743563006/diff/80001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/client_side_detection_host_unittest.cc (left):

https://codereview.chromium.org/2743563006/diff/80001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/client_side_detection_host_unittest.cc:25: #include
"components/safe_browsing/csd.pb.h"
remove #include "components/safe_browsing/csd.pb.h"?

https://codereview.chromium.org/2743563006/diff/80001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/client_side_model_loader.cc (left):

https://codereview.chromium.org/2743563006/diff/80001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/client_side_model_loader.cc:23: #include
"components/safe_browsing/csd.pb.h"
remove this line?

https://codereview.chromium.org/2743563006/diff/80001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/download_protection_service.cc (right):

https://codereview.chromium.org/2743563006/diff/80001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/download_protection_service.cc:58: #include
"components/safe_browsing/csd.pb.h"
Remove "#include "components/safe_browsing/csd.pb.h"?

https://codereview.chromium.org/2743563006/diff/80001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/threat_details_unittest.cc (left):

https://codereview.chromium.org/2743563006/diff/80001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/threat_details_unittest.cc:26: #include
"components/safe_browsing/csd.pb.h"
remove?

[timvolodine, 2017-03-13 20:08:44]

Yes, thanks for catching this! I was compiling for android (after a merge) and
hence didn't notice it.

https://codereview.chromium.org/2743563006/diff/80001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/client_side_detection_host_unittest.cc (left):

https://codereview.chromium.org/2743563006/diff/80001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/client_side_detection_host_unittest.cc:25: #include
"components/safe_browsing/csd.pb.h"
On 2017/03/13 18:57:54, Jialiu Lin wrote:
> remove #include "components/safe_browsing/csd.pb.h"?

Done.

https://codereview.chromium.org/2743563006/diff/80001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/client_side_model_loader.cc (left):

https://codereview.chromium.org/2743563006/diff/80001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/client_side_model_loader.cc:23: #include
"components/safe_browsing/csd.pb.h"
On 2017/03/13 18:57:54, Jialiu Lin wrote:
> remove this line?

Done.

https://codereview.chromium.org/2743563006/diff/80001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/download_protection_service.cc (right):

https://codereview.chromium.org/2743563006/diff/80001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/download_protection_service.cc:58: #include
"components/safe_browsing/csd.pb.h"
On 2017/03/13 18:57:54, Jialiu Lin wrote:
> Remove "#include "components/safe_browsing/csd.pb.h"?

Done.

https://codereview.chromium.org/2743563006/diff/80001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/threat_details_unittest.cc (left):

https://codereview.chromium.org/2743563006/diff/80001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/threat_details_unittest.cc:26: #include
"components/safe_browsing/csd.pb.h"
On 2017/03/13 18:57:54, Jialiu Lin wrote:
> remove?

Done.

[timvolodine, 2017-03-13 20:08:48]

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

[commit-bot: I haz the power, 2017-03-13 20:09:40]

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

[Jialiu Lin, 2017-03-13 20:31:42]

grep -R "components/safe_browsing/csd.pb.h"
chrome/renderer/safe_browsing/phishing_classifier_delegate_browsertest.cc:#include
"components/safe_browsing/csd.pb.h"
chrome/renderer/safe_browsing/DEPS:  "+components/safe_browsing/csd.pb.h",
chrome/renderer/safe_browsing/phishing_classifier_delegate.cc:#include
"components/safe_browsing/csd.pb.h"
chrome/browser/download/DEPS:  "+components/safe_browsing/csd.pb.h",
chrome/browser/safe_browsing/download_protection_service_unittest.cc:#include
"components/safe_browsing/csd.pb.h"
chrome/browser/safe_browsing/client_side_detection_host.cc:#include
"components/safe_browsing/csd.pb.h"
chrome/browser/safe_browsing/client_side_detection_service.cc:#include
"components/safe_browsing/csd.pb.h"
chrome/utility/safe_browsing/mac/DEPS:  "+components/safe_browsing/csd.pb.h",
chrome/tools/safe_browsing/DEPS:  "+components/safe_browsing/csd.pb.h"


You might want to do the same thing for the other moved protos. Alternatively
(more safely), remove all the generated proto files (you can find them in
out/Default/gen/chrome/common/safe_browsing/, replace "Default" with your own
ninja target), and compile everything to see if there is any compilation error.
Trybots are not reliable if you move protos.

https://codereview.chromium.org/2743563006/diff/100001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/client_side_detection_service.cc (right):

https://codereview.chromium.org/2743563006/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/client_side_detection_service.cc:28: #include
"components/safe_browsing/csd.pb.h"
one more?

[timvolodine, 2017-03-13 20:37:08]

On 2017/03/13 20:31:42, Jialiu Lin wrote:
> grep -R "components/safe_browsing/csd.pb.h"
>
chrome/renderer/safe_browsing/phishing_classifier_delegate_browsertest.cc:#include
> "components/safe_browsing/csd.pb.h"
> chrome/renderer/safe_browsing/DEPS:  "+components/safe_browsing/csd.pb.h",
> chrome/renderer/safe_browsing/phishing_classifier_delegate.cc:#include
> "components/safe_browsing/csd.pb.h"
> chrome/browser/download/DEPS:  "+components/safe_browsing/csd.pb.h",
> chrome/browser/safe_browsing/download_protection_service_unittest.cc:#include
> "components/safe_browsing/csd.pb.h"
> chrome/browser/safe_browsing/client_side_detection_host.cc:#include
> "components/safe_browsing/csd.pb.h"
> chrome/browser/safe_browsing/client_side_detection_service.cc:#include
> "components/safe_browsing/csd.pb.h"
> chrome/utility/safe_browsing/mac/DEPS:  "+components/safe_browsing/csd.pb.h",
> chrome/tools/safe_browsing/DEPS:  "+components/safe_browsing/csd.pb.h"
> 
> 
> You might want to do the same thing for the other moved protos. Alternatively
> (more safely), remove all the generated proto files (you can find them in
> out/Default/gen/chrome/common/safe_browsing/, replace "Default" with your own
> ninja target), and compile everything to see if there is any compilation
error.
> Trybots are not reliable if you move protos.
> 
>
https://codereview.chromium.org/2743563006/diff/100001/chrome/browser/safe_br...
> File chrome/browser/safe_browsing/client_side_detection_service.cc (right):
> 
>
https://codereview.chromium.org/2743563006/diff/100001/chrome/browser/safe_br...
> chrome/browser/safe_browsing/client_side_detection_service.cc:28: #include
> "components/safe_browsing/csd.pb.h"
> one more?

right, will re-check this with a clean linux compile (instead of android), sorry
seems like I was a bit to hasty on this )

[commit-bot: I haz the power, 2017-03-13 21:42:42]

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

[commit-bot: I haz the power, 2017-03-13 21:42:44]

Dry run: This issue passed the CQ dry run.

[timvolodine, 2017-03-14 23:28:22]

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

[commit-bot: I haz the power, 2017-03-14 23:29:28]

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

[timvolodine, 2017-03-15 00:02:01]

Thanks for the comments Jialiu,
recompiled on linux cleanly from scratch, fixed the remaining references:

$ grep -R --exclude-dir=third_party "components/safe_browsing/csd.pb.h" | wc -c
0
$ grep -R --exclude-dir=third_party "safe_browsing:csd_proto" | wc -c
0

let me know if you think anything else needs checking
PTAL

https://codereview.chromium.org/2743563006/diff/100001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/client_side_detection_service.cc (right):

https://codereview.chromium.org/2743563006/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/client_side_detection_service.cc:28: #include
"components/safe_browsing/csd.pb.h"
On 2017/03/13 20:31:42, Jialiu Lin wrote:
> one more?

Done.

[commit-bot: I haz the power, 2017-03-15 01:08:46]

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

[commit-bot: I haz the power, 2017-03-15 01:08:47]

Dry run: This issue passed the CQ dry run.

[Jialiu Lin, 2017-03-15 01:09:29]

Thanks!
nparker@, vakh@, let me know if you have any strong opinion of whether we should
move csd.protos only or we should move all the remaining protos. 

And timvolodine@, maybe I missed it, I don't see 
chrome/common/safe_browsing/download_file_types.proto in this CL. Any reason you
left this proto out? Let's wait for nparker@ and vakh@'s reply before we make
further change to this CL.

[timvolodine, 2017-03-15 12:23:32]

On 2017/03/15 01:09:29, Jialiu Lin wrote:
> Thanks!
> nparker@, vakh@, let me know if you have any strong opinion of whether we
should
> move csd.protos only or we should move all the remaining protos. 
> 
> And timvolodine@, maybe I missed it, I don't see 
> chrome/common/safe_browsing/download_file_types.proto in this CL. Any reason
you
> left this proto out? Let's wait for nparker@ and vakh@'s reply before we make
> further change to this CL.

correct, the download_file_types.proto is used in a script
src/chrome/browser/resources/safe_browsing/gen_file_type_proto.py which I
thought would best be done in a separate patch, as this one is pretty large
already..

[Nathan Parker, 2017-03-15 21:23:01]

On 2017/03/15 12:23:32, timvolodine wrote:
> On 2017/03/15 01:09:29, Jialiu Lin wrote:
> > Thanks!
> > nparker@, vakh@, let me know if you have any strong opinion of whether we
> should
> > move csd.protos only or we should move all the remaining protos. 
> > 
> > And timvolodine@, maybe I missed it, I don't see 
> > chrome/common/safe_browsing/download_file_types.proto in this CL. Any reason
> you
> > left this proto out? Let's wait for nparker@ and vakh@'s reply before we
make
> > further change to this CL.
> 
> correct, the download_file_types.proto is used in a script
> src/chrome/browser/resources/safe_browsing/gen_file_type_proto.py which I
> thought would best be done in a separate patch, as this one is pretty large
> already..

Hey Tim -- I think we're still at a point where the changes in this CL, a)
aren't necessary for WebView, and b) aren't moving us towards a stated long term
goal.  We have no plans to componentize the code that uses these protos, and
moving them to a component actually expands their scope since they can become
dependencies of other non- chrome/ code, whereas now they are more isolated.  Is
there another goal I've missed?  Thanks for your patience.

 -- Nathan

[vakh (use Gerrit instead), 2017-03-15 21:23:29]

- I am not sure what's the advantage of moving these protos to the components
when they are being used strictly inside chrome/browser/safe_browsing or
chrome/renderer/...
  - The common practice is to have the protos defined close to where they are
being used so I think we should not move them i.e. in the same directory or in a
protos/ child directory.
  - Unless, we are also planning to move the code that uses these protos to the
component. Are we?

- The component/safe_browsing/common/... directory contains benign stuff not
specific to any feature. It doesn't seem like the right place to put a proto.
Can you please explain why we are moving csd.proto there?

[timvolodine, 2017-03-16 12:31:01]

On 2017/03/15 21:23:29, vakh (Varun Khaneja) wrote:
> - I am not sure what's the advantage of moving these protos to the components
> when they are being used strictly inside chrome/browser/safe_browsing or
> chrome/renderer/...
>   - The common practice is to have the protos defined close to where they are
> being used so I think we should not move them i.e. in the same directory or in
a
> protos/ child directory.
>   - Unless, we are also planning to move the code that uses these protos to
the
> component. Are we?

Thanks for the comments Nathan and Varun. Below some more context for this
patch.

The idea of this patch is to allow https://codereview.chromium.org/2667343006/
to land. That patch moves the renderer files to component with the effect of
providing support for reporting in WebView. It's possible I took a slightly
bigger slice than strictly needed for WebView, however it seems logical and in
the general 'spirit' of componentization to group related files where possible.

Hopefully this clarifies things a bit. Let me know what you think.

> 
> - The component/safe_browsing/common/... directory contains benign stuff not
> specific to any feature. It doesn't seem like the right place to put a proto.
> Can you please explain why we are moving csd.proto there?

The original protos live in chrome/common/safe_browsing/ and by analogy move to
components/safe_browsing/common/.

[Jialiu Lin, 2017-03-16 20:19:53]

On 2017/03/16 12:31:01, timvolodine wrote:
> On 2017/03/15 21:23:29, vakh (Varun Khaneja) wrote:
> > - I am not sure what's the advantage of moving these protos to the
components
> > when they are being used strictly inside chrome/browser/safe_browsing or
> > chrome/renderer/...
> >   - The common practice is to have the protos defined close to where they
are
> > being used so I think we should not move them i.e. in the same directory or
in
> a
> > protos/ child directory.
> >   - Unless, we are also planning to move the code that uses these protos to
> the
> > component. Are we?
> 
> Thanks for the comments Nathan and Varun. Below some more context for this
> patch.
> 
> The idea of this patch is to allow https://codereview.chromium.org/2667343006/
> to land. That patch moves the renderer files to component with the effect of
> providing support for reporting in WebView. It's possible I took a slightly
> bigger slice than strictly needed for WebView, however it seems logical and in
> the general 'spirit' of componentization to group related files where
possible.
> 
> Hopefully this clarifies things a bit. Let me know what you think.
As I replied in #39, a large portion of chrome/renderer/safe_browsing/* are
going to be deprecated soon, and they are not used by non-desktop chrome. You
don't need to move them.
The general 'spirit' of componentization is to promote code reuse, and make code
maintenance easier. Moving additional proto seems have very limited effect on
both.

> 
> > 
> > - The component/safe_browsing/common/... directory contains benign stuff not
> > specific to any feature. It doesn't seem like the right place to put a
proto.
> > Can you please explain why we are moving csd.proto there?
> 
> The original protos live in chrome/common/safe_browsing/ and by analogy move
to
> components/safe_browsing/common/.
I agree with vakh@ here, csd.proto is very functional specific. Its previous
location was not ideal.
That's why I moved it directly into components/safe_browsing instead of
c/safe_browsing/common directory. 


Let's talk more about this componentization plan in tomorrow's meeting.

[timvolodine, 2017-03-17 12:46:41]

On 2017/03/16 20:19:53, Jialiu Lin wrote:
> On 2017/03/16 12:31:01, timvolodine wrote:
> > On 2017/03/15 21:23:29, vakh (Varun Khaneja) wrote:
> > > - I am not sure what's the advantage of moving these protos to the
> components
> > > when they are being used strictly inside chrome/browser/safe_browsing or
> > > chrome/renderer/...
> > >   - The common practice is to have the protos defined close to where they
> are
> > > being used so I think we should not move them i.e. in the same directory
or
> in
> > a
> > > protos/ child directory.
> > >   - Unless, we are also planning to move the code that uses these protos
to
> > the
> > > component. Are we?
> > 
> > Thanks for the comments Nathan and Varun. Below some more context for this
> > patch.
> > 
> > The idea of this patch is to allow
https://codereview.chromium.org/2667343006/
> > to land. That patch moves the renderer files to component with the effect of
> > providing support for reporting in WebView. It's possible I took a slightly
> > bigger slice than strictly needed for WebView, however it seems logical and
in
> > the general 'spirit' of componentization to group related files where
> possible.
> > 
> > Hopefully this clarifies things a bit. Let me know what you think.

> As I replied in #39, a large portion of chrome/renderer/safe_browsing/* are
> going to be deprecated soon, and they are not used by non-desktop chrome. You
> don't need to move them.

Can the deprecation happen after the move as well? I don't think we should block
on something because it might be deprecated in the future.

> The general 'spirit' of componentization is to promote code reuse, and make
code
> maintenance easier. Moving additional proto seems have very limited effect on
> both.

Exactly, both proto reuse (from component by webview) and maintenance (code and
protos in one place) benefit from this, no?

Generally speaking components tend to be self-contained i.e. they can have code
for all platforms, not only for android WebView. Examples
components/autofill,bookmarks,sync,os_crypt, ...

> 
> > 
> > > 
> > > - The component/safe_browsing/common/... directory contains benign stuff
not
> > > specific to any feature. It doesn't seem like the right place to put a
> proto.
> > > Can you please explain why we are moving csd.proto there?
> > 
> > The original protos live in chrome/common/safe_browsing/ and by analogy move
> to
> > components/safe_browsing/common/.
> I agree with vakh@ here, csd.proto is very functional specific. Its previous
> location was not ideal.
> That's why I moved it directly into components/safe_browsing instead of
> c/safe_browsing/common directory. 

You mean previous location of csd.proto with the other protos (which seems
logical) was in the wrong place? what about the other protos are they in the
right place?

> 
> 
> Let's talk more about this componentization plan in tomorrow's meeting.

Sure let's discuss,
It is not clear to me what the concerns are exactly. I thought this was supposed
to be relatively straightforward..

[vakh (use Gerrit instead), 2017-03-20 18:38:13]

Please correct me if I'm wrong but it looks like this CL won't be worked on
anymore.
If that's the case, please consider removing the reviewers.
If that's not the case, please ignore this message :)