99615 - Freeze nsIPromptService

Status: RESOLVED FIXED

(Core Graveyard :: Embedding: APIs, defect)

Description

[Chak Nanga]

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.

Comment 1

[Conrad Carlen (not reading bugmail)]

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

Comment 2

[Conrad Carlen (not reading bugmail)]

-> 0.9.6

Comment 3

[David Epstein]

change qa contact to Dharma.

Comment 4

[Conrad Carlen (not reading bugmail)]

Mass move to 0.9.7

Comment 5

[Conrad Carlen (not reading bugmail)]

mass move to 0.9.8

Comment 6

[Conrad Carlen (not reading bugmail)]

Mass move to 0.9.9

Comment 7

[timeless]

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.

Comment 8

[Judson Valeski]

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.

Comment 9

[Boris Zbarsky [:bzbarsky]]

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

Comment 10

[timeless]

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

Comment 11

[Conrad Carlen (not reading bugmail)]

> 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.

Comment 12

[Aaron Leventhal]

Related bugs:

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

Comment 13

[Conrad Carlen (not reading bugmail)]

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.

Comment 14

[Conrad Carlen (not reading bugmail)]

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

Comment 15

[timeless]

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

Comment 16

[Conrad Carlen (not reading bugmail)]

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

Comment 17

[Christian :Biesinger (don't email me, ping me on IRC)]

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

Comment 18

[Christian :Biesinger (don't email me, ping me on IRC)]

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.

Comment 19

[timeless]

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.

Comment 20

[Darin Fisher]

-> 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.

Comment 21

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #20)
> if someone wants to wack nsIPromptService, then invent nsIPromptService2.

that is, in fact, exactly what bug 228207 is about ;)

Comment 22

[Darin Fisher]

Attachment: v1 patch

Comment 23

[Darin Fisher]

Attachment: v1.1 patch

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

Comment 24

[Darin Fisher]

Attachment: v1.2 patch

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

Comment 25

[timeless]

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)

Comment 26

[Darin Fisher]

timeless: yup, i'll make those additions.

Comment 27

[Boris Zbarsky [:bzbarsky]]

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()

Comment 28

[Darin Fisher]

Attachment: v1.3 patch

revised

Comment 29

[Boris Zbarsky [:bzbarsky]]

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

sr=bzbarsky

Comment 30

[Darin Fisher]

fixed-on-trunk

i'll send mail out to the appropriate newsgroups

Comment 31

[Christian :Biesinger (don't email me, ping me on IRC)]

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 32

[Mike Kaply [:mkaply]]

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

a=mkaply

Comment 33

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: 1.7 branch patch

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

Comment 34

[Christian :Biesinger (don't email me, ping me on IRC)]

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