Cleanup nsGlobalWindow's alert/confirm/prompt

RESOLVED FIXED in mozilla1.9.3a5

Status

()

Core
DOM
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: Dolske, Assigned: Dolske)

Tracking

Trunk
mozilla1.9.3a5
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

8 years ago
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.
Attachment #443254 - Flags: review?(jst)

Updated

8 years ago
Attachment #443254 - Flags: review?(jst) → review+
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.
(Assignee)

Comment 2

8 years ago
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]
Attachment #443254 - Attachment is obsolete: true
(Assignee)

Comment 3

8 years ago
Pushed http://hg.mozilla.org/mozilla-central/rev/7b02623b93fe
Status: NEW → RESOLVED
Last Resolved: 8 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.