348808 - use application selector instead of file selector dialog when picking helper apps

Status: RESOLVED FIXED

(Firefox :: File Handling, defect)

Description

[Mike Beltzner [:beltzner, not reading bugmail]]

Windows provides an application selection dialog that is distinct from the file selection dialog for things like "Open With..." We should use that picker instead of the basic file picker in places where we ask the user to select a helper application.

Comment 1

[Mike Connor [:mconnor]]

This looks pretty poorly documented.  Rob, any idea who we can poke at MS to find out how to do this?

Comment 2

[Robert Strong (they/them - no direct email)]

Is this in reference to the explorer's "Open With..." ui? Is there an application that can be referenced that uses this?

Comment 3

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

i found these through google, maybe it could be useful to you
------------------
WinExec("rundll32.exe shell32.dll,OpenAs_RunDLL c:\\yourpath\\yourfilehere.ext", SW_SHOW);
------------------
ShellExecute(NULL, NULL, _T("rundll32.exe"),
    _T("shell32.dll,OpenAs_RunDLL filename.ext"), NULL, SW_SHOWNORMAL);
------------------
OpenAs(char *file)
{
SHELLEXECUTEINFO sei = { 0 };
sei.cbSize = sizeof(sei);
sei.fMask = SEE_MASK_INVOKEIDLIST;
sei.lpVerb = "openas";
sei.lpFile = file;
sei.nShow = SW_NORMAL;
ShellExecuteEx(&sei);

}

OpenAs("c:\\config.sys"); 

Comment 4

[Mike Connor [:mconnor]]

There's some C# and VB apps that do it, i.e. via the technique used here: http://www.codeproject.com/csharp/openwith.asp

Comment 5

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

this missed the milestone fx3 alpha 1

Comment 6

[Jim Mathies [:jimm]]

Hey All,

Unfortunately, ShellExecuteEx isn’t going to cut it – this is an actionable call. You would call this when you’re ready to hand the file off to another application. The “Open With” dialog, which is generated by Explorer, is part of the built-in process Explorer goes through when trying to locate handlers. I’ve searched around the simple win32 API inspector calls, and also looked through all the Shell COM interfaces available (including the new application registration interfaces for Vista) but have not found an API that simply brings up the “Open With” dialog and returns the path of the application selected. 

Looking through the Firefox code, I find five references to the nsFilePicker class where the filter passed in is filterApps –

/toolkit/mozapps/downloads/src/nsHelperAppDlg.js.in, line 945 -- fp.appendFilters(nsIFilePicker.filterApps);
/browser/components/feeds/src/FeedWriter.js, line 454 -- fp.appendFilters(Ci.nsIFilePicker.filterApps);
/browser/components/preferences/feeds.js, line 324 -- fp.appendFilters(Ci.nsIFilePicker.filterApps);
/browser/components/preferences/changeaction.js, line 228 -- fp.appendFilters(nsIFilePicker.filterApps);
/mail/components/preferences/changeaction.js, line 196 -- fp.appendFilters(nsIFilePicker.filterApps);

Seems like it would be fairly straight forward to create a new “nsAppPicker” class, which could then be used in place of the existing file picker where appropriate. This ‘app picker’ would in turn provide the file picker functionality through a “browse” option. 

Writing the code that pulls this information out of the win32 system shouldn’t be any trouble.  I’d be happy to take a shot at working this up, although there’s the question of how this fits in with the rest of the platforms we support. I suppose we could start by putting something together that’s win32 specific and leave the other platforms alone for now.  However, I’ve noticed a few comments on this issue on the net, not all of which are specific to win32.  (There are “Open With” style app pickers on a number of platforms we support apparently.)  On some platforms these dialogs might be easily generated with a single call (Gnome apparently has something like this) and some platforms like win32 it’s a little more involved.  I’m curious what some of the more seasoned veterans cc’d on this bug think.. Mike? Robert?  I’m not sure which direction to head at this point.

Comment 7

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Windows Class culd see in HKEY_CURRENT_USER\Software\Microsoft\Windows\CurrentVersion\Explorer\FileExts
to check if exists a openwith mru, to provide user with last used programs for a given extension. 
example: 
HKEY_CURRENT_USER\Software\Microsoft\Windows\CurrentVersion\Explorer\FileExts\.wma\HKEY_CURRENT_USER\Software\Microsoft\Windows\CurrentVersion\Explorer\FileExts\.wma\OpenWithProgids
get Winamp.File and WMP11.AssocFile.WMA


Then get the default key from HKEY_CLASSES_ROOT\.[mru fileext], get HKEY_CLASSES_ROOT/[default key] and read its defaulticon and shell/open values
example:
HKEY_CLASSES_ROOT/Winamp.File
get DefaultIcon C:\Program Files (x86)\Winamp\winamp.exe,1
get Shell/Open/Command "C:\Program Files (x86)\Winamp\winamp.exe" "%1"
HKEY_CLASSES_ROOT/WMP11.AssocFile.WMA
...

A generic list should be showed under the mru, taking applications from HKEY_CLASSES_ROOT/Applications where there is a shell/open/command.

I don't know if there is a kernel shortcut to do that

Comment 8

[Jim Mathies [:jimm]]

Attachment: source changes to existing files [1/2]

Comment 9

[Jim Mathies [:jimm]]

Attachment: new files [2/2]

Comment 10

[Jim Mathies [:jimm]]

Attached:

New directories -

toolkit/components/apppicker
toolkit/components/apppicker/content

Attached files - 

toolkit/components/apppicker/content/appPicker.js
toolkit/components/apppicker/content/appPicker.xul

browser/themes/winstripe/browser/expando-dn.png
browser/themes/winstripe/browser/expando-up.png
browser/themes/winstripe/browser/openwith-div.png

toolkit/locales/en-US/chrome/global/appPicker.dtd

---

Some notes - 

The code for listing and process applications is currently sitting in nsWindowsShellService, however it seems to fit better with nsOSHelperAppService. Unfortunately nsOSHelperAppService isn't really oriented toward what I've done - it seems more tailored toward wrapping platform specific mimeinfo's. nsOSHelperAppService is apparently slated for an overhaul 'soon' through bug 384374.

There are minor issues related to proper icon extraction and execution when dealing with handlers that are invoked through rundll. I've noticed at least one - "Window Picture Gallery" which doesn't display the correct moz-icon, even though the correct icon is in the dll. The resulting app launch will also fail, probably because nsHelperAppDlg.js holds a file pointer to the dll rather than the full execution path and parameters. I'm not sure if I should strip these out of the lists I'm generating or go looking for the causes and try to fix them.

Comment 11

[Jim Mathies [:jimm]]

Forgot to mention - the fix for bug 358297 is rolled in with this.

Comment 12

[Jim Mathies [:jimm]]

Attachment: screenshot

Comment 13

[Mike Hommey [:glandium]]

Should a clone of this bug be made for other platforms ?

Comment 14

[Jim Mathies [:jimm]]

Comment on attachment 270231 [details]
new files [2/2]

integrating with changes in Bug 384374...

Comment 15

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

i'm looking at the screenshot in comment #12 and at the Vista picker

i think you should use Application anywhere, you actually have Application in the title and Program in the separator, title should be different, users don't know what is an helper...
You should add the Suggested Applications separator before the first items as in Vista
Send this item to:, should be Open this file with:
The button should be "Browse for another application" or have a label to make users understand what is it there for

What do you show in the list if no applications are found for a certain data type?

Comment 16

[Jim Mathies [:jimm]]

This was an initial attempt at implementing this on Windows. Since posted there’s been a great deal of discussion on where this code should land and what the dialog should present. At this point, the conventional wisdom is to simplify the dialog rather than add additional lists, separators, etc. Please see the new content handling UI mockups for some examples of where this is going.

http://wiki.mozilla.org/ContentHandling:User_Interface/Proposed_UI2

As far as string labels go that’s TBD.

> “What do you show in the list if no applications are found for a certain data type?”
 
There is a small label below the list that shows up which explains no applications were found. 

Comment 17

[Jim Mathies [:jimm]]

Mike H. - yes, but we need to get things "planted" first before we move forward. 

Comment 18

[Jim Mathies [:jimm]]

Attachment: base patch minus new files

Comment 19

[Jim Mathies [:jimm]]

Attachment: new files zipped

Comment 20

[Jim Mathies [:jimm]]

Attachment: new files diff'd against an empty file for viewing

Comment 21

[Jim Mathies [:jimm]]

Attachment: screenshots various platforms/states

Comment 22

[Jim Mathies [:jimm]]

Attachment: base patch v.2

same patch with cvs diff minus the -U option.

Comment 23

[Jim Mathies [:jimm]]

hmm, didn't help, I guess the bugzilla "diff" link always compacts white space?

Comment 24

[Jim Mathies [:jimm]]

Attachment: base patch v.3

Updated to support patch in bug 386078.

Comment 25

[Robert Strong (they/them - no direct email)]

Comment on attachment 272979 [details] [diff] [review]
base patch v.3

Hey Jim, after going through the first js file it appears that coding style isn't being maintained. I'm going to perform a high level style review of the patch over several comments and then perform a second review after those comments have been addressed. Cheers!

