Closed Bug 147142 Opened 22 years ago Closed 22 years ago

Fix for 129614 breaks helper overriding in galeon

Categories

(Core Graveyard :: File Handling, defect)

Product:
Core Graveyard
Component:
File Handling
Platform:
x86
Linux
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: mozillabugs.philipl, Assigned: bzbarsky)

References

Details

(Keywords: topembed)

Attachments

(2 files, 4 obsolete files)

Updated updated patch
15.81 KB, patch
bzbarsky
: review+
bugzilla
: superreview+
Details | Diff | Splinter Review
This is needed to get that patch to build
1.16 KB, patch
Details | Diff | Splinter Review 
How to ruing an embedder's day 101:

The fix recently checked in for bug 129614 has rendered our current helper app
code uesless, and more importantly, prevented any simple alternative.

Originally we overried nsIHelperAppLauncherDialog so that when a user clicked on
a link we would query the user if they wished to save or launch a helper. If they
decided to launch a helper, we would query our simple helper db which is mainly
used to remember user choices (the grunt work is done by gnome vfs) for the
appropriate helper. We would then create an instance of our progress listener,
initialise it with the relevant info (including the fact that it is downloading
for a helper) and then pass the listener back to the ExternalHelperAppService
using |SetWebListener|. The key to this behaviour is that 
ExternalHelperAppService would callback to
|nsIHelperAppLauncherDialog::ShowProgressDialog|.

|nsIExternalHelperAppService::SaveToDisk| was switched to create an instance
of nsIDownload instead of using the callback a few months ago; this caused us
a little grief but we worked it out in the end.

The fix to 129614 switches |nsIExternalHelperAppService::LaunchWithApplication|
over to this new method, consequently breaking our code. The problem is now 
that we can no longer pass our helper info to our progress listener (we
implement nsIDownload) as we did before. The ExternalHelperAppService does not
distinguish between the two download types when instantiating nsIDownload, nor
can it really, because the nsIDownload api doesn't permit it. A kludgy detection
method is to test whether an openingWith string was passed but this doesn't help
much because it only tells us that a download is for a helper and not for saving
to disk. It does not help us establish what the helper app is. The simplest way
to do this is to have access to the mimetype of the download. We can then lookup
the helper in our helper db with it. But, again, there is no way to pass the
mimetype into the nsIDownload, nor is there a way to derive it from the info
that is provided. Either the mimetype string needs passing directly or the
nsIChannel associated with the download should be passed, probably as a
parameter to |nsIDownload::Init|

If this is not done, there is no nice way to pass the relevant info. The best I
can think of is a global lookup table indexing helper info against download urls
that our nsIHelperAppLauncherDialog would add to and our nsIDownload would
lookup. This is ugly and I do not want to do it. It seems perverse that mozilla
is preventing our code communicating with itself.
Blocks: 98995
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: Fix for 129614 breaks helper overriding in galeon → Fix for 129614 breaks helper overriding in galeon
wow.

have you guys considered contributing your code to mozilla.org? if you did that 
you wouldn't have to maintain it because we'd do that ...
Assignee: law → blaker
So you're basically saying that implementing a drop-in helper app setup is well-
nigh impossible at the moment?  If so, adding topembed.

Is this nsIDownload instantiation happening before or after the user has 
selected what to do with the content?  If after, it seems to me like the most 
robust solution would be to pass the nsIMIMEInfo involved to the baby 
nsIDownload.  That should include all the information we have on the content 
and how it's being handled.... If before, what API change would make this 
easiest in your opinion?
Keywords: topembed
You've got it in one; the old method was a little kludgy, but now it's basically
impossible.

nsIDownload instantiation is occuring after the user is queried for helper
choices, but most importantly, this instantiation is done by mozilla and not 
through a callback to our code, as was done before.

Although passing the nsIMIMEInfo would solve our problem (the nsIMIMEInfo
provides the mimetype we can use to lookup in our db), that isn't using it to 
it's full potential.

A pretty good solution would look like this:

It would be the same as the current implementation with the following changes:

