Hooking up Android N inline notification replies end-to-end

Hooking up Android inline notification replies end-to-end

- This part captures the remote input and sends it from java to native code, where it is already hooked up from PlatformNotificationServiceImpl onwards.
- Also added an end-to-end test to check reply is sent all the way through to the javascript notificationclick event.
- Feature is only supported by Android N+ and Android Wear K+, and currently is still hidden behind experimental_web_platform_features flag.

Committed: https://crrev.com/d469d710efef5c56381c67cef9c23db402290b8c
Cr-Commit-Position: refs/heads/master@{#425722}

[awdf, 2016-10-14 14:19:33]

awdf@chromium.org changed reviewers:
+ peter@chromium.org

[awdf, 2016-10-17 13:26:55]

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

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

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

[awdf, 2016-10-17 13:27:28]

Patchset #2 (id:20001) has been deleted

[awdf, 2016-10-17 13:27:41]

Patchset #2 (id:40001) has been deleted

[awdf, 2016-10-17 13:27:49]

Patchset #1 (id:1) has been deleted

[awdf, 2016-10-17 13:43:07]

Description was changed from

==========
IN PROGRESS - Trying to add an end-to-end test for
NotificationPlatformBridge.java

- should test reply is recieved
- but something is going wrong with the message port

trying to make the sw tell the test when it worked


Actually hook up the Android > Native (TODO remove logging, tidy)


Wrote an end-to-end test that I *hope* will work eventually

BUG=
==========

to

==========
Hooking up Android inline notification replies end-to-end

- This part captures the remote input and sends it from java to native code,
where it is already hooked up from PlatformNotificationServiceImpl onwards.
- Also added an end-to-end test to check reply is sent all the way through to
the javascript notificationclick event.
- Feature is only supported by Android N+ and Android Wear K+, and currently is
still hidden behind experimental_web_platform_features flag.
==========

[awdf, 2016-10-17 13:45:13]

On 2016/10/14 14:19:33, awdf wrote:

Should be good for review now - I've deleted the older patches where we were
trying to track down the bug which caused the test to fail (thanks for spotting
it btw!)

[commit-bot: I haz the power, 2016-10-17 13:57:36]

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

[commit-bot: I haz the power, 2016-10-17 13:57:37]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Peter Beverloo, 2016-10-17 14:11:40]

Please update notification_platform_bridge_mac.mm with the new |reply| parameter
to avoid breaking the Mac build.

https://codereview.chromium.org/2418153002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridge.java
(right):

https://codereview.chromium.org/2418153002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridge.java:223:
// RemoteInput was added in KITKATCH_WATCH.
KITKATCH!

https://codereview.chromium.org/2418153002/diff/60001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridgeTest.java
(right):

https://codereview.chromium.org/2418153002/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridgeTest.java:300:
* notificationclick event on the service worker.
This comment seems to have weird wrapping?

https://codereview.chromium.org/2418153002/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridgeTest.java:307:
public void testReplyToNotification_emptyReply() throws Exception {
here (and line 351): "_" -> "With"? I don't think we use underscores in method
names.

https://codereview.chromium.org/2418153002/diff/60001/chrome/browser/notifica...
File chrome/browser/notifications/notification_platform_bridge_android.cc
(right):

https://codereview.chromium.org/2418153002/diff/60001/chrome/browser/notifica...
chrome/browser/notifications/notification_platform_bridge_android.cc:128: const
base::android::JavaParamRef<_jstring*>& java_reply) {
Are you sure that we can't just have

    const JavaParamRef<string>& java_reply

like everything else, and check the "is_null()" method for the NULL check?

I can't find the pattern that you're using anywhere else. A const reference to a
param reference to a pointer to a _jstring sounds dodgy. Let's check with torne@
if we're not sure.

https://codereview.chromium.org/2418153002/diff/60001/chrome/test/data/notifi...
File chrome/test/data/notifications/android_test.html (right):

https://codereview.chromium.org/2418153002/diff/60001/chrome/test/data/notifi...
chrome/test/data/notifications/android_test.html:17: sendToTest(event.data);
nit: consider naming the function and unregistering the listener after having
used it. That avoids the footgun of accidentally running it twice when a test
uses showNotification() w/ replies multiple times.

messagePort.addEventListener('message', function listener(event) {
  if (event.data.startsWith('reply: ')) {
    messagePort.removeEventListener('message', listener);
    sendToTest(event.data);
  }
});

[awdf, 2016-10-17 14:15:02]

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

[commit-bot: I haz the power, 2016-10-17 14:15:17]

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

[awdf, 2016-10-17 14:15:19]

The CQ bit was unchecked by awdf@chromium.org

[awdf, 2016-10-17 14:51:24]

https://codereview.chromium.org/2418153002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridge.java
(right):

https://codereview.chromium.org/2418153002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridge.java:204:
webApkPackage, actionIndex, getNotificationReply(intent));
I will work on doing the progress notification thing next, as recommended by
Android at
https://developer.android.com/guide/topics/ui/notifiers/notifications.html#di...
- this works as a first implementation though.