>Index: browser/components/preferences/changeaction.js
>===================================================================
>RCS file: /cvsroot/mozilla/browser/components/preferences/changeaction.js,v
>retrieving revision 1.4
>diff -u -p -8 -r1.4 changeaction.js
>--- browser/components/preferences/changeaction.js	13 Sep 2005 18:39:25 -0000	1.4
>+++ browser/components/preferences/changeaction.js	19 Jul 2007 15:33:05 -0000
>@@ -16,16 +16,17 @@
> #
> # The Initial Developer of the Original Code is
> # Ben Goodger.
> # Portions created by the Initial Developer are Copyright (C) 2000
> # the Initial Developer. All Rights Reserved.
> #
> # Contributor(s):
> #   Ben Goodger <[e-mail]>
>+#   Jim Mathies <[e-mail]>
> #
> # Alternatively, the contents of this file may be used under the terms of
> # either the GNU General Public License Version 2 or later (the "GPL"), or
> # the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
> # in which case the provisions of the GPL or the LGPL are applicable instead
> # of those above. If you wish to allow use of your version of this file only
> # under the terms of either the GPL or the LGPL, and not to allow others to
> # use your version of this file under the terms of the MPL, indicate your
>@@ -215,28 +216,53 @@ var gChangeActionDialog = {
>       this._lastSelectedSave.click();
>       return;
>     }
>     this._lastSelectedSave = aSelectedItem;
>   },
>   
>   changeApp: function ()
>   {
>+#ifdef XP_WIN
>+    var params = {};
>+    params.title         = this._bundle.getString("fpTitleChooseApp");
>+    params.filename      = this._item.typeName;
>+    params.description   = this._item.type;
>+    params.mimeInfo      = this._item.mimeInfo;
>+    params.nsihandlerapp = null;
>+
>+	window.openDialog(
Spaces instead of a tab

>+                  "chrome://global/content/appPicker.xul",
>+                  null,
>+                  "chrome,modal,centerscreen,titlebar,dialog=yes",
>+                  params);
This can be changed to the following without loss of readability
                  "chrome://global/content/appPicker.xul", null,
                  "chrome,modal,centerscreen,titlebar,dialog=yes", params);