a) Make the nsIMIMEInfo member of the nsIHelperAppLauncher instance passed to
nsIHelperAppLauncherDialog read-write instead of read only. Then the embedder
can query the user using whatever helper app db they desire and then update
the nsIMIMEInfo accordingly. 

b) Pass the nsIMIMEInfo member to the nsIDownload which can then use that 
information to launch a helper upon download completion.

The embedder overrides nsIHelperAppLauncherDialog and nsIDownload as is 
currently the case.

A complete embedding solution would also do the following:

c) Provide an embedder overridable interface which the ExternalHelperAppService
would call to to retrieve helper info. It would take in a mimetype string 
and return an nsIMIMEInfo structure. the stale and unimplemented
nsIMIMEDataSource interface fits this task perfectly and was, in fact, the
way I originally envisaged overriding helper apps handling, until I discovered
it was not used in mozilla at all.

This means that the nsIMIMEInfo passed to nsIHelperAppLauncherDialog would 
initially contain settings relevant to the embedder because it came from the
embedder's own helper app db backend. The embedder would then not need to the
lookups in their db in nsIHelperAppLauncherDialog as is currently the case.

Furthermore, implementing and using nsIMIMEDataSource allows for backend-only
overriding where the overrider makes no changes to the UI. An example of this
would be to refactor galeon's gnome-vfs driven backend out from the UI code,
which could then be dropped into mozilla when used in a gnome environment to
provide a comprehensive helper app db unlike the current situation where the
mozilla helper app db starts off empty. Timeless asked if we had code we might
want to contribute: here's a place.

Currently, the mozilla code that should be implementing nsIMIMEDataSource is
a mess: Raw RDF accessing is done in nsExternalHelperAppService and also in the 
UI javascript of the helper app manager. This code is for all intents and
purposes duplicated in both locations. That RDF access code should be refactored
into an nsIMIMEDataSource implementation.

To summarise:

Step b) is absolutely essential to get overriding to work but is
not very efficient: It requires the embedder to lookup in it's helper db in both
nsIHelperAppLauncherDialog and nsIDownload. It also does not allow for "just
this time" choices by the user, as the choices must be written back to the db by
 nsIHelperAppLauncherDialog to be read in by nsIDownload.

Step a) means that a single lookup can be done in nsIHelperAppLauncherDialog,
any "just this time" changes made accordingly and then pass the nsIMIMEInfo
back to nsIHelperAppLauncher which in turns passes it to nsIDownload. However,
mozilla will still waste it's time looking up in it's own helper app db.

Step c) means that mozilla will do the lookup to the embedder's db - pass a 
relevant nsIMIMEInfo to nsIHelperAppLauncherDialog which will make any user
directed changes and write back to the db if appropriate and then pass the
nsIMIMEInfo back as before.

I highly recommend the full three step solution because it opens the way for
drop in helper app backends, but as I said, step b) is essential for any sort
of overriding to be possible.
ps: If this course of action is approved, I can do steps a) and b) without too 
much effort. Step c) would probably need to be done by someone rather more
conversant with RDF than I.
Step a) should already be done... that MIME info should be writable.  Is it not?

ccing bryner, who's in the middle of setting up a component which hooks us into
the GNOME helper app stuff and who may be interested in the nsIMIMEDataSource
discussion.
My bad, yes, |GetMIMEInfo| does return a reference which can then be used to
change the nsIMIMEInfo data. The last time I looked at nsIMIMEInfo, it had no
write methods, so this wasn't any use, but that was many moons ago.

Ok, so a) is already done. b) and c) still outstanding.
Yeah, we should try to consolidate here.  My patch which uses gnome-vfs for
helper apps and gconf for external protocol handlers is attached to bug 128668.
blake, is there any reason we can't pass an nsIMIMEInfo to the nsIDownload
object?  Is this interface frozen?

What you're proposing in (c) also sounds similar to the nsIDesktopIntegration
interface I created in bug 128668.  If that patch went in, would it give you a
good way to accomplish what you're trying to do?
To answer your question: Not really. nsIDesktopIntegration allows mozilla to 
query into gnome-vfs if there is information lacking in mozilla's RDF helper db,
or similar, correct? This is half the problem, though it doesn't help with the
already considerable amount of indirection in helper app information retrieval and
final execution in the whole setup; I am not comfortable with that.