https://codereview.chromium.org/2418153002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridge.java:223:
// RemoteInput was added in KITKATCH_WATCH.
On 2016/10/17 14:11:39, Peter Beverloo wrote:
> KITKATCH!

! Fixed.

https://codereview.chromium.org/2418153002/diff/60001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridgeTest.java
(right):

https://codereview.chromium.org/2418153002/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridgeTest.java:300:
* notificationclick event on the service worker.
On 2016/10/17 14:11:39, Peter Beverloo wrote:
> This comment seems to have weird wrapping?

Fixed.

https://codereview.chromium.org/2418153002/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridgeTest.java:307:
public void testReplyToNotification_emptyReply() throws Exception {
On 2016/10/17 14:11:39, Peter Beverloo wrote:
> here (and line 351): "_" -> "With"? I don't think we use underscores in method
> names.

Done.

https://codereview.chromium.org/2418153002/diff/60001/chrome/browser/notifica...
File chrome/browser/notifications/notification_platform_bridge_android.cc
(right):

https://codereview.chromium.org/2418153002/diff/60001/chrome/browser/notifica...
chrome/browser/notifications/notification_platform_bridge_android.cc:128: const
base::android::JavaParamRef<_jstring*>& java_reply) {
On 2016/10/17 14:11:39, Peter Beverloo wrote:
> Are you sure that we can't just have
> 
>     const JavaParamRef<string>& java_reply
> 
> like everything else, and check the "is_null()" method for the NULL check?
> 
> I can't find the pattern that you're using anywhere else. A const reference to
a
> param reference to a pointer to a _jstring sounds dodgy. Let's check with
torne@
> if we're not sure.

:O wow, I have no idea where that came from.. perhaps autocomplete?

Fixed now anyway.

https://codereview.chromium.org/2418153002/diff/60001/chrome/test/data/notifi...
File chrome/test/data/notifications/android_test.html (right):

https://codereview.chromium.org/2418153002/diff/60001/chrome/test/data/notifi...
chrome/test/data/notifications/android_test.html:17: sendToTest(event.data);
On 2016/10/17 14:11:40, Peter Beverloo wrote:
> nit: consider naming the function and unregistering the listener after having
> used it. That avoids the footgun of accidentally running it twice when a test
> uses showNotification() w/ replies multiple times.
> 
> messagePort.addEventListener('message', function listener(event) {
>   if (event.data.startsWith('reply: ')) {
>     messagePort.removeEventListener('message', listener);
>     sendToTest(event.data);
>   }
> });

Done.

[Peter Beverloo, 2016-10-17 14:54:09]

lgtm % comment

Thanks! :)

https://codereview.chromium.org/2418153002/diff/100001/chrome/browser/notific...
File chrome/browser/notifications/notification_platform_bridge_android.h
(right):

https://codereview.chromium.org/2418153002/diff/100001/chrome/browser/notific...
chrome/browser/notifications/notification_platform_bridge_android.h:51: const
base::android::JavaParamRef<_jstring*>& java_reply);
update

[awdf, 2016-10-17 15:40:16]

Changed the way it gets the result from the bundle in
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridge.java
, see my comment below - still lgty?

https://codereview.chromium.org/2418153002/diff/100001/chrome/browser/notific...
File chrome/browser/notifications/notification_platform_bridge_android.h
(right):

https://codereview.chromium.org/2418153002/diff/100001/chrome/browser/notific...
chrome/browser/notifications/notification_platform_bridge_android.h:51: const
base::android::JavaParamRef<_jstring*>& java_reply);
On 2016/10/17 14:54:09, Peter Beverloo wrote:
> update

Done.

https://codereview.chromium.org/2418153002/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridge.java
(right):

