Open Bug 400852 Opened 13 years ago Updated 1 year ago

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

Categories

(Firefox :: File Handling, defect)

x86
Windows Vista
defect
Not set
normal

Tracking

()

People

(Reporter: stevee, Unassigned)

References

(Blocks 3 open bugs)

Details

Attachments

(3 files, 7 obsolete files)

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.
Flags: blocking-firefox3?
Depends on: 348808
Assignee: nobody → jmathies
Flags: blocking-firefox3? → blocking-firefox3+
Target Milestone: --- → Firefox 3 M10
Whiteboard: [proto]
Duplicate of this bug: 385114
Priority: -- → P2
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.

Keywords: uiwanted
spoke to mconnor, he's thinking put them in the existing proto handler dialog list for now.
Whiteboard: [proto] → [proto] [needs patch]
Blocks: 397690
Blocks: 406060
Blocks: 393122
Attached patch proto picker update v.1 (obsolete) — Splinter Review
Attached patch proto picker update v.1 (obsolete) — Splinter Review
minor cleanup.
Attachment #290747 - Attachment is obsolete: true
Attached file newfile: nsLocalHandlerAppWin.cpp (obsolete) —
Attached file new file: nsLocalHandlerAppWin.h (obsolete) —
Comment on attachment 290766 [details] [diff] [review]
proto picker update v.1

Minor changes to the proto dialog.
Attachment #290766 - Flags: review?(mano)
Comment on attachment 290766 [details] [diff] [review]
proto picker update v.1

I'm not the right reviewer for this patch,
Attachment #290766 - Flags: review?(mano)
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.
Attached patch proto picker update v.1 (obsolete) — Splinter Review
Attachment #290766 - Attachment is obsolete: true
Attachment #290767 - Attachment is obsolete: true
Attachment #290768 - Attachment is obsolete: true
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.
Attachment #290790 - Attachment is obsolete: true
Attached patch proto picker update v.1 (obsolete) — Splinter Review
lets see if this one comes out.
Attachment #290793 - Flags: review?(gavin.sharp)
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.
Attached patch proto picker update v.2 (obsolete) — Splinter Review
removed patch bustage.
Attachment #290793 - Attachment is obsolete: true
Attachment #291251 - Flags: review?(gavin.sharp)
Attachment #290793 - Flags: review?(gavin.sharp)
Attachment #291251 - Flags: superreview?(cbiesinger)
Whiteboard: [proto] [needs patch] → [proto] [needs r gavin][needs sr biesi][needs ui-r mconnor]
Attachment #291251 - Flags: ui-review?(mconnor)
Just noticed something, I'll need to add a null check on possibleLocalHandlers before this gets checked in. 
Flags: blocking-firefox3+
Flags: blocking-firefox3?
Blocks: 406681
Blocks: 57420
No longer blocks: 406060
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.
Depends on: 397678
Flags: blocking-firefox3? → blocking-firefox3+
Target Milestone: Firefox 3 M10 → Firefox 3 M11
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.
Attachment #291251 - Flags: review?(gavin.sharp) → review+
Whiteboard: [proto] [needs r gavin][needs sr biesi][needs ui-r mconnor] → [proto][needs sr biesi][needs ui-r mconnor]
> "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.
Attachment #291251 - Attachment is obsolete: true
Attachment #295851 - Flags: superreview?(cbiesinger)
Attachment #291251 - Flags: ui-review?(mconnor)
Attachment #291251 - Flags: superreview?(cbiesinger)
Attachment #295852 - Flags: ui-review? → ui-review?(mconnor)
Whiteboard: [proto][needs sr biesi][needs ui-r mconnor] → [proto] [has patch] [needs sr biesi] [needs ui-r mconnor]
Whiteboard: [proto] [has patch] [needs sr biesi] [needs ui-r mconnor] → [proto] [has patch, r+] [needs sr biesi] [needs ui-r mconnor]
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".
Priority: P2 → P5
Flags: wanted-firefox3+
Flags: blocking-firefox3-
Flags: blocking-firefox3+
Priority: P5 → --
Target Milestone: Firefox 3 beta3 → Firefox 3
OS: Windows 2000 → Windows Vista
Note to self - undo changes in Bug 397690 or file another bug once this lands.
Whiteboard: [proto] [has patch, r+] [needs sr biesi] [needs ui-r mconnor]
Attachment #295851 - Flags: superreview?(cbiesinger)
Attachment #295852 - Flags: ui-review?(mconnor)
Assignee: jmathies → nobody
Target Milestone: Firefox 3 → ---
Component: Shell Integration → File Handling
You need to log in before you can comment on or make changes to this bug.