400852 - Use firefox application picker dialog when, on protocol handling dialog, user clicks Choose..

Status: NEW

(Firefox :: File Handling, defect)

Description

[Steve England [:stevee]]

Attachment: The protocol handling dialog. Currently clicking 'Choose...' shows the OS file picker dialog.

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.

Comment 2

[Jim Mathies [:jimm]]

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? 

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

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.

Comment 4

[Jim Mathies [:jimm]]

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.

Comment 5

[Jim Mathies [:jimm]]

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.

Comment 6

[Jim Mathies [:jimm]]

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.

Comment 7

[Dan Mosedale (:dmosedale, :dmose)]

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.

Comment 8

[Jim Mathies [:jimm]]

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

Comment 9

[Jim Mathies [:jimm]]

Attachment: proto picker update v.1

Comment 10

[Jim Mathies [:jimm]]

Attachment: proto picker update v.1

minor cleanup.

Comment 11

[Jim Mathies [:jimm]]

Attachment: newfile: nsLocalHandlerAppWin.cpp

Comment 12

[Jim Mathies [:jimm]]

Attachment: new file: nsLocalHandlerAppWin.h

Comment 13

[Jim Mathies [:jimm]]

Comment on attachment 290766 [details] [diff] [review]
proto picker update v.1

Minor changes to the proto dialog.

Comment 14

[Asaf Romano (gone)]

Comment on attachment 290766 [details] [diff] [review]
proto picker update v.1

I'm not the right reviewer for this patch,

Comment 15

[Reed Loden [:reed]]

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 16

[Jim Mathies [:jimm]]

Attachment: proto picker update v.1

Comment 17

[Reed Loden [:reed]]

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.

Comment 18

[Jim Mathies [:jimm]]

Attachment: proto picker update v.1

lets see if this one comes out.

Comment 19

[Mark Finkle (:mfinkle) (use needinfo?)]

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?

Comment 20

[Jim Mathies [:jimm]]

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.

Comment 21

[Jim Mathies [:jimm]]

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

Comment 22

[Jim Mathies [:jimm]]

Attachment: proto picker update v.2

removed patch bustage.

Comment 23

[Jim Mathies [:jimm]]

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

Comment 24

[Jim Mathies [:jimm]]

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 25

[:Gavin Sharp [email: [e-mail]]]

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.

Comment 26

[Jim Mathies [:jimm]]

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

Comment 27

[Jim Mathies [:jimm]]

Attachment: proto picker update v.3

Comment 28

[Jim Mathies [:jimm]]

Attachment: dialog screenshot w/ def, web, and possible entries

Comment 29

[Jim Mathies [:jimm]]

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

Comment 30

[Jim Mathies [:jimm]]

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