Add a Physical Web JNI bridge to support native Physical Web clients

Add a Physical Web JNI bridge to support native Physical Web clients

On Android, the Physical Web scanner is implemented in the Java layer.
To support native Physical Web clients, the metadata about nearby URLs is
made available via a JNI bridge. If the native client has also registered
as a Physical Web listener, it will also be notified whenever a nearby URL
is found or lost, or when the estimated distance to the device broadcasting
the URL is updated.

This change includes the methods necessary to initialize the bridge.

BUG=636490
Committed: https://crrev.com/144c8c4437f3ceae3dfe8d07e8a76aaf3e3e7a0f
Cr-Commit-Position: refs/heads/master@{#424843}

[mattreynolds, 2016-10-07 18:47:41]

Description was changed from

==========
Add a Physical Web JNI bridge to support native Physical Web clients

On Android, the Physical Web scanner is implemented in the Java layer.
To support native Physical Web clients, the metadata about nearby URLs is
made available via a JNI bridge. If the native client has also registered
as a Physical Web listener, it will also be notified whenever a nearby URL
is found or lost, or when the estimated distance to the device broadcasting
the URL is updated.

BUG=636490
==========

to

==========
Add a Physical Web JNI bridge to support native Physical Web clients

On Android, the Physical Web scanner is implemented in the Java layer.
To support native Physical Web clients, the metadata about nearby URLs is
made available via a JNI bridge. If the native client has also registered
as a Physical Web listener, it will also be notified whenever a nearby URL
is found or lost, or when the estimated distance to the device broadcasting
the URL is updated.

BUG=636490
==========

[mattreynolds, 2016-10-07 18:47:42]

mattreynolds@chromium.org changed reviewers:
+ cco3@chromium.org

[cco3, 2016-10-07 20:40:43]

It might get through reviews faster if you break it up (maybe implement each
method separately,  starting with the init).  Just a suggestion.

https://codereview.chromium.org/2377513002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java
(right):

https://codereview.chromium.org/2377513002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:197:
@VisibleForTesting
This shouldn't be needed anymore (removeUrl is now called in upstream code)

https://codereview.chromium.org/2377513002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:202:
if (mNearbyUrls.contains(urlInfo.getUrl())) {
Could you just negate this?  if (!) return

https://codereview.chromium.org/2377513002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:270:
* Gets all UrlInfos and PwsResults for nearby URLs and returns them as
I think standard javadoc has one short sentence and then the rest of the
description on a separate line.

https://codereview.chromium.org/2377513002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:663:
private void onNativeLaunched() {
does this get called magically?  I couldn't find it in the .cc.

https://codereview.chromium.org/2377513002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:696:
if (!isNativeInitialized()) {
Would it be a good idea to push the !onboarding checks here?  We could call it
safeNotify[...]

[mattreynolds, 2016-10-07 22:33:41]

https://codereview.chromium.org/2377513002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java
(right):

https://codereview.chromium.org/2377513002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:197:
@VisibleForTesting
On 2016/10/07 20:40:42, cco3 wrote:
> This shouldn't be needed anymore (removeUrl is now called in upstream code)

Done.

https://codereview.chromium.org/2377513002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:202:
if (mNearbyUrls.contains(urlInfo.getUrl())) {
On 2016/10/07 20:40:42, cco3 wrote:
> Could you just negate this?  if (!) return

Done.

https://codereview.chromium.org/2377513002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:270:
* Gets all UrlInfos and PwsResults for nearby URLs and returns them as
On 2016/10/07 20:40:43, cco3 wrote:
> I think standard javadoc has one short sentence and then the rest of the
> description on a separate line.

Done.

https://codereview.chromium.org/2377513002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:663:
private void onNativeLaunched() {
On 2016/10/07 20:40:42, cco3 wrote:
> does this get called magically?  I couldn't find it in the .cc.

It should be called from the constructor but it looks like I lost it.  I added
it back, and renamed the method to make it clearer what it's doing.

https://codereview.chromium.org/2377513002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:696:
if (!isNativeInitialized()) {
On 2016/10/07 20:40:43, cco3 wrote:
> Would it be a good idea to push the !onboarding checks here?  We could call it
> safeNotify[...]

Done.

[cco3, 2016-10-07 22:34:56]

LGTM

[mattreynolds, 2016-10-07 22:35:07]

Good idea, I'll split this CL

[mattreynolds, 2016-10-08 00:11:41]

Description was changed from

==========
Add a Physical Web JNI bridge to support native Physical Web clients

On Android, the Physical Web scanner is implemented in the Java layer.
To support native Physical Web clients, the metadata about nearby URLs is
made available via a JNI bridge. If the native client has also registered
as a Physical Web listener, it will also be notified whenever a nearby URL
is found or lost, or when the estimated distance to the device broadcasting
the URL is updated.

BUG=636490
==========

to

==========
Add a Physical Web JNI bridge to support native Physical Web clients

On Android, the Physical Web scanner is implemented in the Java layer.
To support native Physical Web clients, the metadata about nearby URLs is
made available via a JNI bridge. If the native client has also registered
as a Physical Web listener, it will also be notified whenever a nearby URL
is found or lost, or when the estimated distance to the device broadcasting
the URL is updated.

This change includes the methods necessary to initialize the bridge.

BUG=636490
==========

[mattreynolds, 2016-10-10 19:12:20]

mattreynolds@chromium.org changed reviewers:
+ dfalcantara@chromium.org

[mattreynolds, 2016-10-10 19:12:21]

dfalcantara@chromium.org: Please review changes in

[gone, 2016-10-11 00:18:07]

https://codereview.chromium.org/2377513002/diff/60001/chrome/browser/android/...
File chrome/browser/android/physical_web/physical_web_data_source_android.cc
(right):

https://codereview.chromium.org/2377513002/diff/60001/chrome/browser/android/...
chrome/browser/android/physical_web/physical_web_data_source_android.cc:30:
DCHECK(url_manager_.obj());
Should be more straightforward; check:

AppBannerManagerAndroid::CreateJavaBannerManager() in C++ and
AppBannerManager.create() in Java

[mattreynolds, 2016-10-11 19:44:35]

https://codereview.chromium.org/2377513002/diff/60001/chrome/browser/android/...
File chrome/browser/android/physical_web/physical_web_data_source_android.cc
(right):

https://codereview.chromium.org/2377513002/diff/60001/chrome/browser/android/...
chrome/browser/android/physical_web/physical_web_data_source_android.cc:30:
DCHECK(url_manager_.obj());
On 2016/10/11 00:18:07, dfalcantara wrote:
> Should be more straightforward; check:
> 
> AppBannerManagerAndroid::CreateJavaBannerManager() in C++ and
> AppBannerManager.create() in Java

I think you're suggesting we should be able to do initialization in one JNI call
(ie, without the native->Java call to get the UrlManager instance).  It's
possible but I couldn't find a way to do it cleanly.  For instance, the Init
method already receives a reference to the UrlManager, but to set the
url_manager_ member we'd need to cast the PhysicalWebDataSource* to a
PhysicalWebDataSourceAndroid* and add a public setter method.  It seems cleaner
to me to have PhysicalWebDataSourceAndroid make a second call to fetch the
UrlManager ref.

[gone, 2016-10-11 19:50:33]

lgtm

https://codereview.chromium.org/2377513002/diff/60001/chrome/browser/android/...
File chrome/browser/android/physical_web/physical_web_data_source_android.cc
(right):

https://codereview.chromium.org/2377513002/diff/60001/chrome/browser/android/...
chrome/browser/android/physical_web/physical_web_data_source_android.cc:30:
DCHECK(url_manager_.obj());
On 2016/10/11 19:44:35, mattreynolds wrote:
> On 2016/10/11 00:18:07, dfalcantara wrote:
> > Should be more straightforward; check:
> > 
> > AppBannerManagerAndroid::CreateJavaBannerManager() in C++ and
> > AppBannerManager.create() in Java
> 
> I think you're suggesting we should be able to do initialization in one JNI
call
> (ie, without the native->Java call to get the UrlManager instance).  It's
> possible but I couldn't find a way to do it cleanly.  For instance, the Init
> method already receives a reference to the UrlManager, but to set the
> url_manager_ member we'd need to cast the PhysicalWebDataSource* to a
> PhysicalWebDataSourceAndroid* and add a public setter method.  It seems
cleaner
> to me to have PhysicalWebDataSourceAndroid make a second call to fetch the
> UrlManager ref.

Oh, I was actually referring to the thing you changed between patch sets; you
didn't need to make the local ref to make the global ref.  Sorry for being
unclear!

[mattreynolds, 2016-10-11 19:51:13]

The CQ bit was checked by mattreynolds@chromium.org

[mattreynolds, 2016-10-11 19:51:13]

The patchset sent to the CQ was uploaded after l-g-t-m from cco3@chromium.org
Link to the patchset: https://codereview.chromium.org/2377513002/#ps80001
(title: "avoid unnecessarily creating global ref")

[mattreynolds, 2016-10-11 19:51:30]

Oh okay, thanks Dan!

[commit-bot: I haz the power, 2016-10-11 19:51:59]

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

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

[commit-bot: I haz the power, 2016-10-11 20:01:20]

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

[mattreynolds, 2016-10-11 21:51:16]

The CQ bit was checked by mattreynolds@chromium.org

[mattreynolds, 2016-10-11 21:51:16]

The patchset sent to the CQ was uploaded after l-g-t-m from cco3@chromium.org,
dfalcantara@chromium.org
Link to the patchset: https://codereview.chromium.org/2377513002/#ps100001
(title: "remove env param")

[commit-bot: I haz the power, 2016-10-11 21:51:32]

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-11 22:04:36]

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

[commit-bot: I haz the power, 2016-10-11 22:04:37]

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

[mattreynolds, 2016-10-11 22:41:53]

The CQ bit was checked by mattreynolds@chromium.org

[mattreynolds, 2016-10-11 22:41:53]

The patchset sent to the CQ was uploaded after l-g-t-m from cco3@chromium.org,
dfalcantara@chromium.org
Link to the patchset: https://codereview.chromium.org/2377513002/#ps120001
(title: "remove @CalledByNative annotation on unused method")

[commit-bot: I haz the power, 2016-10-11 22:42: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-10-11 22:53:41]

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

[commit-bot: I haz the power, 2016-10-11 22:53:42]

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

[mattreynolds, 2016-10-11 23:20:04]

The CQ bit was checked by mattreynolds@chromium.org

[commit-bot: I haz the power, 2016-10-11 23:20:41]

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-11 23:36:03]

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

[commit-bot: I haz the power, 2016-10-11 23:36:04]

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

[mattreynolds, 2016-10-12 00:37:43]

The CQ bit was checked by mattreynolds@chromium.org

[commit-bot: I haz the power, 2016-10-12 00:38:16]

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-12 00:49:42]

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

[commit-bot: I haz the power, 2016-10-12 00:49:44]

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

[mattreynolds, 2016-10-12 17:37:37]

The CQ bit was checked by mattreynolds@chromium.org

[commit-bot: I haz the power, 2016-10-12 17:38:03]

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-12 17:47:19]

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

[commit-bot: I haz the power, 2016-10-12 17:47:20]

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

[mattreynolds, 2016-10-12 20:34:18]

The CQ bit was checked by mattreynolds@chromium.org

[commit-bot: I haz the power, 2016-10-12 20:34:41]

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-12 20:47:13]

Description was changed from

==========
Add a Physical Web JNI bridge to support native Physical Web clients

On Android, the Physical Web scanner is implemented in the Java layer.
To support native Physical Web clients, the metadata about nearby URLs is
made available via a JNI bridge. If the native client has also registered
as a Physical Web listener, it will also be notified whenever a nearby URL
is found or lost, or when the estimated distance to the device broadcasting
the URL is updated.

This change includes the methods necessary to initialize the bridge.

BUG=636490
==========

to

==========
Add a Physical Web JNI bridge to support native Physical Web clients

On Android, the Physical Web scanner is implemented in the Java layer.
To support native Physical Web clients, the metadata about nearby URLs is
made available via a JNI bridge. If the native client has also registered
as a Physical Web listener, it will also be notified whenever a nearby URL
is found or lost, or when the estimated distance to the device broadcasting
the URL is updated.

This change includes the methods necessary to initialize the bridge.

BUG=636490
==========

[commit-bot: I haz the power, 2016-10-12 20:47:15]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-10-12 20:50:16]

Description was changed from

==========
Add a Physical Web JNI bridge to support native Physical Web clients

On Android, the Physical Web scanner is implemented in the Java layer.
To support native Physical Web clients, the metadata about nearby URLs is
made available via a JNI bridge. If the native client has also registered
as a Physical Web listener, it will also be notified whenever a nearby URL
is found or lost, or when the estimated distance to the device broadcasting
the URL is updated.

This change includes the methods necessary to initialize the bridge.

BUG=636490
==========

to

==========
Add a Physical Web JNI bridge to support native Physical Web clients

On Android, the Physical Web scanner is implemented in the Java layer.
To support native Physical Web clients, the metadata about nearby URLs is
made available via a JNI bridge. If the native client has also registered
as a Physical Web listener, it will also be notified whenever a nearby URL
is found or lost, or when the estimated distance to the device broadcasting
the URL is updated.

This change includes the methods necessary to initialize the bridge.

BUG=636490

Committed: https://crrev.com/144c8c4437f3ceae3dfe8d07e8a76aaf3e3e7a0f
Cr-Commit-Position: refs/heads/master@{#424843}
==========

[commit-bot: I haz the power, 2016-10-12 20:50:17]

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