1195692 - Android API 23 (M) removes deprecated Notification.setLatestEventInfo()

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Sebastian Kaspari (:sebastian; :pocmo)]

Notification.setLatestEventInfo() is deprecated and has been removed with Android API level 23:

> 1:52.55 /Users/sebastian/Projects/Mozilla/fx-team/mobile/android/base/updater/UpdateService.java:311: error: cannot find symbol
> 1:52.55             notification.setLatestEventInfo(this, getResources().getString(R.string.updater_start_title),
> 1:52.55                         ^
> 1:52.55   symbol:   method setLatestEventInfo(UpdateService,String,String,PendingIntent)
> 1:52.55   location: variable notification of type Notification
> 1:52.56 /Users/sebastian/Projects/Mozilla/fx-team/mobile/android/base/updater/UpdateService.java:344: error: cannot find symbol
> 1:52.56             notification.setLatestEventInfo(this, getResources().getString(R.string.updater_apply_title),
> 1:52.56                         ^
> 1:52.56   symbol:   method setLatestEventInfo(UpdateService,String,String,PendingIntent)
> 1:52.56   location: variable notification of type Notification
> 1:52.56 /Users/sebastian/Projects/Mozilla/fx-team/mobile/android/base/updater/UpdateService.java:485: error: cannot find symbol
> 1:52.57         notification.setLatestEventInfo(this, getResources().getString(R.string.updater_downloading_title_failed),
> 1:52.57                     ^
> 1:52.57   symbol:   method setLatestEventInfo(UpdateService,String,String,PendingIntent)
> 1:52.57   location: variable notification of type Notification
> 1:52.84 Note: com.google.android.gms.ads.internal.purchase.zzb accesses a declared method 'asInterface(android.os.IBinder)' dynamically
> 1:52.92 Note: com.google.android.gms.common.internal.DowngradeableSafeParcel accesses a field 'NULL' dynamically
> 1:52.92       Maybe this is program field 'com.google.android.gms.common.internal.safeparcel.SafeParcelable { java.lang.String NULL; }'
> 1:52.93       Maybe this is library field 'android.util.JsonToken { android.util.JsonToken NULL; }'
> 1:52.93       Maybe this is library field 'java.sql.Types { int NULL; }'
> 1:52.93       Maybe this is library field 'org.json.JSONObject { java.lang.Object NULL; }'
> 1:53.13 /Users/sebastian/Projects/Mozilla/fx-team/mobile/android/base/sync/CommandProcessor.java:284: error: cannot find symbol
> 1:53.13     notification.setLatestEventInfo(context, notificationTitle, uri, contentIntent);
> 1:53.13                 ^
> 1:53.13   symbol:   method setLatestEventInfo(Context,String,String,PendingIntent)
> 1:53.13   location: variable notification of type Notification

The deprecation annotation just says: "Use {@link Builder} instead."

Comment 1

[Sebastian Kaspari (:sebastian; :pocmo)]

UpdateService uses some more deprecated methods (like the Notification constructor). We probably should just migrate this class to use the notification builder. And maybe not even the one of the Android SDK but the one in the support library.

Comment 2

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: 1195692-UpdateService.patch

Eugen: I picked you as reviewer because you've been the only one editing this file this year. Let me know if you want someone else to review the patch. :)

Comment 3

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: 1195692-CommandProcessor.patch

Now only NotificationHandler / AlertNotification is left. This might need a bit more work.

Comment 4

[Eugen Sawin [:esawin]]

Comment on attachment 8649213 [details] [diff] [review]
1195692-UpdateService.patch

Review of attachment 8649213 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good! From what I read, this has fallbacks, so we should be OK with older supported platforms, too.

::: mobile/android/base/updater/UpdateService.java
@@ +304,1 @@
>  

I think we can remove the blank line.

Comment 5

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8649225 [details] [diff] [review]
1195692-CommandProcessor.patch

Review of attachment 8649225 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/sync/CommandProcessor.java
@@ +274,5 @@
>      notificationIntent.putExtra(TabQueueDispatcher.SKIP_TAB_QUEUE_FLAG, true);
>      PendingIntent contentIntent = PendingIntent.getActivity(context, 0, notificationIntent, 0);
> +
> +    NotificationCompat.Builder builder = new NotificationCompat.Builder(context);
> +    builder.setSmallIcon(R.drawable.flat_icon);

Should we add a bigIcon here (in a follow up)?

Comment 6

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Michael Comella (:mcomella) from comment #5)
> Should we add a bigIcon here (in a follow up)?

So far I only tried to recreate the previous behavior. Now with the NotificationCompat class we actually could start to use some of the newer features where supported and make our notifications more appealing.

Comment 7

[Sebastian Kaspari (:sebastian; :pocmo)]

url:        https://hg.mozilla.org/integration/fx-team/rev/7bc736ff4529fd1c6f1abc31825ce56c2bc03ac0
changeset:  7bc736ff4529fd1c6f1abc31825ce56c2bc03ac0
user:       Sebastian Kaspari <[e-mail]>
date:       Fri Aug 21 11:52:25 2015 +0200
description:
Bug 1195692 - UpdateService: Use NotificationCompat.Builder to create notifications. r=esawin

Comment 8

[Sebastian Kaspari (:sebastian; :pocmo)]

url:        https://hg.mozilla.org/integration/fx-team/rev/62a945fe01762d7b7cc424e204ad5270f55218e1
changeset:  62a945fe01762d7b7cc424e204ad5270f55218e1
user:       Sebastian Kaspari <[e-mail]>
date:       Fri Aug 21 11:54:47 2015 +0200
description:
Bug 1195692 - CommandProcessor: Use NotificationCompat.Builder to create notifications. r=mcomella

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7bc736ff4529
https://hg.mozilla.org/mozilla-central/rev/62a945fe0176

Comment 10

[Sebastian Kaspari (:sebastian; :pocmo)]

The last call to setLatestEventInfo() is in NotificationHandler on AlertNotification objects. While trying to understand what this class is used for I found bug 893289 (with a WIP patch) about killing AlertNotification all together.

Comment 11

[Sebastian Kaspari (:sebastian; :pocmo)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=51ed0b236d9e

Comment 12

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: MozReview Request: Bug 1195692 - Replace AlertNotification with NotificationCompat.Builder based implementation. r?mcomella

Bug 1195692 - Replace AlertNotification with NotificationCompat.Builder based implementation. r?mcomella

Comment 13

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Sebastian Kaspari (:sebastian) from comment #12)
> Created attachment 8665023 [details]
> MozReview Request: Bug 1195692 - Replace AlertNotification with
> NotificationCompat.Builder based implementation. r?mcomella
> 
> Bug 1195692 - Replace AlertNotification with NotificationCompat.Builder
> based implementation. r?mcomella

Parts of this patch have been inspired by wesj's WIP patch in bug 893289. I've been testing this via demo pages for the Notification Web API. I've just been unable to follow the code path for setting progress. It almost feels like it's not used: I could not find any users of nsAlertsService::OnProgress. But that's the mysterious native world.

Comment 14

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8665023 [details]
MozReview Request: Bug 1195692 - Replace AlertNotification with NotificationCompat.Builder based implementation. r?mcomella

https://reviewboard.mozilla.org/r/20123/#review18249

Looks good – don't forget to dupe bug 893289.

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/4b20c11bc860

Comment 16

[Sebastian Kaspari (:sebastian; :pocmo)]

This was the last piece for this bug.

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/4b20c11bc860