Created attachment 443254 [details] [diff] [review] 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 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.
Created attachment 445148 [details] [diff] [review] 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]