We should move our NativeWindow.toast api to its own jsm so that consumers don't need access to the chrome window to use it.

Comment on attachment 8471367 [details] [diff] [review] Patch This moves the toast code to a jsm as, well as the notifications it handles. It also creates a lazy loaded shim into NativeWindow for backwards compatibility.

This converts all the toasts uses in the tree to use the new JSM. This is kinda weird because browser.js now needs to hold two references to Toast.jsm, one in NativeWindow and one on the Window itself. I also moved the "short" and "long" strings to constants in Toast.

Comment on attachment 8471370 [details] [diff] [review] Patch 2 >diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js >+XPCOMUtils.defineLazyModuleGetter(this, "Toast", >+ "resource://gre/modules/Toast.jsm"); >+ > XPCOMUtils.defineLazyModuleGetter(NativeWindow, "toast", > "resource://gre/modules/Toast.jsm", "Toast"); A comment about why the "NativeWindow" version is needed would be nice

Comment on attachment 8471367 [details] [diff] [review] Patch ># HG changeset patch ># Parent 52ffcb8fc6bc84c5ef6effe673ca84e232002c71 ># User Wes Johnston <wjohnston@mozilla.com> > >diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js >--- a/mobile/android/chrome/content/browser.js >+++ b/mobile/android/chrome/content/browser.js >@@ -134,16 +134,17 @@ XPCOMUtils.defineLazyModuleGetter(this, > Services.obs.addObserver(observer, notification, false); > }); > }); > > // Lazily-loaded JS modules that use observer notifications > [ > ["Home", ["HomeBanner:Get", "HomePanels:Get", "HomePanels:Authenticate", "HomePanels:RefreshView", > "HomePanels:Installed", "HomePanels:Uninstalled"], "resource://gre/modules/Home.jsm"], >+ ["Toast", ["Toast:Click", "Toast:Hidden"], "resource://gre/modules/Toast.jsm"], Is this needed? Do we ever send the notifications without first using Toast.jsm? Update: OK, now that I've read Toast.jsm I see that you never use Services.obs.addObserver in it so you are depending on this lazy loader to send notifications to Toast.jsm, which I think is a bad idea. I think the common pattern for a JSM is usually adding an "init()" method in the JSM file somewhere and calling it. This is good and bad. When doing this pattern we need to make sure the JSM is lazy loaded everywhere, or the init method is called too early and it's a perf and memory issue. I think you can call Services.obs.addObserver inside the Toast.show method if you haven't already. That's delayed enough. >diff --git a/mobile/android/modules/Toast.jsm b/mobile/android/modules/Toast.jsm >+Cu.import("resource://gre/modules/Messaging.jsm"); I bet this could be lazy loaded too >+ observe: function(aSubject, aTopic, aData) { >+ if (aTopic == "Toast:Click") { >+ this.onClick(aData); >+ } else if (aTopic == "Toast:Hidden") { >+ this.onHidden(aData); >+ } What if you ad a check here on _callbacks and if it's empty you removeObservers? >+ show: function(aMessage, aDuration, aOptions) { >+ if (aOptions && aOptions.button) { >+ this._callbacks[msg.button.id] = aOptions.button.callback; What if you addObservers if _callbacks is empty

I moved the observer notifications over, but then decided that we really should just use Messaging.jsm's callback mechanism for this anyway. So I removed them, removed the buttonId, and fixed up GeckoApp. That should probably be two patches, but while we're moving junk around, it seemed easier to just do it in one. If you want, I could take patch v1 here and diff it with v2 to generate a second patch. Note, I also removed a feature here. There's no no (good) way to know in JS when a button toast is hidden. I'm fine with that :)

qref

