Freeze nsIPromptService Please refer to http://www.mozilla.org/projects/embedding/EmbedInterfaceFreeze.html for the issues to be addressed, if any, for this interface. Please follow the guidelines outlined in "How to mark an interface as frozen?" section of the document above.

Accepting. This one is going to involve some work. See bug 95649 for how this API is wrong from a UI standpoint.

-> 0.9.6

change qa contact to Dharma.

Mass move to 0.9.7

mass move to 0.9.8

Mass move to 0.9.9

i hate to say this, but I'd like to sign off on this interface before it freezes. I consider myself to be both a stake holder and a concerned party.

removing topembed+ (as this currently isn't considered a stopper for mozilla1.0. we're not going to be able to fix *everything* related to API freezing), adding embed (as this is something critical to the embedding API). timeless, please post your feedback regarding the current patch/approach ASAP (rather than later). consider this fair warning that this is going to freeze. if you raise your concerns just before we're ready to check in, considering them will be more difficult.

See bug 50348 for another possible problem with nsIPromptService (the ability to tell alert() whether to preserve whitespace may need to be added).

this is not a complete list, it's just a startingpoint. 1. the api currently doesn't handle accesskeys 2. the api implementation doesn't work well with alerts 3. some api derivative hasn't managed to show a lock icon for password requests more later

> 1. the api currently doesn't handle accesskeys Aaron is working on that. What's the bug #? > 2. the api implementation doesn't work well with alerts > 3. some api derivative hasn't managed to show a lock icon for password requests See bug 95649. This bug is for freezing the iface. Problems with the iface (the reason it should not be frozen) block this bug and should be hashed out there.

Related bugs: - bug 134066 - make nsIPromptService support accesskey - bug 133582 - nsIPromptService dialogs with accesskeys should not put initital focus on checkbox

Removing bug 95649 from blockers. I agree completely with that bug but this iface is too widely used for too long by too many embedding clients. We can make a new iface, convert internal callers to it, and provide a glue layer for embeddors who haven't implemented the new iface.

Also removing bug 50348 from the blockers since that is about the implementation of the prompt service, not its API.

if you're going to freeze this interface in such a manner, please rename it so we aren't forced to version it poorly.

Version it poorly? The new interface won't be derived from nsIPromptService. I don't see where versioning will happen.

What about not freezing this interface just yet and instead fix the issues with it first.

as for your removal of bug 95649... if embeddors were using an unfrozen interface, they have to expect it changing. I do not agree with removing it from the list this bug depends on.

So valeski asked me to explain what's wrong with this interface (comment 8). conrad said that he doesn't want this bug to contain dicussion about the poor interface (comment 11), and that he doesn't care that this interface needs to be replaced (comment 13). mpt wrote in Bug 95649 Comment 17 "No, it's a major bug." see attachment 48155 [details] A utopian redesign of the nsPromptService API. What I want: nsIPromptService should not have the flaws already described in other bugs and should be guided by the api design done for bug 95649. The api should also address the other concerns such as bug 50348. What I don't want is for our frozen interface to not address those bugs and get to own the name nsIPromptService. My suggestion is that we don't freeze the prompt service until we at least address the preceding. failing that, we could call it nsIFrozenEmbeddingPromptService1 but that's long and bulky and no one would really like it. There really are lots of problems with the interface, even mpt's utopian redesign doesn't solve all of them. alecf and sfraser/pinkerton in their embedding work have shown some of the failings. in the current model we have something like this: event creator needs to create an event. that coder picks an event type. that coder asks for strings. that coder makes a function call to the prompt service. the embedder can't do much with any of this because everything is localized strings. it's cost prohibitive and nonsensical for an embedding implementation to get the same strings from the bundle again to do comparisons to decide to drop a dialog. it just doesn't fit the embedder's world view. an embedder might want to add additional text (like a recommendation) or totally change the behavior of the triggered event. there's really no reason (other than current api design) for the event creator to be responsible for retrieving strings from some service and passing them to the prompt service. one result is that alecf had to make a special string bundle service which allows for all the strings to live in one place. it still doesn't solve sfraser/pinkteron's issues that the coder makes bad decissions about ui type. it also means a lot of string manipulation in a place where it really just shouldn't be. what should happen is event authors have a component name, component event, requestor and details. name:<networking:http>, event:<connection failure>, origin:<inline-image>, opaque:{site:"http://localhost.remotenet", trigger:<timeout>}. The prompt service is given those things and then collects whatever it does from its implementation. If the Mac OS X gecko embedding client wants to include a string suggesting what to do when a connection fails then it can, because it's responsible for *all* of the localizable strings. the event creator is only responsible for strings specific to the instance of the event. the prompt service implementation can then decide to change how a specific dialog is rendered. pinkerton's team could decide that something is actually informational instead of an error, to drop it entirely, or log it somewhere. (especially for the example here, they'd match <connection failure> & <inline-image> and decide to drop the error entirely.) the current prompt service provides details with the expectation that the prompt service implementation will just paint them and would never want to drop, filter, rearrange, supplement or otherwise interpret them. this isn't the case. note that the real API could use constants instead of strings for many of the fields, that's a detail we'd hash out later when people are willing to work on designing a good interface. Unfortunately, my utopian interface isn't going to happen anytime soon. I'm not going to hold freezing nsIPromptService for it, but I don't want my interface to be nsIPromptService3 or 4 or 5. We should at least have something reasonable instead of rushing to freeze thing one knowing that we'll need two more revisions.

