Implement GetDisplayed on android M+

Implement GetDisplayed on android M+

BUG=

Committed: https://crrev.com/6cfe928c1b1b7fda6c4b1ae07df28bb924037961
Cr-Commit-Position: refs/heads/master@{#438527}

[Miguel Garcia, 2016-11-30 10:16:21]

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

[commit-bot: I haz the power, 2016-11-30 10:16:39]

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

[Miguel Garcia, 2016-11-30 11:08:04]

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

[commit-bot: I haz the power, 2016-11-30 11:08:08]

Dry run: 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-11-30 11:54:17]

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

[commit-bot: I haz the power, 2016-11-30 11:54:18]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Miguel Garcia, 2016-11-30 13:28:13]

Patchset #1 (id:1) has been deleted

[Miguel Garcia, 2016-11-30 13:28:58]

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

[Miguel Garcia, 2016-11-30 13:28:59]

No test for now but I am trying to figure out how to change the mock so that I
can write one. Will probably send that as a separate CL.

[Peter Beverloo, 2016-12-02 14:30:11]

https://codereview.chromium.org/2536313003/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationManagerProxy.java
(right):

https://codereview.chromium.org/2536313003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationManagerProxy.java:24:
@TargetApi(android.os.Build.VERSION_CODES.M)
micro nit: please import "android.os.Build" and use "Build.VERSION_CODES.M" here
instead.

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

https://codereview.chromium.org/2536313003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridge.java:588:
NotificationConstants.EXTRA_NOTIFICATION_INFO_PROFILE_ID, profileId);
Please add the extras on the Notification.Builder with the addExtras() method:

https://developer.android.com/reference/android/app/Notification.Builder.html...

