In SeaMonkey, the unchanged "Opening ..." dialog (unknownContentType.xul) is used (I don't know if this is also the case for Firefox). On my Linux system, no "default applications" are found. Perhaps as I don't use a mailcap file and/or as I'm not using gnome. If I select a helper application, then the "Browse..." button hides and the name of my helper application is shown. As soon as I open a file of the same type, some time later, I get the "Browse..." button again. If I don't change anything (keep the "Browse..." button where it is), then my previously selected helper application is used, but I expect the name of the helper application to be visible here. The old code had a mix of "hide handler menulist" and "show handler menulist". My code changes this to *one* "hide handler menulist" (which is the default status, now) and switches it to visible whenever it is required.

Comment on attachment 436285 [details] [diff] [review] Patch to fix this bug > The old code had a mix of "hide handler menulist" and "show handler menulist". > My code changes this to *one* "hide handler menulist" (which is the default > status, now) and switches it to visible whenever it is required. It seems like a sensible approach, a little step towards making this code more linear. I have a couple of suggestions and should point out a few issues with the patch: > initAppAndSaveToDiskValues: function() { >+ >+ // Hide "handler" menulist by default. >+ this.dialogElement("modeDeck").setAttribute("selectedIndex", "1"); >+ > var modeGroup = this.dialogElement("mode"); I suggest you just add the attribute to the XUL file. The rest of the code makes assumptions on the initial state of the controls, so it should be fine to simply change the initial state. The issue with this is, apparently, that the code inside this "if" condition... if (this.mLauncher.targetFileIsExecutable || ( (mimeType == "application/octet-stream" || mimeType == "application/x-msdownload") && !openWithDefaultOK)) { ...assumes that the menulist is initially visible. However, on closer inspection it seems that hitting this code path (which shows a user interface with a disabled "open" choice) is improbable, if not impossible. For executables, in fact, we normally use the "basic" version of the user interface: var shouldntRememberChoice = (mimeType == "application/octet-stream" || mimeType == "application/x-msdownload" || this.mLauncher.targetFileIsExecutable); if (shouldntRememberChoice && !this.openWithDefaultOK()) { // ...... show basic UI ...... } else { this.initAppAndSaveToDiskValues(); A bit of boolean expression simplification shows that the not-basic-but-open-disabled-UI is only shown when targetFileIsExecutable AND openWithDefaultOK are BOTH true. Weird! On Windows, this is impossible; on other platforms, digging through the code reveals that targetFileIsExecutable is generally false (except rarely, undeterministically, on OS2) as the executable permission bit is not set on the downloaded files. Long story short: if Shawn, that originally asked me to look at this patch, agrees that the not-basic-but-open-disabled-UI is unneeded, just remove that code instead of fixing it (but ensure that we use the "basic" user interface in that case). > } else if (this.mLauncher.MIMEInfo.preferredAction == this.nsIMIMEInfo.useHelperApp) { > // Open with given helper app. > modeGroup.selectedItem = this.dialogElement("open"); > openHandler.selectedIndex = 1; >+ // Show the "handler" menulist since we have a (user-specified) >+ // application now. >+ this.dialogElement("modeDeck").setAttribute("selectedIndex", "0"); The initAppAndSaveToDiskValues function has two parts, one that determines the visibility of the controls and one that selects the appropriate control depending on the previous user choices. This is the second path; the visibility change should go in the first part instead. If you have any question feel free to ask :-)

- Moved default selection index to XUL - Dropped duplicated code for case of executable files (Comment #1) > The initAppAndSaveToDiskValues function has two parts, one that determines > the visibility of the controls and one that selects the appropriate control > depending on the previous user choices. This is the second path; the > visibility change should go in the first part instead. For me this whole code is just complicated. I can't see any logical separation into "parts" there. Please tell me, what exactly I have to change. Do you think I should commit a second patch with useless trailing whitespaces removed? This code has many of them and maybe it would be a good idea to get rid of them. I configured Emacs to highlight trailing whitespaces in red and in this code file, I have much red stuff...

Comment on attachment 437044 [details] [diff] [review] Second patch (In reply to comment #2) > For me this whole code is just complicated. Yes, it is much more complicated than it could be :-) > Please tell me, what exactly I have to change. > } else if (this.mLauncher.MIMEInfo.preferredAction == this.nsIMIMEInfo.useHelperApp) { >+ // Show the "handler" menulist since we have a (user-specified) >+ // application now. >+ this.dialogElement("modeDeck").setAttribute("selectedIndex", "0"); Move this code inside this "if" block instead: if (this.chosenApp && this.chosenApp.executable && this.chosenApp.executable.path) { This condition is about the availability of a user-chosen helper application, while the other is about the choice of using that application by default. > // If we don't have a "default app" then disable that choice. >- if (!openWithDefaultOK) { >+ if (!this.openWithDefaultOK) { openWithDefaultOK() > var mimeType = this.mLauncher.MIMEInfo.MIMEType; > var shouldntRememberChoice = (mimeType == "application/octet-stream" || > mimeType == "application/x-msdownload" || > this.mLauncher.targetFileIsExecutable); > if (shouldntRememberChoice && !this.openWithDefaultOK()) { When this.openWithDefaultOK() and targetFileIsExecutable are both true, we end up displaying the full user interface, with the ability to open the executable directly from the browser (not on Windows, however). I think this may be correct, however I'll ask for a second opinion just in case. > Do you think I should commit a second patch with useless trailing whitespaces > removed? This code has many of them and maybe it would be a good idea to get > rid of them. I configured Emacs to highlight trailing whitespaces in red and > in this code file, I have much red stuff... That would be nice! Just open a separate bug for it, and attach a patch after this one has been checked in. We want to do that kind of changes when there are no recent pending patches from other people for the same file, as that might force them to regenerate their pacthes before check-in.

Manuel, can you provide a patch with the above issues fixed? With regard to the rare case where openWithDefaultOK() and targetFileIsExecutable are both true, showing the full UI is probably fine. The not-basic-but-open-disabled-UI is actually a leftover from an old bug (see bug 347230 comment 14).

I'm currently working on my patch, but I've re-added the case for "targetFileIsExecutable" and "openWithDefaultOK". I now just disable the possibility to open with default application (which would allow to directly execute) if "targetFileIsExecutable" is true. This handles the last case with just a few code lines. A helper application for this type would still work, then. If we want to simplify the code there, then this maybe should get another bug report to discuss about this and possible security problems. It would be *bad* if there is even a small possibility to accidently execute a file "from web". My hope is to work out a patch, which may easily land on HG, so there is a chance this annoying bug is fixed in SeaMonkey 2.1. This evening, I'll attach another patch.

Added all the fixes, you suggested, without one: >> // If we don't have a "default app" then disable that choice. >>- if (!openWithDefaultOK) { >>+ if (!this.openWithDefaultOK) { > > openWithDefaultOK() As I dropped the "var openWithDefaultOK = this.openWithDefaultOK;" in this function (only used once), the "this" is required here. IMHO there is no need to make things more complicated here as needed. My try to fix the problem which *may* lead to a small security hole is in the "openWithDefaultOK" function. I added the "this.mLauncher.targetFileIsExecutable" for all "non Windows" platforms, so this function now does what it should: It tells us if it is a good idea to open the given file with the default application. I did a short try with a freshly built SeaMonkey and the patch seems to do what it should. Maybe further tests are required to get sure this bug is fixed with this patch.

Missed a small mistake... https://bugzilla.mozilla.org/attachment.cgi?id=438550&action=diff#a/toolkit/mozapps/downloads/nsHelperAppDlg.js_sec6 This one: if (!this.openWithDefaultOK) { should be: if (!this.openWithDefaultOK()) { ^^ If required, I'll upload another patch with this mistake fixed, tomorrow.

(In reply to comment #5) > I'm currently working on my patch, but I've re-added the case for > "targetFileIsExecutable" and "openWithDefaultOK". I now just disable the > possibility to open with default application (which would allow to directly > execute) if "targetFileIsExecutable" is true. This handles the last case with > just a few code lines. Excellent, this preserves exactly the current behavior, and makes it clear. > A helper application for this type would still work, then. Only if the file isn't executable and, on non-Windows, has a default handler; otherwise, the "basic", save-only user interface is shown. This is consistent with the current behavior but I'm not sure if this is what we want. See also the "NOTE" in bug 369431 comment 0. > If we want to simplify the code there, then this maybe should get another bug > report to discuss about this and possible security problems. It would be *bad* > if there is even a small possibility to accidently execute a file "from web". Yes, if we're changing the behavior of the user interface, we should be careful and get feedback from the UI experts. I'd appreciate if you could open a bug in order to fix the case above; in that case, link to it from bug 369431 so that interested people can know what we're working on.

Comment on attachment 438550 [details] [diff] [review] Third patch (In reply to comment #7) > If required, I'll upload another patch with this mistake fixed, tomorrow. This patch looks fine, with the parentheses mistake fixed. You should upload a new patch, but there's no need to request review from me again. Since you have to create a new patch anyway, please fix these small cosmetic issues while you're at it (we don't fix those on old code, but generally we try to keep this style for new code in this area): >+ // If choice shouldn't be remembered and opening with default isn't >+ // OK (we don't let users open executable files for security concerns), >+ // then set up simple ui Dot at end of sentence (even if only one sentence or only one line). >+ // On other platforms, default is Ok if there is a default app >+ // and target file is not executable > // Note that nsIMIMEInfo providers need to ensure that this holds true > // on each platform. Reduce indentation of the entire block, and add dot at end of sentence. >- // Hide the default handler item too, in case the user picks a >- // custom handler at a later date which triggers the menulist to show. >+ // Hide the default handler item in the "handler" menulist As above.

Fixed mistakes.

Comment on attachment 439021 [details] [diff] [review] Fourth patch Thank you Manuel, I think this is polished and ready for check-in. We just need a second-pass review from Shawn, which is the real expert on this code and originally requested my attention, as maybe I'm still missing something.

Final review for should come in a week or two. With regard to removing the trailing whitespaces, we're opening another bug to do that plus re-indenting the code, but probably we'll do that after your patch is checked in.

Comment on attachment 439021 [details] [diff] [review] Fourth patch For review comments with context, please see http://reviews.visophyte.org/r/439021/ on file: toolkit/mozapps/downloads/nsHelperAppDlg.js line 485 > // If choice shouldn't be remembered and opening with default isn't > // OK (we don't let users open executable files for security concerns), > // then set up simple ui. nit: the phrasing is a little off. I'm suggesting this: If the choice should not be remembered and opening with the default is not OK (e.g. we do not let users open executable files for security reasons), then setup the simple UI. on file: toolkit/mozapps/downloads/nsHelperAppDlg.js line 684 > // On other platforms, default is Ok if there is a default app and > // target file is not executable. phrasing nit: lets try this: On non-windows platforms, the default is OK if there is a default application and the target file is not executable. on file: toolkit/mozapps/downloads/nsHelperAppDlg.js line 688 > return this.mLauncher.MIMEInfo.hasDefaultHandler && > !this.mLauncher.targetFileIsExecutable; nit: we should store this.mLauncher in a local variable since we access it twice here. on file: toolkit/mozapps/downloads/nsHelperAppDlg.js line 700 > // Show the "handler" menulist, since we have a (default) > // application now. nit: not sure why you have parens around default. on file: toolkit/mozapps/downloads/nsHelperAppDlg.js line 749 > // Show the "handler" menulist, since we have a (user-specified) > // application now. ditto here about parens around user-specified r=sdwilsh This should get a test though. You can probably add to http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/downloads/tests/chrome/test_unknownContentType_dialog_layout.xul to accomplish this.

(Paolo may be able to offer assistance on the test)

I'd be glad to help you with the test, which has to be updated before the patch can be checked in. I think we can start with testing that we don't cause regressions in the visibility of the controls under default conditions, which can easily done with the existing infrastructure in test_unknownContentType_dialog_layout.xul. While developing it should be sufficient to run this test on its own. For reference, I use this command line from the objdir: TEST_PATH=toolkit/mozapps/downloads/tests/chrome/test_unkownContentType_dialog_layout.xul make mochitest-chrome If you need information while updating the test, feel free to ask!

Manuel, I just wanted you to know that we're proceeding with bug 559833 (fixing indentation on the file), probably you'll have to build the fifth patch over the re-indented version.

So I'll wait for this bug to get fixed and will continue after that, trying if my patch still works and changing it where needed. I hoped to have this fix in SeaMonkey 2.1, as this really sucks on Linux to not see where files get opened with, but it seems to get pretty unlikely to get this into SM 2.1 as the whole thing seems to get more difficult as I expected at first, so waiting for the fix in bug 559833 is no problem. As soon as possible, I'll try to add the missing changes, but maybe it would be a good idea if someone else would do the "test part", as I would have to learn anything about them from scratch. I never used them, so far (disabled in .mozconfig to speed up build), so I don't know how they are used and how they work.

(In reply to comment #17) > As soon as possible, I'll try to add the missing changes, but maybe it would be > a good idea if someone else would do the "test part", as I would have to learn > anything about them from scratch. I never used them, so far (disabled in > .mozconfig to speed up build), so I don't know how they are used and how they > work. I can definitely help in updating this test, which probably requires just a small change, while you can begin learning about the testing framework by running the test on your computer. A good start would be to make a build with tests enabled, and skim through the documentation at https://developer.mozilla.org/en/Mozilla_automated_testing and https://developer.mozilla.org/en/Mochitest (for this test case). Running all the tests takes a lot of time, but while you develop or test a specific feature you can generally run the single test you are interested in. For this test, it should be as simple as executing the command-line in comment 15 (it is a single line, with a space between ".xul" and "make" but not between "chrome" and "/").