1253585 - Remove legacy downloads test code and re-enable test_unknownContentType_dialog_layout.xul

Status: RESOLVED FIXED

(Toolkit :: Downloads API, defect, P1)

Description

[:Paolo Amadini]

Except for test_unknownContentType_dialog_layout.xul, that is disabled unnecessarily, we should just remove other test code that runs only in legacy configurations where MOZ_JSDOWNLOADS is not defined.

Comment 1

[:Paolo Amadini]

Attachment: MozReview Request: Bug 1253585 - Remove legacy downloads test code and re-enable test_unknownContentType_dialog_layout.xul. r=mak

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

Comment 2

[:Paolo Amadini]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9d968cb2bdd3

Comment 3

[Marco Bonardo [:mak]]

Comment on attachment 8726726 [details]
MozReview Request: Bug 1253585 - Remove legacy downloads test code and re-enable test_unknownContentType_dialog_layout.xul. r=mak

https://reviewboard.mozilla.org/r/38199/#review35299

LGTM.
My only "complain" is that among these tests there may be some that could be useful if converted to cover the new API... but not a good enough reason to keep them around.

::: toolkit/components/downloads/test/unit/head_download_manager.js
(Diff revision 1)
> -Services.prefs.setBoolPref("browser.download.manager.showAlertOnComplete", false);

do we have a bug filed to remove all the remaining references to browser.download.manager.showAlertOnComplete and in general old DM prefs?

Comment 4

[:Paolo Amadini]

(In reply to Marco Bonardo [::mak] from comment #3)
> My only "complain" is that among these tests there may be some that could be
> useful if converted to cover the new API... but not a good enough reason to
> keep them around.

I agree. I think most tests are fully covered under the new API, that has also many other tests that weren't present originally. If there are any remaining cases that are not covered, they are probably edge cases, and we can easily add back the tests when touching the related code.

> do we have a bug filed to remove all the remaining references to
> browser.download.manager.showAlertOnComplete and in general old DM prefs?

No, good point, I just filed bug 1254558.

Comment 5

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/51f170c854f0

Comment 6

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

https://hg.mozilla.org/mozilla-central/rev/51f170c854f0

Comment 7

[Paul Silaghi, QA [:pauly]]

Is there something QA can do here? If not, please set the [qe-verify-] flag. 
Thanks.

Comment 8

[:Paolo Amadini]

No, this was meant to be "qe-verify-". Thanks!