Closed Bug 1114593 Opened 6 years ago Closed 6 years ago

Replace defaultDownloadsDirectory calls in DirectoryProvider.js

Categories

(Firefox for Android :: Download Manager, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 37
Tracking Status
fennec 37+ ---

People

(Reporter: Paolo, Assigned: Margaret)

References

Details

(Keywords: addon-compat)

Attachments

(1 file, 1 obsolete file)

Android's DirectoryProvider.js uses nsIDownloadManager.defaultDownloadsDirectory to reply to "DfltDwnld" and "UpdRootD" queries:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/components/DirectoryProvider.js#65

Unfortunately, we cannot just replace defaultDownloadsDirectory with Downloads.getSystemDownloadsDirectory because the latter is asynchronous.

Actually, the "DfltDwnld" entry can just be removed - it's not used by the Download Manager on Android, and add-ons can now use the asynchronous function directly.

With regard to "UpdRootD", I'm not sure, but the simplest thing would be to short-circuit it to what getSystemDownloadsDirectory would have returned, that is actually just env.get("DOWNLOADS_DIRECTORY").
Summary: Replace defaultDownloadsDirectory with Downloads.getSystemDownloadsDirectory in DirectoryProvider.js → Replace defaultDownloadsDirectory calls in DirectoryProvider.js
Assignee: nobody → margaret.leibovic
tracking-fennec: --- → ?
Attachment #8540298 - Flags: review?(paolo.mozmail)
Attachment #8540298 - Flags: review?(mark.finkle)
/r/1641 - Bug 1114593 - Replace defaultDownloadsDirectory calls in DirectoryProvider.js

Pull down this commit:

hg pull review -r 37fbf34713facace6b61c08643821601605559a1
snorp, is there a way I can test the updater locally to make sure this directory provider change didn't bust anything?
Flags: needinfo?(snorp)
https://reviewboard.mozilla.org/r/1639/#review1043

::: mobile/android/components/DirectoryProvider.js
(Diff revision 1)
> -    } else if (prop == DOWNLOAD_DIR) {
> +      // implementation would have returned.

I think you need to keep the DOWNLOAD_DIR test too.
/r/1641 - Bug 1114593 - Replace defaultDownloadsDirectory calls in DirectoryProvider.js

Pull down this commit:

hg pull review -r 8f481cff876bdb9c7053f3f2b13861929096e0d3
Attachment #8540298 - Flags: review?(mark.finkle) → review+
https://reviewboard.mozilla.org/r/1639/#review1065

::: mobile/android/components/DirectoryProvider.js
(Diff revision 2)
>      } else if (prop == XRE_UPDATE_ROOT_DIR) {
>        let env = Cc["@mozilla.org/process/environment;1"].getService(Ci.nsIEnvironment);
>        if (env.exists(ENVVAR_UPDATE_DIR)) {
>          let path = env.get(ENVVAR_UPDATE_DIR);
>          if (path) {
>            let localFile = Cc["@mozilla.org/file/local;1"].createInstance(Ci.nsILocalFile);
>            localFile.initWithPath(path);
>            return localFile;
>          }
>        }
> -      let dm = Cc["@mozilla.org/download-manager;1"].getService(Ci.nsIDownloadManager);
> +      return env.get("DOWNLOADS_DIRECTORY");

Should cast to nsIFile. FileUtils is alrady imported, you can also replace the old createInstance with "new FileUtils.File()" while you're touching this code.

::: mobile/android/components/DirectoryProvider.js
(Diff revision 2)
>      } else if (prop == DOWNLOAD_DIR) {
> -      let dm = Cc["@mozilla.org/download-manager;1"].getService(Ci.nsIDownloadManager);
> -      return dm.defaultDownloadsDirectory;
> +      // Downloads.getSystemDownloadsDirectory is asynchronous, but getFile is
> +      // synchronous, so just return what the getSystemDownloadsDirectory
> +      // implementation would have returned.
> +      let env = Cc["@mozilla.org/process/environment;1"].getService(Ci.nsIEnvironment);
> +      return env.get("DOWNLOADS_DIRECTORY");

This can only be called by add-ons, it's never called by download code. It can be removed or you can add a comment to that effect.
Attachment #8540298 - Flags: review?(paolo.mozmail)
(In reply to :Paolo Amadini from comment #7)
> https://reviewboard.mozilla.org/r/1639/#review1065
> 
> ::: mobile/android/components/DirectoryProvider.js
> (Diff revision 2)
> >      } else if (prop == XRE_UPDATE_ROOT_DIR) {
> >        let env = Cc["@mozilla.org/process/environment;1"].getService(Ci.nsIEnvironment);
> >        if (env.exists(ENVVAR_UPDATE_DIR)) {
> >          let path = env.get(ENVVAR_UPDATE_DIR);
> >          if (path) {
> >            let localFile = Cc["@mozilla.org/file/local;1"].createInstance(Ci.nsILocalFile);
> >            localFile.initWithPath(path);
> >            return localFile;
> >          }
> >        }
> > -      let dm = Cc["@mozilla.org/download-manager;1"].getService(Ci.nsIDownloadManager);
> > +      return env.get("DOWNLOADS_DIRECTORY");
> 
> Should cast to nsIFile. FileUtils is alrady imported, you can also replace
> the old createInstance with "new FileUtils.File()" while you're touching
> this code.

Will do.

> ::: mobile/android/components/DirectoryProvider.js
> (Diff revision 2)
> >      } else if (prop == DOWNLOAD_DIR) {
> > -      let dm = Cc["@mozilla.org/download-manager;1"].getService(Ci.nsIDownloadManager);
> > -      return dm.defaultDownloadsDirectory;
> > +      // Downloads.getSystemDownloadsDirectory is asynchronous, but getFile is
> > +      // synchronous, so just return what the getSystemDownloadsDirectory
> > +      // implementation would have returned.
> > +      let env = Cc["@mozilla.org/process/environment;1"].getService(Ci.nsIEnvironment);
> > +      return env.get("DOWNLOADS_DIRECTORY");
> 
> This can only be called by add-ons, it's never called by download code. It
> can be removed or you can add a comment to that effect.

mfinkle pointed out that it's used by about:memory:

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/aboutmemory/content/aboutMemory.js#2039
Blocks: 1115006
(In reply to :Margaret Leibovic from comment #8)
> mfinkle pointed out that it's used by about:memory:
> 
> http://mxr.mozilla.org/mozilla-central/source/toolkit/components/aboutmemory/
> content/aboutMemory.js#2039

Ah, missed that use. Filed bug 1115006 to convert about:memory to Downloads.jsm, assuming this is the only use left.
(In reply to :Margaret Leibovic from comment #3)
> snorp, is there a way I can test the updater locally to make sure this
> directory provider change didn't bust anything?

It's a pain to test the updater, but this change shouldn't affect it. We don't use any Gecko stuff for the download directory[0].

[0] http://dxr.mozilla.org/mozilla-central/source/mobile/android/base/updater/UpdateService.java?from=UpdateService.java&case=true#449
Flags: needinfo?(snorp)
https://hg.mozilla.org/mozilla-central/rev/f52833a4cf74
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 37
tracking-fennec: ? → 37+
Attachment #8540298 - Attachment is obsolete: true
Attachment #8618959 - Flags: review+
You need to log in before you can comment on or make changes to this bug.