214985 - nsIHelperAppLauncherDialog + Helper App Service changes

Status: RESOLVED FIXED

(Core Graveyard :: Embedding: APIs, defect)

Description

[Ben Goodger (use ben at mozilla dot org for email)]

Some problems - 

1) The Helper App Service sets the "always ask" flag for unknown types to
|false| meaning that the Unknown Content Type dialog is shown once and then
never again. This is bad UI. The dialog should not be shown by the app once and
then not again, especially when the Helper App editing mechanism in Seamonkey
preferences is broken - but in general the inconsistency is inappropriate. 

2) nsIHelperAppLauncherDialog::PromptForSaveToFile (which is actually not
obsolete, despite what some files say, it's still used by both XUL front ends)
needs to take a nsIHelperAppLauncher parameter. Previously the code in the
Helper App Service was calling PromptForSaveToFile without this parameter
expecting the UCT dialog to have been shown (with
nsIHelperAppLauncherDialog::Show, which *does* take a nsIHelperAppLauncher
param), but my downloading changes in Firebird have caused a need for showing
the prompt dialog without having shown the UCT dialog, thus I need to pass a
helper app launcher object to the function for it to work. 

I'm attaching two patches... one to embedding/ to update the
nsIHelperAppLauncherDialog API and all the embedding tests, and one to uriloader
to patch the PromptForSaveToFile call site, and to fix (1) above.

Comment 1

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: Patch to External Helper App Service

Comment 2

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: Patch to Embedding API/Call Sites

Comment 3

[timeless]

Index: components/ui/helperAppDlg/nsIHelperAppLauncherDialog.idl

please remove the tab from:
+	nsILocalFile promptForSaveToFile(in nsIHelperAppLauncher aLauncher, 

i think bz intended to fix (1)

Comment 4

[Jason Kersey]

I was also going to fix 1, but I somehow never got around to it.  Thanks ben!

Comment 5

[Brendan Eich [:brendan]]

Comment on attachment 129137 [details] [diff] [review]
Patch to Embedding API/Call Sites

Nits: line up params in overflow lines with start of first param:

>+++ components/ui/helperAppDlg/nsIHelperAppLauncherDialog.idl	4 Aug 2003 03:07:37 -0000
>@@ -77,8 +77,10 @@
> 	// aDefaultFileName --> default file name to provide (can be null)
> 	// aSuggestedFileExtension --> sugested file extension
> 	// aFileLocation --> return value for the file location
>-	nsILocalFile promptForSaveToFile(in nsISupports aWindowContext, in wstring aDefaultFile, in wstring aSuggestedFileExtension );
>-
>+	nsILocalFile promptForSaveToFile(in nsIHelperAppLauncher aLauncher, 
>+                                   in nsISupports aWindowContext, 
>+                                   in wstring aDefaultFile, 
>+                                   in wstring aSuggestedFileExtension);

Four more spaces needed here...

>+++ tests/mfcembed/components/HelperAppDlg.cpp	4 Aug 2003 03:07:38 -0000
>@@ -256,7 +256,8 @@
> // We prompt the user for the filename to which the content
> // will be saved into
> //
>-NS_IMETHODIMP CHelperAppLauncherDialog::PromptForSaveToFile(nsISupports *aWindowContext, 
>+NS_IMETHODIMP CHelperAppLauncherDialog::PromptForSaveToFile(nsIHelperAppLauncher* aLauncher,
>+                                                             nsISupports *aWindowContext, 
>                                                              const PRUnichar *aDefaultFile, 
>                                                              const PRUnichar *aSuggestedFileExtension, 
>                                                              nsILocalFile **_retval)

One too many spaces here (in preexisting lines, I know -- fix 'em anyway?).

r=brendan (who owns these files?  Anyone still active in the community?).

/be

Comment 6

[Ben Goodger (use ben at mozilla dot org for email)]

Hey jag, I'm changing the seamonkey behaviour here slightly. Just fyi. 

Comment 7

[timeless]

> Four more spaces needed here...
brendan, that's the tab i mentioned. he just needs to replace the tab w/ spaces.

Comment 8

[Ben Goodger (use ben at mozilla dot org for email)]

Fixed. 

Comment 9

[Boris Zbarsky [:bzbarsky]]

(1) was not something that needed fixing.  It was behavior that was very
carefully designed to minimally annoy users while providing some protection from
bad OS setups (unfortunately all too common).  Here's the mail I just sent Ben
and hyatt (since the latter certainly does not read bugmail; dunno about the
former):

I assume that you carefully looked at the reasons we had for the "show the
dialog only once" behavior?  If you neglected to do that, let me summarize:

1)  We do not want to show this dialog AT ALL if the OS provides a way to handle
that MIME type -- we should just use the OS-provided way.

2)  The OS-provided way is, unfortunately, often bogus (especially on Linux).

As a result, we show the dialog once.  The user selects a way to handle the
file.  We assume, unless expressly told otherwise, that this is the way the user
wants files of this type handled in general.

The alternative is to keep asking every single time the type is encountered,
even though we now know perfectly well how the user wants it handled.  This
seems sub-optimal.

So what exactly was the reasoning behind this change?  "Inconsistent behavior"
does not hold water, since the first time the dialog comes up we DO NOT know how
the user wishes the type to be handled, while subsequent times we DO know how
the user wishes the type to be handled.  Since the situations are totally
different, the difference in behavior is very much warranted. 

Comment 10

[Brendan Eich [:brendan]]

> 2)  The OS-provided way is, unfortunately, often bogus (especially on Linux).

Do you mean only on Linux?

If the OS is usually or almost always well-configured for Windows and Mac, then
we shouldn't be asking at all on those OSes.

/be

Comment 11

[Boris Zbarsky [:bzbarsky]]

> Do you mean only on Linux?

No, I mean what I said.  Ben provided (in private email discussion we've been
having on the topic) an excellent example of ZIP files on Windows -- users
typically want them to open in WinZip when loaded in the shell and to be saved
to disk when loaded in a browser (from a remote sight).

Comment 12

[Brendan Eich [:brendan]]

bz: ok, but why does that require the dialog even once?  Maybe you guys have
worked this all out in email, in which case I will gladly wait for a summary.

/be

Comment 13

[Boris Zbarsky [:bzbarsky]]

We need the dialog because the user may no want to use the OS-default handler
for some content types when those same types are coming from a remote host,
basically...

Ben is working on speccing out how this UI should behave in various cases, as
far as I can tell; all I did was email him the reasons the UI behaves as it does
right now and some comments on his assumptions.

Comment 14

[Boris Zbarsky [:bzbarsky]]

I've received no response from Ben since August 11, and in the meantime the
SeaMonkey UI behavior has been changed to be less usable by a back-end change
that did not get review from anyone remotely familiar with the code in
question... What's up with that, exactly?

Comment 15

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: seamonkey changes

Enhance Seamonkey downloading UI to use more meaningful text for "always ask"
checkbox":

- invert logic of "always ask" checkbox so that it becomes a "always handle"
checkbox.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 131472 [details] [diff] [review]
seamonkey changes

Wouldn't "foo != !bar" be better written as "foo == bar" for foo and bar both
boolean?

We may also want to rename the label and accesskey entities to reflect the new
identity of the checkbox.

Past that, looks pretty good, assuming this was carefully tested with
never-before-encountered types, as well as with types that have the "always
ask" flag set in preferences.

Comment 17

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: seamonkey changes

address bz's comments

Comment 18

[Brendan Eich [:brendan]]

We need a seamonkey fix for 1.5.  Reopening and plusing bug to put on radar.

bz, re: comment 14: I am sorry for r='ing the patch (attachment 129136 [details] [diff] [review]) to
nsExternalHelperAppService.cpp instead of getting you to do the deed.  I didn't
do my homework at either the who-should-review or the why-is-the-code-that-way
levels (I thought I knew the latter, but I have no good excuse for the former).

/be

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 131516 [details] [diff] [review]
seamonkey changes

r=bzbarsky (and sr too, if you want, I guess).	Thanks for the quick
turn-around, Ben.

Comment 20

[Brendan Eich [:brendan]]

Comment on attachment 131516 [details] [diff] [review]
seamonkey changes

I can sr= this.  Flagging for 1.5 approval too.

One nit, not worth fixing right now: the repeated needUpdate = needUpdate ||
... statements are cute, but they useless reassign needUpdate to itself if
needUpdate converts to true.  It seems clearer to use 'if (!needUpdate)
needUpdate = ...' instead.

/be

Comment 21

[Asa Dotzler [:asa]]

Comment on attachment 131516 [details] [diff] [review]
seamonkey changes

a=asa (on behalf of drivers) for checkin to the Mozilla 1.5 branch. Please add
the fixed1.5 keyword when this is landed on the branch. Thanks.

Comment 22

[Ben Goodger (use ben at mozilla dot org for email)]

Works on the trunk, I'll put on the 1.5 branch tonight. 

Comment 23

[Boris Zbarsky [:bzbarsky]]

This was in the right place, actually.  It just needed to be marked RESOLVED FIXED.