Build a Bridge to Eddystone Encoder in Components

Build a Bridge to Eddystone Encoder in Components

This change links the Eddystone Encoder in components to the android
Physical Web Share feature through the Java Native Interface.

Add Platform Independent Eddystone Encoder

This change creates an eddystone encoder in cpp, which can be used
across platforms for future physical web projects. Right now,
specifically, it will be used for Physical Web Sharing. Here are the
eddystone specs:
https://github.com/google/eddystone/blob/master/protocol-specification.md

BUG=685856
Review-Url: https://codereview.chromium.org/2753683002
Cr-Commit-Position: refs/heads/master@{#458214}
Committed: https://chromium.googlesource.com/chromium/src/+/7c7a2ac892ad5377c3e25b26cba0c74c9be4c59a

[iankc, 2017-03-14 21:46:52]

Description was changed from

==========
Build a Bridge to Eddystone Encoder in Components

This change links the Eddystone Encoder in components to the android
Physical Web Share feature through the Java Native Interface.

Add Platform Independent Eddystone Encoder

This change creates an eddystone encoder in cpp, which can be used
across platforms for future physical web projects. Right now,
specifically, it will be used for Physical Web Sharing. Here are the
eddystone specs:
https://github.com/google/eddystone/blob/master/protocol-specification.md

BUG=685856
==========

to

==========
Build a Bridge to Eddystone Encoder in Components

This change links the Eddystone Encoder in components to the android
Physical Web Share feature through the Java Native Interface.

Add Platform Independent Eddystone Encoder

This change creates an eddystone encoder in cpp, which can be used
across platforms for future physical web projects. Right now,
specifically, it will be used for Physical Web Sharing. Here are the
eddystone specs:
https://github.com/google/eddystone/blob/master/protocol-specification.md

BUG=685856
==========

[iankc, 2017-03-14 21:46:52]

iankc@google.com changed reviewers:
+ cco3@chromium.org, mattreynolds@chromium.org

[iankc, 2017-03-14 21:48:08]

Hey Hey,

While I'm waiting on getting the components/eddystone in, I thought I would get
the bridge started. You can ignore everthing under components/eddystone as that
is part of another cl https://codereview.chromium.org/2731273004/

[cco3, 2017-03-14 21:55:54]

lgtm

https://codereview.chromium.org/2753683002/diff/1/chrome/browser/android/phys...
File chrome/browser/android/physical_web/eddystone_encoder_bridge.cc (right):

https://codereview.chromium.org/2753683002/diff/1/chrome/browser/android/phys...
chrome/browser/android/physical_web/eddystone_encoder_bridge.cc:50: return
base::android::ToJavaByteArray(env, &encoded_url[0],
&encoded_url[0] doesn't look quite right, but I'm no expert.

[mattreynolds, 2017-03-14 23:21:12]

LGTM with nits

https://codereview.chromium.org/2753683002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBroadcastService.java
(right):

https://codereview.chromium.org/2753683002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebBroadcastService.java:39:
private static final String TAG = "PhysicalWebBService";
Remove if unused.

https://codereview.chromium.org/2753683002/diff/1/chrome/browser/android/phys...
File chrome/browser/android/physical_web/eddystone_encoder_bridge.cc (right):

https://codereview.chromium.org/2753683002/diff/1/chrome/browser/android/phys...
chrome/browser/android/physical_web/eddystone_encoder_bridge.cc:50: return
base::android::ToJavaByteArray(env, &encoded_url[0],
On 2017/03/14 21:55:54, cco3 wrote:
> &encoded_url[0] doesn't look quite right, but I'm no expert.

C++11 added vector::data(), let's use it instead:

return base::android::ToJavaByteArray(env, encoded_url.data(),
encoded_url.size());

https://codereview.chromium.org/2753683002/diff/1/chrome/browser/android/phys...
File chrome/browser/android/physical_web/eddystone_encoder_bridge_unittest.cc
(right):

https://codereview.chromium.org/2753683002/diff/1/chrome/browser/android/phys...
chrome/browser/android/physical_web/eddystone_encoder_bridge_unittest.cc:47:
jbyteArray aByteArray = a.obj();
snake_case

https://codereview.chromium.org/2753683002/diff/1/chrome/browser/android/phys...
chrome/browser/android/physical_web/eddystone_encoder_bridge_unittest.cc:89:
ToJavaByteArray(Env(), &expected_array[0], expected_array_length);
Let's change this one too.

&expected_array[0] -> expected_array->data()

[iankc, 2017-03-15 19:08:51]

https://codereview.chromium.org/2753683002/diff/1/chrome/browser/android/phys...
File chrome/browser/android/physical_web/eddystone_encoder_bridge.cc (right):

https://codereview.chromium.org/2753683002/diff/1/chrome/browser/android/phys...
chrome/browser/android/physical_web/eddystone_encoder_bridge.cc:50: return
base::android::ToJavaByteArray(env, &encoded_url[0],
On 2017/03/14 23:21:12, mattreynolds wrote:
> On 2017/03/14 21:55:54, cco3 wrote:
> > &encoded_url[0] doesn't look quite right, but I'm no expert.
> 
> C++11 added vector::data(), let's use it instead:
> 
> return base::android::ToJavaByteArray(env, encoded_url.data(),
> encoded_url.size());

Done.

https://codereview.chromium.org/2753683002/diff/1/chrome/browser/android/phys...
File chrome/browser/android/physical_web/eddystone_encoder_bridge_unittest.cc
(right):

https://codereview.chromium.org/2753683002/diff/1/chrome/browser/android/phys...
chrome/browser/android/physical_web/eddystone_encoder_bridge_unittest.cc:47:
jbyteArray aByteArray = a.obj();
On 2017/03/14 23:21:12, mattreynolds wrote:
> snake_case

Done.

https://codereview.chromium.org/2753683002/diff/1/chrome/browser/android/phys...
chrome/browser/android/physical_web/eddystone_encoder_bridge_unittest.cc:89:
ToJavaByteArray(Env(), &expected_array[0], expected_array_length);
On 2017/03/14 23:21:12, mattreynolds wrote:
> Let's change this one too.
> 
> &expected_array[0] -> expected_array->data()

This is an array and not a vector. I don't think that it has a data function.

[iankc, 2017-03-16 18:47:24]

iankc@google.com changed reviewers:
+ olivierrobin@google.com

[iankc, 2017-03-16 18:48:21]

Hey Olivier,

This is the second part of the Eddystone Encoder change. It is a bridge that
links the components/eddystone to the clank feature through the Java Native
Interface. Thanks!

[iankc, 2017-03-16 18:48:21]

Hey Olivier,

This is the second part of the Eddystone Encoder change. It is a bridge that
links the components/eddystone to the clank feature through the Java Native
Interface. Thanks!

[Olivier, 2017-03-17 08:53:02]

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

[Olivier, 2017-03-17 08:53:02]

RS LGTM, but I am not an expert of the Android development.

https://codereview.chromium.org/2753683002/diff/80001/chrome/browser/android/...
File chrome/browser/android/physical_web/eddystone_encoder_bridge.h (right):

https://codereview.chromium.org/2753683002/diff/80001/chrome/browser/android/...
chrome/browser/android/physical_web/eddystone_encoder_bridge.h:5: #ifndef
CHROME_BROWSER_ANDROID_EDDYSTONE_ENCODER_BRIDGE_H_
Not sure this is enforced for Android, but guard is missing PHYSICAL_WEB_

[iankc, 2017-03-17 17:34:48]

The CQ bit was checked by iankc@google.com

[iankc, 2017-03-17 17:34:49]

The patchset sent to the CQ was uploaded after l-g-t-m from cco3@chromium.org,
mattreynolds@chromium.org
Link to the patchset: https://codereview.chromium.org/2753683002/#ps80001
(title: "Personal Nits 1")

[commit-bot: I haz the power, 2017-03-17 17:35:27]

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-17 17:44:57]

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

[commit-bot: I haz the power, 2017-03-17 17:44:58]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[iankc, 2017-03-17 17:50:00]

iankc@google.com changed reviewers:
+ dtrainor@chromium.org

[iankc, 2017-03-17 18:05:15]

Hey David,

I'm working on a Physical Web Sharing feature that allows the user to share a
URL from their phone using Bluetooth Low Energy.

To convert the URL from the Chrome tab to an encoded Eddystone byte array we
created a C++ class in components. This change links the components function to
the Java over the JNI Bridge.

[David Trainor- moved to gerrit, 2017-03-20 18:09:38]

lgtm % nit

https://codereview.chromium.org/2753683002/diff/100001/chrome/browser/android...
File chrome/browser/android/physical_web/eddystone_encoder_bridge_unittest.cc
(right):

https://codereview.chromium.org/2753683002/diff/100001/chrome/browser/android...
chrome/browser/android/physical_web/eddystone_encoder_bridge_unittest.cc:66:
TEST_F(EddystoneEncoderBridgeTest, testNullUrl) {
testNullUrl -> TestNullUrl.  Same for all others IIRC.

[iankc, 2017-03-20 19:12:05]

https://codereview.chromium.org/2753683002/diff/100001/chrome/browser/android...
File chrome/browser/android/physical_web/eddystone_encoder_bridge_unittest.cc
(right):

https://codereview.chromium.org/2753683002/diff/100001/chrome/browser/android...
chrome/browser/android/physical_web/eddystone_encoder_bridge_unittest.cc:66:
TEST_F(EddystoneEncoderBridgeTest, testNullUrl) {
On 2017/03/20 18:09:38, David Trainor-ping if over 24h wrote:
> testNullUrl -> TestNullUrl.  Same for all others IIRC.

Done.

[iankc, 2017-03-20 19:12:12]

The CQ bit was checked by iankc@google.com

[iankc, 2017-03-20 19:12:12]

The patchset sent to the CQ was uploaded after l-g-t-m from
mattreynolds@chromium.org, cco3@chromium.org, olivierrobin@chromium.org,
dtrainor@chromium.org
Link to the patchset: https://codereview.chromium.org/2753683002/#ps120001
(title: "Fixing Daves Nits and Rebasing")

[commit-bot: I haz the power, 2017-03-20 19:13:06]

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-20 19:21:31]

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

[commit-bot: I haz the power, 2017-03-20 19:21:32]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[iankc, 2017-03-20 20:26:02]

The CQ bit was checked by iankc@google.com

[iankc, 2017-03-20 20:26:02]

The patchset sent to the CQ was uploaded after l-g-t-m from
dtrainor@chromium.org, mattreynolds@chromium.org, cco3@chromium.org,
olivierrobin@chromium.org
Link to the patchset: https://codereview.chromium.org/2753683002/#ps140001
(title: "Rebasing Again")

[commit-bot: I haz the power, 2017-03-20 20:26:58]

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-20 22:52:08]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1490041562194850,
"parent_rev": "0bf02bd5b61add98b21b51c184579e15ecc49c45", "commit_rev":
"7c7a2ac892ad5377c3e25b26cba0c74c9be4c59a"}

[commit-bot: I haz the power, 2017-03-20 22:53:16]

Description was changed from

==========
Build a Bridge to Eddystone Encoder in Components

This change links the Eddystone Encoder in components to the android
Physical Web Share feature through the Java Native Interface.

Add Platform Independent Eddystone Encoder

This change creates an eddystone encoder in cpp, which can be used
across platforms for future physical web projects. Right now,
specifically, it will be used for Physical Web Sharing. Here are the
eddystone specs:
https://github.com/google/eddystone/blob/master/protocol-specification.md

BUG=685856
==========

to

==========
Build a Bridge to Eddystone Encoder in Components

This change links the Eddystone Encoder in components to the android
Physical Web Share feature through the Java Native Interface.

Add Platform Independent Eddystone Encoder

This change creates an eddystone encoder in cpp, which can be used
across platforms for future physical web projects. Right now,
specifically, it will be used for Physical Web Sharing. Here are the
eddystone specs:
https://github.com/google/eddystone/blob/master/protocol-specification.md

BUG=685856

Review-Url: https://codereview.chromium.org/2753683002
Cr-Commit-Position: refs/heads/master@{#458214}
Committed:
https://chromium.googlesource.com/chromium/src/+/7c7a2ac892ad5377c3e25b26cba0...
==========

[commit-bot: I haz the power, 2017-03-20 22:53:18]

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