Comment on attachment 8471370 [details] [diff] [review] Patch 2 Review of attachment 8471370 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/chrome/content/browser.js @@ +2697,5 @@ > } > }; > > +XPCOMUtils.defineLazyModuleGetter(this, "Toast", > + "resource://gre/modules/Toast.jsm"); I can understand the NativeWindow compatibility lazy module getter being added next to the NativeWindow declaration, but I think this Toast getter should be included at the top of the file where we include any other module. ::: mobile/android/components/ContentDispatchChooser.js @@ -48,5 @@ > // If we have more than one option, let the OS handle showing a list (if needed). > if (aHandler.possibleApplicationHandlers.length > 1) { > aHandler.launchWithURI(aURI, aWindowContext); > } else { > - let win = this._getChromeWin(); You can also get rid of the _getChromeWin function, since this is the only place it's used. ::: mobile/android/modules/Toast.jsm @@ +34,5 @@ > var Toast = { > _callbacks: {}, > > + LONG: "long", > + SHORT: "short", Nice, I'm glad these are finally constants.

Comment on attachment 8471763 [details] [diff] [review] Patch 1 - v2 Review of attachment 8471763 [details] [diff] [review]: ----------------------------------------------------------------- Nice, this callback API is really nice for this. Can we add a really stupid little robocop test for this? Something that just shows a toast and makes sure the correct text appears? This could be really similar to testHomeBanner. If you want to get fancy, you could also test clicking on the button toast button and making sure something happens. The beauty of little modules is that hopefully they're easier to test! ::: mobile/android/chrome/content/browser.js @@ +2697,5 @@ > } > }; > > +XPCOMUtils.defineLazyModuleGetter(NativeWindow, "toast", > + "resource://gre/modules/Toast.jsm", "Toast"); Add a comment explaining that this is a compatibility layer for older add-ons. ::: mobile/android/modules/Toast.jsm @@ +11,5 @@ > + > +this.EXPORTED_SYMBOLS = ["Toast"]; > + > +// Copied from browser.js > +// TODO: We should move this method to a common importable location Can you file a mentor bug for this? I thought I had filed this bug at one point, but I can't find it. I also have add-ons that use this copy-pasta, so it would be nice to just have something they can import. @@ +28,5 @@ > + return aURI; > +} > + > +var Toast = { > + show: function(aMessage, aDuration, aOptions) { While we're here writing a new file, I think we should update this to drop the "a" prefixes. @@ +35,5 @@ > + message: aMessage, > + duration: aDuration > + }; > + > + var callback; Nit: let

We also need to update MDN! You could make a new page for this API, then just add a "deprecated in Firefox 34" thing to the NativeWindow.toast page with a link to the new page.

https://hg.mozilla.org/integration/fx-team/rev/14a934f9c207 https://hg.mozilla.org/integration/fx-team/rev/fb6d69690255 I updated the PageActions.jsm documentation last night too. I'll do the same for this (assuming it sticks).

> Can we add a really stupid little robocop test for this? Something that just > shows a toast and makes sure the correct text appears? This could be really > similar to testHomeBanner. If you want to get fancy, you could also test > clicking on the button toast button and making sure something happens. The > beauty of little modules is that hopefully they're easier to test! Mfinkle wrote those in bug 1042252. They're not landed yet though. I'll make that happen :) > Add a comment explaining that this is a compatibility layer for older > add-ons. This... changed in a separate patch. I have a utility now that logs something to the console the first time you use one of these deprecated NativeWindow apis. > Can you file a mentor bug for this? I thought I had filed this bug at one > point, but I can't find it. I also have add-ons that use this copy-pasta, so > it would be nice to just have something they can import. Filed Bug 1054018

Backed out under suspicion of causing various Android failures: https://hg.mozilla.org/integration/fx-team/rev/5e8369ca0489

Clearing out my Android bugs, but noming this to make sure the Android peeps have a chance to triage it.

I'd be interested in working on this.

(In reply to Alex Johnson(:alex_johnson) from comment #14) > I'd be interested in working on this. Hi Alex! We are moving away from toasts and are starting to use snackbars (bug 1157526, bug 1224521). As a result of this we are going to deprecate the toast API and it's very likely that we are going to automatically map calls to the old API to create snackbars (bug 1216051). So we probably won't need to move the code to its own module. If you want to then you can look into delegating calls from NativeWindow.toast to Snackbars.jsm: bug 1216051.

Since we're removing the toast code, let's not fix this bug.