Closed Bug 1114593 Opened 10 years ago Closed 10 years ago

Replace defaultDownloadsDirectory calls in DirectoryProvider.js

Categories

(Firefox for Android Graveyard :: Download Manager, defect)

defect
Not set
normal

Tracking

(fennec37+)

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)
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 37
tracking-fennec: ? → 37+
Attachment #8540298 - Attachment is obsolete: true
Attachment #8618959 - Flags: review+
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: