563556 - Cleanup nsGlobalWindow's alert/confirm/prompt

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Justin Dolske [:Dolske]]

Attachment: Patch v.1

The implementation of alert/confirm/prompt seem a little confusing, I'd like to clean them up to pave the way for other changes.

* Call into nsIPromptService directly, instead of doing a QI on the docshell to nsIPrompt (which ends up delegating to wwatcher's nsIPromptService anyway). 

* Prompt() is currently using an nsIAuthPrompt implementation, which seems rather odd. There's no reason for it not to just use nsIPromptService's Prompt (as alert/confirm do with their respective nsIPromptService clones).

* The nsIDOMWindowInternal IDL for Prompt() has two args I've never heard of before, and are ignored anyway. Changed IDL changed to match reality.
  -- bug 334893 comment 18 removed the title arg support
  -- bug 453571 seems to have just mechanically changed some vararg-like
     stuff to use [optional], presumably this just copied the existing args.
  -- I did some CVS digging, but don't really understand why these extra args
     ever existed. Some old Netscape-ism?
  -- Seems like the first arg shouldn't be [optional], but left it as-is to
     avoid any potential web-compat issues.

Comment 1

[Johnny Stenback (:jst)]

Comment on attachment 443254 [details] [diff] [review]
Patch v.1

+  nsCOMPtr<nsIPromptService> promptSvc = do_GetService("@mozilla.org/embedcomp/prompt-service;1");
+  NS_ENSURE_TRUE(promptSvc, NS_ERROR_FAILURE);

Maybe pass in &rv to do_GetService() and use NS_ENSURE_SUCCESS(rv, rv) here?

+  nsCOMPtr<nsIDOMWindow> domWin(do_GetInterface(mDocShell));
+  NS_ENSURE_TRUE(domWin, NS_ERROR_FAILURE);

No need for domWin here, you can simply pass |this|.

Both of those appear a couple of times.

r=jst with that fixed.

Comment 2

[Justin Dolske [:Dolske]]

Attachment: Patch v.2

Fixed nits, and also now passing in a dummy inout arg to promptSvc->Prompt(). [xpconnect was unhappy with the existing |null| when using the JS impl from bug 563274]

Comment 3

[Justin Dolske [:Dolske]]

Pushed http://hg.mozilla.org/mozilla-central/rev/7b02623b93fe