If you try to open http://biesi.damowmow.com/foo.pif, the simple version of the unknownContentType popup is shown (the one with just "Cancel" or "Save" buttons). The "Would you like to save this file?" label is not shown because of a collapsed property set to the string "false" instead of the boolean false. The label is shown with Firefox 2 and this regressed since at least 2006-07-03 in the 1.9 branch.

Can we get a test please? This would likely be a chrome test.

This is a chrome test (I've only tested on Linux)

Comment on attachment 329345 [details] [diff] [review] the chrome test >--- a/toolkit/mozapps/downloads/tests/chrome/Makefile.in >+++ b/toolkit/mozapps/downloads/tests/chrome/Makefile.in >+ test_bug_445005.xul \ >+ bug445005_data.txt \ >+ bug445005_data.txt^headers^ \ >+ bug445005_data.pif \ >+ bug445005_data.pif^headers^ \ Please keep the alphabetical ordering here in this list please >+++ b/toolkit/mozapps/downloads/tests/chrome/test_bug_445005.xul nit: license block please >+<?xml version="1.0"?> >+<?xml-stylesheet href="chrome://global/skin" type="text/css"?> >+<?xml-stylesheet >+ href="chrome://mochikit/content/tests/SimpleTest/test.css" type="text/css"?> >+<!-- >+/** >+ * Test for bug 445005. The unknownContentType popup can have two different >+ * layouts depending on whether a helper application can be selected or not. >+ * This tests that both layouts have correct collapsed elements. given this description, it seems like more of a generic test. I'd prefer to then have the name of the test be more descriptive. How about "test_unkownContentType_dialog_layout.xul"? >+ <script type="application/javascript" >+ src="chrome://mochikit/content/MochiKit/packed.js"/> >+ <script type="application/javascript" >+ src="chrome://mochikit/content/tests/SimpleTest/SimpleTest.js"/> Grab utils.js that's available (you won't need to declare the constants, and you'll need it for another comment later on) nit: I generally say that it's not worthwhile to indent the JS in these files sine it reduces the amount of space we have on each line for code/comments >+ let testIndex = 0; >+ let tests = [{ nit: opening brace on new line please (and indent) >+ // This URL will trigger the simple UI, where only the Save an Cancel buttons are available >+ url: "http://localhost:8888/chrome/toolkit/mozapps/downloads/tests/chrome/bug445005_data.pif", >+ elements: { >+ basicBox: { collapsed: false }, >+ normalBox: { collapsed: true } >+ } >+ }, { nit: opening brace on new line please (and indent) >+ // This URL will trigger the full UI >+ url: "http://localhost:8888/chrome/toolkit/mozapps/downloads/tests/chrome/bug445005_data.txt", >+ elements: { >+ basicBox: { collapsed: true }, >+ normalBox: { collapsed: false } >+ } >+ }]; nit: closing bracket on new line please >+ let windowMediatorListener = { note: I've found nsIWindowWatcher's registerNotifcation to be much easier to work with (http://www.xulplanet.com/references/xpcomref/ifaces/nsIWindowWatcher.html#method_registerNotification) >+ let win = aXulWindow.docShell >+ .QueryInterface(Ci.nsIInterfaceRequestor) >+ .getInterface(Ci.nsIDOMWindow); >+ win.addEventListener("load", function(event) { >+ win.removeEventListener("load", arguments.callee, false); For clarity, I'd love it if you explicitly called out the function. >+ // Use a timeout to let the dialog initialize >+ setTimeout(function() { use executeSoon in utils.js please Sorry I'm so picky. Looks good otherwise, and I'm *really* glad to get test coverage on this.

This new version contains both the test and the original fix for easier management. (In reply to comment #3) > >+<!-- > >+/** > >+ * Test for bug 445005. The unknownContentType popup can have two different > >+ * layouts depending on whether a helper application can be selected or not. > >+ * This tests that both layouts have correct collapsed elements. > given this description, it seems like more of a generic test. I'd prefer to > then have the name of the test be more descriptive. How about > "test_unkownContentType_dialog_layout.xul"? This name looks fine, I've renamed the file. > nit: I generally say that it's not worthwhile to indent the JS in these files > sine it > reduces the amount of space we have on each line for code/comments Agreed, I've looked at another test in this directory and applied the same style. > >+ let windowMediatorListener = { > note: I've found nsIWindowWatcher's registerNotifcation to be much easier to > work with > (http://www.xulplanet.com/references/xpcomref/ifaces/nsIWindowWatcher.html#method_registerNotification) Yes, that simplifies things quite a bit. I hope to have addressed other comments, thanks for the review.

Comment on attachment 329541 [details] [diff] [review] bugfix and chrome tests, v2 >+ if (win.document.documentURI != >+ "chrome://mozapps/content/downloads/unknownContentType.xul") might want to make the url string a const so it stays on one line r=sdwilsh

I also forgot to rename the content of the unknownContentType_dialog_layout_data.{txt,pif} files, I'll update in a few hours.

Updated version. I guess it's ok for checkin?

Pushed to mozilla-central: http://hg.mozilla.org/mozilla-central/index.cgi/rev/2935954b1bb2

Comment on attachment 329631 [details] [diff] [review] bugfix and chrome tests, v2.1 Approved for 1.9.0.2. Please land in CVS. a=ss

Shawn, do you have time to check this in today?

Checking in toolkit/mozapps/downloads/src/nsHelperAppDlg.js.in; new revision: 1.68; previous revision: 1.67 Can't land the tests, because that would require more backporting of frameworks that I don't really have time for.

Verified fixed in 1.9.0.2 with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.2) Gecko/2008090212 Firefox/3.0.2.