Closed Bug 1152842 Opened 7 years ago Closed 7 years ago

Remove legacy Download Manager support in test_bug383369.html

Categories

(Toolkit :: Downloads API, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox40 --- affected
firefox41 --- fixed

People

(Reporter: Paolo, Assigned: cedric.raudin, Mentored)

References

Details

(Whiteboard: [good first bug][lang=js])

Attachments

(1 file, 1 obsolete file)

Instead of using the legacy Download Manager, test_bug383369.html should be disabled for applications that don't support the new Downloads API.

This involves removing the code that sets useJSTransfer, assuming it is always true, and removing the code called when it is false:

http://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/tests/mochitest/mixedcontent/test_bug383369.html?force=1#47
OS: Windows 7 → All
Hardware: x86_64 → All
Whiteboard: [good first bug] → [good first bug][lang=js]
Hi, I would like to work on this. I am new to open source contributions so it will be fine if someone could guide me through. I have already build Firefox
Thank you! If you're familiar with basic JavaScript and you've read the documentation on how to create and submit a patch, you're ready to start :-)

If you run this command from your build terminal:

  ./mach test security/manager/ssl/tests/mochitest/mixedcontent/test_bug383369.html

You should see that the two tests defined there passed.

Then look at the portion of the code referenced by the link in the description of this bug in comment 0. What it does it to read the property:

  Services.downloads.activeDownloadCount

In modern versions of Firefox, we don't use the internal "Services.downloads" object anymore, and reading this property is always expected to throw an exception. Previously, we also had to support "Services.downloads".

Following this reasoning, you can remove all the code that would run only if the property didn't throw.

You can then check that the test still passes, and create a patch for the change. Let me know if you have any question about the code or if you need any pointers to more documentation!
Attached patch bug-1152852-fix.patch (obsolete) — Splinter Review
Attachment #8597617 - Flags: review?(paolo.mozmail)
Comment on attachment 8597617 [details] [diff] [review]
bug-1152852-fix.patch

Hello, sorry, it took longer than usual to get to this review. The patch looks fine, though all the code that sets up the old-style download observers can be removed as well (notice the "return" statement inside the conditional).

Can you please upload a new patch with the extra code removed? I'll then run it through our automated testing infrastructure and integrate it with our main repository if all tests pass.
Attachment #8597617 - Flags: review?(paolo.mozmail) → feedback+
Attachment #8604400 - Flags: review?(paolo.mozmail)
Comment on attachment 8604400 [details] [diff] [review]
bug-1152842-fix.patch

Looks good! I've started automated tests for the changes:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=97c3a9457ed1

We now just have to wait and see whether the relevant tests are successful. Some unrelated tests may fail, but they'll have bugs on file for them.
Attachment #8604400 - Flags: review?(paolo.mozmail) → review+
Attachment #8597617 - Attachment is obsolete: true
The tests look good to me, so I've set the "checkin-needed" keyword on the bug and marked the old patch as "obsolete". The next time, if you're confident the test results are good, you can also do this yourself to accelerate the process!
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/4a382e5000bc
Assignee: nobody → cedric.raudin
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Thanks Cedric for the bug fix!
You need to log in before you can comment on or make changes to this bug.