Closed Bug 422974 Opened 12 years ago Closed 12 years ago

Prism uses old "Remember password?" mechanism

Categories

(Mozilla Labs :: Prism, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: matthew, Assigned: Dolske)

References

Details

(Keywords: dev-doc-complete)

Attachments

(3 files, 5 obsolete files)

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.
Blocks: 436054
Attached patch Patch v.1 (obsolete) — Splinter Review
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.
Attached patch Patch v.2 (obsolete) — Splinter Review
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)
Attached patch Patch for SeaMonkey, v.1 (obsolete) — Splinter Review
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)
Once this lands, please assign to me so I can fix Prism. !Gracias!
OS: Windows Vista → All
Hardware: PC → All
Attached patch Patch for Prism (obsolete) — Splinter Review
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)
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+
Attached patch Patch v.3 (obsolete) — Splinter Review
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?
Attachment #330302 - Flags: review? → review?(bzbarsky)
(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 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 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 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)
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
Attached patch Patch v.4Splinter Review
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.
Fixed gavin's nit, and pushed changeset f3577b7629fb.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Updated patch for SeaMonkey.
Attachment #333312 - Flags: review?(neil)
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;
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  
(In reply to comment #25)

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

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

I suppose this new API should be documented somewhere, but not sure where... http://developer.mozilla.org/En/XUL:notificationbox ?
Keywords: dev-doc-needed
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.
You need to log in before you can comment on or make changes to this bug.