Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a9pre) Gecko/2007102304 Minefield/3.0a9pre ID:2007102304 1. New profile, start firefox 2. Visit a page that has a protocol link, eg: attachment 282694 [details] 3. Click on the link. A 'Launch Application' dialog appears. (See attachment) 4. Click on the 'Choose...' button to select an application to handle this ed2k protocol link Expected: - Firefox's application picker dialog appears Actual: - Windows' generic file picker dialog appears.

I'm looking for some opinions on this functionality - On Vista doing this is no problem, there's a whole new system of registering apps for protos built-in that i've tapped into, and thankfully it appears a number of apps are using it. On XP however, there really isn't any way to find a good list of potential proto handler apps. (In some cases the Vista registration info is set on XP which I can use, but in most cases it's not present.) So for the most part, enabling this dialog on XP will result in an app picker dialog that has the same (single) entry as the protocol handler dialog. I'm wondering what people think firefox should do here - should we skip off the app picker when there's only one option and go straight to the file picker, or instead go for uniformity and bring up the app picker with just one item in the list first. Anyone have any feelings either way on this?

Worse case, we could create a list of potential apps and then look for them installed on the user's system. XP should have enough info in the registry to help us determine if an app is installed.

Another thought, why not skip off the seperate app picker dialog completely, and just add possible local proto handlers (if they exist) to the existing list in the proto handler dialog. That seems to me to be the best solution at this point. We already have a list ready to go, why bother the user with yet another dialog to deal with.

Anther comment, the proto dialog and the app picker are basically the same thing One is for protos, and one is for files but they have the same purpose - to find a user preferred hander. I'd say lets make the proto handler dialog mimic the app picker dialog (or vice versa) in look and feel and just populate it. That way we have a nice unified app picker system regardless of what content they are dealing with.

I guess what I'll do for starters is hook my new proto routines up to the existing proto handler list and skip the app picker in this case. When we get to overhauling the proto dialog for release maybe we can unify the two dialogs a bit.

Ultimately, post fx-3, the plan is to experiment with one or more MRU lists of apps. As far as coalescing the look and feel of the two dialogs, ultimately the hope is that there will only be one dialog. In the meantime, a bug has been filed on making them more similar for Fx3, but it didn't get marked as blocking. For now, I tend to think that your observation in comment 2 is important: it depends on how many apps there are total. E.g. the most common case of mailto, where we'll probably be listing a bunch of different webapps, may not want to have all possible mailers on the system stuffed into the main dialog. Since this dialog was mostly of mconnor's design, I'd be interested in hearing his and faaborg's thoughts here.

spoke to mconnor, he's thinking put them in the existing proto handler dialog list for now.

minor cleanup.

Comment on attachment 290766 [details] [diff] [review] proto picker update v.1 Minor changes to the proto dialog.

Comment on attachment 290766 [details] [diff] [review] proto picker update v.1 I'm not the right reviewer for this patch,

Please include new files as part of your actual patch instead of attaching them separately. It makes it very hard to review with multiple attachments for one basic patch.

Comment on attachment 290790 [details] [diff] [review] proto picker update v.1 This needs to be a unified diff, and the patch is missing the two new files.

lets see if this one comes out.

Comment on attachment 290793 [details] [diff] [review] proto picker update v.1 drive by review: In dialog.js you are checking for duplicates in possibleHandlers vs. possibleLocalHandlers, but could the default system handler also appear in either of the previous 2 lists of handlers?

possibleLocalHandlers - yes, hence the little hack that does a string search across the title of the default system app. I've been working on a better patch that exposes the default handler as an nsIHandlerApp to improve on this, but I'm not sure it'll make it into FF3, since it requires a lot of changes. possibleApplicationHandlers - not according to the code, or the comments on the attribute. A non-system default preferred handler will show up here, but not the system default. There is an issue here though that should be spun out - if you choose browse, and choose the default system helper that's already listed, I believe it'll be added twice to the list. Nothing major, and not likely, and can be delt with once we get the default wrapped.

I've got an update coming up for this in a bit to address a conflict that showed up over the weekend.

removed patch bustage.

Just noticed something, I'll need to add a null check on possibleLocalHandlers before this gets checked in.

Added dependency on 397678 so we have access to the new handler app path parsing code for apps like iTunes. Without that the list can get messed up on XP.

Comment on attachment 291251 [details] [diff] [review] proto picker update v.2 >Index: netwerk/mime/public/nsIMIMEInfo.idl >+ /** >+ * Returns a list of nsILocalHandlerApp objects containing >+ * handlers associated with this mimeinfo. "with this handlerinfo", now? >Index: toolkit/mozapps/handling/content/dialog.js > populateList: function populateList() >+ // XXX short term fix for the lack of an nsIHandlerApp object containing >+ // the system default handler. >+ if (this._handlerInfo.defaultDescription != handler.name) >+ this.insertListItem(handler); File a bug, cite # here with as "FIXME: bug XXXXXX" rather than just XXX? >+ insertListItem: function insertListItem(app, select) >+ let parent = document.getElementById("items"); >+ document.getElementById("items").insertBefore(elm, this._itemChoose); Use "parent"? >Index: uriloader/exthandler/win/nsLocalHandlerAppWin.h >+#endif /*NSLOCALHANDLERAPPMAC_H_*/ Forgot to update the comment? >Index: uriloader/exthandler/win/nsMIMEInfoWin.cpp >+nsMIMEInfoWin::GetPossibleProtoHandlers(nsIArray **_retval) >+ if (!appList) >+ return NS_ERROR_FAILURE; >+ if (!regKey) >+ return NS_ERROR_FAILURE; >+ if (!capKey) >+ return NS_ERROR_FAILURE; NS_ERROR_OUT_OF_MEMORY, perhaps? >+ nsAutoString workingRegistryPath; >+ >+ workingRegistryPath.AppendLiteral("SOFTWARE\\RegisteredApplications"); NS_NAMED_LITERAL_STRING? >+nsMIMEInfoWin::GetPossibleLocalHandlers(nsIArray **_retval) >+ if (mClass == eMIMEInfo) >+ return GetPossibleMimeHandlers(_retval); >+ else if (mClass == eProtocolInfo) >+ return GetPossibleProtoHandlers(_retval); >+ >+ return NS_ERROR_FAILURE; NS_ERROR_UNEXPECTED with a NS_NOTREACHED maybe? I don't really understand and thus didn't review the nsLocalHandlerAppWin addition and associated build-foo - is that just an unrelated change to improve the GetName implementation on Windows? Biesi is a better reviewer for most of the exthandler code anyhow. I haven't had a chance to test this since I'm away from my Windows machines.

> "with this handlerinfo", now? Yep, sounds good. > File a bug, cite # will do. > Use "parent"? > Forgot to update the comment? Doh! > NS_ERROR_OUT_OF_MEMORY, perhaps? > NS_NAMED_LITERAL_STRING? will do. > NS_ERROR_UNEXPECTED with a NS_NOTREACHED maybe? NS_ERROR_UNEXPECTED seems like a good fit. > is that just an unrelated change to improve > the GetName implementation on Windows? Yes, it's actually bug 397690, but I needed it here for display so I rolled it in.

I'm downgrading this since it primarily improves usability on Vista, and isn't necessarily needed for the final. The proto picker works just fine as-is so this was more of an enhancement than a "bug fix".

Note to self - undo changes in Bug 397690 or file another bug once this lands.