464800 - Download manager title window is not cleared when switching to Private Browsing

Status: VERIFIED FIXED

(Firefox :: Private Browsing, defect, P2)

Description

[Marcia Knous [:marcia]]

Attachment: Screenshot of Download Window after switching to PB

Seen while running Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1b2pre) Gecko/20081113 Minefield/3.1b2pre, also seen on Windows XP

STR:
1. Download a file in regular browsing mode and then pause it.
2. Switch to PB mode.
3. Observe the attached screenshot

AFAICS the bug does not occur on Mac.

Comment 1

[Marcia Knous [:marcia]]

Not sure whether this belongs in PB or DM but this still occurs using the latest nightly on trunk.

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Same happens on OS X.

Comment 3

[Mike Connor [:mconnor]]

not sure whether this should be wanted or blocking, but we should fix it.  Should be trivial...

Comment 4

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

Agree that this is ugly polish that needs to be fixed, will obviously take the trivial patch, but don't think it blocks.

Comment 5

[(no longer active)]

Attachment: Patch (v1)

Simple patch plus a unit test.

Comment 6

[(no longer active)]

I accidentally cleared the wanted flag.  :(

Beltzner, can you please re-set it?  Thanks!

Comment 7

[Shawn Wilsher :sdwilsh]

Comment on attachment 366179 [details] [diff] [review]
Patch (v1)

>       case "private-browsing":
>         if (aData == "enter" || aData == "exit") {
>+          // Reset the title.  It will be updated if needed by the progress listener.
>+          document.title = document.documentElement.getAttribute("statictitle");
>+
More detailed comment as to why we are clearing the title please

>+++ b/toolkit/mozapps/downloads/tests/chrome/test_bug_464800.xul
can we get a more descriptive name please?  I hate tests with just bug numbers in them as you have no idea what it does until you open the test.

>+<window title="Download Manager Test"
more useful title here would be nice too

>+  let prefBranch = Cc["@mozilla.org/preferences-service;1"].
>+                   getService(Ci.nsIPrefBranch);
>+  prefBranch.setBoolPref("browser.privatebrowsing.keep_current_session", true);
Add comment about setting up our state please

>+  // Close the UI if necessary
>+  let wm = Cc["@mozilla.org/appshell/window-mediator;1"].
>+           getService(Ci.nsIWindowMediator);
>+  let win = wm.getMostRecentWindow("Download:Manager");
>+  if (win) win.close();
>+
>+  let obs = Cc["@mozilla.org/observer-service;1"].
>+            getService(Ci.nsIObserverService);
>+  const DLMGR_UI_DONE = "download-manager-ui-done";
>+
>+  let testObs = {
>+    observe: function(aSubject, aTopic, aData) {
>+      if (aTopic != DLMGR_UI_DONE)
>+        return;
I really need to abstract this stuff into some utility function :/

>+      let win = aSubject.QueryInterface(Ci.nsIDOMWindow);
>+      is(win.document.title, "Downloads",
>+        "The downloads window title is correct outside of the private browsing mode");
This will fail in any locale that isn't English, right?  If this is available in a properties file, you should pull the string from there.

>+      is(win.document.title, "Downloads",
>+        "The downloads window title is correct inside the private browsing mode");
ditto

>+      is(win.document.title, "Downloads",
>+        "The downloads window title is correct after leaving the private browsing mode");
and ditto

>+      prefBranch.clearUserPref("browser.privatebrowsing.keep_current_session");
excellent - I had a comment about making sure you do this, but you already did! :)

r=sdwilsh with these changes

Comment 8

[(no longer active)]

(In reply to comment #7)
> >+      let win = aSubject.QueryInterface(Ci.nsIDOMWindow);
> >+      is(win.document.title, "Downloads",
> >+        "The downloads window title is correct outside of the private browsing mode");
> This will fail in any locale that isn't English, right?  If this is available
> in a properties file, you should pull the string from there.

Ah, yes, good point.  That string is in a dtd file, but it's also available as the statictitle attribute on the document element, so I guess I'll just use it instead.

I'll address the rest of your comments upon landing as well.

Comment 9

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/69322c1764ff

Comment 10

[(no longer active)]

Comment on attachment 366179 [details] [diff] [review]
Patch (v1)

Simple polish fix with an automated test; requesting approval to land on 1.9.1.

Comment 11

[(no longer active)]

Backed out due to Mac test failures:

<http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1236848594.1236851651.20213.gz>

Comment 12

[(no longer active)]

Shawn, the failure seems to be caused by the fact that two downloads have been inside the download manager after leaving the private browsing mode.  The odd thing is that no downloads were there before starting the private browsing mode!  Do you have any idea where such zombie downloads could come from?

Comment 13

[Shawn Wilsher :sdwilsh]

Nope - not so sure why they'd be there.

Comment 14

[Bartłomiej Brzozowiec (BartZilla)]

(In reply to comment #9)
> http://hg.mozilla.org/mozilla-central/rev/69322c1764ff

Typo in comment in downloads.js ("switchiung"):
       9 +          // active downloads, if any, after switchiung the private browsing

Comment 15

[(no longer active)]

Attachment: Patch (v2)

OK, it seems like each of the download manager chrome tests is responsible for cleaning up after its predecessors.  Renaming the test file changed the execution order, and hence the test failed under the new order because a previous test was leaving downloads in the moz_downloads table.

This patch fixes this issue, and the unit test passes now.

Comment 16

[Shawn Wilsher :sdwilsh]

Comment on attachment 367361 [details] [diff] [review]
Patch (v2)

In the future, please just upload a patch that contains the small change needed to the unit tests to make this easier to review.

r=sdwilsh

Comment 17

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/9f4b54ad549d

Comment 18

[(no longer active)]

Comment on attachment 367361 [details] [diff] [review]
Patch (v2)

This is a fix to a polish problem which comes with a unit test.  Nominating to get approval to land on 1.9.1.

Comment 19

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Verified fixed on Windows and OS X with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090322 Minefield/3.6a1pre ID:20090322035551

Comment 20

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

Comment on attachment 367361 [details] [diff] [review]
Patch (v2)

a191=beltzner

Comment 21

[(no longer active)]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/4bb10b07f850

Comment 22

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Verified on 1.9.1 with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b4pre) Gecko/20090410 Shiretoko/3.5b4pre ID:20090410035055