>+
>+    if ( params.nsihandlerapp && 
>+         params.nsihandlerapp.executable && 
>+         params.nsihandlerapp.executable.isFile() ) 
>+    {
Does not follow the coding style of this file... should be
    if (params.nsihandlerapp && 
        params.nsihandlerapp.executable && 
        params.nsihandlerapp.executable.isFile()) {

>+      var customApp = document.getElementById("customApp");
>+      customApp.file = params.nsihandlerapp.executable;
>+      this._item.customHandler = params.nsihandlerapp.executable;      
>+      return true;
>+    }
>+#else
>     const nsIFilePicker = Components.interfaces.nsIFilePicker;
>     var fp = Components.classes["@mozilla.org/filepicker;1"]
>                        .createInstance(nsIFilePicker);
>     var winTitle = this._bundle.getString("fpTitleChooseApp");
>     fp.init(window, winTitle, nsIFilePicker.modeOpen);
>     fp.appendFilters(nsIFilePicker.filterApps);
>     if (fp.show() == nsIFilePicker.returnOK && fp.file) {
>       var customApp = document.getElementById("customApp");
>       customApp.file = fp.file;
>       this._item.customHandler = fp.file;      
>       return true;
>     }
>+#endif
>     return false;
>   },
>   
>   changeCustomFolder: function ()
>   {
>     const nsIFilePicker = Components.interfaces.nsIFilePicker;
>     var fp = Components.classes["@mozilla.org/filepicker;1"]
>                        .createInstance(nsIFilePicker);
>Index: netwerk/mime/public/nsIMIMEInfo.idl
>===================================================================
>RCS file: /cvsroot/mozilla/netwerk/mime/public/nsIMIMEInfo.idl,v
>retrieving revision 1.28
>diff -u -p -8 -r1.28 nsIMIMEInfo.idl
>--- netwerk/mime/public/nsIMIMEInfo.idl	17 Jul 2007 22:59:58 -0000	1.28
>+++ netwerk/mime/public/nsIMIMEInfo.idl	19 Jul 2007 15:34:11 -0000
>@@ -37,16 +37,17 @@
>  * ***** END LICENSE BLOCK ***** */
> 
> #include "nsISupports.idl"
> 
> interface nsIURI;
> interface nsIFile;
> interface nsIUTF8StringEnumerator;
> interface nsIHandlerApp;
>+interface nsIArray;
> 
> typedef long nsHandlerInfoAction;
> 
> /**
>  * nsIHandlerInfo gives access to the information about how a given protocol
>  * scheme or MIME-type is handled.
>  */
> [scriptable, uuid(2ec1216d-59e7-424c-aa1e-70aa3c897520)]
Interface changes require a change of the uuid

Comment 26

[Robert Strong (they/them - no direct email)]

Comment on attachment 272979 [details] [diff] [review]
base patch v.3

>Index: toolkit/mozapps/downloads/src/nsHelperAppDlg.js.in
>===================================================================
>RCS file: /cvsroot/mozilla/toolkit/mozapps/downloads/src/nsHelperAppDlg.js.in,v
>retrieving revision 1.47
>diff -u -p -8 -r1.47 nsHelperAppDlg.js.in
>--- toolkit/mozapps/downloads/src/nsHelperAppDlg.js.in	5 Jul 2007 19:31:46 -0000	1.47
>+++ toolkit/mozapps/downloads/src/nsHelperAppDlg.js.in	19 Jul 2007 15:34:41 -0000
>...
>@@ -941,18 +942,102 @@ nsUnknownContentTypeDialog.prototype = {
>       return true;
>     },
> 
>     // dialogElement:  Convenience. 
>     dialogElement: function(id) {
>       return this.mDialog.document.getElementById(id);
>     },
> 
>+    // Retrieve the pretty description from the file
>+    getFileDisplayName: function getFileDisplayName(file) 
>+    { 
coding style
>+#ifdef XP_WIN
>+        const nsILocalFileWin = Components.interfaces.nsILocalFileWin;
>+
>+        if (file instanceof nsILocalFileWin) 
If this is the only use you might as well write this as
if (file instanceof Components.interfaces.nsILocalFileWin)

nit: it appears that your editor sporadically adds a space / spaces to the end of the lines you are adding

>+#ifdef XP_WIN
>+
Kill the extra line

>+    // Protect against the lack of an extension    
>+    var fileExtension = "";
>+    try {
>+        fileExtension = this.mLauncher.MIMEInfo.primaryExtension;
>+    } catch(ex) {}
The var name fileExtension throws me a bit though I'm not familiar with the MIMEInfo interface. This doesn't appear to really be the file's extension... could a more descriptive var name be used?

>+    // Try to use the pretty description of the type, if one is available.
>+    var typeString = this.mLauncher.MIMEInfo.description;
better var name... perhaps handlerDesc though I don't think that is quite right either.

>+
>+    if (typeString.length == 0) 
>+    {
coding style

>+      // If there is none, use the extension to identify the file, e.g. "ZIP file"
>+      if (fileExtension.length > 0)
>+      {
coding style

>+        typeString = this.dialogElement("strings").getFormattedString("fileType", [fileExtension.toUpperCase()]);
>+      } else {
>+        // If we can't even do that, just give up and show the MIME type. 
>+        typeString = this.mLauncher.MIMEInfo.MIMEType;
>+      }
>+    }
>+
>+    var params = {};
>+    params.title         = this.dialogElement("strings").getString("chooseAppFilePickerTitle");
>+    params.filename      = this.mLauncher.suggestedFileName;
>+    params.description   = typeString;
>+    params.mimeInfo      = this.mLauncher.MIMEInfo;
>+    params.nsihandlerapp = null;
>+
>+	this.mDialog.openDialog(
Spaces instead of a tab

>+                  "chrome://global/content/appPicker.xul",
>+                  null,
>+                  "chrome,modal,centerscreen,titlebar,dialog=yes",
>+                  params);
>+
>+    if ( params.nsihandlerapp && 
>+         params.nsihandlerapp.executable && 
>+         params.nsihandlerapp.executable.isFile() ) 
>+    {
coding style as in previous comment

>+        // Show the "handler" menulist since we have a (user-specified) 
>+        // application now.
>+        this.dialogElement("modeDeck").setAttribute("selectedIndex", "0");
>+
>+        // Remember the file they chose to run.
>+        this.chosenApp = params.nsihandlerapp;
>+
>+        // Update dialog
>+        var otherHandler = this.dialogElement("otherHandler");
>+        otherHandler.removeAttribute("hidden");
>+        otherHandler.setAttribute("path", this.getPath(this.chosenApp.executable));
>+        otherHandler.label = this.getFileDisplayName(this.chosenApp.executable);
>+        this.dialogElement("openHandler").selectedIndex = 1;
>+        this.dialogElement("openHandler").setAttribute("lastSelectedItemID", "otherHandler");
>+        
>+        this.dialogElement("mode").selectedItem = this.dialogElement("open");
>+    } else {
coding style... it appears that this file primarily uses
}
else {
with a few } else { thrown in

>+        var openHandler = this.dialogElement("openHandler");
>+        var lastSelectedID = openHandler.getAttribute("lastSelectedItemID");
>+        if (!lastSelectedID)
>+        lastSelectedID = "defaultHandler";
indent

>Index: uriloader/exthandler/win/nsMIMEInfoWin.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/uriloader/exthandler/win/nsMIMEInfoWin.cpp,v
>retrieving revision 1.5
>diff -u -p -8 -r1.5 nsMIMEInfoWin.cpp
>--- uriloader/exthandler/win/nsMIMEInfoWin.cpp	5 Jul 2007 19:31:46 -0000	1.5
>+++ uriloader/exthandler/win/nsMIMEInfoWin.cpp	19 Jul 2007 15:34:45 -0000
>...
>@@ -63,16 +73,107 @@ nsMIMEInfoWin::LaunchDefaultWithFile(nsI
>   local->IsExecutable(&executable);
>   if (executable)
>     return NS_ERROR_FAILURE;
> 
>   return local->Launch();
> }
> 
> NS_IMETHODIMP
>+nsMIMEInfoWin::LaunchWithURI(nsIURI* aURI)
>+{
>+  nsCOMPtr<nsILocalFile> docToLoad;
>+  nsresult rv;
>+  
>+  if (mPreferredAction == useHelperApp) {
>+    if (!mPreferredApplication)
>+      return NS_ERROR_FILE_NOT_FOUND;
>+
>+    // check for and possibly launch with web application
>+    nsCOMPtr<nsIWebHandlerApp> webHandler = 
>+      do_QueryInterface(mPreferredApplication, &rv);
>+    if (NS_SUCCEEDED(rv)) {
>+      return LaunchWithWebHandler(webHandler, aURI);         
>+    }
Coding style - remove curly braces

>+    // ok, we must have a local handler app
>+    nsCOMPtr<nsILocalHandlerApp> localHandler = 
>+      do_QueryInterface(mPreferredApplication, &rv);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+
>+    nsCOMPtr<nsIFile> executable;
>+    rv = localHandler->GetExecutable(getter_AddRefs(executable));
>+    NS_ENSURE_SUCCESS(rv, rv);
>+
>+    rv = GetLocalFileFromURI(aURI, getter_AddRefs(docToLoad));
>+    NS_ENSURE_SUCCESS(rv, rv);
>+
>+    // Deal with local dll based handlers
>+    nsCString filename;
>+    executable->GetNativeLeafName(filename);
>+    if ( filename.Length() > 3 )
>+    {
coding style
if (filename.Length() > 3) {

>+      nsCString extension(Substring(filename, filename.Length() - 3, 3));
>+      nsCString dllExt("dll");
>+      ToLowerCase(extension);
>+      if ( extension.Equals(dllExt) )
>+      {
same here

I'm going to r- this and ask that you go through the patch and cleanup the coding style and resubmit.

Comment 28

[:glob ✱]

why not use shell.dll's OpenAs_RunDLLA() ?

Comment 29

[Jim Mathies [:jimm]]

Attachment: new files zipped

Comment 30

[Jim Mathies [:jimm]]

Attachment: base patch v.4

- white space / bracketing / tabs cleaned up
- uid bumped

Comment 31

[Jim Mathies [:jimm]]

> why not use shell.dll's OpenAs_RunDLL() ?

We need to be able to remember the application chosen, so we need an interface that returns the application selected and does not act on the file at the time of the call. From my understanding of the OpenAs_RunDLLA call, it acts similar to how shell execute works - the user chooses an app and the file is then handed off immediately.

Comment 32

[Robert Strong (they/them - no direct email)]

Comment on attachment 273611 [details] [diff] [review]
base patch v.4

Justin is going to also review this... Thanks Justin

Comment 33

[Jim Mathies [:jimm]]

Quick note to myself here - I've muddled with ShellExecute in here to handle the launch of these handlers. I need to get back in and take a look at that to be sure I haven't opened something up al la bug 389580.

Comment 34

[Robert Strong (they/them - no direct email)]

Jim, did you handle the case where the app has a dde handler? Such as IE where the app is launched and the argument is sent to the app via dde?

Comment 35

[Robert Strong (they/them - no direct email)]

btw: I'm not sure if it is even necessary... depends on how shellexecute is used and all.

Comment 36

[Jim Mathies [:jimm]]

Well, since the original list of apps is based on a search for shell/open/command entries, I don't believe I need to worry about DDE. My concern was that I've added support for rundll handlers, which I do a lot of processing on and also do a "%1" replace on, which I'm thinking now may be broken in some cases where %1 is not used.  

Comment 37

[Robert Strong (they/them - no direct email)]

An app that supports DDE may be launched by the OS via the open command and then the OS will send the argument via DDE. So, there will not be a %1 in the open command at all in this instance (look at the registration for IE as an example). I am also hoping that after we get our startup story worked out so we no longer need to do a complete restart under certain circumstances that we do the exact same thing in order to remove this as an attack vector.

Comment 38

[Jim Mathies [:jimm]]

>Jim, did you handle the case where the app has a dde handler? Such as IE where
>the app is launched and the argument is sent to the app via dde?

Are you referring to the cases where there's a shell/open/ddeexec entry for the file type? If thats the case, these won't land in the application list I generate. 

I've been looking at shell/open/command templates and they all seem to support the standard %1 format. For example, in the case of IE, it's registered as standard template under 

HKEY_CLASSES_ROOT\Applications\iexplore.exe\shell\open\command =
"C:\Program Files\Internet Explorer\iexplore.exe" %1

which I pick up in certain cases and everything works just fine.

Comment 39

[Robert Strong (they/them - no direct email)]

I have one entry for devenv.exe (VisualStudio.8.0) that has a shell edit that uses dde
"C:\Program Files\Microsoft Visual Studio 8\Common7\IDE\devenv.exe" /dde

I doubt many use dde for this entry but it appears possible.

Comment 40

[Jim Mathies [:jimm]]

Ok, do you feel I should put some time in on it? Currently the patch will skip off these - 

// executable is rundll32, eveything else is a list of parameters,  
// including the dll handler. 
if (!GetDllLaunchInfo(executable, docToLoad, args, PR_FALSE)) 
     return NS_ERROR_INVALID_ARG; 

Note, this is only in the case of a dll handler, regular exes get launched the way they have always been in ff, using a presumed %1 on the tail end of the application path and LaunchWithIProcess call. 

Comment 41

[Robert Strong (they/them - no direct email)]

Honestly I don't think you should but it would be a good thing to make a note of it in case we run into an exception to this rule in the future.

Comment 42

[Justin Dolske [:Dolske]]

Comment on attachment 273611 [details] [diff] [review]
base patch v.4

>+    params.nsihandlerapp = null;

params.handlerApp would probably be a better name here? ...oh, I guess this isn't a property you're adding.

>+    /** 
>+     * Returns a list of nsILocalHandlerApp objects containing
>+     * handlers associated with this mimeinfo. Implemented per 
>+     * platform using information in this object to generate the
>+     * best list. Typically used for an "open with" style user 
>+     * option.
>+     * 
>+     * @return nsIArray of nsILocalHandlerApp
>+     */
>+    readonly attribute nsIArray possibleLocalHandlers;

Just |localHandlers|? "Possible" makes me wonder if the attribute may or may not have a value, or if the handler itself may or may not support what they claim?

>+        if (file instanceof nsILocalFileWin) {
>+          try {
>+            return file.getVersionInfoField("FileDescription");
>+          }
>+          catch (e) {
>+          }
>+        }

Use "} catch" here instead of "}\ncatch". [Style for this unfortunately varies by file sometimes.]


>+        typeString = this.dialogElement("strings").getFormattedString("fileType", [fileExtension.toUpperCase()]);

Line lengths should be < 80 columns, here and a couple other places.

>+        if (!lastSelectedID)
>+        lastSelectedID = "defaultHandler";

Indent here (and a couple other similar places). Maybe diff is just acting weird? Are you using tabs?


> NS_IMETHODIMP
>+nsMIMEInfoWin::LaunchWithURI(nsIURI* aURI)

So, I think with all the work dmose & co are doing with protocol handling, this patch might already be stale? Might also need an update to make sure there are no security regressions from the 2.0.0.5 / 2.0.0.6 firedrills.

(...more later...)

Comment 43

[Jim Mathies [:jimm]]

> params.handlerApp

Sounds fine with me.

>Just |localHandlers|? "Possible" 

Sure, i'd like to match it to what shawn was adding for web handlers too.

> Use "} catch" here
> nsMIMEInfoWin::LaunchWithURI(nsIURI* aURI)

Having been raked over the formatting coals now a few times I really should go over this one more time to make sure it's 'spec'. :/ I'll check for staleness too and take care of this on the next rev. Any additional comments before I do would be much appreciated.

(There is one thing I noticed before anybody tags me on it - ive got a hard coded filter on "firefox.exe" in the list code I need to replace with a get module call as well.)

Comment 44

[Jim Mathies [:jimm]]

>Just |localHandlers|? "Possible" 
>Sure, i'd like to match it to what shawn was adding for web handlers too.

From myk's bug 385740:

/** 
  * Applications that can handle this content type.  The list will include 
  * the preferred application handler, if any. 
  */ 
 attribute nsIArray possibleApplicationHandlers; 

Here we have:

 /**  
  * Returns a list of nsILocalHandlerApp objects containing 
  * handlers associated with this mimeinfo. Implemented per  
  * platform using information in this object to generate the 
  * best list. Typically used for an "open with" style user  
  * option. 
  *  
  * @return nsIArray of nsILocalHandlerApp 
  */ 
 readonly attribute nsIArray possibleLocalHandlers; 

I believe possibleApplicationHandlers comes from user configured RDF, the second comes from the local system. That's where I came up with 'possible'. 

Comment 45

[Myk Melez [:myk] [@mykmelez]]

> I believe possibleApplicationHandlers comes from user configured RDF, the
> second comes from the local system. That's where I came up with 'possible'. 

The name possibleApplicationHandlers was chosen to contrast with preferredApplicationHandler, another attribute on nsIHandlerInfo.  But we could switch that to just applicationHandlers as well, if that would make more sense.  cc:ing dmose, responsible for super-reviewing my patch that adds that attribute, for his thoughts.


> (There is one thing I noticed before anybody tags me on it - ive got a hard
> coded filter on "firefox.exe" in the list code I need to replace with a get
> module call as well.)

On the Mac, the code checks to see if a potential handler is the currently running app:

http://lxr.mozilla.org/mozilla/source/uriloader/exthandler/mac/nsInternetConfigService.cpp#152

Perhaps something like that would work here as well.

Comment 46

[Jim Mathies [:jimm]]

Attachment: new files zipped

Comment 47

[Jim Mathies [:jimm]]

Attachment: base patch v.5

Notes:

- Removed staleness
- Added support for selecting the right folder when choosing "browse" from the app picker dialog
- Did a second pass on formatting, line length 
- Fixed hardcoded "firefox.exe" filter
- Renamed incoming param nsihanderapp to handlerApp

I haven't changed the attribute name, I'll wait for feedback.

As far as the security issues that cropped up recently with protocol handlers, the current implementation of LaunchWithURI (which is overridden in nsMIMEInfoWin with this patch) traps for protocol handlers prior to the code the patch adds. The new code will be dealing with a local file as a parameter, so I don't see any issues with it.

Comment 49

[Justin Dolske [:Dolske]]

Comment on attachment 275302 [details] [diff] [review]
base patch v.5


>+    // Try to use the pretty description of the type, if one is available.
>+    var typeString = this.mLauncher.MIMEInfo.description;
>+
>+    if (typeString.length == 0) {

if (!typeString)

>+      // If there is none, use the extension to 
>+      // identify the file, e.g. "ZIP file"
>+      if (fileExtension.length > 0) {

if (fileExtension)

[In JS, null and empty-string are both false]


>+    if (NS_SUCCEEDED(rv)) {
>+      return LaunchWithWebHandler(webHandler, aURI);
>+    }

Nix braces.

>+    if (filename.Length() > 3) {
>+      nsCString extension(Substring(filename, filename.Length() - 3, 3));
>+      nsCString dllExt("dll");
>+      ToLowerCase(extension);
>+
>+      if (extension.Equals(dllExt)) {

Is this code strict enough? For example, could it get input like "xyz.1", "xdll", or ".dll", and should it catch such input?

>+        if (result >= 32)
>+          return NS_OK;

What's 32? Or is that a commonly known value? [I guess I see this in an existing place, so I guess it is?]


>+// Returns the dll & path of a rundll command handler. 
>+PRBool nsMIMEInfoWin::CleanupCmdHandlerPath(nsAString& aCommandHandler)
>+{
[...]
>+  nsAutoArrayPtr<PRUnichar> destination(new PRUnichar[bufLength]);
>+  if (!destination)
>+    return NS_ERROR_OUT_OF_MEMORY;

Seems like this shouldn't be returned in a PRBool function.

>+  aApp = do_CreateInstance("@mozilla.org/uriloader/local-handler-app;1");
>+  if (!aApp) return PR_FALSE;

Newline before |return|.

>+  if (NS_FAILED(rv)) return PR_FALSE;

And here. :) Etc.

>+  // Check for the NoOpenWith flag, if it exists
>+  PRUint32 iValue;
>+  if (NS_SUCCEEDED(appKey->ReadIntValue(
>+      NS_LITERAL_STRING("NoOpenWith"), &iValue)) &&
>+      iValue == 1)
>+    return PR_FALSE;

Hmm, that's a big conditional. Maybe split it out, so it's |if (NS_SUCCEEDED(rv) && iValue == 1)|?


>+    nsAutoArrayPtr<PRUnichar> destination(new PRUnichar[bufLength]);
>+    if (!destination)
>+      return NS_ERROR_OUT_OF_MEMORY;\

Another NS_ return type in a PRBool function.


>+// Helper - case insensitive compare between a string and an array of
>+// strings.
>+static PRBool IsPathEqual(nsAString& appPath,
>+                          nsTArray<nsCAutoString>& trackList)
>+{
>+  nsCAutoString tmp = NS_ConvertUTF16toUTF8(appPath);
>+  ToLowerCase(tmp);
>+
>+  for (PRUint32 i=0; i<trackList.Length(); i++) {
>+    if (tmp.Equals(trackList[i])) return PR_TRUE;

Seems like either you should add a comment that trackList is assumed to already be in lowercase, or you should convert it in the conditional.

Also, space the operators in the for() statement.

>+  // Don't include firefox.exe in the list
>+  char exe[4096];
>+  if (GetModuleFileName(NULL, (LPTSTR)exe, 4096)) {

Does GetModuleFileName ensure the buffer will always be null terminated, even when the result is 4095 or 4096 bytes?

....and done! As I noted before, I didn't deeply look at what the Windows-specific bits are doing, so rstrong still should review that.

Comment 50

[Justin Dolske [:Dolske]]

Comment on attachment 275302 [details] [diff] [review]
base patch v.5

Bah, WTF? I set additional-review?, but bugzilla cleared my r+.

(sorry for the spam)

Comment 51

[Jim Mathies [:jimm]]

Couple comments back:

>Is this code strict enough? For example, could it get input like "xyz.1",
>"xdll", or ".dll", and should it catch such input?

Yes I think your right, I should be checking for '.dll' instead.

>Does GetModuleFileName ensure the buffer will always be null terminated, even
>when the result is 4095 or 4096 bytes?

Win32's max path is much less than 4096, so technically it can't fail due to a lack of buffer width. This brings up a question about character buffers though. I've always made a habit of using memset(buf,0,sizeof(buf)) on buffers like this just to be safe. I don't see that in our code, so I haven't been using it. Would I get dinged on that?

Comment 52

[Jim Mathies [:jimm]]

Attachment: base patch v.6

Comment 53

[Justin Dolske [:Dolske]]

(In reply to comment #51)

> Win32's max path is much less than 4096, so technically it can't fail due to a
> lack of buffer width. 

I always worry about people finding new ways to send impossibly long input... :-)

> This brings up a question about character buffers though.
> I've always made a habit of using memset(buf,0,sizeof(buf)) on buffers like
> this just to be safe. I don't see that in our code, so I haven't been using it.

Most of Mozilla's ns*String string code handles nulls, lengths, and buffers internally (oh my!), so it's only the interactions with good 'ole C strings that these issues come up.

Doing a memset(), especially on large buffers, in inefficient. In cases like this, I usually just do something like:

char buffer[4096];
buffer[4095] = NULL;
foobar(buffer, sizeof(buffer) - 1);

The typical example of this is with strncpy(). But if your foobar() function is known to be sane with respect such cases [as, for example, strlcpy() is], then you can just skip doing this.

Comment 54

[Myk Melez [:myk] [@mykmelez]]

Note that this patch conflicts with the patch in bug 377784.  But the conflict should be easy to resolve, as it's just in changeaction.js, and the code this patch adds to that file is well isolated and should be easy to move into applications.js, which bug 377784 adds while removing changeaction.js.

Comment 55

[Robert Strong (they/them - no direct email)]

Comment on attachment 277557 [details] [diff] [review]
base patch v.6

>Index: browser/components/preferences/changeaction.js
>===================================================================
>RCS file: /cvsroot/mozilla/browser/components/preferences/changeaction.js,v
>retrieving revision 1.4
>diff -u -p -8 -r1.4 changeaction.js
>--- browser/components/preferences/changeaction.js	13 Sep 2005 18:39:25 -0000	1.4
>+++ browser/components/preferences/changeaction.js	21 Aug 2007 16:54:45 -0000
>+#ifdef XP_WIN
>+    var params = {};
>+    params.title         = this._bundle.getString("fpTitleChooseApp");
>+    params.filename      = this._item.typeName;
>+    params.description   = this._item.type;
>+    params.mimeInfo      = this._item.mimeInfo;
>+    params.handlerApp    = null;
>+
>+    window.openDialog("chrome://global/content/appPicker.xul", null,
>+           "chrome,modal,centerscreen,titlebar,dialog=yes", params);
I wonder if this should be openSubDialog as is used in advanced.js. Gavin or mano should be able to answer that.
Also, the alignment on this is a bit funky.

>Index: toolkit/mozapps/downloads/src/nsHelperAppDlg.js.in
>===================================================================
>RCS file: /cvsroot/mozilla/toolkit/mozapps/downloads/src/nsHelperAppDlg.js.in,v
>retrieving revision 1.49
>diff -u -p -8 -r1.49 nsHelperAppDlg.js.in
>--- toolkit/mozapps/downloads/src/nsHelperAppDlg.js.in	29 Jul 2007 03:16:01 -0000	1.49
>+++ toolkit/mozapps/downloads/src/nsHelperAppDlg.js.in	21 Aug 2007 16:56:16 -0000
>...
>@@ -943,20 +944,103 @@ nsUnknownContentTypeDialog.prototype = {
>       return true;
>     },
> 
>     // dialogElement:  Convenience. 
>     dialogElement: function(id) {
>       return this.mDialog.document.getElementById(id);
>     },
> 
>+    // Retrieve the pretty description from the file
>+    getFileDisplayName: function getFileDisplayName(file)
>+    { 
>+#ifdef XP_WIN
>+        const nsILocalFileWin = Components.interfaces.nsILocalFileWin;
>+
>+        if (file instanceof nsILocalFileWin) {
nit: const only used once so you might as well just do
if (file instanceof Components.interfaces.nsILocalFileWin) {

>+          try {
>+            return file.getVersionInfoField("FileDescription");
>+          } catch (ex) {
>+          }
>+        }
>+#endif
>+        return file.leafName;
>+    },
>+
>     // chooseApp:  Open file picker and prompt user for application.
>     chooseApp: function() {
>+#ifdef XP_WIN
>+    // Protect against the lack of an extension    
nit: bunch of extra spaces

>+    var fileExtension = "";
>+    try {
>+        fileExtension = this.mLauncher.MIMEInfo.primaryExtension;
>+    } catch(ex) {
>+    }
>+
>+    // Try to use the pretty description of the type, if one is available.
>+    var typeString = this.mLauncher.MIMEInfo.description;
>+
>+    if (!typeString) {
>+      // If there is none, use the extension to 
>+      // identify the file, e.g. "ZIP file"
>+      if (fileExtension) {
>+        typeString =
>+          this.dialogElement("strings").
>+          getFormattedString("fileType", [fileExtension.toUpperCase()]);
>+      } else {
>+        // If we can't even do that, just give up and show the MIME type.
>+        typeString = this.mLauncher.MIMEInfo.MIMEType;
>+      }
>+    }
>+
>+    var params = {};
>+    params.title = 
>+      this.dialogElement("strings").getString("chooseAppFilePickerTitle");
>+    params.filename = this.mLauncher.suggestedFileName;
>+    params.description = typeString;
>+    params.mimeInfo = this.mLauncher.MIMEInfo;
>+    params.handlerApp = null;
>+
>+    this.mDialog.openDialog("chrome://global/content/appPicker.xul", null,
>+                  "chrome,modal,centerscreen,titlebar,dialog=yes", params);
>+
>+    if (params.handlerApp &&
>+        params.handlerApp.executable &&
>+        params.handlerApp.executable.isFile()) {
>+        // Show the "handler" menulist since we have a (user-specified) 
>+        // application now.
nit: might be wrong but I believe this should be "we have an (user-specified) application now" or "we have a user-specified application now"

>+        this.dialogElement("modeDeck").setAttribute("selectedIndex", "0");
>+
>+        // Remember the file they chose to run.
>+        this.chosenApp = params.handlerApp;
>+
>+        // Update dialog
>+        var otherHandler = this.dialogElement("otherHandler");
>+        otherHandler.removeAttribute("hidden");
>+        otherHandler.setAttribute("path",
>+          this.getPath(this.chosenApp.executable));
>+        otherHandler.label = 
>+          this.getFileDisplayName(this.chosenApp.executable);
>+        this.dialogElement("openHandler").selectedIndex = 1;
>+        this.dialogElement("openHandler").setAttribute("lastSelectedItemID",
>+          "otherHandler");
>+        
nit: bunch of extra spaces

>+        this.dialogElement("mode").selectedItem = this.dialogElement("open");
>+    } else {
>+        var openHandler = this.dialogElement("openHandler");
>+        var lastSelectedID = openHandler.getAttribute("lastSelectedItemID");
>+        if (!lastSelectedID)
>+          lastSelectedID = "defaultHandler";
>+        openHandler.selectedItem = this.dialogElement(lastSelectedID);
>+    }
Indentation is off

>+
>+#else
>       var nsIFilePicker = Components.interfaces.nsIFilePicker;
>-      var fp = Components.classes["@mozilla.org/filepicker;1"].createInstance(nsIFilePicker);
>+      var fp = 
>+        Components.classes["@mozilla.org/filepicker;1"].createInstance(nsIFilePicker);
nit: either remove this change or change it so it is under 80

>Index: uriloader/exthandler/nsMIMEInfoImpl.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/uriloader/exthandler/nsMIMEInfoImpl.cpp,v
>retrieving revision 1.66
>diff -u -p -8 -r1.66 nsMIMEInfoImpl.cpp
>--- uriloader/exthandler/nsMIMEInfoImpl.cpp	21 Aug 2007 00:47:48 -0000	1.66
>+++ uriloader/exthandler/nsMIMEInfoImpl.cpp	21 Aug 2007 16:56:19 -0000
>@@ -396,19 +396,18 @@ nsMIMEInfoBase::LaunchWithURI(nsIURI* aU
> 
>   if (mPreferredAction == useHelperApp) {
>     if (!mPreferredApplication)
>       return NS_ERROR_FILE_NOT_FOUND;
> 
>     // check for and possibly launch with web application
>     nsCOMPtr<nsIWebHandlerApp> webHandler = 
>       do_QueryInterface(mPreferredApplication, &rv);
>-    if (NS_SUCCEEDED(rv)) {
>+    if (NS_SUCCEEDED(rv))
>       return LaunchWithWebHandler(webHandler, aURI);         
>-    }
Looks like dmose already did this

>     // ok, we must have a local handler app
>     nsCOMPtr<nsILocalHandlerApp> localHandler = 
>       do_QueryInterface(mPreferredApplication, &rv);
>     NS_ENSURE_SUCCESS(rv, rv);
> 
>     nsCOMPtr<nsIFile> executable;
>     rv = localHandler->GetExecutable(getter_AddRefs(executable));

>Index: uriloader/exthandler/win/nsMIMEInfoWin.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/uriloader/exthandler/win/nsMIMEInfoWin.cpp,v
>retrieving revision 1.7
>diff -u -p -8 -r1.7 nsMIMEInfoWin.cpp
>--- uriloader/exthandler/win/nsMIMEInfoWin.cpp	26 Jul 2007 05:03:00 -0000	1.7
>+++ uriloader/exthandler/win/nsMIMEInfoWin.cpp	21 Aug 2007 16:56:20 -0000
>@@ -41,16 +41,24 @@
> #include "nsArrayEnumerator.h"
> #include "nsCOMArray.h"
> #include "nsILocalFile.h"
> #include "nsIVariant.h"
> #include "nsMIMEInfoWin.h"
> #include "nsNetUtil.h"
> #include <Windows.h>
> #include <shellapi.h>
>+#include "nsAutoPtr.h"
>+#include "nsIMutableArray.h"
>+#include "nsTArray.h"
>+#include "shlobj.h"
>+#include "windows.h"
>+#include "nsIWindowsRegKey.h"
>+#include "nsIProcess.h"
>+
> 
> NS_IMPL_ISUPPORTS_INHERITED1(nsMIMEInfoWin, nsMIMEInfoBase, nsIPropertyBag)
> 
> nsMIMEInfoWin::~nsMIMEInfoWin()
> {
> }
> 
> nsresult
>@@ -65,16 +73,103 @@ nsMIMEInfoWin::LaunchDefaultWithFile(nsI
>   local->IsExecutable(&executable);
>   if (executable)
>     return NS_ERROR_FAILURE;
> 
>   return local->Launch();
> }
> 
> NS_IMETHODIMP
>+nsMIMEInfoWin::LaunchWithFile(nsIFile* aFile)
>+{
>+  nsresult rv;
>+
>+  // it doesn't make any sense to call this on protocol handlers
>+  NS_ASSERTION(mClass == eMIMEInfo,
>+               "nsMIMEInfoBase should have mClass == eMIMEInfo");
>+
>+  if (mPreferredAction == useSystemDefault) {
>+    return LaunchDefaultWithFile(aFile);
>+  }
>+
>+  if (mPreferredAction == useHelperApp) {
>+    if (!mPreferredApplication)
>+      return NS_ERROR_FILE_NOT_FOUND;
>+
>+    // at the moment, we only know how to hand files off to local handlers
>+    nsCOMPtr<nsILocalHandlerApp> localHandler = 
>+      do_QueryInterface(mPreferredApplication, &rv);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+
>+    nsCOMPtr<nsIFile> executable;
>+    rv = localHandler->GetExecutable(getter_AddRefs(executable));
>+    NS_ENSURE_SUCCESS(rv, rv);
>+
>+    nsCAutoString path;
>+    aFile->GetNativePath(path);
>+
>+    // Deal with local dll based handlers
>+    nsCString filename;
>+    executable->GetNativeLeafName(filename);
>+    if (filename.Length() > 4) {
>+      nsCString extension(Substring(filename, filename.Length() - 4, 4));
>+      nsCString dllExt(".dll");
>+      ToLowerCase(extension);
>+
>+      if (extension.Equals(dllExt)) {
>+        nsAutoString args;
>+
>+        // executable is rundll32, eveything else is a list of parameters, 
>+        // including the dll handler.
>+       nsCOMPtr<nsILocalFile> locFile(do_QueryInterface(aFile));
nit: indentation

>@@ -165,8 +260,665 @@ nsMIMEInfoWin::LoadUriInternal(nsIURI * 
>...
>+
>+  // Trim any command parameters so that we have a native path we can
>+  // initialize a local file with.
>+  RemoveCmdParameters(handlerFilePath);
>+
>+  aCommandHandler = handlerFilePath;
>+  return PR_TRUE;
>+}
>+
>+// Given a path to a local file, return it's nsILocalHandlerApp instance.
nit: s/it's/its/


>+PRBool nsMIMEInfoWin::GetAppsVerbCommandHandler(nsAString& appExeName,
>...
>+  rv = appKey->Open(nsIWindowsRegKey::ROOT_KEY_CLASSES_ROOT,
>+                    applicationsPath,
>+                    nsIWindowsRegKey::ACCESS_QUERY_VALUE);
>+  if (NS_FAILED(rv)) return PR_FALSE;
newline before return

>+
>+  nsAutoString appFilesystemCommand;
>+  if (NS_SUCCEEDED(appKey->ReadStringValue(EmptyString(), 
>+                                           appFilesystemCommand))) {
>+    if (!CleanupCmdHandlerPath(appFilesystemCommand)) return PR_FALSE;
newline before return

>...
>+  nsresult rv = appKey->Open(nsIWindowsRegKey::ROOT_KEY_CLASSES_ROOT,
>+                             applicationsPath,
>+                             nsIWindowsRegKey::ACCESS_QUERY_VALUE);
>+  if (NS_FAILED(rv)) return PR_FALSE;
newline before return

>...
>+  rv = appKey->Open(nsIWindowsRegKey::ROOT_KEY_CLASSES_ROOT,
>+                    applicationsPath,
>+                    nsIWindowsRegKey::ACCESS_QUERY_VALUE);
>+  if (NS_FAILED(rv)) return PR_FALSE;
newline before return

>...
>+  nsresult rv = appKey->Open(nsIWindowsRegKey::ROOT_KEY_CLASSES_ROOT,
>+                             appProgId,
>+                             nsIWindowsRegKey::ACCESS_QUERY_VALUE);
>+  if (NS_FAILED(rv)) return PR_FALSE;
newline before return

>...
>+  // Don't include firefox.exe in the list
>+  char exe[MAX_PATH+1];
>+  PRUint32 len = GetModuleFileName(NULL, (LPTSTR)exe, MAX_PATH);
>+  if (len < MAX_PATH && len != 0) {
>+    nsCString ffexe = nsDependentCString((const char*)exe);
>+    PRUint32 index = lower.Find(ffexe);
>+    if (index != -1) return;
newline before return

>+  }
>+
>+  nsCOMPtr<nsILocalHandlerApp> aApp;
>+  if (!GetLocalHandlerApp(appFilesystemCommand, aApp)) return;
newline before return

>...
>+  // trackList data is always lowercase, see ProcessPath
>+  // above.
>+  nsCAutoString tmp = NS_ConvertUTF16toUTF8(appPath);
>+  ToLowerCase(tmp);
>+
>+  for (PRUint32 i = 0; i < trackList.Length(); i++) {
>+    if (tmp.Equals(trackList[i])) return PR_TRUE;
newline before return

>...
>+    rv = regKey->Open(nsIWindowsRegKey::ROOT_KEY_CLASSES_ROOT,
>+                      workingRegistryPath,
>+                      nsIWindowsRegKey::ACCESS_QUERY_VALUE);
>+    if (NS_SUCCEEDED(rv)) {
>+      PRUint32 iCount = 0;
>+      if (NS_SUCCEEDED(regKey->GetValueCount(&iCount)) && iCount > 0) {
>+        for (PRUint32 index = 0; index < iCount; index++) {
>+          nsAutoString appName;
>+          if (NS_FAILED(regKey->GetValueName(index, appName))) continue;
newline before continue

>+
>+          // HKEY_CLASSES_ROOT\Applications\firefox.exe = "path params"
>+          nsAutoString appFilesystemCommand;
>+          if (!GetAppsVerbCommandHandler(appName,
>+                                         appFilesystemCommand,
>+                                         PR_FALSE) ||
>+              IsPathEqual(appFilesystemCommand, trackList)) continue;
newline before continue

>...
>+    if (NS_SUCCEEDED(rv)) {
>+      PRUint32 iCount = 0;
>+      if (NS_SUCCEEDED(regKey->GetValueCount(&iCount)) && iCount > 0) {
>+        for (PRUint32 index = 0; index < iCount; index++) {
>+          // HKEY_CLASSES_ROOT\.ext\OpenWithProgids\Windows.XPSReachViewer
>+          nsAutoString appProgId;
>+          if (NS_FAILED(regKey->GetValueName(index, appProgId))) continue;
newline before continue

>+
>+          nsAutoString appFilesystemCommand;
>+          if (!GetProgIDVerbCommandHandler(appProgId,
>+                                           appFilesystemCommand,
>+                                           PR_FALSE) ||
>+              IsPathEqual(appFilesystemCommand, trackList)) continue;
newline before continue

>...
>+    if (NS_SUCCEEDED(rv)) {
>+      PRUint32 iCount = 0;
>+      if (NS_SUCCEEDED(regKey->GetValueCount(&iCount)) && iCount > 0) {
>+        for (PRUint32 index = 0; index < iCount; index++) {
>+          nsAutoString appName, appValue;
>+          if (NS_FAILED(regKey->GetValueName(index, appName))) continue;
newline before continue

>+          if (appName == NS_LITERAL_STRING("MRUList")) continue;
newline before continue

>+          if (NS_FAILED(regKey->ReadStringValue(appName, appValue))) continue;
newline before continue

>+          
>+          // HKEY_CLASSES_ROOT\Applications\firefox.exe = "path params"
>+          nsAutoString appFilesystemCommand;
>+          if (!GetAppsVerbCommandHandler(appValue,
>+                                         appFilesystemCommand,
>+                                         PR_FALSE) ||
>+              IsPathEqual(appFilesystemCommand, trackList)) continue;
newline before continue

>...
>+    if (NS_SUCCEEDED(rv)) {
>+      PRUint32 iCount = 0;
>+      if (NS_SUCCEEDED(regKey->GetValueCount(&iCount)) && iCount > 0) {
>+        for (PRUint32 index = 0; index < iCount; index++) {
>+          nsAutoString appIndex, appProgId;
>+          if (NS_FAILED(regKey->GetValueName(index, appProgId))) continue;
newline before continue

>+
>+          nsAutoString appFilesystemCommand;
>+          if (!GetProgIDVerbCommandHandler(appProgId,
>+                                           appFilesystemCommand,
>+                                           PR_FALSE) ||
>+              IsPathEqual(appFilesystemCommand, trackList)) continue;
newline before continue

>...
>+        if (NS_SUCCEEDED(rv)) {
>+          PRUint32 iCount = 0;
>+          if (NS_SUCCEEDED(regKey->GetValueCount(&iCount)) && iCount > 0) {
>+            for (PRUint32 index = 0; index < iCount; index++) {
>+              nsAutoString appName;
>+              if (NS_FAILED(regKey->GetValueName(index, appName))) continue;
newline before continue

>+              
>+              // HKEY_CLASSES_ROOT\Applications\firefox.exe = "path params"
>+              nsAutoString appFilesystemCommand;
>+              if (!GetAppsVerbCommandHandler(appName, appFilesystemCommand, 
>+                                             PR_FALSE) ||
>+                  IsPathEqual(appFilesystemCommand, trackList)) continue;
newline before continue

>...
>+  if (NS_SUCCEEDED(rv)) {
>+    PRUint32 iCount = 0;
>+    if (NS_SUCCEEDED(regKey->GetValueCount(&iCount)) && iCount > 0) {
>+      for (PRUint32 index = 0; index < iCount; index++) {
>+        nsAutoString appName;
>+        if (NS_FAILED(regKey->GetValueName(index, appName))) continue;
newline before continue

>+
>+        // HKEY_CLASSES_ROOT\Applications\firefox.exe = "path params"
>+        nsAutoString appFilesystemCommand;
>+        if (!GetAppsVerbCommandHandler(appName, appFilesystemCommand,
>+                                       PR_FALSE) ||
>+            IsPathEqual(appFilesystemCommand, trackList)) continue;
newline before continue

>...
>+  if (NS_SUCCEEDED(rv)) {
>+    PRUint32 iCount = 0;
>+    if (NS_SUCCEEDED(regKey->GetChildCount(&iCount)) && iCount > 0) {
>+      for (PRUint32 index = 0; index < iCount; index++) {
>+        nsAutoString appName;
>+        if (NS_FAILED(regKey->GetChildName(index, appName))) continue;
newline before continue

>+
>+        // HKEY_CLASSES_ROOT\Applications\firefox.exe = "path params"
>+        nsAutoString appFilesystemCommand;
>+        if (!GetAppsVerbCommandHandler(appName, appFilesystemCommand,
>+                                       PR_FALSE) ||
>+            IsPathEqual(appFilesystemCommand, trackList)) continue;
newline before continue

>Index: uriloader/exthandler/win/nsMIMEInfoWin.h
>===================================================================
>RCS file: /cvsroot/mozilla/uriloader/exthandler/win/nsMIMEInfoWin.h,v
>retrieving revision 1.9
>diff -u -p -8 -r1.9 nsMIMEInfoWin.h
>--- uriloader/exthandler/win/nsMIMEInfoWin.h	26 Jul 2007 04:24:30 -0000	1.9
>+++ uriloader/exthandler/win/nsMIMEInfoWin.h	21 Aug 2007 16:56:20 -0000
>@@ -34,36 +34,75 @@
>...
>   private:
>     nsCOMPtr<nsIFile>      mDefaultApplication;
>+    
>+    // Given a path to a local handler, return it's
nit: s/it's/its/

>+    // nsILocalHandlerApp instance.
>+    PRBool GetLocalHandlerApp(nsAString& aCommandHandler,
>+                      nsCOMPtr<nsILocalHandlerApp>& aApp);
nit: indent consistently

>+
>+    // Return the cleaned up file path associated 
>+    // with a command verb located in root/Applications.
>+    PRBool GetAppsVerbCommandHandler(nsAString& appExeName,
>+       nsAString& applicationPath, PRBool bEdit);

r=me with these nits fixed.

I'm fine with the Win32 specific parts but since I am not familiar with this code I'd like either myk or dmose to give it a once over.

Comment 56

[Robert Strong (they/them - no direct email)]

Comment on attachment 277557 [details] [diff] [review]
base patch v.6

Jim please resubmit with the nits fixed and request review from dmose or myk. Thanks!

Comment 57

[Jim Mathies [:jimm]]

>I wonder if this should be openSubDialog as is used in advanced.js. Gavin or
>mano should be able to answer that.

Looking at the docs, that does seem more appropriate. I changed it up, will compile and test and make sure everything works correctly and will post.

Comment 58

[Jim Mathies [:jimm]]

>Looking at the docs, that does seem more appropriate.

Looks like this method is only available on prefs windows.

Comment 59

[Jim Mathies [:jimm]]

Attachment: base patch v.7

Comment 60

[Jim Mathies [:jimm]]

>Note that this patch conflicts with the patch in bug 377784.  But the conflict
>should be easy to resolve, as it's just in changeaction.js, and the code this
>patch adds to that file is well isolated and should be easy to move into
>applications.js, which bug 377784 adds while removing changeaction.js.

Since bug 377784 isn't checked in yet, I guess just wait till it is maybe before flagging this for another review? 

Comment 61

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #60)
> >Note that this patch conflicts with the patch in bug 377784.  But the conflict
> >should be easy to resolve, as it's just in changeaction.js, and the code this
> >patch adds to that file is well isolated and should be easy to move into
> >applications.js, which bug 377784 adds while removing changeaction.js.
> 
> Since bug 377784 isn't checked in yet, I guess just wait till it is maybe
> before flagging this for another review? 

If this is ready, then it would be better to get it reviewed and checked in.  These patches can get committed in either order; the last one in the pool just has to do the conflict resolution.

Comment 62

[Jim Mathies [:jimm]]

Either way it lands I don't mind, switching this over to the new file looks to be an easy one.

Comment 63

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 278584 [details] [diff] [review]
base patch v.7

Argh, sorry, somehow I completely forgot about this review request.  So, reviewing the parts that touch the preferences (changeaction.js), those look good, although they'll need to be updated for the new Applications prefpane that landed this evening.

But dmose says biesi needs to review the uriloader code, as he's the owner of that module, so requesting review from him for that piece.

Comment 64

[Mike Connor [:mconnor]]

Pushing into M9, need to get traction from biesi.

Comment 65

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 278584 [details] [diff] [review]
base patch v.7

Would be nice if you could mention
http://msdn2.microsoft.com/en-us/library/aa969371.aspx and
http://msdn2.microsoft.com/en-us/library/aa969373.aspx near the top of
GetPossibleLocalHandlers

It seems that your patch has windows line-endings on _some_ lines... please fix

+      nsCString dllExt(".dll");
+      ToLowerCase(extension);
+
+      if (extension.Equals(dllExt)) {

Why not just:
  if (extension.LowerCaseEqualsLiteral(".dll"))

+        // executable is rundll32, eveything else is a list of parameters, 

typo (eveything)

+        if (!GetDllLaunchInfo(executable, locFile, args, PR_FALSE))

Doesn't this assume that the DLL came from HKCR\Applications? Couldn't it have
come from HKCR\progid\shell\open\command?


Please reuse RemoveParameters from nsOSHelperAppService.cpp, instead of copying
it to nsMIMEInfoWin.cpp

Please find a way to share the similar code between CleanupCmdHandlerPath and
GetDefaultAppInfo (nsOSHelperAppService.cpp), and note the comment there:

  // Similarly replace embedded environment variables... (this must be done
  // AFTER |RemoveParameters| since it may introduce spaces into the path
  // string)

+PRBool nsMIMEInfoWin::GetLocalHandlerApp(nsAString& aCommandHandler,
+                                         nsCOMPtr<nsILocalHandlerApp>& aApp)

usual style would be to make that an nsILocalHandlerApp**, and use
getter_AddRefs in the caller (and use CallCreateInstance in the impl)

+  nsCOMPtr<nsIFile> file(do_QueryInterface(locfile));

no need, nsILocalFile inherits from nsIFile

+PRBool nsMIMEInfoWin::GetAppsVerbCommandHandler(nsAString& appExeName,
+                                                nsAString& applicationPath,
+                                                PRBool bEdit)

make in parameters const (appExeName)... also use nsString& where you can, it's
more efficient

It looks like you never pass true for bEdit?

+  PRUint32 iValue;

please don't use hungarian notation, just call this value

+  applicationsPath = NS_LITERAL_STRING("Applications\\");

AssignLiteral

+    if (!CleanupCmdHandlerPath(appFilesystemCommand)) return PR_FALSE;

put the return on its own line

+  nsCOMPtr<nsILocalFile> localTarget = aFile;
+  if (!localTarget)
+    return PR_FALSE;

there is no need to nullcheck this again, you did that a few lines above.
however, is there a need for the localTarget variable at all?

+  nsCString appExeName;
+  localDll->GetNativeLeafName(appExeName);
...
+  applicationsPath.Append(NS_ConvertUTF8toUTF16(appExeName));

native leaf names aren't UTF-8 strings. Getting a native name is also lossy on
windows. Why not just make appExeName an nsString and use GetLeafName?

+  applicationsPath = NS_LITERAL_STRING("Applications\\");

.AssignLiteral


(seems like for this bEdit parameter gets only passed PR_FALSE too)

+    if (index > -1) {

if (index != kNotFound)

+    NS_NAMED_LITERAL_STRING(rundllSegment, "rundll32.exe ");
+
+    PRInt32 index = appFilesystemCommand.Find(rundllSegment);
...
+    index = params.Find(NS_LITERAL_STRING("%1"));

It would be nice to be consistent... (temporary variable or not)

+    if (index == -1) // no parameter

kNotFound

+    nsCAutoString target;
+    localTarget->GetNativeTarget(target);
+    params.Replace(index, 2, NS_ConvertUTF8toUTF16(target));

GetTarget

Is there a specific reason why you're using an autostring here while you used a
non-autostring for appExePath?

+  nsAutoString appProgId(appProgIDName);

what's the point of this variable?

GetProgIDVerbCommandHandler also only gets PR_FALSE for the bEdit argument

+  nsCString lower = NS_ConvertUTF16toUTF8(appFilesystemCommand);
+  ToLowerCase(lower);

why convert to UTF-8 instead of leaving it as a wide string?

+  char exe[MAX_PATH+1];
+  PRUint32 len = GetModuleFileName(NULL, (LPTSTR)exe, MAX_PATH);

why not call the wide version here? Also, the cast seems unnecessary.

+    nsCString ffexe = nsDependentCString((const char*)exe);
+    PRUint32 index = lower.Find(ffexe);

I don't see the point of the ffexe variable? couldn't you pass exe to find?

(but if not, ffexe should be declared like this:
  nsDependentCString ffexe(exe);
though I'd just do that inline as Find(nsDependentCString(exe)))

+    if (index != -1)

kNotFound

+  nsCAutoString tmp = NS_ConvertUTF16toUTF8(appPath);

I'd store wide strings in trackList, then you don't need this conversion

IsPathEqual doesn't seem like the best name, perhaps IsPathInList?


+  nsCAutoString aFileExt;
+  nsAutoString workingRegistryPath;
+  nsTArray<nsCAutoString> trackList;

You declare most variables where you need them... why do you declare these at
the top of the function?

+  PRBool bGeneral = PR_FALSE;

please no hungarian notation... and this doesn't seem like a good name, perhaps
extKnown or something?

+  if (aFileExt.IsEmpty() || aFileExt.Length() > 10) {

what's the significance of 10?

+  if (bGeneral == PR_FALSE) {

if (!bGeneral)

+    workingRegistryPath = fileExtToUse;
+
+    rv = regKey->Open(nsIWindowsRegKey::ROOT_KEY_CLASSES_ROOT,
+                      workingRegistryPath,
+                      nsIWindowsRegKey::ACCESS_QUERY_VALUE);

Why not directly pass fileExtToUse to Open?

+        if (appProgId != NS_LITERAL_STRING("XPSViewer.Document")) {

if (!appProgId.EqualsLiteral("XPSViewer.Document"))

+            ProcessPath(appList,trackList,appFilesystemCommand);

please add spaces after the commas (same for the other calls to this function)

+      PRUint32 iCount = 0;

again, just name this count (multiple times in this function)

+          if (appName == NS_LITERAL_STRING("MRUList"))

.EqualsLiteral("MRUList")



+  if (description == NS_LITERAL_STRING("XPSViewer.Document")) {

.EqualsLiteral("XPSViewer.Document")

Comment 66

[Christian :Biesinger (don't email me, ping me on IRC)]

oh, also: nsIMIMEInfo needs a new IID

Comment 67

[Jim Mathies [:jimm]]

>http://msdn2.microsoft.com/en-us/library/aa969373.aspx near the top of
>GetPossibleLocalHandlers

Added to the perceived type processing comments.

> It seems that your patch has windows line-endings on _some_ lines... please fix

I would have assumed CVS would have taken care of this. I'm honestly not sure how to fix this. Considering it's in a windows specific src file, does it really makes any difference? 

>Doesn't this assume that the DLL came from HKCR\Applications? Couldn't it have
>come from HKCR\progid\shell\open\command?

Application handlers should be in the applications list. Plus, since we're starting from the dll, not the extension, we really can't trace back.  To support better trace back, we'd need additional modifications to handler app to contain more meta data, and I assume that would then require changes to things like the configured helper database. (this info would have to persist across sessions and I'm guessing we don't serialize the entire object to disk when we store preferences like that?) Overall, those changes seem out of the scope of this patch.

>Please reuse RemoveParameters from nsOSHelperAppService.cpp, instead of copying
>it to nsMIMEInfoWin.cpp

done.

>Please find a way to share the similar code between CleanupCmdHandlerPath and
>GetDefaultAppInfo (nsOSHelperAppService.cpp), 

done. 

// Similarly replace embedded environment variables... (this must be done
// AFTER |RemoveParameters| since it may introduce spaces into the path
// string)

I've moved this, although technically if the path to the dll handler contains an expanded directory name containing spaces, it should always be encapsulated in quotes. I've never seen this although maybe it's a result of some sort of misconfigured handler we supported in the past.

>usual style would be to make that an nsILocalHandlerApp**, and use
>getter_AddRefs in the caller (and use CallCreateInstance in the impl)

Ok, but just out of curiousity, why do we avoid pass by reference so much in our code when it adds so much extra work?

>no need, nsILocalFile inherits from nsIFile

updated.

>It looks like you never pass true for bEdit?

Correct, I was using it during testing but ended up not needing it, but left it in for future use. It may be handy if we support local edit handlers at some point, or decide to include these in the handler application list. An extra bool pref obviously isn't going to kill performance.

> please don't use hungarian notation, just call this value

updated.

> AssignLiteral

updated.

> put the return on its own line

updated.

> is there a need for the localTarget variable at all?

removed.

> .AssignLiteral

updated.

> It would be nice to be consistent...

done.

+    if (index > -1) {
+    if (index == -1) // no parameter

updated.

> GetTarget

updated.

> Is there a specific reason why you're using an autostring here while you used > a non-autostring for appExePath?

Honestly, strings in mozilla were a bear for me in getting things to compile. This was the first patch I worked on after starting. Auto strings tended to be the simplest to work with so I ended up standardizing on their use. I recognize they have added memory use, but in an area like this, I didn't see the harm in relying on them.

>+  nsAutoString appProgId(appProgIDName);
>
>what's the point of this variable?

I'm making a copy of the string before modifying it. (I append to it). For some reason our nsAString class doesn't allow it to be passed in as local variable. 

>
>+  nsCString lower = NS_ConvertUTF16toUTF8(appFilesystemCommand);
>+  ToLowerCase(lower);
>
>why convert to UTF-8 instead of leaving it as a wide string?
>
>+  char exe[MAX_PATH+1];
>+  PRUint32 len = GetModuleFileName(NULL, (LPTSTR)exe, MAX_PATH);
>
>why not call the wide version here? Also, the cast seems unnecessary.
>
>+    nsCString ffexe = nsDependentCString((const char*)exe);
>+    PRUint32 index = lower.Find(ffexe);
>
>I don't see the point of the ffexe variable? couldn't you pass exe to find?
>
>(but if not, ffexe should be declared like this:
>  nsDependentCString ffexe(exe);
>though I'd just do that inline as Find(nsDependentCString(exe)))

More string confusion, nsAString doesn't work with ToLowerCase, and the trackList is stored as 8 bit instead of 16.

>I'd store wide strings in trackList, then you don't need this conversion

Isn't that a major performance hit in doing the compares? We're comparing doing a one time utf16 to utf8 conversion, and then multiple compares across the result vs. no conversion, and multiple compares across 16 bit strings.

> IsPathEqual doesn't seem like the best name, perhaps IsPathInList?

updated.

>+  nsCAutoString aFileExt;
>+  nsAutoString workingRegistryPath;
>+  nsTArray<nsCAutoString> trackList;
>
>You declare most variables where you need them... why do you declare these at
>the top of the function?

updated.

>please no hungarian notation... and this doesn't seem like a good name, perhaps
>extKnown or something?

updated.

>what's the significance of 10?

Just a safely catch on an extreme bound. I've removed it.

>if (!bGeneral)

updated.

> Why not directly pass fileExtToUse to Open?

The convention throughout is to make a copy of it and then append something onto the end. I was just sticking with that even though this case didn't require an append.

> again, just name this count (multiple times in this function)

updated.

>.EqualsLiteral("MRUList")
>.EqualsLiteral("XPSViewer.Document")

updated.

Comment 68

[Jim Mathies [:jimm]]

Attachment: new files zipped

Comment 69

[Jim Mathies [:jimm]]

Attachment: base patch v.8

Comment 70

[Jim Mathies [:jimm]]

I noticed another optimization here, we can use nsOSHelperAppService::GetExtensionFromWindowsMimeDatabase instead of manually getting the extension type in GetPossibleLocalHandlers. I'll try to post another patch in the next few days. Imho, this shouldn't hold up reviews though, it's a minor update.

Comment 71

[Jim Mathies [:jimm]]

Note also, some additional sharing can take place between the patch for bug 397678 and this. I'm going to hold up 397678 until this gets checked in and address that in that patch. 

Comment 72

[Jim Mathies [:jimm]]

Attachment: base patch v.8 (minus win return chars)

Minor update minus the windows carriage returns.

Comment 73

[Christian :Biesinger (don't email me, ping me on IRC)]

> Overall, those changes seem out of the scope of this patch.

I guess. OK then, but it would be good to change that.

>Ok, but just out of curiousity, why do we avoid pass by reference so much in
>our code when it adds so much extra work?

I don't actually know...

> An extra bool pref obviously isn't going to kill performance.

Yeah, true. In that case please avoid the hungarian notation here as well :)

> nsAString doesn't work with ToLowerCase

It does, if you include nsUnicharUtils.h (and the trackList could easily store 16-bit strings instead)

>Isn't that a major performance hit in doing the compares? We're comparing doing
>a one time utf16 to utf8 conversion, and then multiple compares across the
>result vs. no conversion, and multiple compares across 16 bit strings.

OK (though I doubt the performance difference would matter). But then, please use something like this to declare tmp:

  NS_ConvertUTF16toUTF8 tmp(appPath);

(avoids a string copy)

Comment 74

[Christian :Biesinger (don't email me, ping me on IRC)]

+  //nsCOMPtr<nsIFile> file(do_QueryInterface(locfile));
+  //aApp->SetExecutable(file);

please remove that

+  nsCString lower = NS_ConvertUTF16toUTF8(appFilesystemCommand);

should also be changed like I suggested for tmp above

+  if (fileExt.IsEmpty() && fileExt.Length() == 0) {

er, aren't the two parts of the condition equivalent?

+          fileExt = NS_ConvertUTF16toUTF8(mimeFileExt);

CopyUTF16toUTF8(mimeFileExt, fileExt);

Is there a case where there is no primary extension but where the registry contains an extension? I mean, shouldn't this extension finding be handled in nsOSHelperAppService when it creates the mime info?

uriloader/exthandler/win/nsOSHelperAppService.cpp
+  if ( !CleanupCmdHandlerPath(handlerCommand) )

please remove those spaces

+  if (index > kNotFound) {

should be !=

Comment 75

[Jim Mathies [:jimm]]

coming right up...

Comment 76

[Jim Mathies [:jimm]]

>Is there a case where there is no primary extension but where the registry
>contains an extension? I mean, shouldn't this extension finding be handled in
>nsOSHelperAppService when it creates the mime info?

I found once case where this was called, but mimeinfo hadn't been setup with an extension. It resulted in a empty open with list. (This was in the preferences after we made some changes there.) I think it works either way. There's nothing preventing us from making sure wherever we create these we insure there's a extension configured. However, as it stands, this requires no other changes and works with our current prefs pane. It's such a small block of code, it can be moved any time we want.

Comment 77

[Jim Mathies [:jimm]]

Attachment: new files zipped (minus win ret chars)

Comment 78

[Jim Mathies [:jimm]]

Attachment: base patch v.8

Comment 79

[Jim Mathies [:jimm]]

Attachment: new files zipped

Comment 80

[Shawn Wilsher :sdwilsh]

Comment on attachment 284952 [details] [diff] [review]
base patch v.8

This is blocking, so it doesn't need approval.

Comment 81

[Reed Loden [:reed]]

Checking in browser/components/preferences/applications.js;
/cvsroot/mozilla/browser/components/preferences/applications.js,v  <--  applications.js
new revision: 1.25; previous revision: 1.24
done
Checking in netwerk/mime/public/nsIMIMEInfo.idl;
/cvsroot/mozilla/netwerk/mime/public/nsIMIMEInfo.idl,v  <--  nsIMIMEInfo.idl
new revision: 1.39; previous revision: 1.38
done
Checking in toolkit/components/Makefile.in;
/cvsroot/mozilla/toolkit/components/Makefile.in,v  <--  Makefile.in
new revision: 1.72; previous revision: 1.71
done
Checking in toolkit/locales/jar.mn;
/cvsroot/mozilla/toolkit/locales/jar.mn,v  <--  jar.mn
new revision: 1.43; previous revision: 1.42
done
RCS file: /cvsroot/mozilla/toolkit/locales/en-US/chrome/global/appPicker.dtd,v
done
Checking in toolkit/locales/en-US/chrome/global/appPicker.dtd;
/cvsroot/mozilla/toolkit/locales/en-US/chrome/global/appPicker.dtd,v  <--  appPicker.dtd
initial revision: 1.1
done
Checking in toolkit/mozapps/downloads/src/nsHelperAppDlg.js.in;
/cvsroot/mozilla/toolkit/mozapps/downloads/src/nsHelperAppDlg.js.in,v  <--  nsHelperAppDlg.js.in
new revision: 1.55; previous revision: 1.54
done
Checking in uriloader/exthandler/nsMIMEInfoImpl.cpp;
/cvsroot/mozilla/uriloader/exthandler/nsMIMEInfoImpl.cpp,v  <--  nsMIMEInfoImpl.cpp
new revision: 1.69; previous revision: 1.68
done
Checking in uriloader/exthandler/nsMIMEInfoImpl.h;
/cvsroot/mozilla/uriloader/exthandler/nsMIMEInfoImpl.h,v  <--  nsMIMEInfoImpl.h
new revision: 1.39; previous revision: 1.38
done
Checking in uriloader/exthandler/win/nsMIMEInfoWin.cpp;
/cvsroot/mozilla/uriloader/exthandler/win/nsMIMEInfoWin.cpp,v  <--  nsMIMEInfoWin.cpp
new revision: 1.8; previous revision: 1.7
done
Checking in uriloader/exthandler/win/nsMIMEInfoWin.h;
/cvsroot/mozilla/uriloader/exthandler/win/nsMIMEInfoWin.h,v  <--  nsMIMEInfoWin.h
new revision: 1.10; previous revision: 1.9
done
Checking in uriloader/exthandler/win/nsOSHelperAppService.cpp;
/cvsroot/mozilla/uriloader/exthandler/win/nsOSHelperAppService.cpp,v  <--  nsOSHelperAppService.cpp
new revision: 1.76; previous revision: 1.75
done
Checking in uriloader/exthandler/win/nsOSHelperAppService.h;
/cvsroot/mozilla/uriloader/exthandler/win/nsOSHelperAppService.h,v  <--  nsOSHelperAppService.h
new revision: 1.30; previous revision: 1.29
done
RCS file: /cvsroot/mozilla/toolkit/components/apppicker/Makefile.in,v
done
Checking in toolkit/components/apppicker/Makefile.in;
/cvsroot/mozilla/toolkit/components/apppicker/Makefile.in,v  <--  Makefile.in
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/toolkit/components/apppicker/jar.mn,v
done
Checking in toolkit/components/apppicker/jar.mn;
/cvsroot/mozilla/toolkit/components/apppicker/jar.mn,v  <--  jar.mn
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/toolkit/components/apppicker/content/appPicker.css,v
done
Checking in toolkit/components/apppicker/content/appPicker.css;
/cvsroot/mozilla/toolkit/components/apppicker/content/appPicker.css,v  <--  appPicker.css
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/toolkit/components/apppicker/content/appPicker.js,v
done
Checking in toolkit/components/apppicker/content/appPicker.js;
/cvsroot/mozilla/toolkit/components/apppicker/content/appPicker.js,v  <--  appPicker.js
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/toolkit/components/apppicker/content/appPicker.xul,v
done
Checking in toolkit/components/apppicker/content/appPicker.xul;
/cvsroot/mozilla/toolkit/components/apppicker/content/appPicker.xul,v  <--  appPicker.xul
initial revision: 1.1
done

Comment 82

[Jim Mathies [:jimm]]

Hey all, noticed we're listing applications selected in the pref pane more than once. If you select a handler manually, then select it again, it's listed twice. This should probably be broken out into a seperate bug?

Comment 83

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #82)
> Hey all, noticed we're listing applications selected in the pref pane more than
> once. If you select a handler manually, then select it again, it's listed
> twice. This should probably be broken out into a seperate bug?

Yup, please file a new bug on this and cc: me on it.

Comment 84

[Alfred Kayser]

appPicker.css should have been in /skin/, probably /global/skin/ or /mozapps/skin/, so that themers can modify it.
Created bug 400460 to address this.

Comment 85

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

Comment on attachment 284952 [details] [diff] [review]
base patch v.8

>Index: browser/components/preferences/applications.js

>   chooseApp: function(aEvent) {

>+    if (params.handlerApp && 
>+        params.handlerApp.executable && 
>+        params.handlerApp.executable.isFile()) {

Is there a reason this didn't use _isValidHandlerExecutable?

Comment 86

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #85)
> (From update of attachment 284952 [details] [diff] [review])
> >Index: browser/components/preferences/applications.js
> 
> >   chooseApp: function(aEvent) {
> 
> >+    if (params.handlerApp && 
> >+        params.handlerApp.executable && 
> >+        params.handlerApp.executable.isFile()) {
> 
> Is there a reason this didn't use _isValidHandlerExecutable?

Probably because the first seven versions of patch were against changeaction.js, where there was no _isValidHandlerExecutable.

Comment 87

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

(In reply to comment #86)
> Probably because the first seven versions of patch were against
> changeaction.js, where there was no _isValidHandlerExecutable.

Ah, fair enough! I filed bug 403857.

Comment 88

[Jim Mathies [:jimm]]

Yeah sorry I did that switch over to the new file at the very end and didn't realize that was in there.

Comment 89

[zhang xiaoyun [:ThinkVision]]

find a variable mistake in http://mxr.mozilla.org/comm-1.9.2/source/mozilla/toolkit/components/apppicker/content/appPicker.js
Here it is:
193 #ifdef XP_MACOSX
194     const nsILocalFileMac = Components.interfaces.nsILocalFileMac;
195     if (file instanceof nsILocalFileMac) {
196       try {
197         return lfm.bundleDisplayName;
198       } catch (e) {
199       }
200     }
201 #endif
I think variable "lfm" should be "file".