Closed Bug 1216051 Opened 4 years ago Closed 4 years ago

Deprecate NativeWindow.toast or delegate calls to Snackbar API

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 45
Tracking Status
firefox45 --- fixed

People

(Reporter: sebastian, Assigned: alex_johnson)

References

Details

(Keywords: dev-doc-complete)

Attachments

(1 file, 1 obsolete file)

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.
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/
I like option 3. Android still allows the use of Toasts.
Depends on: 1224523
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.
Bug 1216051 - Delegated NativeWindow.toast to Snackbar API. r?sebastian
Attachment #8692242 - Flags: review?(s.kaspari)
(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 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
Attachment #8692242 - Flags: review?(s.kaspari)
Attachment #8692242 - Flags: feedback+
Assignee: nobody → alex_johnson24
Status: NEW → ASSIGNED
Attachment #8692242 - Flags: feedback+ → review?(s.kaspari)
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/
Keywords: dev-doc-needed
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?
Attachment #8692242 - Flags: review?(s.kaspari)
Attachment #8692242 - Flags: feedback+
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.
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.
Bug 1216051 - Moved the check for toast into Snackbars.jsm. r?sebastian
Attachment #8692959 - Flags: review?(s.kaspari)
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.
Attachment #8692242 - Flags: review?(s.kaspari)
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.
Attachment #8692959 - Flags: review?(s.kaspari) → review+
Oh, and let's fold the two commits into one before landing. :)
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/
Attachment #8692242 - Flags: feedback+
Attachment #8692959 - Attachment is obsolete: true
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/
Attachment #8692242 - Flags: review?(s.kaspari) → review+
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!
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/6614f51e468a
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45
Keywords: dev-doc-needed
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.
You need to log in before you can comment on or make changes to this bug.