Componentize SafeBrowsingApi{Bridge,Handler}

Componentize SafeBrowsingApi{Bridge,Handler}

Move these classes into components/safe_browsing_db/ and update references

BUG=679018
Review-Url: https://codereview.chromium.org/2622773002
Cr-Commit-Position: refs/heads/master@{#443116}
Committed: https://chromium.googlesource.com/chromium/src/+/52f0c6086c1a89e3a294855e4ced218cf74991bc

[Nate Fischer, 2017-01-10 00:39:02]

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

[commit-bot: I haz the power, 2017-01-10 00:39:50]

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-01-10 01:38:16]

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

[commit-bot: I haz the power, 2017-01-10 01:38:16]

Dry run: Try jobs failed on following builders:
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[Nate Fischer, 2017-01-10 04:23:47]

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

[commit-bot: I haz the power, 2017-01-10 04:24:23]

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-01-10 05:07:55]

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

[commit-bot: I haz the power, 2017-01-10 05:07:56]

Dry run: Try jobs failed on following builders:
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[Nate Fischer, 2017-01-10 23:50:45]

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

[commit-bot: I haz the power, 2017-01-10 23:51: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, 2017-01-11 00:43:57]

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

[commit-bot: I haz the power, 2017-01-11 00:43:58]

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

[Nate Fischer, 2017-01-11 02:58:33]

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

[commit-bot: I haz the power, 2017-01-11 02:59:02]

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

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

[commit-bot: I haz the power, 2017-01-11 03:01:32]

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...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Nate Fischer, 2017-01-11 03:41:07]

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

[commit-bot: I haz the power, 2017-01-11 03:41:38]

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

[Nate Fischer, 2017-01-11 04:19:33]

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

[Nate Fischer, 2017-01-11 04:27:48]

Jialiu,

No change in overall logic. This moves SafeBrowsingApi{Bridge,Handler} into the
components layer. These classes did not depend on chrome layer, so the move
itself is pretty straight forward.

I added some files to deal with JNI stuff. linux_android_rel_ng &
android_n5x_swarming_rel were previously the only bots failing, so hopefully the
latest patch fixed that issue. If not, I'll take another look and see if I can
fix it.

The rest is adjusting BUILD files, includes, and references.

Thanks!

[commit-bot: I haz the power, 2017-01-11 05:14:28]

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

[commit-bot: I haz the power, 2017-01-11 05:14:29]

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

[Jialiu Lin, 2017-01-11 18:17:56]

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

[Jialiu Lin, 2017-01-11 18:17:56]

Looking good. 
I'll defer to vakh@ for owner review. I'm not too familiar with Clank stuff...

Also +cc nparker@ if he has additional comments.

[vakh (use Gerrit instead), 2017-01-11 22:31:47]

Minor nits. Otherwise lgtm.

https://codereview.chromium.org/2622773002/diff/100001/components/safe_browsi...
File components/safe_browsing_db/android/BUILD.gn (right):

https://codereview.chromium.org/2622773002/diff/100001/components/safe_browsi...
components/safe_browsing_db/android/BUILD.gn:3: # found in the LICENSE file.
nit: empty line here

https://codereview.chromium.org/2622773002/diff/100001/components/safe_browsi...
File
components/safe_browsing_db/android/java/src/org/chromium/components/safe_browsing/SafeBrowsingApiBridge.java
(right):

