563274 - Overhaul nsIPrompt-related prompting code

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Justin Dolske [:Dolske]]

The current prompting-related code, centering around nsIPrompt and it's dozen-or-so related interfaces, is a confusing, complicated mess. It's overdue for some cleanup and simplification, and this paves the way for a tab-modal prompt implementation.

General approach:

* Move most of the code from /embedding to a new /toolkit JS component.
* Greatly simplify the code, so that the number of steps between, say, nsGlobalWindow::Alert() and actually showing a prompt is less ridiculous.
* Get rid of the wrappers that allow implementing nsIAuthPrompt2 with an nsIAuthPrompt implementation.
* Modify commonDialog.xul / commonDialog.js to accept/return arguments via a nsIPropertyBag, instead of nsIDialogParamBlock (which is, quite possibly, the worst interface I've ever seen).

For backwards compatability reasons, the /embedding (window watcher) code will retain a few of these interfaces, which will simply delegate to the new component. We should update docs and m-c code to avoid wwatcher, but this prevents needlessly breaking extensions for now.

Apps embedding Gecko will need modified to work with the new world (eg, by having a their own component that overrides the new /toolkit component), but overall I think this is a big win -- the code is _much_ more understandable and easier to work with. In a followup bug I'll be implementing a new set of interfaces for the standard web-content dialogs [alert(), confirm, prompt()], such that embedders who are primarily interested in those (and not the full complexity of nsIPrompt) can do so with a fairly trivial component. I'll probably whip up a js-ctypes version for GTK as an example.

Comment 1

[Boris Zbarsky [:bzbarsky]]

So we're basically breaking the frozen prompting interfaces, right?  We should probably announce this widely _now_ so embeddors can start redesigning around the new APIs...

I can only cheer on the rest of this proposal.  ;)

Comment 2

[Justin Dolske [:Dolske]]

Attachment: Patch v.0 (WIP)

alert() and confirm() are working, other stuff probably isn't. :)

Comment 3

[Justin Dolske [:Dolske]]

Yeah, there's going to be some thawing of frozen things, although I'm trying to minimize the impact where possible. I'd like to flesh out a few more details before announcing anything, lest that cause needless confusion as to what's happening. But, yes, soon.

Comment 4

[Justin Dolske [:Dolske]]

Attachment: Patch v.1 (WIP)

Mostly working, still things to finish and cleanup.

Comment 5

[Justin Dolske [:Dolske]]

Attachment: Patch v.2

Pretty close to being done here, confirmEx() and select() now working, and this passes the login manager auth/prompt tests.

Comment 6

[Justin Dolske [:Dolske]]

Attachment: Patch v.3

There are a couple of rough spots still here, but it's complete and ready for a review pass. Note that applying this patch requires the application of the bugs in the dependency list.

Not sure how to handle reviews for moving from embedding to toolkit... Let's say bsmedberg for sr and "ok to change embedding", and gavin for r on the toolkit prompt implementation?

Also, bug 548847 is likely to land first, so I'll need to deal with the changes there in an update to this patch, but there's enough meat here to review before adding E10S complexity. :)

A few comments on the patch:

* I think I've avoided changing any interfaces or breaking any callers (other than embedding), with a few rare exceptions. I'd still like to fix a few of these awkward things (notably the wrappers for using a nsIAuthPrompt as a nsIAuthPrompt2), but better to do that separately.

* As-is, embedders would need to provide their own version of nsPrompter.js (probably via js-ctypes, since a lot of this is just setting up string stuff). If needed, the ModalPrompter implementation in nsPrompter.js could be spun off to an embedder-overridable XPCOM component. That would reduce the amount of embedders' code, but adds another XPCOM call and makes it more awkward to do future prompt changes.

* The existing test_authentication.js and test_authpromptwrapper.js tests have slight changes to make them pass. This basically makes them compatible with the host/realm formats that login manager changed a year or two ago, so I don't think this is likely to break anybody.

