Closed Bug 1253585 Opened 4 years ago Closed 4 years ago

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

Categories

(Toolkit :: Downloads API, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla48
Iteration:
48.1 - Mar 21
Tracking Status
firefox47 --- wontfix
firefox48 --- fixed

People

(Reporter: Paolo, Assigned: Paolo)

References

(Blocks 1 open bug)

Details

(Whiteboard: [fxprivacy])

Attachments

(1 file)

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.
Depends on: 483200
Depends on: 630567
Depends on: 676989
Blocks: 1216897
Iteration: --- → 47.3 - Mar 7
Flags: qe-verify?
Priority: -- → P1
Whiteboard: [fxprivacy]
Blocks: 1068656
No longer blocks: 1216897
Attachment #8726726 - Flags: review?(mak77) → review+
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?
(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.
Iteration: 47.3 - Mar 7 → 48.1 - Mar 21
Flags: qe-verify? → qe-verify+
QA Contact: paul.silaghi
https://hg.mozilla.org/mozilla-central/rev/51f170c854f0
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Is there something QA can do here? If not, please set the [qe-verify-] flag. 
Thanks.
Flags: needinfo?(paolo.mozmail)
No, this was meant to be "qe-verify-". Thanks!
Flags: qe-verify-
Flags: qe-verify+
Flags: needinfo?(paolo.mozmail)
You need to log in before you can comment on or make changes to this bug.