Implement Android UI of web notification inline replies

Implement Android UI of web notification inline replies

This patch takes the web notification options that specify an action
should display a text input and use them to show an inline reply button
on Android. The response is ignored for now, and will be routed back to
JavaScript in a follow-up patch.

TEST=PlatformNotificationServiceTest.DisplayPersistentNotificationMatches, StandardNotificationBuilderTest.*
BUG=599859

[Nina, 2016-04-06 11:55:02]

nsatragno@chromium.org changed reviewers:
+ mvanouwerkerk@chromium.org

[Nina, 2016-04-06 11:55:03]

Michael, please take a look. Thanks!

[Michael van Ouwerkerk, 2016-04-06 12:32:06]

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

[commit-bot: I haz the power, 2016-04-06 12:32:20]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1864053002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1864053002/1

[commit-bot: I haz the power, 2016-04-06 12:58:38]

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

[commit-bot: I haz the power, 2016-04-06 12:58:39]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Michael van Ouwerkerk, 2016-04-06 13:27:35]

Thanks Nicolas! In addition to my comments, the bots are a bit red.

https://codereview.chromium.org/1864053002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/notifications/CustomNotificationBuilder.java
(right):

https://codereview.chromium.org/1864053002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/notifications/CustomNotificationBuilder.java:145:
addActionToBuilder(builder, mSettingsAction, null /* defaultPlaceholder */);
Why not pass mDefaultPlaceholder here as well? I'm not sure why this is
different from StandardNotificationBuilder.

https://codereview.chromium.org/1864053002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationBuilderBase.java
(right):

