445005 - "Would you like to save..." label now shown on unknownContentType popup

Status: RESOLVED FIXED

(Toolkit :: Downloads API, defect)

Description

[Sylvain Pasche]

Attachment: Use booleans for the collapsed property

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.

Comment 1

[Shawn Wilsher :sdwilsh]

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

Comment 2

[Sylvain Pasche]

Attachment: the chrome test

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

Comment 3

[Shawn Wilsher :sdwilsh]

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.

Comment 4

[Sylvain Pasche]

Attachment: bugfix and chrome tests, v2

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 5

[Shawn Wilsher :sdwilsh]

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

Comment 6

[Sylvain Pasche]

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

Comment 7

[Sylvain Pasche]

Attachment: bugfix and chrome tests, v2.1

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

Comment 8

[Shawn Wilsher :sdwilsh]

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

Comment 9

[Samuel Sidler (old account; do not CC)]

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

Comment 10

[Samuel Sidler (old account; do not CC)]

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

Comment 11

[Shawn Wilsher :sdwilsh]

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.

Comment 12

[Al Billings [:abillings - ex-MoCo]]

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.