Prism uses old "Remember password?" mechanism

RESOLVED FIXED

Status

Mozilla Labs
Prism
RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: Matthew Wilson, Assigned: Dolske)

Tracking

({dev-doc-complete})

Details

Attachments

(3 attachments, 5 obsolete attachments)

(Reporter)

Description

10 years ago
With Firefox 3 beta 4, Prism 0.2 extension on Windows.

I created a Prism app for Google Reader.

When I launched it and tried to log in, I received the old-style prompt

  "Do you want Prism to remember this password?"

and had to reply before the request was submitted.

In Firefox 3, the behaviour is for the request to be submitted immediately, and then, on page load, to be asked if you want to remember the password.
Looks like the code in nsLoginManagerPrompter.js is hard wired to look for a "tabbrowser" in "navigator:browser" window types. So it won't work for Prism (or any other application that is not designed in just the right way). Some form of a callback would be a nice addition in the code.

See http://mxr.mozilla.org/seamonkey/source/toolkit/components/passwordmgr/src/nsLoginManagerPrompter.js#888
Yeah, the dependency on "browser" internals like the presence of a tabbrowser/getBrowser()/etc. for notification bar prompting isn't ideal. I think ideally we'd let the toolkit prompting code notify the browser code and let it do the prompting itself, somewhat like how popup blocking works (with its DOMPopupBlocked event fired on the window). Don't think we can rework this for 1.9, but we should get a bug on file.
(Assignee)

Updated

10 years ago
Blocks: 436054
(Assignee)

Comment 3

10 years ago
Created attachment 328611 [details] [diff] [review]
Patch v.1

So, this works. Comments on the appropriateness of the implementation?

