Closed Bug 806743 Opened 7 years ago Closed 7 years ago

See if toolkit/components/places/tests/unit/test_download_history.js makes sense in the new PB world

Categories

(Firefox :: Private Browsing, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 21

People

(Reporter: ehsan, Assigned: andreshm)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 6 obsolete files)

No description provided.
I'm modifying the test in bug 723005 to pass.
(In reply to Josh Matthews [:jdm] from comment #1)
> I'm modifying the test in bug 723005 to pass.

Hmm, if I'm reading attachment 676400 [details] [diff] [review] correctly, that just removes the test, right?
Depends on: 723005
Josh: ping?
Yes.
So should we close this bug then?
It would probably be good to write a browser-chrome test that initiated a download and checked that it didn't end up in the history and that we didn't receive a link visited notification for it.
Assignee: josh → nobody
Assignee: nobody → andres
Status: NEW → ASSIGNED
Attached patch Patch v1 (obsolete) — Splinter Review
Created a browser-chrome test as suggest in comment 6
Attachment #693586 - Flags: review?(josh)
Comment on attachment 693586 [details] [diff] [review]
Patch v1

Review of attachment 693586 [details] [diff] [review]:
-----------------------------------------------------------------

Excellent! Thanks!
Attachment #693586 - Flags: review?(josh) → review+
Comment on attachment 693586 [details] [diff] [review]
Patch v1

Although the commit message could be more informative :)
Attached patch Patch v2 (obsolete) — Splinter Review
Updated commit message
Attachment #693586 - Attachment is obsolete: true
Keywords: checkin-needed
Please run this through try first. This may break on mobile.
Keywords: checkin-needed
After looking at the logs. The test finished without errors, but after that there is an alert or the window loses focus, any idea what can it be?

INFO TEST-END | chrome://mochitests/content/browser/toolkit/components/downloads/test/browser/browser_download_history.js | finished in 10475ms
TEST-INFO | checking window state
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/components/downloads/test/browser/browser_download_history.js | Found an unexpected alert:alert at the end of test run

INFO TEST-END | chrome://mochitests/content/browser/toolkit/components/downloads/test/browser/browser_download_history.js | finished in 1003ms
TEST-INFO | checking window state
TEST-INFO | chrome://mochitests/content/browser/toolkit/components/downloads/test/browser/browser_download_history.js | must wait for focus
Hmm, I'm not sure about the alert problem. However, it occurs to me that this test isn't actually testing what I was thinking it was - instead of setting up the save and download process manually, I think it would be a more useful test to initiate a download from a link on in a private window, wait until it's complete and ensure that there was no link visited notification and no record in the history.
Attached patch Patch v3 (obsolete) — Splinter Review
Updated patch to initialize download from a link.
Attachment #693886 - Attachment is obsolete: true
Attachment #696075 - Flags: review?(josh)
Comment on attachment 696075 [details] [diff] [review]
Patch v3

Review of attachment 696075 [details] [diff] [review]:
-----------------------------------------------------------------

Nice. Sorry about the lag on my end.
Attachment #696075 - Flags: review?(josh) → review+
So is this ready to land?
The test finished without errors, but after the test finishes there is a timeout, any idea?

INFO TEST-END | chrome://mochitests/content/browser/toolkit/components/downloads/test/browser/browser_download_history.js | finished in 2157ms
TEST-INFO | checking window state
TEST-INFO | chrome://mochitests/content/browser/toolkit/components/downloads/test/browser/browser_download_history.js | must wait for focus
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/components/downloads/test/browser/browser_download_history.js | application timed out after 330 seconds with no output
Maybe something in

>   2.123 +      if (aDownload.targetFile.exists()) {
>   2.124 +        aDownload.targetFile.remove(false);
>   2.125 +      }

is throwing?
I tried using a try/catch block, but still it fails. But I'm sure now that is failing only on Windows. See: https://tbpl.mozilla.org/?tree=Try&rev=1d5c5394e902
I'll keep researching.
Any updates on this?
Over in bug 817477, I'm preparing to remove support for global private browsing altogether.  This is one of the last remaining pieces that I would like us to fix before that bug lands...
Blocks: 817477
> >   2.123 +      if (aDownload.targetFile.exists()) {
> >   2.124 +        aDownload.targetFile.remove(false);
> >   2.125 +      }

I have investigated this and this part of code doesn't throw any errors as Andres mentioned in comment 21.  I suspect that when the targetFile.remove(false) is executed,  another UI (alert) appears so the last browser window loses the focus on Win.  I have tried to invoke window.focus() in registerCleanupFunction() but without luck.

Any thoughts?
Comment on attachment 696075 [details] [diff] [review]
Patch v3

>+        // Wait for acceptDialog to execute.
>+        function accept() {
>+          if (dialog.acceptDialog) {
>+            dialog.acceptDialog();
>+            executeSoon(accept);
>+          }
>+        }
>+        executeSoon(accept);

This doesn't look right to me. Shouldn't the executeSoon be outside of the if block, to ensure we wait until the acceptDialog method is available? Otherwise we just attempt to run the code over and over.
(In reply to Josh Matthews [:jdm] from comment #25)
> Comment on attachment 696075 [details] [diff] [review]
> Patch v3
> 
> >+        // Wait for acceptDialog to execute.
> >+        function accept() {
> >+          if (dialog.acceptDialog) {
> >+            dialog.acceptDialog();
> >+            executeSoon(accept);
> >+          }
> >+        }
> >+        executeSoon(accept);
> 
> This doesn't look right to me. Shouldn't the executeSoon be outside of the
> if block, to ensure we wait until the acceptDialog method is available?
> Otherwise we just attempt to run the code over and over.

Let me rewrite that part. ;-)
Attached patch v4 (obsolete) — Splinter Review
I have updated the patch based on comment 25

However, still getting issues on windows.
https://tbpl.mozilla.org/?rev=86bb39abbaa8&tree=Try

Any suggestions how to fix that?
Attachment #696075 - Attachment is obsolete: true
Attachment #711376 - Flags: review?(josh)
Aha! It's the completed downloads alert - on OS X and Linux, there's a platform system that handles it. On Windows, we pop open http://mxr.mozilla.org/mozilla-central/source/toolkit/components/alerts/resources/content/alert.xul, which is of type alert:alert (http://mxr.mozilla.org/mozilla-central/source/toolkit/components/alerts/nsAlertsService.cpp#116, for reference). You should set browser.download.manager.showAlertOnComplete to false when running this test, and clear it when cleaning up.
Attached patch v5 (obsolete) — Splinter Review
Thanks for the tip.

Pushed to try and hope it passes Try.
https://tbpl.mozilla.org/?tree=Try&rev=3f0a313a3828
Attachment #711393 - Flags: review?(josh)
Attachment #711376 - Attachment is obsolete: true
Attachment #711376 - Flags: review?(josh)
Attachment #711393 - Flags: review?(josh) → review+
Attached patch v6 (obsolete) — Splinter Review
The last patch still fails on Windows.

I have updated it again and it passes try.
https://tbpl.mozilla.org/?tree=Try&rev=9ce3e88e795c
Attachment #711393 - Attachment is obsolete: true
Attachment #711922 - Flags: review?(josh)
Attachment #711922 - Flags: review?(josh) → review+
Attachment #711922 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/ddedb68b1cc6
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 21
You need to log in before you can comment on or make changes to this bug.