https://codereview.chromium.org/1864053002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationBuilderBase.java:243:
public NotificationBuilderBase setDefaultActionPlaceholder(String
defaultPlaceholder) {
Is defaultPlaceholder nullable? That's what will be used in addActionToBuilder
if it is not set.

https://codereview.chromium.org/1864053002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationBuilderBase.java:280:
Notification.Builder builder, Action action, String defaultPlaceholder) {
It looks like defaultPlaceholder needs @Nullable. The documentation doesn't say
anything about it though, so better make sure we cover this in a test.

https://codereview.chromium.org/1864053002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationUIManager.java
(right):

https://codereview.chromium.org/1864053002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationUIManager.java:457:
if (actionTitles.length != actionIcons.length) {
Extend this bit for types and placeholders.

https://codereview.chromium.org/1864053002/diff/1/chrome/browser/notification...
File chrome/browser/notifications/notification_ui_manager_android.cc (right):

https://codereview.chromium.org/1864053002/diff/1/chrome/browser/notification...
chrome/browser/notifications/notification_ui_manager_android.cc:38: const
std::vector<base::NullableString16>& v) {
s/v/strings/

https://codereview.chromium.org/1864053002/diff/1/chrome/browser/notification...
chrome/browser/notifications/notification_ui_manager_android.cc:41: jobjectArray
joa = env->NewObjectArray(v.size(), string_clazz.obj(), NULL);
nit: s/joa/array/

https://codereview.chromium.org/1864053002/diff/1/chrome/browser/notification...
chrome/browser/notifications/notification_ui_manager_android.cc:41: jobjectArray
joa = env->NewObjectArray(v.size(), string_clazz.obj(), NULL);
nit: s/NULL/nullptr /* initialElement *//

https://codereview.chromium.org/1864053002/diff/1/chrome/browser/notification...
chrome/browser/notifications/notification_ui_manager_android.cc:42:
jni_generator::CheckException(env);
use base::android::CheckException

https://codereview.chromium.org/1864053002/diff/1/chrome/browser/notification...
chrome/browser/notifications/notification_ui_manager_android.cc:46:
env->SetObjectArrayElement(joa, i, nullptr);
This is a no-op, they are all initialized with nullptr above.

https://codereview.chromium.org/1864053002/diff/1/chrome/browser/notification...
chrome/browser/notifications/notification_ui_manager_android.cc:49:
ConvertUTF16ToJavaString(env, v[i].string());
nit: inline this in the SetObjectArrayElement call below.

https://codereview.chromium.org/1864053002/diff/1/chrome/browser/notification...
File chrome/browser/notifications/platform_notification_service_impl.cc (right):

https://codereview.chromium.org/1864053002/diff/1/chrome/browser/notification...
chrome/browser/notifications/platform_notification_service_impl.cc:479: default:
I think there's a clang check for switches over enums. If there is no default
clause, it should fail the build if not all enum values are handled. So I'm
pretty sure that you can safely delete it. Try it by commenting out lines
476-480.

[Nina, 2016-04-06 16:05:04]

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

[commit-bot: I haz the power, 2016-04-06 16:05:36]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1864053002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1864053002/20001

[Nina, 2016-04-06 16:05:49]

I think I fixed all the bot failures but one. Will re-run them again to be 100%
certain.

https://codereview.chromium.org/1864053002/diff/1/chrome/android/BUILD.gn
File chrome/android/BUILD.gn (right):

https://codereview.chromium.org/1864053002/diff/1/chrome/android/BUILD.gn#new...
chrome/android/BUILD.gn:236: "//ui/message_center/notification.h",
This is the thing I'm not entirely sure about. If you know of a better place to
put it, I'd be happy to move it.

https://codereview.chromium.org/1864053002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/notifications/CustomNotificationBuilder.java
(right):

https://codereview.chromium.org/1864053002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/notifications/CustomNotificationBuilder.java:145:
addActionToBuilder(builder, mSettingsAction, null /* defaultPlaceholder */);
On 2016/04/06 13:27:35, Michael van Ouwerkerk wrote:
> Why not pass mDefaultPlaceholder here as well? I'm not sure why this is
> different from StandardNotificationBuilder.

Well, the settings action is a button so it would not need a placeholder.
Passing it would make no difference. Either way is fine for me.

https://codereview.chromium.org/1864053002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationBuilderBase.java
(right):

https://codereview.chromium.org/1864053002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationBuilderBase.java:243:
public NotificationBuilderBase setDefaultActionPlaceholder(String
defaultPlaceholder) {
On 2016/04/06 13:27:35, Michael van Ouwerkerk wrote:
> Is defaultPlaceholder nullable? That's what will be used in addActionToBuilder
> if it is not set.

It should be nullable. I believe this displays the default Android placeholder
for the inline reply...

https://codereview.chromium.org/1864053002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationBuilderBase.java:280:
Notification.Builder builder, Action action, String defaultPlaceholder) {
On 2016/04/06 13:27:35, Michael van Ouwerkerk wrote:
> It looks like defaultPlaceholder needs @Nullable. The documentation doesn't
say
> anything about it though, so better make sure we cover this in a test.

Hmmm, I added a test that creates a notification with an action that has no
placeholder and a null default placeholder. Should the fact that it doesn't
crash be enough?

https://codereview.chromium.org/1864053002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationUIManager.java
(right):

https://codereview.chromium.org/1864053002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationUIManager.java:457:
if (actionTitles.length != actionIcons.length) {
On 2016/04/06 13:27:35, Michael van Ouwerkerk wrote:
> Extend this bit for types and placeholders.

Done.

https://codereview.chromium.org/1864053002/diff/1/chrome/browser/notification...
File chrome/browser/notifications/notification_ui_manager_android.cc (right):

https://codereview.chromium.org/1864053002/diff/1/chrome/browser/notification...
chrome/browser/notifications/notification_ui_manager_android.cc:38: const
std::vector<base::NullableString16>& v) {
On 2016/04/06 13:27:35, Michael van Ouwerkerk wrote:
> s/v/strings/

Copy pasting = These things happen. Fixed.

https://codereview.chromium.org/1864053002/diff/1/chrome/browser/notification...
chrome/browser/notifications/notification_ui_manager_android.cc:41: jobjectArray
joa = env->NewObjectArray(v.size(), string_clazz.obj(), NULL);
On 2016/04/06 13:27:35, Michael van Ouwerkerk wrote:
> nit: s/NULL/nullptr /* initialElement *//

Done.

https://codereview.chromium.org/1864053002/diff/1/chrome/browser/notification...
chrome/browser/notifications/notification_ui_manager_android.cc:42:
jni_generator::CheckException(env);
On 2016/04/06 13:27:35, Michael van Ouwerkerk wrote:
> use base::android::CheckException

Done.

https://codereview.chromium.org/1864053002/diff/1/chrome/browser/notification...
chrome/browser/notifications/notification_ui_manager_android.cc:46:
env->SetObjectArrayElement(joa, i, nullptr);
On 2016/04/06 13:27:35, Michael van Ouwerkerk wrote:
> This is a no-op, they are all initialized with nullptr above.

Removed.

https://codereview.chromium.org/1864053002/diff/1/chrome/browser/notification...
chrome/browser/notifications/notification_ui_manager_android.cc:49:
ConvertUTF16ToJavaString(env, v[i].string());
On 2016/04/06 13:27:35, Michael van Ouwerkerk wrote:
> nit: inline this in the SetObjectArrayElement call below.

Done.

https://codereview.chromium.org/1864053002/diff/1/chrome/browser/notification...
File chrome/browser/notifications/platform_notification_service_impl.cc (right):

https://codereview.chromium.org/1864053002/diff/1/chrome/browser/notification...
chrome/browser/notifications/platform_notification_service_impl.cc:479: default:
On 2016/04/06 13:27:35, Michael van Ouwerkerk wrote:
> I think there's a clang check for switches over enums. If there is no default
> clause, it should fail the build if not all enum values are handled. So I'm
> pretty sure that you can safely delete it. Try it by commenting out lines
> 476-480.

There is, and since we're not jumping layers there is no point in the
NOTREACHED(). Removed.

[Nina, 2016-04-06 16:06:28]

Description was changed from

==========
Implement Android UI of web notification inline replies

This patch takes the web notification options that specify an action
should display a text input and use them to show an inline reply button
on Android. The response is ignored for now, and will be routed back to
JavaScript in a follow-up patch.

TEST=PlatformNotificationServiceTest.DisplayPersistentNotificationMatches
BUG=599859
==========

to

==========
Implement Android UI of web notification inline replies

This patch takes the web notification options that specify an action
should display a text input and use them to show an inline reply button
on Android. The response is ignored for now, and will be routed back to
JavaScript in a follow-up patch.

TEST=PlatformNotificationServiceTest.DisplayPersistentNotificationMatches,
StandardNotificationBuilderTest.*
BUG=599859
==========

[commit-bot: I haz the power, 2016-04-06 16:29:43]

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

[commit-bot: I haz the power, 2016-04-06 16:29:44]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_compile_dbg on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[Michael van Ouwerkerk, 2016-04-07 13:40:20]

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

[commit-bot: I haz the power, 2016-04-07 13:40:32]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1864053002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1864053002/60001

[Michael van Ouwerkerk, 2016-04-07 14:19:20]

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

https://codereview.chromium.org/1864053002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/notifications/CustomNotificationBuilder.java:145:
addActionToBuilder(builder, mSettingsAction, null /* defaultPlaceholder */);
My point is that these basically identical calls are somehow different between
CustomNotificationBuilder and StandardNotificationBuilder. That's confusing to
read, even if what is passed here is irrelevant.

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

https://codereview.chromium.org/1864053002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationBuilderBase.java:282:
if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.M) {
As discussed offline, I understand this should be a no-op on M phones, and might
actually work on wear devices paired with an M phone, which would be really
neat. We'll have to test this manually. Maybe add some documentation here of the
intended behavior with M.

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

https://codereview.chromium.org/1864053002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationUIManager.java:447:
* @param actionTitles Titles of actions to display alongside the notification.
Add an actionTypes param above this line.

https://codereview.chromium.org/1864053002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationUIManager.java:448:
* @param actionIcons Icons of actions to display alongside the notification.
Add an actionPlaceholders line below this one, noting the entries may be null.

https://codereview.chromium.org/1864053002/diff/60001/chrome/android/java/str...
File chrome/android/java/strings/android_chrome_strings.grd (right):

https://codereview.chromium.org/1864053002/diff/60001/chrome/android/java/str...
chrome/android/java/strings/android_chrome_strings.grd:1503: Write a reply…
Let's include vetting this string in the UI review. One more item for that todo
list.

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

https://codereview.chromium.org/1864053002/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/notifications/StandardNotificationBuilderTest.java:99:
"default placeholder", notification.actions[0].getRemoteInputs()[0].getLabel());
Will this work on bots that run older Android versions than M?

https://codereview.chromium.org/1864053002/diff/60001/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/1864053002/diff/60001/chrome/chrome_browser.g...
chrome/chrome_browser.gypi:4141: 'source_file':
'../ui/message_center/notification.h',
I do doubt whether you're meant to include sources from other directories like
this. Is there precedent for this?

https://codereview.chromium.org/1864053002/diff/60001/chromium.gyp_env
File chromium.gyp_env (right):

https://codereview.chromium.org/1864053002/diff/60001/chromium.gyp_env#newcode1
chromium.gyp_env:1: { 'GYP_DEFINES': 'OS=android target_arch=arm use_goma=1', }
Did you mean to include this file in your CL?

[commit-bot: I haz the power, 2016-04-07 15:15:16]

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

[commit-bot: I haz the power, 2016-04-07 15:15:17]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Michael van Ouwerkerk, 2016-05-24 14:40:39]

mvanouwerkerk@chromium.org changed reviewers:
- mvanouwerkerk@chromium.org