1082714 - Rewrite download's device storage notification in JS

Status: RESOLVED WONTFIX

(Toolkit :: Downloads API, defect)

Description

[Dave Hylands [:dhylands]]

Bug 1072535 added some notification code into toolkit/components/jsdownloads/src/DownloadPlatform.cpp

This is a followup bug to rewrite that code in JS and remove it from the DownloadPlatform::DownloadDone function.

Comment 1

[:Paolo Amadini]

Thanks! Looks like a good first bug.

Comment 2

[Anirudh GP(:anirudhgp)]

Hey id like to work on this bug. How do i start?

Comment 3

[Dave Hylands [:dhylands]]

In order to test this, you'll need to be able to build FirefoxOS.

Do you have a supported device? If not, then I think you can use the emulator.

https://developer.mozilla.org/en-US/Firefox_OS/Firefox_OS_build_prerequisites

I use linux for all of my builds. Apparently, you can build on a Mac, but I don't have one, so I can't help on that front.

My build tree for my flame occupies about 55Gb of drive space, so you'll need lots of free drive space.

Once you've got a build, then you look at bug 1072535 comment 23. You'll need to undo the changes to the gecko/toolkit/components/jsdownloads/src/DownloadPlatform.cpp and put the js implementation in gecko/toolkit/components/jsdownload/src/DownloadIntegration.jsm

Comment 4

[Anirudh GP(:anirudhgp)]

I dont have a supported device. I'll build it on my linux pc and run it through an emulator.

Comment 5

[Dave Hylands [:dhylands]]

adb works on the emulator, so you can use ./config.sh emulator-kk and get kit kat based emulator image.

You can run the emulator using ./run-emulator.sh

Comment 6

[Anirudh GP(:anirudhgp)]

(In reply to Dave Hylands [:dhylands] from comment #5)
> adb works on the emulator, so you can use ./config.sh emulator-kk and get
> kit kat based emulator image.
> 
> You can run the emulator using ./run-emulator.sh

I'm building on linux. It's takiing a while so ill get on with this as soon as my build finishes.

Comment 7

[Anirudh GP(:anirudhgp)]

Hey! Can i still take up this bug?

Comment 8

[Dave Hylands [:dhylands]]

(In reply to Anirudh GP(:anirudhgp) from comment #7)
> Hey! Can i still take up this bug?

Yep - It's still open, and you're assigned as the person fixing it :)

Comment 9

[Anirudh GP(:anirudhgp)]

To which file do i add the JS implementation of the code?

Comment 10

[Dave Hylands [:dhylands]]

I'll needinfo paolo since he can probably provide better insight than I can.

Comment 11

[:Paolo Amadini]

Hi Anirudh! The code to migrate to JavaScript is located here:

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/jsdownloads/src/DownloadPlatform.cpp#133

I think the first thing I would do is to verify that the code is exercised. You can run the tests using the command:

./mach test toolkit/components/jsdownloads/test/unit/test_DownloadCore.js

Then I would remove the entire "if" block, rebuild, and check that the test fails or times out.

The JavaScript equivalent would be located just before the downloadDone call from JavaScript. See the code here:

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/jsdownloads/src/DownloadIntegration.jsm#642

You need a new try-catch block like the one that is already there, and you can use the internal function Services.prefs.getBoolPref (nsIPrefBranch interface) and Services.obs (nsIObserverService interface) to do the work you need.

Let me or Dave know if you have questions about any of the above!

Comment 12

[Anirudh GP(:anirudhgp)]

Ok so I did some research on MDN and I deduced the following. Please let me know if there's a mistake.

nsCOMPtr<nsIObserverService> obs = mozilla::services::GetObserverService(); should be changed to 
Services.obs.addObserver(obs,"topic", false); but I'm not sure what to write the topic as.

nsCOMPtr<nsISupportsString> pathString= do_CreateInstance(NS_SUPPORTS_STRING_CONTRACTID); should be changed to 
var pathString =Components.classes[NS_SUPPORTS_STRING_CONTRACTID]
                    .createInstance(Components.interfaces.nsISupportsString);

if (mozilla::Preferences::GetBool("device.storage.enabled", true)) should be changed to
if (Services.prefs.getBoolPref("device.storage.enabled"))

(void)obs->NotifyObservers(pathString, "download-watcher-notify",MOZ_UTF16("modified")); should be changed to
Components.classes["@mozilla.org/observer-service;1"]
          .getService(obs)
          .notifyObservers(pathString, "myTopicID",  MOZ_UTF16("modified")); again, not sure what "mytopic" should be 

I dont know if this is right.

Comment 13

[:Paolo Amadini]

What I recommend doing, rather than trying to convert each call individually, is to see examples of how the calls you need are used in other Mozilla code, for example for Services.obs.notifyObservers:

https://dxr.mozilla.org/mozilla-central/search?q=Services.obs.notifyObservers&case=false

Then think about the purpose of the original code, and what you need to do to achieve the same in the JavaScript module using the techniques you see used in the tree.

In the meantime, did you succeed in running the tests with "mach"?

Comment 14

[jassmultani2]

Hello Sir, I would really like to work on this bug, i would need some basic tips on how to start !
I would be really glad if you can help me with the same.

Comment 15

[Dave Hylands [:dhylands]]

Device Storage is being removed from the codebase, so this partcular bug isn't relevant anymore.