Blake Kaplan (:mrbkap) (inactive) Assignee
	Description • 7 years ago

The fix for bug 1301056 caused bug 1350058 and there weren't any automated tests to alert us ahead of time.

	Blake Kaplan (:mrbkap) (inactive) Assignee
	Comment 2 • 7 years ago

Felipe, this patch works fine for e10s but not for non-e10s. I don't know what aWindowContext is in non-e10s. I'm tempted to just disable the test for non-e10s.

	Blake Kaplan (:mrbkap) (inactive) Assignee
	Comment 5 • 7 years ago

The second version of this patch didn't restore the original helper app dialog after it was done and that was causing problems on try. The final version only runs on e10s and restores everything properly.

	:Felipe Gomes (needinfo for replies!)
	Comment 6 • 7 years ago
mozreview-review

Comment on attachment 8861700 [details]
Bug 1359651 - Add a mochitest for auto-closing behavior when dealing with an external file type.

https://reviewboard.mozilla.org/r/133678/#review136960

This looks fine to me, but I never worked on a download test (or it was a long time ago). Does the downloaded file need to be cleaned up by the test? Or does it go to a temporary location that gets automatically cleaned up later? You should probably check this with someone who knows before landing

	Blake Kaplan (:mrbkap) (inactive) Assignee
	Comment 7 • 7 years ago

Calling cancel on aLauncher will delete the temporary file. See, e.g. [1]. Poking around the mSaver code also seems to remove the temporary file if we haven't finished writing to it already.

[1] http://searchfox.org/mozilla-central/rev/876c7dd30586f9c6f9c99ef7444f2d73c7acfe7c/uriloader/exthandler/nsExternalHelperAppService.cpp#2486

	Pulsebot
	Comment 10 • 7 years ago

Pushed by mrbkap@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/64a46281b300
Add a mochitest for auto-closing behavior when dealing with an external file type. r=Felipe

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 11 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/64a46281b300

	Gerry Chang [:gchang]
	Comment 12 • 7 years ago

Hi :mrbkap, 
Per comment #21 in bug 1301056, can you create an uplift request for Beta54?

	Blake Kaplan (:mrbkap) (inactive) Assignee
	Comment 13 • 7 years ago

Comment on attachment 8861700 [details]
Bug 1359651 - Add a mochitest for auto-closing behavior when dealing with an external file type.

Approval Request Comment
[Feature/Bug causing the regression]: n/a (bug 1359651)
[User impact if declined]: none, test only
[Is this code covered by automated tests?]: this is a test
[Has the fix been verified in Nightly?]:
[Needs manual test from QE? If yes, steps to reproduce]: 
[List of other uplifts needed for the feature/fix]: n/a
[Is the change risky?]: no, test only.
[Why is the change risky/not risky?]: n/a
[String changes made/needed]: n/a

	Gerry Chang [:gchang]
	Comment 14 • 7 years ago

Comment on attachment 8861700 [details]
Bug 1359651 - Add a mochitest for auto-closing behavior when dealing with an external file type.

Add tests for bug 1301056. Beta54+. Should be in 54 beta 4.

	Ryan VanderMeulen [:RyanVM]
	Comment 15 • 7 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-beta/rev/7d4d2b3c8002