Second, and more importantly, the RDF helper db still possess no public interface.
The RDF accessing is done using raw RDF calls in nsExternalHelperAppService to
retrive info and again, in raw RDF in the javascript for the helper app db UI in
prefs. This means that as embedders we have no easy way to maintain the mozilla
helper app db. It is, for all intents and purposes, read only with very limited
opaque write support. That was why I created our simple xml flat file db in
galeon; so I'd actually be able to maintain it.

That is why step c) is important. The RDF accessing is refactored to implement
nsIMIMEDataSource (the interface is defined, why not use it?) and then
nsExternalHelperAppService and the front end javascript work through that. Now,
I can drop in galeon's existing helper app backend as a nsIMIMEDataSource
implementation and nsIDesktopIntegration becomes unnecessary because everything
is handled in the initial lookup.

Alternately, with step c) in place, I could
theoretically dispose of our xml db and use mozilla's RDF one by rewriting galeon
to use nsIMIMEDataSource for it's helper app UI. In that case I would use
nsIDesktopIntegration to pull information from gnome-vfs. This course is unlikely
because we'd like to keep our helper app db independent of mozilla in case we
every reach a stage where we can provide alternative rendering engines.

So, ultimately, nsIDesktopIntegration and nsIMIMEDataSource are not an either-or
situation. It will only be practical for galeon to use nsIDesktopIntegration if
nsIMIMEDataSource is implemented, although it won't be necessary to do so.

Essentially, nsIDesktopIntegration does not provide us with the level of 
control we need over helper app functionality.

Finally, I must reiterate that step b) is crucial. Without it, step c) is
academic.
Does this impact other platforms?
Yes.
To try and move things along, I have created a patch to implement step b); 
modifying nsIDownload to take an nsIMIMEInfo instead of the openingWith string.
I have also modifed to users of nsIDownload accordingly.

I believe I have modifed the default nsIProgressDialog implementation correctly,
but there seems to be a problem where it doesn't get registered and can't be
invoked. This problem is independent of my patch.
Keywords: patch
Patch got bit-rotted by fix for bug 140136, so here's an update.

Apparently I don't have sufficient access rights to obsolete my own
attachments. Go figure...

Review per-chance? This bug will not go away if ignored long enough, and we're
SOL until it gets resolved.
*sigh* I missed a couple of spots when I went from mimeInfo -> MIMEInfo.
Eeeek. Can some one tell me the following situation is not positively horrifying?

netwerk/mime/src:

nsMIMEService.[cpp,h]
nsXMLMIMEDataSource.[cpp,h]

Provides an implementation of nsIMIMEService back-ended by an nsIMIMEDataSource
implementation, which despite it's name doesn't seem to actively use an XML file
to store MIME info, but rather provides a read-only hard coded minimal mime 
database of mozilla's internally supported types.

xpfe/appshell/src:

nsMacMIMEDataSource.[cpp,h]

Provides a mac specific nsIMIMEDataSource implementation which is also 
read-only, which appears to pull info from the mac's global mime database.

uriloader/exthandler:

nsExternalHelperAppService.[cpp,h]

Provides an RDF based nsIMIMEService implementation which is not encapsulated
through nsIMIMEDataSource, though I think it should be. It is worth noting
that the rest of the code in this file does not use the nsIMIMEService impl
code through xpcom, but directly.

This code is complemented by raw RDF access code in 
xpfe/components/prefwindow/resources/content/pref-applications-*.js

which *just so happens* to read and write to the same rdf file that 
nsExternalHelperAppService does.

If you are not actively biting your nails already, let me explicate the doomsday
scenario:

xpcom provides no infrastructure to choose one implementation of an interface
over another: It's the last one to register who wins, but there's no clear way 
of finding out which implementation that will be.

Thus, we have two competing nsIMIMEService implementations...
Thus, on a mac, we have two competing nsIMIMEDataSource implementations...

Currently, probably through dumb luck, nsExternalHelperAppService is the last
and consequently active implementation of nsIMIMEService.