https://codereview.chromium.org/2622773002/diff/100001/components/safe_browsi...
components/safe_browsing_db/android/java/src/org/chromium/components/safe_browsing/SafeBrowsingApiBridge.java:17:
public final class SafeBrowsingApiBridge {
Since this file (and therefore, class) is in components/safe_browsing anyway, I
think we should drop "SafeBrowsing" from the name and call it:
ApiBridge.

https://codereview.chromium.org/2622773002/diff/100001/components/safe_browsi...
components/safe_browsing_db/android/java/src/org/chromium/components/safe_browsing/SafeBrowsingApiBridge.java:18:
private static final String TAG = "SafeBrowsingApi";
TAG = "ApiBridge"

https://codereview.chromium.org/2622773002/diff/100001/components/safe_browsi...
components/safe_browsing_db/android/java/src/org/chromium/components/safe_browsing/SafeBrowsingApiBridge.java:60:
SafeBrowsingApiHandler handler, long callbackId, String uri, int[]
threatsOfInterest) {
Similarly: ApiHandler here

[Nathan Parker, 2017-01-11 22:53:30]

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

[Nathan Parker, 2017-01-11 22:53:31]

lgtm

https://codereview.chromium.org/2622773002/diff/100001/chrome/browser/android...
File chrome/browser/android/chrome_jni_registrar.cc (left):

https://codereview.chromium.org/2622773002/diff/100001/chrome/browser/android...
chrome/browser/android/chrome_jni_registrar.cc:373: {"SafeBrowsingApiBridge",
safe_browsing::RegisterSafeBrowsingApiBridge},
What's the replacement for this?

https://codereview.chromium.org/2622773002/diff/100001/components/safe_browsi...
File
components/safe_browsing_db/android/java/src/org/chromium/components/safe_browsing/SafeBrowsingApiBridge.java
(right):

https://codereview.chromium.org/2622773002/diff/100001/components/safe_browsi...
components/safe_browsing_db/android/java/src/org/chromium/components/safe_browsing/SafeBrowsingApiBridge.java:17:
public final class SafeBrowsingApiBridge {
On 2017/01/11 22:31:47, vakh (Varun Khaneja) wrote:
> Since this file (and therefore, class) is in components/safe_browsing anyway,
I
> think we should drop "SafeBrowsing" from the name and call it:
> ApiBridge.

ApiBridge is kind of a weird name to begin with, since it doesn't tell which
API.  Maybe PlatformCheckApiBridge?  Not sure.

We may want to do all the renaming in a separate CL, so this is just moving
things around.

[Nathan Parker, 2017-01-11 22:54:01]

Is it standard to have "/components/" in the path twice for .java files in
components?

[Nate Fischer, 2017-01-11 23:02:13]

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

[commit-bot: I haz the power, 2017-01-11 23:03:41]

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

[Nate Fischer, 2017-01-11 23:32:33]

On 2017/01/11 at 22:54:01, nparker wrote:
> Is it standard to have "/components/" in the path twice for .java files in
components?

I think it can go either way. I counted 67 java files that do it this way
("/components/" appears twice), and 37 that do it the other way (104 that have
"/components/" one or more times, minus the 67 I already counted).

Here's the codesearch links I used:
https://cs.chromium.org/search/?q=f:/components/.*java/src/org.*/components/....
https://cs.chromium.org/search/?q=f:/components/.*java/src/org.*java$&sq=pack...

[Nate Fischer, 2017-01-11 23:39:17]

Addressing vakh@'s comments.

My inline-draft is actually incorrect (the UI won't let me amend it). I actually
changed the TAG to "ApiBridge" as suggested because "SafeBrowsingApiBridge"
exceeds the 20 char limit.

https://codereview.chromium.org/2622773002/diff/100001/chrome/browser/android...
File chrome/browser/android/chrome_jni_registrar.cc (left):

https://codereview.chromium.org/2622773002/diff/100001/chrome/browser/android...
chrome/browser/android/chrome_jni_registrar.cc:373: {"SafeBrowsingApiBridge",
safe_browsing::RegisterSafeBrowsingApiBridge},
On 2017/01/11 at 22:53:31, Nathan Parker wrote:
> What's the replacement for this?

This is added in components/safe_browsing_db/android/jni_registrar.cc. That file
is depended on in chrome/browser/BUILD.gn at
https://cs.chromium.org/chromium/src/chrome/browser/BUILD.gn?l=2143. Will we
need this dependency to be added for other conditional paths?

https://codereview.chromium.org/2622773002/diff/100001/components/safe_browsi...
File components/safe_browsing_db/android/BUILD.gn (right):

https://codereview.chromium.org/2622773002/diff/100001/components/safe_browsi...
components/safe_browsing_db/android/BUILD.gn:3: # found in the LICENSE file.
On 2017/01/11 at 22:31:47, vakh (Varun Khaneja) wrote:
> nit: empty line here

Done.

https://codereview.chromium.org/2622773002/diff/100001/components/safe_browsi...
File
components/safe_browsing_db/android/java/src/org/chromium/components/safe_browsing/SafeBrowsingApiBridge.java
(right):

https://codereview.chromium.org/2622773002/diff/100001/components/safe_browsi...
components/safe_browsing_db/android/java/src/org/chromium/components/safe_browsing/SafeBrowsingApiBridge.java:17:
public final class SafeBrowsingApiBridge {
On 2017/01/11 at 22:53:31, Nathan Parker wrote:
> On 2017/01/11 22:31:47, vakh (Varun Khaneja) wrote:
> > Since this file (and therefore, class) is in components/safe_browsing
anyway, I
> > think we should drop "SafeBrowsing" from the name and call it:
> > ApiBridge.
> 
> ApiBridge is kind of a weird name to begin with, since it doesn't tell which
API.  Maybe PlatformCheckApiBridge?  Not sure.
> 
> We may want to do all the renaming in a separate CL, so this is just moving
things around.

I agree it's a funky name, but it may be easier to rename all in one CL like
@nparker suggests. I can take it up after the demo.

https://codereview.chromium.org/2622773002/diff/100001/components/safe_browsi...
components/safe_browsing_db/android/java/src/org/chromium/components/safe_browsing/SafeBrowsingApiBridge.java:18:
private static final String TAG = "SafeBrowsingApi";
On 2017/01/11 at 22:31:47, vakh (Varun Khaneja) wrote:
> TAG = "ApiBridge"

I changed it to SafeBrowsingApiBridge for now. If we want to rename the class
later, I can update it at that time.

https://codereview.chromium.org/2622773002/diff/100001/components/safe_browsi...
components/safe_browsing_db/android/java/src/org/chromium/components/safe_browsing/SafeBrowsingApiBridge.java:60:
SafeBrowsingApiHandler handler, long callbackId, String uri, int[]
threatsOfInterest) {
On 2017/01/11 at 22:31:47, vakh (Varun Khaneja) wrote:
> Similarly: ApiHandler here

Ack. Putting this off to do renaming in another CL. Let me know if this should
instead be done in this CL.

[Nate Fischer, 2017-01-11 23:39:52]

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

[commit-bot: I haz the power, 2017-01-11 23:41:08]

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

[vakh (use Gerrit instead), 2017-01-12 00:06:34]

lgtm

Renaming later sounds good. Thanks.

[Nate Fischer, 2017-01-12 00:19:17]

ntfschr@chromium.org changed reviewers:
+ mariakhomenko@chromium.org, michaelbai@chromium.org

[Nate Fischer, 2017-01-12 00:19:17]

Adding mariakhomenko@ for
 - chrome/android/java/**/SafeBrowsingApi{Bridge,Handler}.java
 - chrome/browser/android/*
Adding michaelbai@ for chrome_main_delegate_android.cc

[michaelbai, 2017-01-12 01:08:39]

chrome_main_delegate_android.cc LGTM

[commit-bot: I haz the power, 2017-01-12 01:09:50]

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

[commit-bot: I haz the power, 2017-01-12 01:09:51]

Dry run: This issue passed the CQ dry run.

[Nate Fischer, 2017-01-12 01:27:59]

ntfschr@chromium.org changed reviewers:
+ miguelg@chromium.org
- mariakhomenko@chromium.org

[Nate Fischer, 2017-01-12 01:28:00]

Adding miguelg@ instead of mariakhomenko@

[Miguel Garcia, 2017-01-12 01:29:16]

lgtm for chrome/android

[Nate Fischer, 2017-01-12 01:31:50]

The CQ bit was checked by ntfschr@chromium.org

[Nate Fischer, 2017-01-12 01:31:51]

The patchset sent to the CQ was uploaded after l-g-t-m from nparker@chromium.org
Link to the patchset: https://codereview.chromium.org/2622773002/#ps140001
(title: "Addressing vakh@'s comments")

[commit-bot: I haz the power, 2017-01-12 01:33:11]

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-01-12 01:41:33]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1484184710554290,
"parent_rev": "362c69d27d39af6a9f6945099acb593cd6d96579", "commit_rev":
"52f0c6086c1a89e3a294855e4ced218cf74991bc"}

[commit-bot: I haz the power, 2017-01-12 01:42:20]

Description was changed from

==========
Componentize SafeBrowsingApi{Bridge,Handler}

Move these classes into components/safe_browsing_db/ and update references

BUG=679018
==========

to

==========
Componentize SafeBrowsingApi{Bridge,Handler}

Move these classes into components/safe_browsing_db/ and update references

BUG=679018

Review-Url: https://codereview.chromium.org/2622773002
Cr-Commit-Position: refs/heads/master@{#443116}
Committed:
https://chromium.googlesource.com/chromium/src/+/52f0c6086c1a89e3a294855e4ced...
==========

[commit-bot: I haz the power, 2017-01-12 01:42:21]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/52f0c6086c1a89e3a294855e4ced...

[Wez, 2017-02-07 19:23:07]

wez@chromium.org changed reviewers:
+ grt@chromium.org, wez@chromium.org

[Wez, 2017-02-07 19:23:08]

Hallo grt@chromium.org!
Due to a depot_tools patch which mistakenly removed the OWNERS check for
non-source files (see crbug.com/684270), the following files landed in this CL
and need a retrospective review from you:
	chrome/app/DEPS
Thanks,
Wez

[grt (UTC plus 2), 2017-02-08 09:38:02]

chrome/app rubberstamp lgtm