* There's one change to test_modal_prompts.html (added by me in bug 564724), which fixes a bug in the current Select() implementation for an edge case (select with zero selectable items). Didn't seem work adding a hack to emulate the current behavior.

Comment 7

[Benjamin Smedberg]

Comment on attachment 445243 [details] [diff] [review]
Patch v.3

Doesn't look like this piece involves any real API changes. I suggested to dolske on IRC that we use toolkit/prompts instead of toolkit/components/prompted, but Mossop should make the final call since he's module owner now.

Comment 8

[Dave Townsend [:mossop]]

Having the difference between components and mozapps is kinda sucky but I do prefer separating out each individual component from the other top level directories in toolkit, so go with toolkit/components/prompts.

Comment 9

[Justin Dolske [:Dolske]]

Smokey / smorgan: any concerns with this patch, from an embedding pov?

Comment 10

[Stuart Morgan]

I'm not at all clear on what the impact to Camino is (probably because I don't really know the nsIPrompt code); "would need to provide their own version of nsPrompter.js", for example, doesn't really mean anything to me.

We already register our own prompt services (embedcomp/prompt-service, passwordmanager/authpromptfactory, etc.). Given that, what does this patch break?

Comment 11

[Wladimir Palant]

Comment on attachment 445243 [details] [diff] [review]
Patch v.3

Just a tiny nit:

>+function EmbedPrompter() {
>+    this.init();
>+}
>+EmbedPrompter.prototype = new Prompter();

Why not |EmbedPrompter.prototype = {__proto__: Prompter.prototype}|? I realize that Prompter.init() isn't doing much at the moment but if that ever changes you probably won't want to call it on the prototype object.

Comment 12

[Justin Dolske [:Dolske]]