Thus, the direct coded usage of nsIMIMEService implementation code in
nsExternalHelperAppService is in sync with what the rest of mozilla sees, and
the rdf datastore that the UI javascript access, is the same one that stores
the mime information that the rest of mozilla sees. *phew*

Now, consider the worst case:

The netwerk/mime implementation ends up registering last and becoming the
active nsIMIMEService. Also, let's say the mac specific nsIMIMEDataSource
overrides the netwerk/mime one. What happens?

When mozilla loads a page, it checks the mime-type of each element through
nsIMIMEService, which will query nsIMIMEDataSource which in turn will query
the mac mime db. Let's say we have a .png image. This mac nsIMIMEDataSource
impl doesn't fill out the preferred action field in the nsIMIMEInfo it returns,
so, it stays at the default: saveToDisk..., so mozilla decides to try and
save the png to disk... which probably leads to a call into the external
helper app service, which is hard coded to use it's own mime service...
I'm not 100% sure what happens at this point, nsExternalHelperAppService might
b0rk because it detects that the passed item is on it's hard coded internally
handled list, or it might go ahead with the save, or it might do something
else bad. Whatever the case, it's mimetype queries will be handled by it's
private nsIMIMEService impl and the not the one the rest of mozilla is seeing.

And, the pref panel UI for helper apps is still accessing the regular RDF file.

This is a *mess*

Even if nothing bad along these lines happen to anyone, it's still bad.
The code in netwerk/mime is being actively maintained (See today's check in
for bug 8031, that's how I found out about this mess) but is never invoked.

Neither is the mac nsIMIMEDataSource impl as a result. Do the maintainers of
these files realise that it's futile?

Clearly, this needs cleaning up. the useless mime service and mime data source
code needs to go and nsExternalHelperAppService and the UI javascript need to
be refactored into an nsIMIMEDataSource implementation, as I've always 
maintained. I just didn't realise how big a mess was hiding behind all this 
until today.

This is *bad*.
1.1alpha and no noises that the mozilla end on this one. 1.1alpha is out and
is b0rked. It is crippled from a galeon perspective, and I expect to get grief
from users. Suffice it to say that this needs to be fixed for 1.1, preferrably
sooner.
Boris, can you review this patch?
Assignee: blaker → bzbarsky
Comment on attachment 86155 [details] [diff] [review]
Patch to implement step b) Passing nsIMIMEInfo into nsIDownload, ver. 3

Yikes.	This got lost in my vacation bugmail backlog... My apologies....

>Index: xpfe/components/download-manager/src/Makefile.in

makefile.win also needs to be changed, and maybe the Mac project file (someone
who actually understands the Mac build system should comment on that, or better
yet attach a patch to the .xml if needed).

>Index: xpfe/components/build/Makefile.in

Same here.

>-    get saving()            { return this.openingWith == null || this.openingWith == ""; },
>+    get saving()            { return this.MIMEInfo.preferredAction == nsIMIMEInfo::saveToDisk; },

