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.

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

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?

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.

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?

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.

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?

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

Heh, my bad, I left that part out of my patch.

nominating in the hopes that this will get some traction...

Comment on attachment 89041 [details] [diff] [review] Updated updated patch sr=blake

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?

That would be much appreciated. thanks.

->ashish, who does helper apps for embed QA.