1234148 - Replace toasts in JS modules with snackbars

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Sebastian Kaspari (:sebastian; :pocmo)]

I just saw that we still create toasts in some JS modules:
https://dxr.mozilla.org/mozilla-central/search?q=toast.show+path%3A.js&redirect=false&case=true

We already show snackbars here because we delegate all calls to toast.show to show a snackbar instead (Bug 1216051). But we should clean up the code eventually and use Snackbars.jsm directly:
https://developer.mozilla.org/en-US/Add-ons/Firefox_for_Android/API/Snackbars.jsm

Comment 1

[Sebastian Kaspari (:sebastian; :pocmo)]

Alex: Do you want to pick this up? You know Snackbars.jsm and the toast code. :)

Comment 2

[Alex Johnson (:alex_johnson)]

Yeah, I'd love to.  :)

Comment 3

[Alex Johnson (:alex_johnson)]

Attachment: MozReview Request: Bug 1234148 - Replaced toasts in JS modules with snackbars. r?sebastian

Review commit: https://reviewboard.mozilla.org/r/28779/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/28779/

Comment 4

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8700665 [details]
MozReview Request: Bug 1234148 - Replaced toasts in JS modules with snackbars. r?sebastian

https://reviewboard.mozilla.org/r/28779/#review25559

::: mobile/android/chrome/content/MasterPassword.js:66
(Diff revision 1)
> -    NativeWindow.toast.show(Strings.browser.GetStringFromName("masterPassword.incorrect"), "short");
> +    Snackbars.show(Strings.browser.GetStringFromName("masterPassword.incorrect"), Snackbars.LENGTH_SHORT);

This probably is not going to work: I assume this is called from within preferences. But currently we only show snackbars in BrowserApp.

However this is not a problem introduced by this patch: The NativeWindow.toast code will try to show a snackbar too.

I'll file a follow-up bug where we can see how to handle this problem.

::: mobile/android/chrome/content/aboutLogins.js:30
(Diff revision 1)
>  function copyStringAndToast(string, notifyString) {

NIT: We could update the method name too.

::: mobile/android/modules/DownloadNotifications.jsm:76
(Diff revision 1)
>      // If this is a new download, show a toast as well.

NIT: We could update the comment too.

Comment 5

[Alex Johnson (:alex_johnson)]

Comment on attachment 8700665 [details]
MozReview Request: Bug 1234148 - Replaced toasts in JS modules with snackbars. r?sebastian

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

Comment 6

[Alex Johnson (:alex_johnson)]

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

Comment 7

[Alex Johnson (:alex_johnson)]

Sebastian, should I set checkin-needed on this, or should I wait for bug 1234295 to land?

Comment 8

[Sebastian Kaspari (:sebastian; :pocmo)]

Go ahead and land this. bug 1234295 is more or less independent from the changes here.

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/5a814b26505b

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5a814b26505b