587934 - Login manager: use notifications

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Benjamin Stover (:stechz)]

Currently login manager has reverted to using dialog boxes.  Make it notifications again for remote tabs.

Comment 1

[Wesley Johnston (:wesj)]

Attachment: Move nsLoginManagerPrompter to mobile

This basically gets the correct window, and moves over the nsLoginManagerPrompter implementation from FF to mobile (it doesn't bring over the factory thats also in that file).

There's really only one change to the prompter. In the getNotifyBox function I changed:
> let chromeWin = getChromeWindow(notifyWindow).wrappedJSObject;

to:

> let chromeWin = notifyWindow;
>             
> if(!(chromeWin instanceof Ci.nsIDOMChromeWindow))
>   chromeWin = getChromeWindow(chromeWin);
>
> // This is a bit of a hack, but our chromeWindows don't
> // have a wrappedJSObject
> if(chromeWin.wrappedJSObject)
>   chromeWin = chromeWin.wrappedJSObject;

I think this will all get trickier if we eventually have per-browser notifications. i.e. window.getNotificationBox will need to identify the correct browser. From what I've heard, the plan is for nsILoginManagerPrompter to take in a browser instead of a window element (since e10s doesn't have access to the window anymore). Not sure if we want to start that change here or not.

Comment 2

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

Since we won't have this fix in alpha 1, and we want to push the LoginManager-related changes back into toolkit before shipping 2.0, I think we should work on the <browser> changes and drop using nsIDOMWindow. At the very least we should check for a <browser> related interface instead of nsIDOMChromeWindow in LoginManagerPrompt, so we can get the per-browser notification working in Fennec.

You might be able to check for nsIFrameLoaderOwner

Comment 3

[Wesley Johnston (:wesj)]

Attachment: Use browsers

Not sure if this is what you meant or not. This alters the nsILoginManagerPrompter init method to take in an nsISupports object. Then it QI's to try and find out if its a browser element or an nsIDOMWindow.

We still need a window for prompts, and even for the window.getNotificationBox() method, so if we have a browser it gets the owner window of the browser element (browser.ownerDocument.defaultView).

Finally, it also passes getNotificationBox a browser if we have one, and a window otherwise. Fennec just ignores the element that gets tossed to it right now, but its there for later. I haven't tested moving this over to Firefox yet, but it should work there. Is this at all what you had in mind?

Comment 4

[Benjamin Stover (:stechz)]

Comment on attachment 468465 [details] [diff] [review]
Use browsers