-> me we should freeze this interface for mozilla 1.8, as is with no changes (except for comments if appropriate). if someone wants to wack nsIPromptService, then invent nsIPromptService2. embedders are using this interface in the wild, and i think it'd be in our best interest to preserve this one. timeless: we've long missed our chance to change nsIPromptService. what's done is done, but that doesn't mean that we shouldn't create a better prompt service interface as you suggest.

(In reply to comment #20) > if someone wants to wack nsIPromptService, then invent nsIPromptService2. that is, in fact, exactly what bug 228207 is about ;)

Improved version based on IRC conversation with timeless, plus "@status FROZEN"

forgot to include Makefile.in changes in the v1.1 patch.

Comment on attachment 165469 [details] [diff] [review] v1.2 patch i asked that we note that the impl may choose to provide or not provide a checkbox without regard to the actual parameters. > Because implementations of this interface may loosely interpret the various button types button types and other user interface elements (some impls may omit the window title, or checkboxes entirely)

timeless: yup, i'll make those additions.

Comment on attachment 165469 [details] [diff] [review] v1.2 patch >Index: nsIPromptService.idl >+ * Accesskeys can be attached to buttons and checkboxes by inserting an & >+ * before the accesskey character. "... in the checkbox message or button title." right? For that matter, is there a difference to call those by different names in the arg lists? Aren't they both simply the respective labels? If so, would it make sense to consistently call them such in arg lists and documentation? That wouldn't actually change the API (for JS _or_ C++ callers or implementors). >+ * Puts up an alert dialog with an OK button and a message with a checkbox. Isn't that more like "an OK button and a labeled checkbox" >+ * Contains the default value of the checkbox when this method is >+ * called and the chosen value after this method returns. Instead of "value", I would say "checked state" (and instead of "chosen", I would use "final", perhaps). >+ * Puts up a dialog with OK and Cancel buttons and a message with a single >+ * checkbox. Same comments as alertCheck() here (this is confirmCheck). >+ /** >+ * Button Title Flags "(used to set the labels of buttons in the prompt)" And perhaps "button label flags" would be a better description? >+ /** >+ * Button Default Flags ("used to select which button is the default one") >+ * @param aCheckValue >+ * Contains the default value of the checkbox when this method is >+ * called and the chosen value after this method returns. Again "checked state" may be better than "value" (this is confirmEx). Same for prompt(), promptUsernameAndPassword(), promptPassword()

revised

Comment on attachment 166146 [details] [diff] [review] v1.3 patch sr=bzbarsky

fixed-on-trunk i'll send mail out to the appropriate newsgroups

Comment on attachment 166146 [details] [diff] [review] v1.3 patch it would be nice to have this on the 1.7 branch - it adds this interface to the gecko sdk, which makes it more useful. there is no risk since this patch only contains comment changes.

Comment on attachment 166146 [details] [diff] [review] v1.3 patch a=mkaply

1.3 patch didn't apply cleanly to 1.7 branch, this one does

I decided to leave in NS_PROMPTSERVICE_IID (contrary to what the last patch here shows), in order to avoid any changes that might break people using this. fixed on 1.7 branch