1359651 - Add a mochitest for bug 1301056

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Blake Kaplan (:mrbkap) (inactive)]

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

Comment 1

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Bug 1359651 - Add a mochitest for auto-closing behavior when dealing with an external file type.

Review commit: https://reviewboard.mozilla.org/r/133678/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/133678/

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

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.

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/133678/diff/1-2/

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/133678/diff/2-3/

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

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.

Comment 6

[:Felipe Gomes (needinfo for replies!)]

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

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

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

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/133678/diff/3-4/

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/133678/diff/4-5/

Comment 10

[Pulsebot]

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

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 12

[Gerry Chang [:gchang]]

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

Comment 13

[Blake Kaplan (:mrbkap) (inactive)]

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

Comment 14

[Gerry Chang [:gchang]]

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.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

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