1216051 - Deprecate NativeWindow.toast or delegate calls to Snackbar API

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Sebastian Kaspari (:sebastian; :pocmo)]

Bug 1215026 will introduce an API for showing Snackbars. After that we could:

1) Just deprecate NativeWindow.toast.

2) Delegate calls from NativeWindow.toast to the new Snackbar API. And optionally deprecate NativeWindow.toast.

3) Keep both around. Android hasn't deprecated Toasts so far.

Comment 1

[:Margaret Leibovic]

Do we want to show both snackbars and toasts in our own UI? If we want to transition to only use snackbars, I think add-on APIs should go the same way.

It's a bit unfortunate that the add-on API name maps so directly to the implementation. To avoid breaking existing add-ons, I'm leaning towards option 2, but I don't see a reason to change add-on behavior until we're ready to drop this API in our own UI.

You should also be able to use your LDAP to access the addons MXR to see how often this API is used by addons hosted on AMO:
https://mxr.mozilla.org/addons/

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

I like option 3. Android still allows the use of Toasts.

Comment 3

[Sebastian Kaspari (:sebastian; :pocmo)]

Bug 1157526 replaced all button toasts with snackbars.

* Now we can deprecate at least toast buttons (bug 1224523)
* Bug 1224521 is filed about replacing the remaining toasts. Let's see if we still use toasts after that bug or if we are going to replace all of them. Should we still see a use for toasts in the app then we probably should allow add-ons to use them too.

Comment 4

[Alex Johnson (:alex_johnson)]

Attachment: MozReview Request: Bug 1216051 - Delegated NativeWindow.toast to Snackbar API. r?sebastian

Bug 1216051 - Delegated NativeWindow.toast to Snackbar API. r?sebastian

Comment 5

[Alex Johnson (:alex_johnson)]

(In reply to Alex Johnson(:alex_johnson) from comment #4)
> Created attachment 8692242 [details]
> MozReview Request: Bug 1216051 - Delegated NativeWindow.toast to Snackbar
> API. r?sebastian
> 
> Bug 1216051 - Delegated NativeWindow.toast to Snackbar API. r?sebastian

I think we should delegate NativeWindow.toast to Snackbar API since quite a few addons still use the NativeWindow API: https://mxr.mozilla.org/addons/search?string=NativeWindow.toast

Comment 6

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8692242 [details]
MozReview Request: Bug 1216051 - Delegated NativeWindow.toast to Snackbar API. r?sebastian

https://reviewboard.mozilla.org/r/26243/#review23737

Nice clean up!

::: mobile/android/chrome/content/browser.js
(Diff revision 1)
> -    _callbacks: {},

We should let callers know that this method is now deprecated. We already do this for NativeWindow.pageactions. Search for "deprecated" in browser.js :)

::: mobile/android/chrome/content/browser.js:2302
(Diff revision 1)
> -          // to jar:jar urls so that the frontend can show them
> +        options = {

NIT: Instead of re-creating options we probably can just write to options.action = {
 ...
}

Nice

Comment 7

[Alex Johnson (:alex_johnson)]

Comment on attachment 8692242 [details]
MozReview Request: Bug 1216051 - Delegated NativeWindow.toast to Snackbar API. r?sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26243/diff/1-2/

Comment 8

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8692242 [details]
MozReview Request: Bug 1216051 - Delegated NativeWindow.toast to Snackbar API. r?sebastian

https://reviewboard.mozilla.org/r/26243/#review23761

::: mobile/android/chrome/content/browser.js:2291
(Diff revision 2)
> -      let msg = {
> -        type: "Toast:Show",
> +      let err = Strings.browser.formatStringFromName("nativeWindow.deprecated", ["NativeWindow.toast", "Snackbars"], 2);
> +      Cu.reportError(err);

I think you do not need to add your own error messaging. It's probably enough to add it to the list of deprecated interfaces and the calls will automatically be wrapped:
https://dxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#3136-3141

Can you try this?

Comment 9

[Alex Johnson (:alex_johnson)]

https://reviewboard.mozilla.org/r/26243/#review23761

> I think you do not need to add your own error messaging. It's probably enough to add it to the list of deprecated interfaces and the calls will automatically be wrapped:
> https://dxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#3136-3141
> 
> Can you try this?

I did try that.  But when I tested it, the calls to NativeWindow.toast would just show the error message and would not delegate to Snackbars.

Comment 10

[Alex Johnson (:alex_johnson)]

https://reviewboard.mozilla.org/r/26243/#review23761

> I did try that.  But when I tested it, the calls to NativeWindow.toast would just show the error message and would not delegate to Snackbars.

I actually figured it out.  I moved the check for string durations to Snackbars.jsm and removed the toast from browser.js.  It seems to be working now.

Comment 11

[Alex Johnson (:alex_johnson)]

Attachment: MozReview Request: Bug 1216051 - Moved the check for toast into Snackbars.jsm. r?sebastian

Bug 1216051 - Moved the check for toast into Snackbars.jsm. r?sebastian

Comment 12

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8692242 [details]
MozReview Request: Bug 1216051 - Delegated NativeWindow.toast to Snackbar API. r?sebastian

Ok, thanks! It looks like this only works if the API is moved 1:1 to a JSM file.

Comment 13

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8692959 [details]
MozReview Request: Bug 1216051 - Moved the check for toast into Snackbars.jsm. r?sebastian

https://reviewboard.mozilla.org/r/26361/#review23791

Ah, nice! Just move this (see below) and then we are ready to land this! I'll do some local testing too.

::: mobile/android/modules/Snackbars.jsm:26
(Diff revision 1)
> +    // Takes care of the deprecated toast calls

Let's move this to some function like migrateToastIfNeeded(). Just to keep the code simple here.

Comment 14

[Sebastian Kaspari (:sebastian; :pocmo)]

Oh, and let's fold the two commits into one before landing. :)

Comment 15

[Alex Johnson (:alex_johnson)]

Comment on attachment 8692242 [details]
MozReview Request: Bug 1216051 - Delegated NativeWindow.toast to Snackbar API. r?sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26243/diff/2-3/

Comment 16

[Alex Johnson (:alex_johnson)]

Comment on attachment 8692242 [details]
MozReview Request: Bug 1216051 - Delegated NativeWindow.toast to Snackbar API. r?sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26243/diff/3-4/

Comment 17

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8692242 [details]
MozReview Request: Bug 1216051 - Delegated NativeWindow.toast to Snackbar API. r?sebastian

https://reviewboard.mozilla.org/r/26243/#review23799

Awesome. Thank you!

Comment 18

[Alex Johnson (:alex_johnson)]

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

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/6614f51e468a

Comment 20

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

https://hg.mozilla.org/mozilla-central/rev/6614f51e468a

Comment 21

[Will Bamberg [:wbamberg]]

It looks like alex_johnson has already updated the NativeWindow.toast docs with this deprecation message.  Thanks! So I'm going to mark this dev-doc-complete.