Closed Bug 464800 Opened 12 years ago Closed 12 years ago

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

Categories

(Firefox :: Private Browsing, defect, P2)

defect

Tracking

()

VERIFIED FIXED
Firefox 3.6a1

People

(Reporter: marcia, Assigned: ehsan)

References

Details

(Keywords: polish, verified1.9.1)

Attachments

(2 files, 1 obsolete file)

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.
Flags: in-litmus?
Not sure whether this belongs in PB or DM but this still occurs using the latest nightly on trunk.
Component: General → Private Browsing
QA Contact: general → private.browsing
Same happens on OS X.
OS: Windows Vista → All
Hardware: x86 → All
Assignee: nobody → ehsan.akhgari
Status: NEW → ASSIGNED
Whiteboard: [PB-P3]
not sure whether this should be wanted or blocking, but we should fix it.  Should be trivial...
Flags: wanted-firefox3.1?
Flags: blocking-firefox3.1?
Priority: -- → P2
Target Milestone: --- → Firefox 3.1b3
Whiteboard: [PB-P3] → [PB-P2]
Agree that this is ugly polish that needs to be fixed, will obviously take the trivial patch, but don't think it blocks.
Flags: wanted-firefox3.1?
Flags: wanted-firefox3.1+
Flags: blocking-firefox3.1?
Flags: blocking-firefox3.1-
Flags: wanted-firefox3.1?
Flags: wanted-firefox3.1+
Flags: blocking-firefox3.1?
Flags: blocking-firefox3.1-
Keywords: polish
Attached patch Patch (v1) (obsolete) — Splinter Review
Simple patch plus a unit test.
Attachment #366179 - Flags: review?(sdwilsh)
Whiteboard: [PB-P2] → [has patch][needs review sdwilsh]
I accidentally cleared the wanted flag.  :(

Beltzner, can you please re-set it?  Thanks!
Flags: in-testsuite?
Flags: in-litmus?
Flags: blocking-firefox3.1?
Flags: wanted-firefox3.1? → wanted-firefox3.1+
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
Attachment #366179 - Flags: review?(sdwilsh) → review+
Whiteboard: [has patch][needs review sdwilsh] → [has patch]
(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.
http://hg.mozilla.org/mozilla-central/rev/69322c1764ff
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Flags: in-testsuite? → in-testsuite+
Resolution: --- → FIXED
Whiteboard: [has patch]
Target Milestone: Firefox 3.1b3 → Firefox 3.2a1
Comment on attachment 366179 [details] [diff] [review]
Patch (v1)

Simple polish fix with an automated test; requesting approval to land on 1.9.1.
Attachment #366179 - Flags: approval1.9.1?
Backed out due to Mac test failures:

<http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1236848594.1236851651.20213.gz>
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #366179 - Flags: approval1.9.1?
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?
Whiteboard: [PB-P2]
Nope - not so sure why they'd be there.
(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
Attached patch Patch (v2)Splinter Review
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.
Attachment #366179 - Attachment is obsolete: true
Attachment #367361 - Flags: review?(sdwilsh)
Whiteboard: [PB-P2]
Attachment #367361 - Flags: review?(sdwilsh) → review+
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
http://hg.mozilla.org/mozilla-central/rev/9f4b54ad549d
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
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.
Attachment #367361 - Flags: approval1.9.1?
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
Status: RESOLVED → VERIFIED
Depends on: 486655
Whiteboard: [bug 486655 should land with this patch on 1.9.1]
Comment on attachment 367361 [details] [diff] [review]
Patch (v2)

a191=beltzner
Attachment #367361 - Flags: approval1.9.1? → approval1.9.1+
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/4bb10b07f850
Keywords: fixed1.9.1
Whiteboard: [bug 486655 should land with this patch on 1.9.1]
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
You need to log in before you can comment on or make changes to this bug.