(In reply to comment #10)

> We already register our own prompt services (embedcomp/prompt-service,
> passwordmanager/authpromptfactory, etc.). Given that, what does this patch
> break?

I think what you have should be sufficient, but it's possible it might need some small tweaking to work... I don't _think_ I've broken the embedding case yet, but it would be really helpful if someone embedding Gecko could help verify that.

(In reply to comment #11)

> >+EmbedPrompter.prototype = new Prompter();
> 
> Why not |EmbedPrompter.prototype = {__proto__: Prompter.prototype}|?

Hmm, I can look at that, I just wasn't trying hard to share the same proto chain.

Comment 13

[Stuart Morgan]

Unfortunately we're not buildable on top of 1.9.3 yet, so I can't just build and see.

Comment 14

[Jeff Walden [:Waldo]]

(In reply to comment #11)
> Why not |EmbedPrompter.prototype = {__proto__: Prompter.prototype}|?

__proto__ as a special property is moving toward deprecation; it's non-standard, has an equally powerful standardized alternative for getting (Object.getPrototypeOf), has a nearly-as-functional standardized alternative for setting (Object.create(proto)), and introduces fugliness in creating your own string-valued hash.  Use Object.create(Prompter.prototype) if you want to do this or something like it, and don't use __proto__.

The day when we can remove __proto__ is probably years off (certainly no earlier than the Firefox version after 4.0, when Object.create will first be available), but best practice is to not use it these days -- particularly in the browser where backward compatibility is not a concern.

Comment 15

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 445243 [details] [diff] [review]
Patch v.3

>diff --git a/embedding/components/windowwatcher/public/nsIAuthPromptWrapper.idl b/embedding/components/windowwatcher/public/nsIAuthPromptWrapper.idl
>deleted file mode 100644

Looks to me like this is implemented by Camino, so landing this will presumably break them. I think this patch in general means they'll need to implement nsIPrompt/nsIAuthPrompt (and optionally nsIAuthPrompt2) themselves and move their nsIAuthPromptWrapper logic to their nsIAuthPrompt implementation. They'll also need to add nsIPromptFactory to their nsIPromptService implementation, and maybe also nsIAuthPromptAdapterFactory.

>diff --git a/embedding/components/windowwatcher/src/nsWindowWatcher.cpp b/embedding/components/windowwatcher/src/nsWindowWatcher.cpp

NS_NewAuthPrompter2() reference in nsLoginManagerPrompter.js needs updating.

> nsWindowWatcher::GetPrompt(nsIDOMWindow *aParent, const nsIID& aIID,
>                            void **_retval)

>+  nsresult rv;
>+  nsCOMPtr<nsIPromptFactory> factory = do_GetService("@mozilla.org/prompter;1", &rv);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  return factory->GetPrompt(aParent, aIID, _retval);

For nsIAuthPrompt2 to continue to be optional, I think this needs to fall back to calling nsIAuthPromptAdapterFactory::createAdapter if getPrompt(nsIAuthPrompt2) fails. Or maybe we don't care?

>diff --git a/netwerk/base/src/nsPACMan.cpp b/netwerk/base/src/nsPACMan.cpp

> nsPACMan::GetInterface(const nsIID &iid, void **result)

>+  if (iid.Equals(NS_GET_IID(nsIAuthPrompt))) {
>+    // XXX Is this right?
>+    nsCOMPtr<nsIAuthPromptProvider> promptProvider = do_GetService("@mozilla.org/prompter;1");
>+    NS_ENSURE_TRUE(promptProvider, NS_ERROR_FAILURE);
>+    return promptProvider->GetAuthPrompt(nsIAuthPromptProvider::PROMPT_NORMAL, iid, result);

I don't think this is right. I think this just wants to call nsIPromptFactory::GetPrompt() (the old code boiled down to just calling NS_NewAuthPrompter via nsDefaultAuthPromptConstructor). (see also comment below about nsIAuthPromptProvider)

>diff --git a/netwerk/build/nsNetCID.h b/netwerk/build/nsNetCID.h

>-#define NS_AUTHPROMPT_ADAPTER_FACTORY_CONTRACTID \
>-  "@mozilla.org/network/authprompt-adapter-factory;1"

Wouldn't it be best to keep this contract ID, and keep its implementation in a separate component from Prompter? There's no real benefit to merging it with the Prompter() component, AFAICT, and it would mean fewer changes (e.g. no need to change nsNetUtil.h).

>diff --git a/toolkit/content/commonDialog.js b/toolkit/components/prompter/content/commonDialog.js

>-/* -*- Mode: C;  c-basic-offset: 2; tab-width: 2; indent-tabs-mode: nil; -*- */

why? (same comment applies to selectDialog.js)

>-function showControls()

>-  var nTextBoxes = gCommonDialogParam.GetInt(3);
>-  if (nTextBoxes == 2) {
>-    if (gCommonDialogParam.GetInt(4) == 1) {
>-      initTextbox("password1", 4, 6, false);
>-      initTextbox("password2", 5, 7, false);

Looks like password2 was unused (and still is with your patch), since nsPromptService only passes GetInt(eEditField1Password [4]) == 1 if (GetInt(eNumberEditfields [3]) (nTextBoxes) == 1) (talk about confusing), so you should just remove it from the XUL as well (along with associated CSS/string/etc.).

>+function earlyInit() {

>      case "confirmEx":

Missing a hasInputField = false; to match previous behavior.

>+      case "promptPassword":
>+        numButtons = 2;
>+        iconClass  = "authentication-icon question-icon";
>+        soundID    = Ci.nsISound.EVENT_PROMPT_DIALOG_OPEN;
>+        initTextbox("password1", gArgs.getProperty("pass"));

Hmm, the old code effectively cleared the label here (as you do for prompt()). Was this an intentional change? It makes sense, I guess, but the label alignment seems a bit off on Mac...

>+function commonDialogOnLoad() {

>+    if (gArgs.hasKey("titleText"))
>+        setElementText("info.header", gArgs.getProperty("titleText"), true); // XXX does anything use this?

Looks like no - let's just remove it?

>+function setElementText(aElementID, aValue, aChildNodeFlag) {
>+    let element = document.getElementById(aElementID);
>+    if (!aChildNodeFlag && element)
>+        setLabelForNode(element, aValue, true);
>+    else if (aChildNodeFlag && element)
>+        element.appendChild(document.createTextNode(aValue));

early return if !element to simplify?

>+function setCheckbox(label, checked) {
>+    // Don't do anything if label is empty.
>+    if (label) {

early return?

>+function commonDialogOnAccept() {

>+    let field1 = document.getElementById("loginTextbox");
>+    let field2 = document.getElementById("password1Textbox");

how about user/pass for variable names?

>+// XXX SOB! leave off a case colon and XUL borks the loading of the while JS
>+// file without an error!

I've been noticing problems like this more often lately, and they always end up being a pain to write a testcase for :(

>diff --git a/toolkit/content/selectDialog.js b/toolkit/components/prompter/content/selectDialog.js

>+function dialogOnLoad() {

>+    for (let i = 0; i < items.length; i++) {
>+        let str = items[i];
>+        if (str == "")
>+            str = "<>";
>+        listBox.appendItem(str);
>+        listBox.getItemAtIndex(i).addEventListener("dblclick", dialogDoubleClick, false);

Can't you just add the listener to the entire listbox instead (in the xul, even)?

>+    // play sound
>+    try {
>+        if (soundID) {

Mis-copy from the other file - no soundID here.

Wow, this file was horrible before.

>diff --git a/toolkit/components/prompter/src/nsPrompter.js b/toolkit/components/prompter/src/nsPrompter.js

> Prompter.prototype = {

>+    QueryInterface   : XPCOMUtils.generateQI([Ci.nsIPrompter, Ci.nsIPromptFactory, Ci.nsIAuthPromptAdapterFactory,
>+                                              Ci.nsIAuthPromptProvider, Ci.nsIPromptService, Ci.nsIPromptService2]),

>     /* ----------  nsIPrompter  ---------- */

Get rid of this? (it doesn't exist, doesn't seem necessary)

>+    /* ----------  nsIAuthPromptProvider ---------- */
>+    getAuthPrompt : function (reason, iid) {
>+        this.log("getAuthPrompt() for iid=" + iid);
>+        // TODO who calls this? what to do with reason?

I don't think this component should implement nsIAuthPromptProvider at all - the old code didn't. It seems to be used to obtain a prompt from a channel's notificationCallbacks, and the only current implementation I see (nsDocShell::GetAuthPrompt) just forwards to GetPrompt().

>+    /* ----------  nsIPromptService  ---------- */
>+    /* ----------  nsIPromptService2  ---------- */

Seems like these could benefit from something like:

var nsIPromptServiceMethods =
      ["alert", "alertCheck", "confirm", "confirmCheck", "confirmEx", "prompt",
       "promptUsernameAndPassword", "promptPassword", "select"];
nsIPromptServiceMethods.forEach(function (fn) {
  Prompter.prototype[fn] = function () {
    let p = new ModalPrompter(arguments[0]);
    p[fn].apply(p, Array.slice(arguments, 1));
  }
});

>+let PromptUtils = {

>+    _strBundle : null, // String bundle for L10N
>+    get strBundle() {

>+    _ellipsis : null,
>+    get ellipsis() {

These should use XPCOMUtils.defineLazyGetter

>+    fireEvent : function (domWin, eventType) {
>+        // XXX main use of DOMWillOpenModalDialog is so tabbrowser can focus
>+        //     the tab opening the modal prompt. DOMModalDialogClosed is unused.

see bug 429287

>+ModalPrompter.prototype = {

>+     * ---------- interface disambiguation ----------

>+    promptUsernameAndPassword : function() {
>+        if (arguments.length == 6)

>+    promptPassword : function() {
>+        if (arguments.length == 5)

This doesn't actually work, AFAICT, since both methods take the same number of arguments on both interfaces...

>+    /* ----------  nsIPrompt  ---------- */

>+    confirmCheck : function (title, text, checkLabel, checkValue) {
>+        if (!title)
>+            title = PromptUtils.getLocalizedString("Confirm");

Old code uses "ConfirmCheck" (though it's the same string...)

>+        // Did user click Ok or Cancel?
>+        let ok  = args.getProperty("ok");
>+        return ok;

put this on a single line?

>+    confirmEx : function (title, text, flags, button0, button1, button2,
>+                          checkLabel, checkValue) {

>+        args.setProperty("buttonNumClicked", 1);

Why set this?

>+        // Get the number of the button the user clicked.
>+        let bNum = args.getProperty("buttonNumClicked");
>+        return bNum;

single line?

>+    select : function (title, text, count, list, selected) {

>+        if (ok)
>+            selected.value = args.getProperty("selected");

Old code set this unconditionally (though I guess the selectDialog.js code didn't set the index in the cancel case anyways, so maybe it doesn't matter...)

>+    asyncPromptAuth : function (channel, callback, context, level, authInfo, checkLabel, checkValue) {
>+        dump("------- asyncPromptAuth --------\n");
>+        // XXX I don't think anything calls this directly?

It's called by nsHTTPChannel with a fallback to the sync version. Should lose the dump();

>+AuthPromptAdapter.prototype = {
>+    classDescription : "AuthPromptAdapter",
>+    contractID       : "@mozilla.org/prompter/authprompt-adapter;1",
>+    classID          : Components.ID("{76a140a1-da97-4dd5-8e18-6e9c618ada18}"),
>+    QueryInterface   : XPCOMUtils.generateQI([Ci.nsIAuthPrompt2]),

I think you don't actually need to give this a contractID and register it as its own component, right? No one needs to createInstance it, so you can just give it a QI and return it from createAdapter()?

+EmbedPrompter.prototype.contractID       = "@mozilla.org/embedcomp/prompt-service;1";

Throw a "NS_PROMPTSERVICE_CONTRACTID" here in a comment just for greppability?


r- for the argument-differentiation problem, and because I'm not completely done reviewing but wanted to get the comments I have submitted so you can start addressing them.

Comment 16

[:Gavin Sharp [email: [e-mail]]]

Finished reviewing the patch, here are the rest of the comments:

>+    makeAuthMessage : function (channel, authInfo) {

>+        let [username, password]   = this.getAuthInfo(authInfo);

Omit password since it is unused?

The old code just always uses authInfo.username, whereas getAuthInfo() can include the domain if (flags & NEED_DOMAIN). That's probably not a big deal, I guess, but just using authInfo.username directly and avoiding the call to getAuthInfo is simpler...

>+        // Suppress "the site says: $realm" when we synthesized a missing realm.
>+        if (!authInfo.realm && !isProxy)
>+            realm = null;

Why the !isProxy check?

>+        // Trim obnoxiously long realms.
>+        if (realm.length > 150) {
>+            realm = realm.substring(0, 150);
>+            // Append "..." (or localized equivalent).
>+            realm += this.ellipsisl

typo: "ellipsisl".

+ModalPrompter.prototype = {

>+    promptAuth : function (channel, level, authInfo, checkLabel, checkValue) {

+        if (authInfo.flags & Ci.nsIAuthInformation.ONLY_PASSWORD)
+            ok = this.promptPassword(null, message, passParam, checkLabel, checkValue);
+        else
+            ok = this.promptUsernameAndPassword(null, message, userParam, passParam, checkLabel, checkValue);

This can call the nsIPrompt_* versions directly, right?

+AuthPromptAdapter.prototype = {

>+    promptAuth : function (channel, level, authInfo, checkLabel, checkValue) {

>+        let [username, password] = PromptUtils.getAuthInfo(authInfo);
>+        let userParam = { value: username };
>+        let passParam = { value: password };

The old code doesn't seed in the current values before delegating to oldPrompter, but I guess that can be considered a bug fix. nit: group these lines together (as quoted)?

>+        let [host, realm]  = PromptUtils.getAuthTarget(channel, authInfo);
>+        let authTarget = host + " (" + realm + ")";

The old code uses NS_GetAuthKey, which special cases non-HTTP channels to just return .URI.prePath (i.e. excludes the realm) - should that happen here too?

Comment 17

[Justin Dolske [:Dolske]]

> Looks to me like this is implemented by Camino, so landing this will presumably
> break them. I think this patch in general means they'll need to implement
> [...]

I think most of that is already the case. An alternative (better?) split would be for embedders like Camino to provide their own replacement for the ModalPrompter implementation in nsPrompter.js. I'm open for ideas on how to make that easier, though it doesn't need to happen here.


> > nsWindowWatcher::GetPrompt(nsIDOMWindow *aParent, const nsIID& aIID,
> >                            void **_retval)
> 
> >+  nsresult rv;
> >+  nsCOMPtr<nsIPromptFactory> factory = do_GetService("@mozilla.org/prompter;1", &rv);
> >+  NS_ENSURE_SUCCESS(rv, rv);
> >+  return factory->GetPrompt(aParent, aIID, _retval);
> 
> For nsIAuthPrompt2 to continue to be optional, I think this needs to fall back
> to calling nsIAuthPromptAdapterFactory::createAdapter if
> getPrompt(nsIAuthPrompt2) fails. Or maybe we don't care?

Don't care here, since it's using our implementation which will support nsIAuthPrompt2. [I'd actually like to move to making nsIAuthPrompt2 non-optional, but that can happen in another bug.]


> > nsPACMan::GetInterface(const nsIID &iid, void **result)
> 
> I think this just wants to call nsIPromptFactory::GetPrompt()

Yeah. It's ever so slightly odd to pass a nsIAuthPrompt through this (since I think there's been an artificial distinction between nsIPrompts and nsIAuthPrompts), but that's a distinction I want to remove.


> >diff --git a/netwerk/build/nsNetCID.h b/netwerk/build/nsNetCID.h
> 
> >-#define NS_AUTHPROMPT_ADAPTER_FACTORY_CONTRACTID \
> >-  "@mozilla.org/network/authprompt-adapter-factory;1"
> 
> Wouldn't it be best to keep this contract ID, and keep its implementation in a
> separate component from Prompter? There's no real benefit to merging it with
> the Prompter() component, AFAICT, and it would mean fewer changes (e.g. no need
> to change nsNetUtil.h).

There's actually no need to expose it to xpcom at all, other than for the sake of letting test_authpromptwrapper.js continue to test it... Having it nsPrompter allows the createAdapter there to just do "return new AuthPromptAdapter(oldPrompter);".


> >-/* -*- Mode: C;  c-basic-offset: 2; tab-width: 2; indent-tabs-mode: nil; -*- */
> 
> why? (same comment applies to selectDialog.js)

Just seems like we've tended to leave these out from more recent files, perhaps a correlation with me not using emacs.


> >+      case "promptPassword":
> 
> Hmm, the old code effectively cleared the label here (as you do for prompt()).
> Was this an intentional change? It makes sense, I guess, but the label
> alignment seems a bit off on Mac...

Not intentional, but it does make sense. Not sure what you mean about the label alignment, seems fine to me (it should be the same as promtUsernameAndPassword...).


> >+function setElementText(aElementID, aValue, aChildNodeFlag) {
...
> 
> early return if !element to simplify?

Actually, this was just used in 2 places and aChildNodeFlag was always true, so I just inlined it.


> >+function setCheckbox(label, checked) {
> >+    // Don't do anything if label is empty.
> >+    if (label) {
> 
> early return?

This was only used 1 place, so I inlined it as well.


> >+// XXX SOB! leave off a case colon and XUL borks the loading of the while JS
> >+// file without an error!
> 
> I've been noticing problems like this more often lately, and they always end up
> being a pain to write a testcase for :(

Yeah, I left this as a comment to remind me to file a bug for this. I always end up fixing the problem in my code, and then forget what it was that caused me to not get an error message. :/ Filed bug 571174 and removed the comment.


> >diff --git a/toolkit/content/selectDialog.js b/toolkit/components/prompter/content/selectDialog.js
> 
> >+function dialogOnLoad() {
...
> >+        listBox.getItemAtIndex(i).addEventListener("dblclick", dialogDoubleClick, false);
> 
> Can't you just add the listener to the entire listbox instead (in the xul,
> even)?

Don't think so, as that then doubleclicking on the whitespace below a short list would trigger a dialog submission with whatever row happened to be selected. Could check the event target, but eh...


> >     /* ----------  nsIPrompter  ---------- */
> 
> Get rid of this? (it doesn't exist, doesn't seem necessary)

Yeah, leftover from an early patch. I'm adding a new interface in a future (unfiled bug) but no need to stub it out here.


> >+    /* ----------  nsIAuthPromptProvider ---------- */
> >+    getAuthPrompt : function (reason, iid) {
> >+        this.log("getAuthPrompt() for iid=" + iid);
> >+        // TODO who calls this? what to do with reason?
> 
> I don't think this component should implement nsIAuthPromptProvider at all -
> the old code didn't. It seems to be used to obtain a prompt from a channel's
> notificationCallbacks, and the only current implementation I see
> (nsDocShell::GetAuthPrompt) just forwards to GetPrompt().

Hmm. I don't remember why I implemented this here. :/

 
> >+    /* ----------  nsIPromptService  ---------- */
> >+    /* ----------  nsIPromptService2  ---------- */
> 
> Seems like these could benefit from something like:
> 
> var nsIPromptServiceMethods =
>       ["alert", "alertCheck", "confirm", "confirmCheck", "confirmEx", "prompt",
> ...

Maybe, but bug 562258 is adding a TabPrompter (with selection between it and ModalPrompter at runtime). So I think it's best to leave as-is so the patch is a natural progression.


> >+    promptUsernameAndPassword : function() {
> >+        if (arguments.length == 6)
> >+    promptPassword : function() {
> >+        if (arguments.length == 5)
> 
> This doesn't actually work, AFAICT, since both methods take the same number of
> arguments on both interfaces...

Well, pewp. I missed that. I guess the equally sucky alternative is to look at the argument types. :/


> >+    confirmEx : function (title, text, flags, button0, button1, button2,
> >+                          checkLabel, checkValue) {
> 
> >+        args.setProperty("buttonNumClicked", 1);
> 
> Why set this?

So that if the window is closed, that's selected as the default action. And so getProperty() doesn't throw. (Same as other cases above, where "ok" is set to false).

Note that this is preserving the existing behavior of bug 345067.


> >+    select : function (title, text, count, list, selected) {
> 
> >+        if (ok)
> >+            selected.value = args.getProperty("selected");
> 
> Old code set this unconditionally (though I guess the selectDialog.js code
> didn't set the index in the cancel case anyways, so maybe it doesn't matter...)

This seems correct to me, if the user cancels the dialog it doesn't matter what index was select. If the old behavior was different, any code relying on that was very buggy.


> >+    asyncPromptAuth : function (channel, callback, context, level, authInfo, checkLabel, checkValue) {
> >+        dump("------- asyncPromptAuth --------\n");
> >+        // XXX I don't think anything calls this directly?
> 
> It's called by nsHTTPChannel with a fallback to the sync version. 

Hmm, no, not this particular implementation... AFAICT the network code always ends up going through nsIPromptFactory::GetPrompt, where we delegate nsIAuthPrompt[2] to login manager, which in turn will call nsIPromptService::PromptAuth but never nsIPromptService::AsyncPromptAuth.

This back-and-forth bouncing is quite confusing, which I would like to fix in bug 565582 (see details there, but basically it would move all the prompt interfaces from login manger to nsPrompter, and nsPrompter would drive login manager via events/notifications instead).

Comment 18

[Justin Dolske [:Dolske]]

Attachment: Patch v.4

Updated with gavin's first review chunk (comment 15 + my reply in comment 17). Bits from comment 16 not yet addressed.

Comment 19

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #17)
> I think most of that is already the case.

I was listing things that they'd need to implement that they don't already.

> Don't care here, since it's using our implementation which will support
> nsIAuthPrompt2.

Not for embedders. Keeping the fallback here until we make nsIAuthPrompt2 non-optional (which involves a bunch of other work AFAICT) isn't much trouble, so it seems reasonable.

> There's actually no need to expose it to xpcom at all, other than for the sake
> of letting test_authpromptwrapper.js continue to test it... Having it
> nsPrompter allows the createAdapter there to just do "return new
> AuthPromptAdapter(oldPrompter);".

I wasn't suggesting moving it to its own file - just register it separately (like EmbedPrompter).

> Maybe, but bug 562258 is adding a TabPrompter (with selection between it and
> ModalPrompter at runtime). So I think it's best to leave as-is so the patch is
> a natural progression.

I don't understand why my code makes that any harder to do. Wouldn't it even make it easier, since the run-time switching can be done in one place?

> Well, pewp. I missed that. I guess the equally sucky alternative is to look at
> the argument types. :/

Or use a separate component, I guess - type detection's probably better.

Comment 20

[Justin Dolske [:Dolske]]

(In reply to comment #16)

> >+        // Suppress "the site says: $realm" when we synthesized a missing realm.
> >+        if (!authInfo.realm && !isProxy)
> >+            realm = null;
> 
> Why the !isProxy check?

Proxy prompts always use the EnterLoginForProxy string, which expects a realm. Non-proxy prompts have an alternate string for when there's no realm.

> +        if (authInfo.flags & Ci.nsIAuthInformation.ONLY_PASSWORD)
> +            ok = this.promptPassword(...
> 
> This can call the nsIPrompt_* versions directly, right?

Yeah. Was thinking it was easier to read without that, but then it's harder to _understand_. Fixed.

> >+        let [host, realm]  = PromptUtils.getAuthTarget(channel, authInfo);
> >+        let authTarget = host + " (" + realm + ")";
> 
> The old code uses NS_GetAuthKey, which special cases non-HTTP channels to just
> return .URI.prePath (i.e. excludes the realm) - should that happen here too?

It shouldn't have any impact in our code, and I'd prefer to keep the authTarget string consistent with its format elsewhere.



(In reply to comment #19)

> Not for embedders. Keeping the fallback here until we make nsIAuthPrompt2
> non-optional (which involves a bunch of other work AFAICT) isn't much trouble,
> so it seems reasonable.

Hrm, ok.


> > AuthPromptAdapter
> 
> I wasn't suggesting moving it to its own file - just register it separately
> (like EmbedPrompter).

Ah, ok. Done.


> > Maybe, but bug 562258 is adding a TabPrompter (with selection between it and
> > ModalPrompter at runtime). So I think it's best to leave as-is so the patch is
> > a natural progression.
> 
> I don't understand why my code makes that any harder to do. Wouldn't it even
> make it easier, since the run-time switching can be done in one place?

Ah, yeah, this should work. I misread your code, will look at implementing this next.

Comment 21

[Justin Dolske [:Dolske]]

Attachment: Patch v.5

Updated with all above comments, except for the very last bit from comment 20.

Will do a tryserver run and make a final cleanup pass next.

Comment 22

[Justin Dolske [:Dolske]]

Attachment: Patch v.6

Ok, I think this addresses everything.

Comment 23

[Justin Dolske [:Dolske]]

Pushed http://hg.mozilla.org/mozilla-central/rev/dbe1e5c26aa3

\o/

Comment 24

[Nochum Sossonko [:Natch]]

> debug: true

You left it like that on m-c, was that intentional? It spews a "Prompter: initialized" message to the console and stdout.

Comment 25

[Justin Dolske [:Dolske]]

Oops, missed two incompatible tests. The first failed on tryserver, but it had also failed as a random orange so I didn't look close enough at it. :(

http://hg.mozilla.org/mozilla-central/rev/7f7ba2b353ac
http://hg.mozilla.org/mozilla-central/rev/49307f5b1c62

I'll fix and reenable 7f7ba2b353ac, along with the debug output, tomorrow.

Comment 26

[Justin Dolske [:Dolske]]

Eh, I just went ahead and removed the debug logging now:

http://hg.mozilla.org/mozilla-central/rev/18463a042fca