(API 20 and up. But that's OK.)

You can do so in NotificationBuilderBase. The reason for this is two-fold:

  (1) Keeps Android-version clauses out of this function. The
NotificationBuilders are where we care about that.
  (2) Makes sure that we set the same properties for WebAPKs as well.

The latter is important because we'll want to be able to get notifications shown
by the WebAPK as well. This may need a notification to their API through the
WebApkNotificationClient class. (Filing a bug about this to work with Xi, Peter
and co. would be a great start.)

https://codereview.chromium.org/2536313003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridge.java:650:
if (Build.VERSION.CODENAME.equals("N") || Build.VERSION.SDK_INT >
Build.VERSION_CODES.M) {
Bad rebase?

https://codereview.chromium.org/2536313003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridge.java:693:
private boolean getNotificationsSupported() {
Let's not jump to JNI just to make this determination. Please use this in C++
instead:

#include "base/android/build_info.h"

  if (base::android::BuildInfo::GetInstance()->sdk_int() >=
          base::android::SDK_VERSION_MARSHMALLOW) {
    return false;
  }

https://codereview.chromium.org/2536313003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridge.java:697:
@CalledByNative
Do we need a @TargetApi annotation here?

https://codereview.chromium.org/2536313003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridge.java:720:
private static byte[][] notificationIdsAsBytes(List<String> notifications) {
Please check out base::android::AppendJavaStringArrayToStringVector() :)

https://codereview.chromium.org/2536313003/diff/20001/chrome/test/android/jav...
File
chrome/test/android/javatests/src/org/chromium/chrome/test/util/browser/notifications/MockNotificationManagerProxy.java
(right):

https://codereview.chromium.org/2536313003/diff/20001/chrome/test/android/jav...
chrome/test/android/javatests/src/org/chromium/chrome/test/util/browser/notifications/MockNotificationManagerProxy.java:107:
return null;
You could construct a sequence of minimal StatusBarNotification objects based on
mNotifications and return those, with void data for everything but what you
expect to be there.

That'd allow you to write a unit test for getNotificationsForProfile(), which
really is the only sensible test I think we may want to consider having.

[Miguel Garcia, 2016-12-06 14:21:52]

PTAL still dependent onhttps://codereview.chromium.org/2534443002/ but should be
ready to land once that one does and this one is rebased on top.

https://codereview.chromium.org/2536313003/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationManagerProxy.java
(right):

https://codereview.chromium.org/2536313003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationManagerProxy.java:24:
@TargetApi(android.os.Build.VERSION_CODES.M)
On 2016/12/02 14:30:10, Peter Beverloo wrote:
> micro nit: please import "android.os.Build" and use "Build.VERSION_CODES.M"
here
> instead.

Done.

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

https://codereview.chromium.org/2536313003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridge.java:588:
NotificationConstants.EXTRA_NOTIFICATION_INFO_PROFILE_ID, profileId);
On 2016/12/02 14:30:11, Peter Beverloo wrote:
> Please add the extras on the Notification.Builder with the addExtras() method:
> 
>
https://developer.android.com/reference/android/app/Notification.Builder.html...
> 
> (API 20 and up. But that's OK.)
> 
> You can do so in NotificationBuilderBase. The reason for this is two-fold:
> 
>   (1) Keeps Android-version clauses out of this function. The
> NotificationBuilders are where we care about that.
>   (2) Makes sure that we set the same properties for WebAPKs as well.
> 
> The latter is important because we'll want to be able to get notifications
shown
> by the WebAPK as well. This may need a notification to their API through the
> WebApkNotificationClient class. (Filing a bug about this to work with Xi,
Peter
> and co. would be a great start.)

Done.

https://codereview.chromium.org/2536313003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridge.java:650:
if (Build.VERSION.CODENAME.equals("N") || Build.VERSION.SDK_INT >
Build.VERSION_CODES.M) {
On 2016/12/02 14:30:11, Peter Beverloo wrote:
> Bad rebase?

yep, removed

https://codereview.chromium.org/2536313003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridge.java:693:
private boolean getNotificationsSupported() {
Ah cool

On 2016/12/02 14:30:11, Peter Beverloo wrote:
> Let's not jump to JNI just to make this determination. Please use this in C++
> instead:
> 
> #include "base/android/build_info.h"
> 
>   if (base::android::BuildInfo::GetInstance()->sdk_int() >=
>           base::android::SDK_VERSION_MARSHMALLOW) {
>     return false;
>   }

https://codereview.chromium.org/2536313003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridge.java:697:
@CalledByNative
On 2016/12/02 14:30:11, Peter Beverloo wrote:
> Do we need a @TargetApi annotation here?

@CalledByNative and @TargetApi don't play together well but I don't think so. As
long as it's in the interface it should be fine. We certainly don't get ling
warnings as is.

https://codereview.chromium.org/2536313003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridge.java:720:
private static byte[][] notificationIdsAsBytes(List<String> notifications) {
On 2016/12/02 14:30:11, Peter Beverloo wrote:
> Please check out base::android::AppendJavaStringArrayToStringVector() :)

Ok, I'm convinced :)

https://codereview.chromium.org/2536313003/diff/20001/chrome/test/android/jav...
File
chrome/test/android/javatests/src/org/chromium/chrome/test/util/browser/notifications/MockNotificationManagerProxy.java
(right):

https://codereview.chromium.org/2536313003/diff/20001/chrome/test/android/jav...
chrome/test/android/javatests/src/org/chromium/chrome/test/util/browser/notifications/MockNotificationManagerProxy.java:107:
return null;
Agreed, I am planning to send a follow up cl with this.

On 2016/12/02 14:30:11, Peter Beverloo wrote:
> You could construct a sequence of minimal StatusBarNotification objects based
on
> mNotifications and return those, with void data for everything but what you
> expect to be there.
> 
> That'd allow you to write a unit test for getNotificationsForProfile(), which
> really is the only sensible test I think we may want to consider having.

[Miguel Garcia, 2016-12-06 14:21:54]

PTAL still dependent onhttps://codereview.chromium.org/2534443002/ but should be
ready to land once that one does and this one is rebased on top.

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

https://codereview.chromium.org/2536313003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridge.java:588:
NotificationConstants.EXTRA_NOTIFICATION_INFO_PROFILE_ID, profileId);
On 2016/12/02 14:30:11, Peter Beverloo wrote:
> Please add the extras on the Notification.Builder with the addExtras() method:
> 
>
https://developer.android.com/reference/android/app/Notification.Builder.html...
> 
> (API 20 and up. But that's OK.)
> 
> You can do so in NotificationBuilderBase. The reason for this is two-fold:
> 
>   (1) Keeps Android-version clauses out of this function. The
> NotificationBuilders are where we care about that.
>   (2) Makes sure that we set the same properties for WebAPKs as well.
> 
> The latter is important because we'll want to be able to get notifications
shown
> by the WebAPK as well. This may need a notification to their API through the
> WebApkNotificationClient class. (Filing a bug about this to work with Xi,
Peter
> and co. would be a great start.)

Done.

https://codereview.chromium.org/2536313003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridge.java:650:
if (Build.VERSION.CODENAME.equals("N") || Build.VERSION.SDK_INT >
Build.VERSION_CODES.M) {
On 2016/12/02 14:30:11, Peter Beverloo wrote:
> Bad rebase?

yep, removed

https://codereview.chromium.org/2536313003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridge.java:693:
private boolean getNotificationsSupported() {
Ah cool

On 2016/12/02 14:30:11, Peter Beverloo wrote:
> Let's not jump to JNI just to make this determination. Please use this in C++
> instead:
> 
> #include "base/android/build_info.h"
> 
>   if (base::android::BuildInfo::GetInstance()->sdk_int() >=
>           base::android::SDK_VERSION_MARSHMALLOW) {
>     return false;
>   }

https://codereview.chromium.org/2536313003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridge.java:720:
private static byte[][] notificationIdsAsBytes(List<String> notifications) {
On 2016/12/02 14:30:11, Peter Beverloo wrote:
> Please check out base::android::AppendJavaStringArrayToStringVector() :)

Ok, I'm convinced :)

https://codereview.chromium.org/2536313003/diff/20001/chrome/test/android/jav...
File
chrome/test/android/javatests/src/org/chromium/chrome/test/util/browser/notifications/MockNotificationManagerProxy.java
(right):

https://codereview.chromium.org/2536313003/diff/20001/chrome/test/android/jav...
chrome/test/android/javatests/src/org/chromium/chrome/test/util/browser/notifications/MockNotificationManagerProxy.java:107:
return null;
Agreed, I am planning to send a follow up cl with this.

On 2016/12/02 14:30:11, Peter Beverloo wrote:
> You could construct a sequence of minimal StatusBarNotification objects based
on
> mNotifications and return those, with void data for everything but what you
> expect to be there.
> 
> That'd allow you to write a unit test for getNotificationsForProfile(), which
> really is the only sensible test I think we may want to consider having.

[Peter Beverloo, 2016-12-06 16:54:14]

lgtm

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

https://codereview.chromium.org/2536313003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridge.java:590:
mNotificationManager.notify(platformTag, PLATFORM_ID, notification);
nit: why this change? revert? :)

https://codereview.chromium.org/2536313003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridge.java:691:
@CalledByNative
nit: please add a docblock for consistency

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

https://codereview.chromium.org/2536313003/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/notification_platform_bridge_android.cc:334: if
(j_notification_ids.obj()) {
nit: consider early returning:

if (!j_notification_ids.obj())
  return false;

https://codereview.chromium.org/2536313003/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/notification_platform_bridge_android.cc:338: for
(auto id : notification_ids) {
nit: const auto& id (so that you don't make copies of the strings)

https://codereview.chromium.org/2536313003/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/notification_platform_bridge_android.cc:338: for
(auto id : notification_ids) {
nit: no brackets for one-line statements

[Miguel Garcia, 2016-12-14 15:47:13]

The CQ bit was checked by miguelg@chromium.org

[Miguel Garcia, 2016-12-14 15:47:14]

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/2536313003/#ps60001
(title: "rebase + review")

[commit-bot: I haz the power, 2016-12-14 15:47:27]

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

[Miguel Garcia, 2016-12-14 15:47:32]

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

https://codereview.chromium.org/2536313003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridge.java:590:
mNotificationManager.notify(platformTag, PLATFORM_ID, notification);
On 2016/12/06 16:54:13, Peter Beverloo wrote:
> nit: why this change? revert? :)

Acknowledged.

https://codereview.chromium.org/2536313003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridge.java:691:
@CalledByNative
On 2016/12/06 16:54:13, Peter Beverloo wrote:
> nit: please add a docblock for consistency

Done.

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

https://codereview.chromium.org/2536313003/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/notification_platform_bridge_android.cc:334: if
(j_notification_ids.obj()) {
On 2016/12/06 16:54:13, Peter Beverloo wrote:
> nit: consider early returning:
> 
> if (!j_notification_ids.obj())
>   return false;

Actually I return null when there are no notifications (since I am returning a
raw array) so this should return true. That's why I decided to not do an early
return.

https://codereview.chromium.org/2536313003/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/notification_platform_bridge_android.cc:338: for
(auto id : notification_ids) {
On 2016/12/06 16:54:13, Peter Beverloo wrote:
> nit: const auto& id (so that you don't make copies of the strings)

Done.

https://codereview.chromium.org/2536313003/diff/40001/chrome/browser/notifica...
chrome/browser/notifications/notification_platform_bridge_android.cc:338: for
(auto id : notification_ids) {
On 2016/12/06 16:54:13, Peter Beverloo wrote:
> nit: no brackets for one-line statements

Done.

[commit-bot: I haz the power, 2016-12-14 16:21:31]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1481730433785540,
"parent_rev": "da1997d483a553845c93f3e2d6232546289c6cc4", "commit_rev":
"a848762f100e4177399a90a7601668a24ec0449b"}

[commit-bot: I haz the power, 2016-12-14 16:22:00]

Description was changed from

==========
Implement GetDisplayed on android M+

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

to

==========
Implement GetDisplayed on android M+

BUG=

Review-Url: https://codereview.chromium.org/2536313003
==========

[commit-bot: I haz the power, 2016-12-14 16:22:03]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-12-14 16:25:33]

Description was changed from

==========
Implement GetDisplayed on android M+

BUG=

Review-Url: https://codereview.chromium.org/2536313003
==========

to

==========
Implement GetDisplayed on android M+

BUG=

Committed: https://crrev.com/6cfe928c1b1b7fda6c4b1ae07df28bb924037961
Cr-Commit-Position: refs/heads/master@{#438527}
==========

[commit-bot: I haz the power, 2016-12-14 16:25:33]

Patchset 3 (id:??) landed as
https://crrev.com/6cfe928c1b1b7fda6c4b1ae07df28bb924037961
Cr-Commit-Position: refs/heads/master@{#438527}

[Theresa, 2016-12-15 00:32:19]

A revert of this CL (patchset #3 id:60001) has been created in
https://codereview.chromium.org/2579513003/ by twellington@chromium.org.

The reason for reverting is: Tests on Android's Marshmallow Tablet Tester
started failing. See crbug.com/674335.