>+interface nsILoginManagerPrompter : nsISupports {
>+    /**
>+     * Initialize the prompter. Must be called before using other interfaces.
>+     *
>+     * @param aElt
>+     *        Either a browser or a window object in which the user is doing some
>+     *        login-related action in need to prompt them for something.
>+     *        The prompt will be associated with this window or browser.
>+     */

and

>+    _browser       : null,
>+
>+    __window       : null,
>
>+    get _window() {
>+      if(!this.__window) {
>+        if(!this._browser)
>+          return null;
>+        this.__window = this._browser.ownerDocument.defaultView;
>+      }
>+      return this.__window;
>+    },
>+

and

>+     */
>+    init : function (aElt, aFactory) {
>+        if(aElt instanceof Ci.nsIFrameLoaderOwner) 
>+          this._browser = aElt;
>+        else 
>+          this.__window = aElt;
>+
>+        this._factory = aFactory || null;
>+
>+        var prefBranch = Services.prefs.getBranch("signon.");
>+        this._debug = prefBranch.getBoolPref("debug");
>+        this.log("===== initialized =====");
>+    },

Let's get rid of all these mentions of the browser's window.  The IDL can check for nsIFrameLoader.  It looks like almost everything else the browser window is used for is the prompt service, which we can pass null to since they are for the old school modal dialogs.

>+    /*
>+     * _getNotifyBox
>+     *
>+     * Returns the notification box to this prompter, or null if there isn't
>+     * a notification box available.
>+     */
>+    _getNotifyBox : function () {
>+        let notifyBox = null;
>+        // Given a content DOM window, returns the chrome window it's in.
>+        function getChromeWindow(aWindow) {
>+            return aWindow.QueryInterface(Ci.nsIInterfaceRequestor)
>+                          .getInterface(Ci.nsIWebNavigation)
>+                          .QueryInterface(Ci.nsIDocShellTreeItem)
>+                          .rootTreeItem
>+                          .QueryInterface(Ci.nsIInterfaceRequestor)
>+                          .getInterface(Ci.nsIDOMWindow)
>+                          .QueryInterface(Ci.nsIDOMChromeWindow);
>+        }
>+        try {


>+            // Get topmost window, in case we're in a frame.
>+            var notifyWindow = this._window.top;
>+
>+            // Some sites pop up a temporary login window, when disappears
>+            // upon submission of credentials. We want to put the notification
>+            // bar in the opener window if this seems to be happening.
>+            if (notifyWindow.opener) {
>+                var chromeDoc = getChromeWindow(notifyWindow)
>+                                    .document.documentElement;
>+                var webnav = notifyWindow
>+                                    .QueryInterface(Ci.nsIInterfaceRequestor)
>+                                    .getInterface(Ci.nsIWebNavigation);
>+
>+                // Check to see if the current window was opened with chrome
>+                // disabled, and if so use the opener window. But if the window
>+                // has been used to visit other pages (ie, has a history),
>+                // assume it'll stick around and *don't* use the opener.
>+                if (chromeDoc.getAttribute("chromehidden") &&
>+                    webnav.sessionHistory.count == 1) {
>+                    this.log("Using opener window for notification bar.");
>+                    notifyWindow = notifyWindow.opener;
>+                }
>+            }
>+
>+            let chromeWin = notifyWindow;
>+            
>+            if(!(chromeWin instanceof Ci.nsIDOMChromeWindow))
>+              chromeWin = getChromeWindow(chromeWin);
>+              
>+            // This is a bit of a hack, but our chromeWindows don't
>+            // have a wrappedJSObject
>+            if(chromeWin.wrappedJSObject)
>+              chromeWin = chromeWin.wrappedJSObject;
>+

We can get rid of this code since there is no window anymore.  Instead of using _window, we'll just do all this to browser.ownerDocument.defaultView.  We are losing some functionality here: logins that occur in popups (which Firefox is still concerned about) won't get the notification bar.  We can file a followup bug.

>+            if (chromeWin.getNotificationBox) {
>+                if(this._browser)
>+                  notifyBox = chromeWin.getNotificationBox(this._browser);


>+                else
>+                  notifyBox = chromeWin.getNotificationBox(notifyWindow);

Lose this.  We are assuming that Firefox will change getNotificationBox to use browser.  This code is a hack anyways, we should probably file a followup bug to do this part better.  We can discuss solutions for this there.

Comment 5

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

Agreed. Like we talked about - more browser, less window :)

Comment 6

[Wesley Johnston (:wesj)]

Attachment: No windows

Only take browser (nsIFrameLoaderOwner) elements.

I didn't realize until I moved the LoginManagerPromptFactory over, but this also affects authentication prompts, which call the prompt factory and pass in the window. I'm just passing in null for those cases... for now. Trying to figure out if we need to worry about it in those cases...

Comment 7

[Wesley Johnston (:wesj)]

Attachment: No windows, don't screw with whitespace

Same thing without the whitespace fun. I haven't seen or thought of any problems with this and auth prompts, so I'm putting this up for review. Should I also ask dolske to look at it?

Comment 8

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

Comment on attachment 469926 [details] [diff] [review]
No windows, don't screw with whitespace

>diff --git a/chrome/content/browser.js b/chrome/content/browser.js

>-  getNotificationBox: function getNotificationBox() {
>+  getNotificationBox: function getNotificationBox(aBrowserElt) {

"aBrowser" is good enough

>-function getNotificationBox(aWindow) {
>-  return Browser.getNotificationBox();
>+function getNotificationBox(aBrowserElt) {
>+  return Browser.getNotificationBox(aBrowserElt);

Same

>diff --git a/components/LoginManager.js b/components/LoginManager.js

>diff --git a/components/LoginManagerPrompter.idl b/components/LoginManagerPrompter.idl
>+     * @param aElt
>+     *        Either a browser or a window object in which the user is doing some

Only a "browser" now

>+     *        login-related action in need to prompt them for something.
>+     *        The prompt will be associated with this window or browser.
>+     */
>+    void init(in nsIFrameLoaderOwner aElt);

"aBrowser" here too. It is what we expect

>diff --git a/components/LoginManagerPrompter.js b/components/LoginManagerPrompter.js
>+ * Contributor(s):
>+ *  Justin Dolske <[e-mail]> (original author)
>+ *  Ehsan Akhgari <[e-mail]>

Add yourself!


Note to the reader: I am thinking that we should remove any and all nsIAuthXxx or related code from this file. The Fennec PromptService should handle that now.

>+LoginManagerPromptFactory.prototype = {
>+
>+    classID : Components.ID("{d20128f0-b05e-11df-94e2-0800200c9a66}"),
>+    QueryInterface : XPCOMUtils.generateQI([Ci.nsIPromptFactory,
>+                                            Ci.nsIObserver,
>+                                            Ci.nsISupportsWeakReference]),

Can we remove all of this custom factory and just use a simple XPCOMUtils generated factory?

>+LoginManagerPrompter.prototype = {
>+
>+    classID : Components.ID("97d12931-abe2-11df-94e2-0800200c9a66"),
>+    QueryInterface : XPCOMUtils.generateQI([Ci.nsIAuthPrompt,
>+                                            Ci.nsIAuthPrompt2,
>+                                            Ci.nsILoginManagerPrompter]),

remove nsIAuthPrompt(2)

>+    /* ---------- nsIAuthPrompt prompts ---------- */

remove all of these functions

>+    /* ---------- nsIAuthPrompt helpers ---------- */

and these

>+    /* ---------- nsIAuthPrompt2 prompts ---------- */

and these

Hmm, I do see some things here we may want to port to the PromptService. Things like changing a password causing a notification prompt asking user to remember the change. File a followup bug for any of that work.

>+    /* ---------- nsILoginManagerPrompter prompts ---------- */
>+    init : function (aElt, aFactory) {

"aBrowser"


>+    promptToChangePasswordWithUsernames : function (logins, count, aNewLogin) {

Hmm, maybe the "remember the changed password feature already works, and no followup is needed?

>diff --git a/components/PromptService.js b/components/PromptService.js

>   getPrompt: function getPrompt(domWin, iid) {
>     // This is still kind of dumb; the C++ code delegated to login manager
>     // here, which in turn calls back into us via nsIPromptService2.
>     if (iid.equals(Ci.nsIAuthPrompt2) || iid.equals(Ci.nsIAuthPrompt)) {
>       try {
>         let pwmgr = Cc["@mozilla.org/passwordmanager/authpromptfactory;1"].
>           getService(Ci.nsIPromptFactory);
>-        return pwmgr.getPrompt(domWin, iid);
>+        return pwmgr.getPrompt(null, iid);

Oh crap. I guess we need to change this code too. Actually, our Prompt impl in this file supports the methods. We can just remove this code and allow out methods to kickin.


Looking good. I assume this actually displays a notificationbox too :)

Comment 9

[Wesley Johnston (:wesj)]

Attachment: Killing factories

This kills off the AuthPrompt interfaces and factory. After digging through this... I... don't think we want to do this.

This throws all of the auth prompt stuff to PromptService.js. The methods in there are... pitiful compared to the ones that were in the LoginManagerPrompter, so we basically have to port all of that code over to the PromptService (follow up) in the process create a dependency between the prompt service and the login manager, which was previously buried in @mozilla.org/passwordmanager/authpromptfactory;1.

In short, this just makes the rabbit hole deeper...

Comment 10

[Wesley Johnston (:wesj)]

Attachment: authPrompt

This is a rather direct port of the AuthPrompt and AuthPrompt2 implementations from loginManagerPrompter to the prompt service, and applies on top of the factories patch. It leaves out the asyncPrompt methods. I also tried to combine/refactor a little redundant code. It could be simplified a bit more.

This does work on the mozilla intranet site. If we want having that work as a starting point, we're 99% there.

As per a discussion with mfinkle today, we'd probably only take this as a temporary measure for beta 1, with a different rewrite of this code which further separates the loginManager and prompt service in another iteration.

Comment 11

[Wesley Johnston (:wesj)]

Attachment: auth prompt fix

A little cleanup.

Comment 12

[Wesley Johnston (:wesj)]

I think the first patch here may have bitrotted... or something.

Comment 13

[Wesley Johnston (:wesj)]

Attachment: Fixes

Fixed.

Comment 14

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

Comment on attachment 475136 [details] [diff] [review]
auth prompt fix

>diff --git a/components/PromptService.js b/components/PromptService.js

>   alert: function() {
>-    return this.callProxy("alert", arguments);
>+    return this.callProxy("alert", arguments);IA

typo?

> XPCOMUtils.defineLazyGetter(PromptUtils, "passwdBundle", function () {
>   return Services.strings.createBundle("chrome://passwordmgr/locale/passwordmgr.properties");
> });
> 
> XPCOMUtils.defineLazyGetter(PromptUtils, "bundle", function () {
>   return Services.strings.createBundle("chrome://global/locale/commonDialogs.properties");

We normally, put these at the top

r+

I'll fix the "if(" too! :)

Comment 15

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

pushed:
http://hg.mozilla.org/mobile-browser/rev/09a02e592f71
http://hg.mozilla.org/mobile-browser/rev/fb8bd9e0d758

Comment 16

[Cristian Nicolae (:xti)]

Verified fixed on:
Mozilla/5.0 (Android;Linux armv7l;rv:9.0a1)Gecko/20110919
Firefox/9.0a1 Fennec/9.0a1
Device: Samsung Galaxy S
OS: Android 2.2