Something's not quite right here, though. After banging my head against this today, I found I have to use .wrappedJSObject to get at nsIDOMChromeWindow's |browserDOMWindow| attribute. Otherwise I get an "illegal value" exception thrown when trying to access the attribute. I'm not really sure what's going on with that. :(
Assignee: nobody → dolske
Status: NEW → ASSIGNED
Does it work with an explicit QI to nsIBrowserDOMWindow instead of .wrappedJSObject?
Unfortunately, that won't help. The problem is that the browserDOMWindow set on the chrome window is a JS-implemented object. That means that the stored nsIBrowserDOMWindow in C++ is an XPCWrappedJS. Then, without the wrappedJSObject, looking up chromeWin.browserDOMWindow causes XPCNativeWrapper to try to deep-wrap the XPCWrappedJS, and that isn't allowed, so it throws.

Because chromeWin is a chrome window, using .wrappedJSObject to get at the underlying object is safe and the correct thing to do here.
(Assignee)

Comment 6

10 years ago
Created attachment 328777 [details] [diff] [review]
Patch v.2

This fixes Firefox to work the new way, will need to also add the browser.js change to Seamonkey's naviagtor.js, and wherever Prism's equivalent it.

I don't think this specific change needs a test, as it should be covered by the general password manager notification bar tests. Which I, uhh, need to finish writing first.
Attachment #328611 - Attachment is obsolete: true
Attachment #328777 - Flags: review?(gavin.sharp)
(Assignee)

Comment 7

10 years ago
Created attachment 328789 [details] [diff] [review]
Patch for SeaMonkey, v.1

This should be the obvious change for SeaMonkey. Not sure how to verify it since SM isn't using the new login manager yet, perhaps Mark could give it a quick spin with his patches for login manager?
Attachment #328789 - Flags: review?(bugzilla)

Comment 8

10 years ago
Once this lands, please assign to me so I can fix Prism. !Gracias!
OS: Windows Vista → All
Hardware: PC → All
Created attachment 328955 [details] [diff] [review]
Patch for Prism

This patch adds the <notificationbox> to the XUL and the getNotificationBox method to nsBrowserAccess. When combined with the nsLoginManagerPrompter changes, cool modeless password notifications should happen.
Attachment #328955 - Flags: review?(matthew.gertner)

Updated

10 years ago
Attachment #328955 - Flags: review?(matthew.gertner) → review+
Comment on attachment 328777 [details] [diff] [review]
Patch v.2

>diff --git a/dom/public/idl/base/nsIBrowserDOMWindow.idl b/dom/public/idl/base/nsIBrowserDOMWindow.idl

bz or bsmedberg should probably review this.

>diff --git a/toolkit/components/passwordmgr/src/nsLoginManagerPrompter.js b/toolkit/components/passwordmgr/src/nsLoginManagerPrompter.js

>     _getNotifyBox : function () {
>+        var notifyBox = null;
>+
>+        // Given a content DOM window, returns the chrome window it's in.
>+        function getChromeWindow(aWindow) {
>+            var chromeWin = aWindow 
>+                                .QueryInterface(Ci.nsIInterfaceRequestor)
>+                                .getInterface(Ci.nsIWebNavigation)
>+                                .QueryInterface(Ci.nsIDocShellTreeItem)
>+                                .rootTreeItem
>+                                .QueryInterface(Ci.nsIInterfaceRequestor)
>+                                .getInterface(Ci.nsIDOMWindow);
>+            // no need to QI to nsIDOMChromeWindow
>+            return chromeWin;

Are you sure? Sometimes this works because the element was previously QIed in unrelated code, and relying on that can be fragile. I'd add the explicit QI to be sure.
Attachment #328777 - Flags: review?(gavin.sharp) → review+
(Assignee)

Comment 11

10 years ago
Created attachment 330302 [details] [diff] [review]
Patch v.3

Added QI from gavin's review, requesting review from bz for the nsIBrowserDOMWindow.idl change.
Attachment #328777 - Attachment is obsolete: true
Attachment #330302 - Flags: review?
(Assignee)

Updated

10 years ago
Attachment #330302 - Flags: review? → review?(bzbarsky)

Comment 12

10 years ago
(In reply to comment #10)
>(From update of attachment 328777 [details] [diff] [review])
>>+            // no need to QI to nsIDOMChromeWindow
>>+            return chromeWin;
>Are you sure?
Windows have class info, so yes.

Comment 13

10 years ago
Comment on attachment 330302 [details] [diff] [review]
Patch v.3

>             if (notifyWindow.opener) {
>+                var chromeWin = getChromeWindow(notifyWindow);
...
>             }
> 
> 
>+            // Get the chrome window for the content window we're using.
>+            var chromeWin = getChromeWindow(notifyWindow);
Why not get the chrome window first?

>+            var browserWin = chromeWin.wrappedJSObject.browserDOMWindow;
This wrappedJSObject looks fishy to me, whatever mrbkap says.

Comment 14

10 years ago
Comment on attachment 330302 [details] [diff] [review]
Patch v.3

>+                                .QueryInterface(Ci.nsIInterfaceRequestor)
>+                                .getInterface(Ci.nsIWebNavigation)
Another option is to do
var browser = aWindow.QueryInterface(Ci.nsIInterfaceRequestor)
                     .getInterface(Ci.nsIWebNavigation)
                     .QueryInterface(Ci.nsIDocShell)
                     .chromeEventHandler;
then crawl the parent node chain looking for a notificationbox.

Comment 15

10 years ago
Comment on attachment 328789 [details] [diff] [review]
Patch for SeaMonkey, v.1

I tested this in conjunction with the core patch and a patch to build login manager in SeaMonkey.

>   isTabContentWindow: function isTabContentWindow(aWindow) {
>     var browsers = gBrowser.browsers;
>     for (var i = 0; browsers.item(i); i++)
>       if (browsers[i].contentWindow == aWindow)
>         return true;
>     return false;
>+  },
>+
Nit: file style is not to have blank lines in object literals.

>+  getNotificationBox : function(aWindow)
>+  {
>+    var foundBrowser = gBrowser.getBrowserForDocument(aWindow.document);
>+    if (foundBrowser)
>+      return gBrowser.getNotificationBox(foundBrowser)
>+    return null;
>   }
Nit: getBrowserForDocument was only created for the convenience of ported Firefox extensions. (Mind you, the isTabContentWindow loop is out of date too, it should use i < browsers.length). You can also use browsers[i].parentNode to refer to the notificationbox.
Attachment #328789 - Flags: review?(bugzilla) → review+
Comment on attachment 330302 [details] [diff] [review]
Patch v.3

I'm not sure I can review this: I'm not a toolkit/browser peer last I checked, and I just plain don't know the code you're touching.
Attachment #330302 - Flags: review?(bzbarsky)
(Assignee)

Comment 17

10 years ago
Comment on attachment 330302 [details] [diff] [review]
Patch v.3

(bz r- the IDL change in earlier discussions on IRC. So, I need to look for a new place to glue this interface.)
Attachment #330302 - Attachment is obsolete: true
(Assignee)

Comment 18

9 years ago
Created attachment 333012 [details] [diff] [review]
Patch v.4

Slightly different approach: avoids all the XPCOMmyness, and just hangs |getNotificationBox| directly on the window.
Attachment #328789 - Attachment is obsolete: true
Attachment #328955 - Attachment is obsolete: true
Attachment #333012 - Flags: review?(gavin.sharp)
Comment on attachment 333012 [details] [diff] [review]
Patch v.4

>diff --git a/toolkit/components/passwordmgr/src/nsLoginManagerPrompter.js b/toolkit/components/passwordmgr/src/nsLoginManagerPrompter.js

>             if (notifyWindow.opener) {
>+                var chromeWin = getChromeWindow(notifyWindow);
>+                var chromeDoc = chromeWin.document.documentElement;

Just use:

var chromeDoc = getChromeWindow(notifyWindow).document.documentElement;

here to avoid redeclaring chromeWin?
Attachment #333012 - Flags: review?(gavin.sharp) → review+
I noticed that getBrowser() and getNavToolbox() have been deprecated in favor of "smart" properties. However, other getXxx() functions are still being used so we might be safe.
(Assignee)

Comment 21

9 years ago
Fixed gavin's nit, and pushed changeset f3577b7629fb.
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
(Assignee)

Comment 22

9 years ago
Created attachment 333312 [details] [diff] [review]
Patch for SeaMonkey, v.2

Updated patch for SeaMonkey.
Attachment #333312 - Flags: review?(neil)
(Assignee)

Comment 23

9 years ago
Created attachment 333314 [details] [diff] [review]
Patch for Prism, v.2

Updated patch for Prism.

(Looks like the old patch was already landed, so this reverts the old version too.)
Attachment #333314 - Flags: review?(matthew.gertner)
Comment on attachment 333312 [details] [diff] [review]
Patch for SeaMonkey, v.2

>+function getNotificationBox(aWindow)
>+{
>+  var foundBrowser = gBrowser.getBrowserForDocument(aWindow.document);
>+  if (foundBrowser)
>+    return gBrowser.getNotificationBox(foundBrowser)
>+  return null;
>+}

This won't work for the sidebar, but I think this might:
return aWindow.QueryInterface(Components.interfaces.nsIInterfaceRequestor)
              .getInterface(Components.interfaces.nsIWebNavigation)
              .QueryInterface(Components.interfaces.nsIDocShell)
              .chromeEventHandler.parentNode.wrappedJSObject;

Updated

9 years ago
Attachment #333314 - Flags: review?(matthew.gertner) → review+
Comment on attachment 333312 [details] [diff] [review]
Patch for SeaMonkey, v.2

Please comment this as API used by login manager.

What's the bug# for the sidebar bug?
Attachment #333312 - Flags: review?(neil) → review+
Landed Prism patch #2 (333314)
Completed: At revision: 17712  
(Assignee)

Comment 27

9 years ago
(In reply to comment #25)

> What's the bug# for the sidebar bug?

Bug 430959. You're already CC'd and have commented there. :) 

(Assignee)

Comment 28

9 years ago
I suppose this new API should be documented somewhere, but not sure where... http://developer.mozilla.org/En/XUL:notificationbox ?
Keywords: dev-doc-needed
(In reply to comment #28)
> I suppose this new API should be documented somewhere, but not sure where...
> http://developer.mozilla.org/En/XUL:notificationbox ?
> 

How about:
http://developer.mozilla.org/en/Using_nsILoginManager
http://developer.mozilla.org/en/NsILoginManager
(Assignee)

Comment 30

9 years ago
Well, it's really a general API for getting at the notification box, not something that's pwmgr specific.
Question: if you call this method within a window (ie:

notifyBox = chromeWin.getNotificationBox(notifyWindow);

What's the difference between chromeWin and notifyWindow here?
chromeWin is the XUL window, the actual Firefox window.
notifyWindow is the web content window, displayed in each tab.
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.