Notify native clients when Physical Web URL metadata is updated

Notify native clients when Physical Web URL metadata is updated

Native-layer clients can register for notifications when a Physical Web URL
is found or lost, or when the distance estimate is updated. Notifications
are only sent if the Physical Web feature is explicitly enabled. Native
clients will only receive notifications for URLs that have been resolved
through the Physical Web service.

BUG=636490
Committed: https://crrev.com/5838df0e86f29312233ca3dc587bbcadab6b4b4b
Cr-Commit-Position: refs/heads/master@{#428192}

[mattreynolds, 2016-10-26 17:45:02]

Description was changed from

==========
Notify native clients when Physical Web URL metadata is updated

Native-layer clients can register for notifications when a Physical Web URL
is found or lost, or when the distance estimate is updated. Notifications
are only sent if the Physical Web feature is explicitly enabled. Native
clients will only receive notifications for URLs that have been resolved
through the Physical Web service.

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

to

==========
Notify native clients when Physical Web URL metadata is updated

Native-layer clients can register for notifications when a Physical Web URL
is found or lost, or when the distance estimate is updated. Notifications
are only sent if the Physical Web feature is explicitly enabled. Native
clients will only receive notifications for URLs that have been resolved
through the Physical Web service.

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

[mattreynolds, 2016-10-26 17:45:02]

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

[cco3, 2016-10-26 18:02:44]

couple of comments...

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

https://codereview.chromium.org/2449963005/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:187:
}
I'd prefer just returning here.

https://codereview.chromium.org/2449963005/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:195:
resolveUrl(urlInfo);
same here with the old return statement.  It fits our style throughout.

https://codereview.chromium.org/2449963005/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:646:
if (urlInfo.getDistance() >= 0.0) {
Earlier you did `>`.  Shouldn't these be the same?

[mattreynolds, 2016-10-26 18:42:37]

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

https://codereview.chromium.org/2449963005/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:187:
}
On 2016/10/26 18:02:44, cco3 wrote:
> I'd prefer just returning here.

Done.

https://codereview.chromium.org/2449963005/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:195:
resolveUrl(urlInfo);
On 2016/10/26 18:02:44, cco3 wrote:
> same here with the old return statement.  It fits our style throughout.

Done.

https://codereview.chromium.org/2449963005/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:646:
if (urlInfo.getDistance() >= 0.0) {
On 2016/10/26 18:02:44, cco3 wrote:
> Earlier you did `>`.  Shouldn't these be the same?

Oops, yes. Now they're both >=

[cco3, 2016-10-26 19:19:05]

LGTM

[mattreynolds, 2016-10-26 22:18:31]

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

[mattreynolds, 2016-10-26 22:18:32]

Hi Dan, PTAL

[gone, 2016-10-27 18:13:11]

lgtm % question

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

https://codereview.chromium.org/2449963005/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:728:
nativeOnFound(mNativePhysicalWebDataSourceAndroid, url);
Would it be safer to check if native is still initialized at this point?  You
don't seem to delete the native side anywhere (is that intentional?), but I
think it makes more sense to check the pointer on the same thread that it is
being used—the runnable might take a while to run.

[mattreynolds, 2016-10-27 21:31:35]

Thanks Dan!

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

https://codereview.chromium.org/2449963005/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:728:
nativeOnFound(mNativePhysicalWebDataSourceAndroid, url);
On 2016/10/27 18:13:11, dfalcantara (slow 10.24) wrote:
> Would it be safer to check if native is still initialized at this point?  You
> don't seem to delete the native side anywhere (is that intentional?), but I
> think it makes more sense to check the pointer on the same thread that it is
> being used—the runnable might take a while to run.

Correct, we intentionally don't destroy the native side. I added the check in
case we change the behavior later.

[mattreynolds, 2016-10-27 21:31:49]

The CQ bit was checked by mattreynolds@chromium.org

[mattreynolds, 2016-10-27 21:31:50]

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/2449963005/#ps60001
(title: "double-check native is initialized")

[commit-bot: I haz the power, 2016-10-27 21:33:23]

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

Description was changed from

==========
Notify native clients when Physical Web URL metadata is updated

Native-layer clients can register for notifications when a Physical Web URL
is found or lost, or when the distance estimate is updated. Notifications
are only sent if the Physical Web feature is explicitly enabled. Native
clients will only receive notifications for URLs that have been resolved
through the Physical Web service.

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

to

==========
Notify native clients when Physical Web URL metadata is updated

Native-layer clients can register for notifications when a Physical Web URL
is found or lost, or when the distance estimate is updated. Notifications
are only sent if the Physical Web feature is explicitly enabled. Native
clients will only receive notifications for URLs that have been resolved
through the Physical Web service.

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

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

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-10-27 23:13:24]

Description was changed from

==========
Notify native clients when Physical Web URL metadata is updated

Native-layer clients can register for notifications when a Physical Web URL
is found or lost, or when the distance estimate is updated. Notifications
are only sent if the Physical Web feature is explicitly enabled. Native
clients will only receive notifications for URLs that have been resolved
through the Physical Web service.

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

to

==========
Notify native clients when Physical Web URL metadata is updated

Native-layer clients can register for notifications when a Physical Web URL
is found or lost, or when the distance estimate is updated. Notifications
are only sent if the Physical Web feature is explicitly enabled. Native
clients will only receive notifications for URLs that have been resolved
through the Physical Web service.

BUG=636490

Committed: https://crrev.com/5838df0e86f29312233ca3dc587bbcadab6b4b4b
Cr-Commit-Position: refs/heads/master@{#428192}
==========

[commit-bot: I haz the power, 2016-10-27 23:13:25]

Patchset 4 (id:??) landed as
https://crrev.com/5838df0e86f29312233ca3dc587bbcadab6b4b4b
Cr-Commit-Position: refs/heads/master@{#428192}