Should that not be |this.mMIMEInfo|?  Also, what about the case when mMIMEInfo
is null (the interface says it's optional)?

Finally, nsIMIMEInfo is not defined here, nor is "::" the right thing for a
property in JS.  You want |Components.interfaces.nsIMIMEInfo.saveToDisk|

>-      this.openingWith = aOpeningWith;
>+      this.MIMEInfo = aMIMEInfo;

|this.mMIMEInfo|, again

>+            if ( this.MIMEInfo.preferredApplicationHandler.leafName.length == 0 ) {

mMIMEInfo.  Also, mMIMEInfo and mMIMEInfo.preferredApplicationHandler could
both be null.

>+                this.setValue( "target", this.MIMEInfo.preferredApplicationHandler.leafName 

Againm "mMIMEInfo".  It would make sense to cache this string when you do the
comparison above so you don't have to get it twice, no?

Make those changes and I'll test this out and review again?
I have added an updated makefile.win. A casual glance at the macbuild files
suggests that the concept of specifying include directories (the update I made
to the makefiles) doesnt' carry over to the mac, so changes may not be
necessary at all. But a mac literate person should check that.

I have inserted the relevant null checks and cached the string, but I have not
made the MIMEInfo -> mMIMEInfo changes as I was matching the way the old
openingWith property worked; See the original source code. Someone who can
actually code javascript should confirm if that original behaviour was even
right.
Attachment #85387 - Attachment is obsolete: true
Attachment #86153 - Attachment is obsolete: true
Attachment #86155 - Attachment is obsolete: true
Also, while we're (I'm) moaning about the sorry state of helper app handling, I
feel I should point out that nsIHelperAppLauncherDialog.idl states that:

 Note: The promptForSaveToFile and showProgressDialog methods are
 *       obsolescent.  Caller(s) will be converted to use specific
 *       file-picker and progress-dialog interfaces.

Both methods were only ever called from nsExternalHelperAppService and
|showProgressDialog| is indeed now no longer used (part of the reason this bug
exists) but |promptForSaveToFile| is still in use. When the time comes to
replace it with an instantiation of nsIFilePicker, can I plead that a mechanism
be put in place to surpress the dialog and pass a default path/filename in.
Currently because of the callback, galeon can, as appropriate pass back the
default information and not show the file picker to the user. If the
nsIFilePicker based implementation is done in a short sighted manner, this will
not be possible; forcing us to have nasty inappropriate code in our
nsIFilePicker implementation to emulate correct behaviour.
Comment on attachment 89035 [details] [diff] [review]
Updated patch reflecting bzarsky's comments.

>-    get saving()            { return this.openingWith == null || this.openingWith == ""; },
>+    get saving()            { return this.MIMEInfo == NULL ||
>+			      this.MIMEInfo.preferredAction == Components.interfaces.nsIMIMEInfo.saveToDisk; },

"null", not "NULL".

>+            // Target is the "preferred" application.  Hide if empty.
>+	    if ( this.MIMEInfo) {

Indentation?

>+                var appName = this.MIMEInfo.preferredApplicationHandler.leafName;

preferredApplicationHandler can be "null" as well... you want to test
|this.MIMEInfo && this.MIMEInfo.preferredApplicationHandler| or something like
that, really.

You're right on the mMIMEInfo/MIMEInfo thing; I missed that...

And yes, no mac build changes are needed (I checked).
Ok, added what I hope is sufficent null check paranoia and got rid of the typo.


Third (sixth) time a charm?
Attachment #89035 - Attachment is obsolete: true
Comment on attachment 89041 [details] [diff] [review]
Updated updated patch

r=bzbarsky -- tested this (with the tiny REQUIRES change I had to add) and it
works great.  That is, everything works as before.  ;)
Attachment #89041 - Flags: review+
Heh, my bad, I left that part out of my patch.
nominating in the hopes that this will get some traction...
Keywords: nsbeta1
Comment on attachment 89041 [details] [diff] [review]
Updated updated patch

sr=blake
Attachment #89041 - Flags: superreview+
Checked in on the trunk.  I will mail drivers for branch approval.
bug 129614 hasn't been checked into the 1.0 branch yet so this patch technically
isn't needed on the branch yet. It won't apply cleanly unless bug 129614 is
applied first.
Good point.  Comment added over there.
While I've got your attention, I hope, can you look ay my comment:

http://bugzilla.mozilla.org/show_bug.cgi?id=147142#c16

and tell me what you think of the situation?
I think everyone agrees the situation is a mess... Bill Law and I are hoping to
sit down in a few weeks and sort things out a bit.  This may involve some
rearchitecting of the whole thing, for which I apologize ahead of time....
Fair enough. More generally, I think we need a more public and accessible
mechanism to keep embedders aware of api changes. 99% of the time I find out
the hard way when galeon fails to compile or (even worse as in this case) it
compiles just fine and then does't work right.
Yeah... we should probably make a more determined effort to post relevant
changes to n.p.m.embedding...

In any case, this is now fixed; marking it so.  Philip, would you be so kind as
to verify that this addresses the immediate issues?

I'll likely be filing some bugs on specific architectural aspects of this stuff
over the next few weeks; would you like to be cced on them?
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
That would be much appreciated. thanks.
->ashish, who does helper apps for embed QA.
QA Contact: sairuh → ashishbhatt
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: