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)
Firefox for Android Graveyard
Download Manager
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").
Reporter | ||
Updated•10 years ago
|
Summary: Replace defaultDownloadsDirectory with Downloads.getSystemDownloadsDirectory in DirectoryProvider.js → Replace defaultDownloadsDirectory calls in DirectoryProvider.js
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → margaret.leibovic
tracking-fennec: --- → ?
Assignee | ||
Comment 1•10 years ago
|
||
Attachment #8540298 -
Flags: review?(paolo.mozmail)
Attachment #8540298 -
Flags: review?(mark.finkle)
Assignee | ||
Comment 2•10 years ago
|
||
/r/1641 - Bug 1114593 - Replace defaultDownloadsDirectory calls in DirectoryProvider.js
Pull down this commit:
hg pull review -r 37fbf34713facace6b61c08643821601605559a1
Assignee | ||
Comment 3•10 years ago
|
||
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)
Comment 4•10 years ago
|
||
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.
Assignee | ||
Comment 5•10 years ago
|
||
/r/1641 - Bug 1114593 - Replace defaultDownloadsDirectory calls in DirectoryProvider.js
Pull down this commit:
hg pull review -r 8f481cff876bdb9c7053f3f2b13861929096e0d3
Updated•10 years ago
|
Attachment #8540298 -
Flags: review?(mark.finkle) → review+
Comment 6•10 years ago
|
||
Reporter | ||
Comment 7•10 years ago
|
||
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.
Reporter | ||
Updated•10 years ago
|
Attachment #8540298 -
Flags: review?(paolo.mozmail)
Assignee | ||
Comment 8•10 years ago
|
||
(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
Reporter | ||
Comment 9•10 years ago
|
||
(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.
Comment 10•10 years ago
|
||
(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)
Assignee | ||
Comment 11•10 years ago
|
||
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 37
Updated•10 years ago
|
tracking-fennec: ? → 37+
Assignee | ||
Comment 13•10 years ago
|
||
Attachment #8540298 -
Attachment is obsolete: true
Attachment #8618959 -
Flags: review+
Assignee | ||
Comment 14•10 years ago
|
||
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•