Expose Physical Web metadata to native clients over JNI

Expose Physical Web metadata to native clients over JNI

BUG=636490
Committed: https://crrev.com/8897fde5a48caa529e4b2e82a7da3d518f232e19
Cr-Commit-Position: refs/heads/master@{#427418}

[mattreynolds, 2016-10-13 17:58:35]

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

[mattreynolds, 2016-10-13 17:58:36]

Hi Dan, PTAL.

[gone, 2016-10-13 22:48:30]

https://chromiumcodereview.appspot.com/2403423005/diff/20001/chrome/browser/a...
File chrome/browser/android/physical_web/physical_web_data_source_android.cc
(right):

https://chromiumcodereview.appspot.com/2403423005/diff/20001/chrome/browser/a...
chrome/browser/android/physical_web/physical_web_data_source_android.cc:22: //
Fully-qualified class names.
I think that doing anything this deep in JNI (hard-coding JNI literals) is
disallowed.  Could you just add @CalledByNative methods for everything you're
trying you pull into the native side?

[mattreynolds, 2016-10-18 00:59:30]

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

https://codereview.chromium.org/2403423005/diff/20001/chrome/browser/android/...
chrome/browser/android/physical_web/physical_web_data_source_android.cc:22: //
Fully-qualified class names.
On 2016/10/13 22:48:30, dfalcantara (OOO 10.17-10.23) wrote:
> I think that doing anything this deep in JNI (hard-coding JNI literals) is
> disallowed.  Could you just add @CalledByNative methods for everything you're
> trying you pull into the native side?

I decided to go a different route, now the native side passes a reference to a
container which gets filled by successive java->native calls.

[gone, 2016-10-24 16:35:35]

lgtm, just some nits

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

https://codereview.chromium.org/2403423005/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:265:
* which will receive the list of nearby URL metadata.
nit: indent which under A?

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

https://codereview.chromium.org/2403423005/diff/40001/chrome/browser/android/...
chrome/browser/android/physical_web/physical_web_data_source_android.cc:38:
metadata_item->SetString("scannedUrl",
Are these key constants referenced anywhere else?  Worth pulling out?

https://codereview.chromium.org/2403423005/diff/40001/chrome/browser/android/...
File chrome/browser/android/physical_web/physical_web_data_source_android.h
(right):

https://codereview.chromium.org/2403423005/diff/40001/chrome/browser/android/...
chrome/browser/android/physical_web/physical_web_data_source_android.h:19: //
ListValue so we can append to it over JNI.
Is it worth yanking this out into its own file?

https://codereview.chromium.org/2403423005/diff/40001/chrome/browser/android/...
chrome/browser/android/physical_web/physical_web_data_source_android.h:37:
std::unique_ptr<base::ListValue> GetMetadataList();
Is it worth putting a note here that this hands over the unique list and
shouldn't be called twice?  Maybe a DCHECK in the function body itself?

[mattreynolds, 2016-10-24 22:39:52]

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

https://codereview.chromium.org/2403423005/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:265:
* which will receive the list of nearby URL metadata.
On 2016/10/24 16:35:35, dfalcantara (slow 10.24) wrote:
> nit: indent which under A?

Done.

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

https://codereview.chromium.org/2403423005/diff/40001/chrome/browser/android/...
chrome/browser/android/physical_web/physical_web_data_source_android.cc:38:
metadata_item->SetString("scannedUrl",
On 2016/10/24 16:35:35, dfalcantara (slow 10.24) wrote:
> Are these key constants referenced anywhere else?  Worth pulling out?

Done. They are also hard-coded in the WebUI HTML but I couldn't find an easy way
to pull them out.

https://codereview.chromium.org/2403423005/diff/40001/chrome/browser/android/...
File chrome/browser/android/physical_web/physical_web_data_source_android.h
(right):

https://codereview.chromium.org/2403423005/diff/40001/chrome/browser/android/...
chrome/browser/android/physical_web/physical_web_data_source_android.h:19: //
ListValue so we can append to it over JNI.
On 2016/10/24 16:35:35, dfalcantara (slow 10.24) wrote:
> Is it worth yanking this out into its own file?

I tried this, but ran into issues with the way the UrlManager_jni.h include is
auto-generated.

https://codereview.chromium.org/2403423005/diff/40001/chrome/browser/android/...
chrome/browser/android/physical_web/physical_web_data_source_android.h:37:
std::unique_ptr<base::ListValue> GetMetadataList();
On 2016/10/24 16:35:35, dfalcantara (slow 10.24) wrote:
> Is it worth putting a note here that this hands over the unique list and
> shouldn't be called twice?  Maybe a DCHECK in the function body itself?

Done.

[mattreynolds, 2016-10-24 22:41:17]

mattreynolds@chromium.org changed reviewers:
+ olivierrobin@chromium.org, pkasting@chromium.org

[mattreynolds, 2016-10-24 22:41:18]

olivierrobin@chromium.org: Please review changes in ios/chrome and
components/physical_web

pkasting@chromium.org: Please review changes in components/omnibox

[Peter Kasting, 2016-10-24 23:21:56]

LGTM

[Olivier, 2016-10-25 12:49:54]

lgtm

[mattreynolds, 2016-10-25 17:12:16]

The CQ bit was checked by mattreynolds@chromium.org

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

The patchset sent to the CQ was uploaded after l-g-t-m from
dfalcantara@chromium.org
Link to the patchset: https://codereview.chromium.org/2403423005/#ps100001
(title: "also share with omnibox provider")

[commit-bot: I haz the power, 2016-10-25 17:12:46]

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

[mattreynolds, 2016-10-25 17:29:25]

The CQ bit was unchecked by mattreynolds@chromium.org

[mattreynolds, 2016-10-25 17:30:29]

The CQ bit was checked by mattreynolds@chromium.org

[mattreynolds, 2016-10-25 17:30:29]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org, dfalcantara@chromium.org, olivierrobin@chromium.org
Link to the patchset: https://codereview.chromium.org/2403423005/#ps120001
(title: "remove unit test")

[commit-bot: I haz the power, 2016-10-25 17:30:56]

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-25 18:28:48]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-10-25 18:38:30]

Description was changed from

==========
Expose Physical Web metadata to native clients over JNI

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

to

==========
Expose Physical Web metadata to native clients over JNI

BUG=636490

Committed: https://crrev.com/8897fde5a48caa529e4b2e82a7da3d518f232e19
Cr-Commit-Position: refs/heads/master@{#427418}
==========

[commit-bot: I haz the power, 2016-10-25 18:38:31]

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