Closed Bug 563556 Opened 11 years ago Closed 11 years ago
Global Window's alert/confirm/prompt
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.
Attachment #443254 - Flags: review?(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.
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]
Attachment #443254 - Attachment is obsolete: true
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a5
You need to log in before you can comment on or make changes to this bug.