Closed Bug 99615 Opened 19 years ago Closed 16 years ago

Freeze nsIPromptService

Categories

(Core Graveyard :: Embedding: APIs, defect)

x86
Windows 2000
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla1.8beta1

People

(Reporter: chak, Assigned: darin.moz)

References

Details

(Keywords: embed, fixed1.7.5, topembed-)

Attachments

(2 files, 3 obsolete files)

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.
Status: NEW → ASSIGNED
Depends on: 95649
Target Milestone: --- → mozilla0.9.5
Blocks: 98417
QA Contact: mdunn → depstein
-> 0.9.6
Target Milestone: mozilla0.9.5 → mozilla0.9.6
change qa contact to Dharma.
QA Contact: depstein → dsirnapalli
Mass move to 0.9.7
Target Milestone: mozilla0.9.6 → mozilla0.9.7
mass move to 0.9.8
Target Milestone: mozilla0.9.7 → mozilla0.9.8
Mass move to 0.9.9
Target Milestone: mozilla0.9.8 → mozilla0.9.9
Target Milestone: mozilla0.9.9 → mozilla1.0
Keywords: topembed
Keywords: topembedtopembed+
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.
Keywords: topembed+embed, topembed-
See bug 50348 for another possible problem with nsIPromptService (the ability to
tell alert() whether to preserve whitespace may need to be added).
Depends on: 50348
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.
No longer depends on: 95649
Target Milestone: mozilla1.0 → mozilla1.4alpha
Also removing bug 50348 from the blockers since that is about the implementation
of the prompt service, not its API. 
No longer depends on: 50348
Blocks: 157137
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.
Target Milestone: mozilla1.4alpha → mozilla1.5alpha
Blocks: 248683
-> 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.
Assignee: ccarlen → darin
Status: ASSIGNED → NEW
Target Milestone: mozilla1.5alpha → mozilla1.8beta
Status: NEW → ASSIGNED
(In reply to comment #20)
> if someone wants to wack nsIPromptService, then invent nsIPromptService2.

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

Blocks: 268520
Attached patch v1 patch (obsolete) — Splinter Review
Attachment #165379 - Flags: review?(timeless)
Attached patch v1.1 patch (obsolete) — Splinter Review
Improved version based on IRC conversation with timeless, plus "@status FROZEN"
Attachment #165379 - Attachment is obsolete: true
Attachment #165379 - Flags: review?(timeless)
Attached patch v1.2 patch (obsolete) — Splinter Review
forgot to include Makefile.in changes in the v1.1 patch.
Attachment #165468 - Attachment is obsolete: true
Attachment #165469 - Flags: review?(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)
Attachment #165469 - Flags: review?(timeless) → review+
timeless: yup, i'll make those additions.
Attachment #165469 - Flags: superreview?(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()
Attached patch v1.3 patchSplinter Review
revised
Attachment #165469 - Attachment is obsolete: true
Attachment #166146 - Flags: superreview?(bzbarsky)
Attachment #165469 - Flags: superreview?(bzbarsky)
Comment on attachment 166146 [details] [diff] [review]
v1.3 patch

sr=bzbarsky
Attachment #166146 - Flags: superreview?(bzbarsky) → superreview+
fixed-on-trunk

i'll send mail out to the appropriate newsgroups
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Blocks: 270240
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.
Attachment #166146 - Flags: approval1.7.x?
Comment on attachment 166146 [details] [diff] [review]
v1.3 patch

a=mkaply
Attachment #166146 - Flags: approval1.7.x? → approval1.7.x+
Attached patch 1.7 branch patchSplinter Review
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
Keywords: fixed1.7.x
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.