https://codereview.chromium.org/2418153002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridge.java:226:
reply =
remoteInputResults.getCharSequence(NotificationConstants.KEY_TEXT_REPLY);
I just did some manual testing on my watch and noticed it didn't retrieve the
voice input with getString - I should have been using getCharSequence as per the
developer instructions. 

Works now (at least with Wear paired with Android N, haven't tried pre-N..)

[Peter Beverloo, 2016-10-17 15:48:10]

Yeah, still lgtm, with one optional suggestion.

https://codereview.chromium.org/2418153002/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridge.java
(right):

https://codereview.chromium.org/2418153002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridge.java:229:
return reply != null ? reply.toString() : null;
Since this now is a ternary, the benefit of declaring the |reply| variable on
line 221 is gone. I'd suggest rephrasing this function like:


if (...) {
    CharSequence reply = remoteInputResults.getCharSequence(..);
    if (reply != null) return reply.toString();
}

return null;

[awdf, 2016-10-17 15:59:12]

https://codereview.chromium.org/2418153002/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridge.java
(right):

https://codereview.chromium.org/2418153002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridge.java:229:
return reply != null ? reply.toString() : null;
On 2016/10/17 15:48:10, Peter Beverloo wrote:
> Since this now is a ternary, the benefit of declaring the |reply| variable on
> line 221 is gone. I'd suggest rephrasing this function like:
> 
> 
> if (...) {
>     CharSequence reply = remoteInputResults.getCharSequence(..);
>     if (reply != null) return reply.toString();
> }
> 
> return null;

Agree it doesn't look very pretty now. Done what you suggest, but git cl format
didn't like the one-line 'if' statement (even though I see it's allowed in our
style guide inherited from
http://source.android.com/source/code-style.html#use-standard-brace-style), so I
let it have its way.

[awdf, 2016-10-17 16:00:27]

The CQ bit was checked by awdf@chromium.org

[awdf, 2016-10-17 16:00:28]

The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org
Link to the patchset: https://codereview.chromium.org/2418153002/#ps140001
(title: "Reformat getNotificationReply")

[commit-bot: I haz the power, 2016-10-17 16:00:50]

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-17 17:58:50]

Description was changed from

==========
Hooking up Android inline notification replies end-to-end

- This part captures the remote input and sends it from java to native code,
where it is already hooked up from PlatformNotificationServiceImpl onwards.
- Also added an end-to-end test to check reply is sent all the way through to
the javascript notificationclick event.
- Feature is only supported by Android N+ and Android Wear K+, and currently is
still hidden behind experimental_web_platform_features flag.
==========

to

==========
Hooking up Android inline notification replies end-to-end

- This part captures the remote input and sends it from java to native code,
where it is already hooked up from PlatformNotificationServiceImpl onwards.
- Also added an end-to-end test to check reply is sent all the way through to
the javascript notificationclick event.
- Feature is only supported by Android N+ and Android Wear K+, and currently is
still hidden behind experimental_web_platform_features flag.
==========

[commit-bot: I haz the power, 2016-10-17 17:58:51]

Committed patchset #5 (id:140001)

[commit-bot: I haz the power, 2016-10-17 18:00:51]

Description was changed from

==========
Hooking up Android inline notification replies end-to-end

- This part captures the remote input and sends it from java to native code,
where it is already hooked up from PlatformNotificationServiceImpl onwards.
- Also added an end-to-end test to check reply is sent all the way through to
the javascript notificationclick event.
- Feature is only supported by Android N+ and Android Wear K+, and currently is
still hidden behind experimental_web_platform_features flag.
==========

to

==========
Hooking up Android inline notification replies end-to-end

- This part captures the remote input and sends it from java to native code,
where it is already hooked up from PlatformNotificationServiceImpl onwards.
- Also added an end-to-end test to check reply is sent all the way through to
the javascript notificationclick event.
- Feature is only supported by Android N+ and Android Wear K+, and currently is
still hidden behind experimental_web_platform_features flag.

Committed: https://crrev.com/d469d710efef5c56381c67cef9c23db402290b8c
Cr-Commit-Position: refs/heads/master@{#425722}
==========

[commit-bot: I haz the power, 2016-10-17 18:00:53]

Patchset 5 (id:??) landed as
https://crrev.com/d469d710efef5c56381c67cef9c23db402290b8c
Cr-Commit-Position: refs/heads/master@{#425722}