Fixup tests for Android N in chrome.browser.notifications

Fixup tests for Android N in chrome.browser.notifications

No longer need to @SuppressLint in any of these files :)

BUG=643182, 528076, 501900
Committed: https://crrev.com/edcfa0258d501c3b93e0bbc00dd72f53380940bb
Cr-Commit-Position: refs/heads/master@{#416611}

[Anita, 2016-09-02 16:16:41]

The CQ bit was checked by awdf@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-09-02 16:17:01]

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-09-02 17:37:20]

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

[commit-bot: I haz the power, 2016-09-02 17:37:20]

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

[awdf, 2016-09-05 11:09:08]

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

[commit-bot: I haz the power, 2016-09-05 11:09:19]

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

[awdf, 2016-09-05 11:13:17]

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

[commit-bot: I haz the power, 2016-09-05 11:56:05]

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

[commit-bot: I haz the power, 2016-09-05 11:56:06]

Dry run: This issue passed the CQ dry run.

[awdf, 2016-09-05 12:23:01]

Description was changed from

==========
Fixup tests for Android N in chrome.browser.notifications

No longer need to @SuppressLint in any of these files :)

BUG=643182,605963,528076,501900
==========

to

==========
Fixup tests for Android N in chrome.browser.notifications

No longer need to @SuppressLint in any of these files :)

BUG=643182,528076,501900
==========

[awdf, 2016-09-05 12:32:52]

Patchset #1 (id:1) has been deleted

[awdf, 2016-09-05 12:33:48]

Patchset #1 (id:20001) has been deleted

[awdf, 2016-09-05 12:34:35]

Patchset #1 (id:40001) has been deleted

[Peter Beverloo, 2016-09-05 12:41:34]

Cool, thanks!

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

https://codereview.chromium.org/2303373002/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/notifications/CustomNotificationBuilderTest.java:93:
// Notification.extras was added in Android K.
It's unfortunate that we lose test coverage for these (important) fields in
JellyBean.

I see that |notification.extras|, but also the constants and
|notification.actions|, exist in JellyBean but were annotated with @hide, which
means they wouldn't show up in the API documentations. That's why these tests
used to pass on JB too.

Could we perhaps extract these to utility methods as well, annotating them to
suppress the warning, and rely on the fact that nobody will modify JB anymore?

https://codereview.chromium.org/2303373002/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/notifications/CustomNotificationBuilderTest.java:318:
return ((BitmapDrawable) icon.loadDrawable(context)).getBitmap();
Some of these methods are being repeated in the other files. Should we separate
them out to a NotificationTestUtil file of sorts?

[awdf, 2016-09-05 15:19:19]

Patchset #2 (id:80001) has been deleted

[awdf, 2016-09-05 15:23:10]

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

https://codereview.chromium.org/2303373002/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/notifications/CustomNotificationBuilderTest.java:93:
// Notification.extras was added in Android K.
On 2016/09/05 12:41:34, Peter Beverloo wrote:
> It's unfortunate that we lose test coverage for these (important) fields in
> JellyBean.
> 
> I see that |notification.extras|, but also the constants and
> |notification.actions|, exist in JellyBean but were annotated with @hide,
which
> means they wouldn't show up in the API documentations. That's why these tests
> used to pass on JB too.
> 
> Could we perhaps extract these to utility methods as well, annotating them to
> suppress the warning, and rely on the fact that nobody will modify JB anymore?

Done.

https://codereview.chromium.org/2303373002/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/notifications/CustomNotificationBuilderTest.java:318:
return ((BitmapDrawable) icon.loadDrawable(context)).getBitmap();
On 2016/09/05 12:41:34, Peter Beverloo wrote:
> Some of these methods are being repeated in the other files. Should we
separate
> them out to a NotificationTestUtil file of sorts?

Done.

[Peter Beverloo, 2016-09-05 15:29:59]

lgtm! Thank you!

https://codereview.chromium.org/2303373002/diff/100001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationTestUtil.java
(right):

https://codereview.chromium.org/2303373002/diff/100001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationTestUtil.java:42:
return ((BitmapDrawable) icon.loadDrawable(context)).getBitmap();
Even though "assert" statements are still no-ops in our builds, what about
adding the following for documentation purposes? (Which automagically becomes
functional when "assert" does start doing something.)

  assert Build.VERSION.SDK_INT >= Build.VERSION_CODES.M;

[awdf, 2016-09-05 15:33:21]

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

[commit-bot: I haz the power, 2016-09-05 15:33:29]

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

[awdf, 2016-09-05 16:29:38]

https://codereview.chromium.org/2303373002/diff/100001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationTestUtil.java
(right):

https://codereview.chromium.org/2303373002/diff/100001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationTestUtil.java:42:
return ((BitmapDrawable) icon.loadDrawable(context)).getBitmap();
On 2016/09/05 15:29:59, Peter Beverloo wrote:
> Even though "assert" statements are still no-ops in our builds, what about
> adding the following for documentation purposes? (Which automagically becomes
> functional when "assert" does start doing something.)
> 
>   assert Build.VERSION.SDK_INT >= Build.VERSION_CODES.M;

Done.

It adds a lint warning in Android Studio ("Assertions are unreliable") but
doesn't seem to produce extra warnings when building from the command line, so I
guess I'll just disable that inspection in Android Studio.

[awdf, 2016-09-05 16:30:10]

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

[commit-bot: I haz the power, 2016-09-05 16:30:20]

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-09-05 17:39:58]

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

[commit-bot: I haz the power, 2016-09-05 17:39:59]

Dry run: This issue passed the CQ dry run.

[awdf, 2016-09-06 09:25:05]

The CQ bit was checked by awdf@chromium.org

[awdf, 2016-09-06 09:25:05]

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/2303373002/#ps140001
(title: "add assert as suggested")

[commit-bot: I haz the power, 2016-09-06 09:25:14]

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-09-06 09:28:29]

Description was changed from

==========
Fixup tests for Android N in chrome.browser.notifications

No longer need to @SuppressLint in any of these files :)

BUG=643182,528076,501900
==========

to

==========
Fixup tests for Android N in chrome.browser.notifications

No longer need to @SuppressLint in any of these files :)

BUG=643182,528076,501900
==========

[commit-bot: I haz the power, 2016-09-06 09:28:30]

Committed patchset #4 (id:140001)

[commit-bot: I haz the power, 2016-09-06 09:31:15]

Description was changed from

==========
Fixup tests for Android N in chrome.browser.notifications

No longer need to @SuppressLint in any of these files :)

BUG=643182,528076,501900
==========

to

==========
Fixup tests for Android N in chrome.browser.notifications

No longer need to @SuppressLint in any of these files :)

BUG=643182,528076,501900

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

[commit-bot: I haz the power, 2016-09-06 09:31:19]

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