Closed
Bug 1115006
Opened 9 years ago
Closed 8 years ago
Use Downloads.getSystemDownloadsDirectory to get the download directory for about:memory
Categories
(Firefox for Android Graveyard :: Download Manager, defect)
Firefox for Android Graveyard
Download Manager
Tracking
(firefox48 fixed)
RESOLVED
FIXED
Firefox 48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: Paolo, Assigned: rutuja.r.surve, Mentored)
References
Details
(Whiteboard: [good first bug][lang=js])
Attachments
(3 files, 6 obsolete files)
3.23 KB,
patch
|
Margaret
:
review-
|
Details | Diff | Splinter Review |
55.85 KB,
image/png
|
Details | |
3.45 KB,
patch
|
mconley
:
review+
Margaret
:
review+
|
Details | Diff | Splinter Review |
This code in aboutMemory.js asks for the "DfltDwnld" directory, but it should import "Downloads.jsm" and use Downloads.getSystemDownloadsDirectory instead: http://mxr.mozilla.org/mozilla-central/source/toolkit/components/aboutmemory/content/aboutMemory.js#2039 The code in the Android directory provider can then be removed: http://mxr.mozilla.org/mozilla-central/source/mobile/android/components/DirectoryProvider.js#65
Updated•9 years ago
|
Mentor: margaret.leibovic, paolo.mozmail
Comment 1•9 years ago
|
||
Hi Margaret ,as suggested in the description i have 1-imported Downloads.jsm 2-used Downloads.getSystemDownloadsDirectory() 3-Removed the else if{} code Now i am confused that what is the use of these lines and do they need to be there in aboutMemory.js file 2040 file.append(fp.defaultString); 2041 fpFinish(file);
Comment 2•9 years ago
|
||
Attachment #8543410 -
Flags: review?(margaret.leibovic)
Comment 3•9 years ago
|
||
Comment on attachment 8543410 [details] [diff] [review] 1115006.patch Review of attachment 8543410 [details] [diff] [review]: ----------------------------------------------------------------- This is going in the right direction, but it won't work. Did you test this out? If you did, you should have seen a JS error in the console. ::: mobile/android/components/DirectoryProvider.js @@ -72,5 @@ > - return localFile; > - } > - } > - let dm = Cc["@mozilla.org/download-manager;1"].getService(Ci.nsIDownloadManager); > - return dm.defaultDownloadsDirectory; It looks like your tree is out of date. This changed with bug 1114593. I don't think we should touch this XRE_UPDATE_ROOT_DIR case, but instead remove the DOWNLOAD_DIR case below. But you'll need to do an 'hg pull' to update your tree before you can create a new patch. ::: toolkit/components/aboutmemory/content/aboutMemory.js @@ +2036,5 @@ > fp.init(window, "Save Memory Reports", Ci.nsIFilePicker.modeSave); > } catch(ex) { > // This will fail on Android, since there is no Save as file picker there. > // Just save to the default downloads dir if it does. > + let DownloadsPath = yield Downloads.getSystemDownloadsDirectory(); I don't think you can use a yield in here, since this isn't inside a generator function (the types of functions we use with Task.jsm). We'll probably need to use Task.jsm to do this download asynchronously from inside a task (which would probably be better for application responsiveness anyway).
Attachment #8543410 -
Flags: review?(margaret.leibovic) → review-
Comment 4•9 years ago
|
||
(In reply to surabhi anand from comment #1) > Now i am confused that what is the use of these lines and do they need to be > there in aboutMemory.js file > 2040 file.append(fp.defaultString); > 2041 fpFinish(file); Yes, this is necessary. These lines are used to actually save a memory dump to that file.
Comment 5•9 years ago
|
||
Thanx for detailed review,it was really helpfull understanding my mistake. Now i did some diging to why i can't use yield here directly and also about Task.jsm and how to use it. But i am having some dificulty to understand and utilise Task so guide me if the code below can be framed properly(related to syntax) and will work- Task.async(function* () { let DownloadsPath = yield Downloads.getSystemDownloadsDirectory(); let file = FileUtils.File(DownloadsPath); }), file.append(fp.defaultString); fpFinish(file);
Comment 6•9 years ago
|
||
(In reply to surabhi anand from comment #5) > Thanx for detailed review,it was really helpfull understanding my mistake. > Now i did some diging to why i can't use yield here directly and also about > Task.jsm and how to use it. > But i am having some dificulty to understand and utilise Task so guide me if > the code below can be framed properly(related to syntax) and will work- > > Task.async(function* () > { > let DownloadsPath = yield > Downloads.getSystemDownloadsDirectory(); > let file = FileUtils.File(DownloadsPath); > }), > file.append(fp.defaultString); > fpFinish(file); This won't work, since 'file' won't be defined outside of the generator function. If you just want to create a new task as part of a function, you should use Task.spawn, instead of Task.async, so your implementation would look something like: function saveReportsToFile() { ... try { fp.init(window, "Save Memory Reports", Ci.nsIFilePicker.modeSave); } catch(ex) { // This will fail on Android, since there is no Save as file picker there. // Just save to the default downloads dir if it does. Task.spawn(function* () { let path = yield Downloads.getSystemDownloadsDirectory(); let file = new FileUtils.File(path); file.append(fp.defaultString); fpFinish(file); }); return; } fp.open(fpCallback); } Alternately, we could try replacing the entire 'saveReportsToFile' function with a Task.async function, but that would be a larger change.
Updated•9 years ago
|
Assignee: nobody → san13692
Comment 7•9 years ago
|
||
I just realized that instead of using Task.jsm, we could also just use the promise returned by Downloads.getSystemDownloadsDirectory directly, similarly to the patch in bug 1114586. That patch should be a good example, since it's doing something very similar to what we're trying to do here.
Hey, surabhi! Are you still working on this?
Flags: needinfo?(san13692)
Comment 9•9 years ago
|
||
Yes i will work on it and submit patch soon, and sorry i got some busy with the new semester :)
Flags: needinfo?(san13692)
Comment 10•9 years ago
|
||
I tried to implement promise function.Please give feedback.Thanks
Attachment #8543410 -
Attachment is obsolete: true
Attachment #8557490 -
Flags: feedback?(margaret.leibovic)
Comment 11•9 years ago
|
||
Comment on attachment 8557490 [details] [diff] [review] 1115006_1.patch Review of attachment 8557490 [details] [diff] [review]: ----------------------------------------------------------------- Looking good! Please open about:memory with your local and make sure that this download works correctly. ::: toolkit/components/aboutmemory/content/aboutMemory.js @@ +2037,5 @@ > } catch(ex) { > // This will fail on Android, since there is no Save as file picker there. > // Just save to the default downloads dir if it does. > + let p = Downloads.getSystemDownloadsDirectory(); > + p.then(function(dirPath)=>{ Nit: Please add spaces around the =>. You also don't need to declare a local variable for the promise, you can just use it directly. @@ +2041,5 @@ > + p.then(function(dirPath)=>{ > + let file = FileUtils.File(dirPath); > + file.append(fp.defaultString); > + fpFinish(file); > + }); Please use 2-space indentation, and make this "});" line align with the original function call above.
Attachment #8557490 -
Flags: feedback?(margaret.leibovic) → feedback+
Comment 12•9 years ago
|
||
Hello Margaret ,i tried to run a build of fennec in mobile and after i clicked about:memory nothing happened at all. The page was blank with a background color.No options were displayed.Can you tell me something about it what went wrong?
Flags: needinfo?(margaret.leibovic)
Comment 13•9 years ago
|
||
(In reply to surabhi anand from comment #12) > Hello Margaret ,i tried to run a build of fennec in mobile and after i > clicked about:memory nothing happened at all. The page was blank with a > background color.No options were displayed.Can you tell me something about > it what went wrong? Do you see any errors in the log? I suspect if you have a JS syntax error that would throw off the proper loading on the page.
Flags: needinfo?(margaret.leibovic)
Comment 14•9 years ago
|
||
nop everything went alright and was built and installed
Comment 15•9 years ago
|
||
(In reply to surabhi anand from comment #14) > nop everything went alright and was built and installed I'm sorry, I meant that while you're running the app, do you see things in the log? Any JS errors will appear with the GeckoConsole log tag. There can be a lot of log spew while the app is running, so it might be hard to find errors related to your own work. You could also try adding some of you own logging, or attach the remote debugger to see where things might be going wrong: https://developer.mozilla.org/en-US/docs/Tools/Remote_Debugging/Debugging_Firefox_for_Android_with_WebIDE
Comment 16•9 years ago
|
||
Can i generate the log in the device itself?Because i cant run it in terminal
Comment 17•9 years ago
|
||
(In reply to surabhi anand from comment #16) > Can i generate the log in the device itself?Because i cant run it in terminal How are you installing the app on the device? If you're connected over USB, you can run 'adb logcat' to see the logs from the device.
Comment 18•9 years ago
|
||
As a matter of fact i dont know weather i can run fennec in terminal or not because desktop browser runs. But while running fennec i get this- It looks like your program isn't built. You can run |mach build| to build it. Binary expected at /home/wrath/mozilla-central/obj-arm-linux-androideabi/dist/bin/fennec does not exist.
Comment 19•9 years ago
|
||
i am installing by ./mach install
Comment 20•9 years ago
|
||
i think by adb logcat i found something W/GeckoConsole( 5898): [JavaScript Error: "SyntaxError: expected expression, got '=>'" {file: "chrome://global/content/aboutMemory.js" line: 2041 column: 28 source: " p.then(function(dirPath)=>{ W/GeckoConsole( 5898): "}] D/GeckoTabs( 5898): handleMessage: DOMContentLoaded W/GeckoConsole( 5898): [JavaScript Error: "ReferenceError: onLoad is not defined" {file: "about:memory" line: 1}] D/GeckoTabs( 5898): handleMessage: Content:PageShow
Comment 21•9 years ago
|
||
(In reply to surabhi anand from comment #20) > i think by adb logcat i found something > > W/GeckoConsole( 5898): [JavaScript Error: "SyntaxError: expected expression, > got '=>'" {file: "chrome://global/content/aboutMemory.js" line: 2041 column: > 28 source: " p.then(function(dirPath)=>{ > W/GeckoConsole( 5898): "}] > D/GeckoTabs( 5898): handleMessage: DOMContentLoaded > W/GeckoConsole( 5898): [JavaScript Error: "ReferenceError: onLoad is not > defined" {file: "about:memory" line: 1}] > D/GeckoTabs( 5898): handleMessage: Content:PageShow Great, this looks like what I expected :) I see now, looking at the patch more closely, I see you're using both a function keyword and a fat arrow. You should just use one or the other.
Comment 22•9 years ago
|
||
patch with all the suggested changes:)
Attachment #8557490 -
Attachment is obsolete: true
Attachment #8561057 -
Flags: review?(margaret.leibovic)
Comment 23•9 years ago
|
||
Comment on attachment 8561057 [details] [diff] [review] 1115006_3.patch Review of attachment 8561057 [details] [diff] [review]: ----------------------------------------------------------------- Did you test this? When I tested this locally, I got this error: I/Gecko ( 8334): ************************* I/Gecko ( 8334): A coding exception was thrown in a Promise resolution callback. I/Gecko ( 8334): See https://developer.mozilla.org/Mozilla/JavaScript_code_modules/Promise.jsm/Promise I/Gecko ( 8334): I/Gecko ( 8334): Full message: ReferenceError: FileUtils is not defined I/Gecko ( 8334): Full stack: saveReportsToFile/<@chrome://global/content/aboutMemory.js:2025:11 I/Gecko ( 8334): Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:867:23 I/Gecko ( 8334): this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:746:7 I/Gecko ( 8334): this.PromiseWalker.scheduleWalkerLoop/<@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:688:37 I/Gecko ( 8334): I/Gecko ( 8334): *************************
Attachment #8561057 -
Flags: review?(margaret.leibovic) → review-
Comment 24•8 years ago
|
||
Hey! If this bug is not being solved by someone else, I want to take this up and try to fix this.
Flags: needinfo?(margaret.leibovic)
Updated•8 years ago
|
Assignee: san13692 → jalpreetnanda
Comment 26•8 years ago
|
||
Cool! Can I get some more information regarding what exactly has to be done? Thanks!
Flags: needinfo?(jalpreetnanda)
Comment 27•8 years ago
|
||
(In reply to Jalpreet Singh Nanda [:imjalpreet] from comment #26) > Cool! Can I get some more information regarding what exactly has to be done? > Thanks! You should read the previous comments to understand what's wrong with the current patch, and fix those issues. You can test this by testing to "Measure and save..." button in about:memory. However, testing on my own build, I even see an error with this: 03-10 11:15:03.063 23370 23388 E GeckoConsole: [JavaScript Error: "uncaught exception: 2147500033" {file: "chrome://global/content/aboutMemory.js" line: 2025}] 03-10 11:15:03.066 23370 23388 E GeckoConsole: [JavaScript Error: "NS_ERROR_FILE_ACCESS_DENIED: Component returned failure code: 0x80520015 (NS_ERROR_FILE_ACCESS_DENIED) [nsIMemoryInfoDumper.dumpMemoryReportsToNamedFile]" {file: "chrome://global/content/aboutMemory.js" line: 2013}] 03-10 11:15:03.066 23370 23388 E GeckoConsole: saveReportsToFile/fpFinish@chrome://global/content/aboutMemory.js:2013:5 03-10 11:15:03.066 23370 23388 E GeckoConsole: saveReportsToFile@chrome://global/content/aboutMemory.js:2031:5 So there may be other debugging work that needs to happen to make this report saving work properly.
Flags: needinfo?(margaret.leibovic)
Comment 28•8 years ago
|
||
Hey Jalpreet - are you still working on this?
Flags: needinfo?(jalpreetnanda)
Comment 29•8 years ago
|
||
Sorry, I have been a little busy with my university projects in the last few weeks. So, I may not be able to work on this right now. I will unassign myself, so that others can take this up. I will try to be back soon. Sorry again.
Flags: needinfo?(jalpreetnanda)
Updated•8 years ago
|
Assignee: jalpreetnanda → nobody
Updated•8 years ago
|
Assignee: nobody → rutuja.r.surve
Mentor: mconley
Comment 30•8 years ago
|
||
(In reply to Jalpreet Singh Nanda [:imjalpreet] from comment #29) > Sorry, I have been a little busy with my university projects in the last few > weeks. So, I may not be able to work on this right now. I will unassign > myself, so that others can take this up. I will try to be back soon. Sorry > again. Okay, thank you Jalpreet!
Assignee | ||
Comment 31•8 years ago
|
||
Can we not use (Ci.nsILocalFile.DIRECTORY_TYPE,FileUtils.PERMS_DIRECTORY) or something like this for setting access permissions for handling the NS_ERROR_FILE_ACCESS_DENIED error? Also, in Surabhi's patch, where exactly has the dirPath variable been defined? I've observed that everywhere in the SaveReportstoMemory() function, 'file.path' and not 'dirPath' has been used. Why have we eliminated the 'else' part in the patch altogether? In the line : "return new FileUtils.File(env.get("DOWNLOADS_DIRECTORY"));", does "DOWNLOADS_DIRECTORY" represent the default Download directory or the SystemDownloadsDirectory? We must make the path point to the SystemsDownload Directory. Also, we are using FileUtils in the patch in code for about:memory without importing with a statement like Cu.import("resource://gre/modules/FileUtils.jsm"). Shouldn't we be importing it here? Similarly, shouldnt Downloads.jsm also be imported in DirectoryProvider.js to help it locate the System Downloads directory? Apart from about:memory and DirectoryProvider, do any other files need modifications?
Flags: needinfo?(mconley)
Comment 32•8 years ago
|
||
These are all fantastic questions! (In reply to Rutuja from comment #31) > Can we not use (Ci.nsILocalFile.DIRECTORY_TYPE,FileUtils.PERMS_DIRECTORY) or > something like this for setting access permissions for handling the > NS_ERROR_FILE_ACCESS_DENIED error? I think I need more information before I can answer this. The failure is occurring, I assume, around here: https://hg.mozilla.org/mozilla-central/file/494289c72ba3/toolkit/components/aboutmemory/content/aboutMemory.js#l2013 If you dump file.path, what is it? > Also, in Surabhi's patch, where exactly > has the dirPath variable been defined? I've observed that everywhere in the > SaveReportstoMemory() function, 'file.path' and not 'dirPath' has been > used. Here's a line from Surabhi's patch: ``` Downloads.getSystemDownloadsDirectory().then(function(dirPath) { ``` Downloads.getSystemDownloadsDirectory is a utility function that returns a Promise[1]. The way it's being used here, the "then" method on the Promise is called with a callback. That callback is fired once the system downloads directory has been established. Presumably, "dirPath" is a string that the getSystemDownloadsDirectory Promise resolves to. There is some documentation for getSystemDownloadsDirectory where it is defined here at [2]. > Why have we eliminated the 'else' part in the patch altogether? In the > line : > "return new FileUtils.File(env.get("DOWNLOADS_DIRECTORY"));", does > "DOWNLOADS_DIRECTORY" represent the default Download directory or the > SystemDownloadsDirectory? We must make the path point to the SystemsDownload > Directory. Through a series of rather confusing abstractions, DirectoryProvider.js is a module defined for the Android platform that will return values when code like this runs: ``` let file = Services.dirsvc.get("DfltDwnld", Ci.nsIFile); ``` In this case, the "else" part will execute if the string passed to Services.dirsvc.get is "DftlDwnld", so it'll return a "nsIFile" object pointing at the default downloads directory for the system. Searching through the codebase using dxr (dxr.mozilla.org), it looks like (outside of DownloadIntegration.jsm), DfltDwnld is only ever used by aboutMemory.js. If we switch to using getSystemDownloadsDirectory as this bug expects us to, then we no longer need DfltDwnld support for Fennec (since all usage in DownloadIntegration.jsm is for non-Android platforms - checkout the if-defs around its usage). So I believe that's why it's being used - it's no longer needed for Fennec. > Also, we are using FileUtils in the patch in code for > about:memory without importing with a statement like > Cu.import("resource://gre/modules/FileUtils.jsm"). Shouldn't we be importing > it here? Yes, that's a good point. I'm surprised this isn't throwing due to a reference error for FileUtils. We should definitely be importing FileUtils.jsm. > Similarly, shouldnt Downloads.jsm also be imported in > DirectoryProvider.js to help it locate the System Downloads directory? In this case, no - when caling getSystemDownloadsDirectory, Downloads.jsm (by way of DownloadIntegration.jsm) is getting the download directory via an environment variable instead of querying the DirectoryProvider.js: https://dxr.mozilla.org/mozilla-central/rev/21bf1af375c1fa8565ae3bb2e89bd1a0809363d4/toolkit/components/jsdownloads/src/DownloadIntegration.jsm#406 > Apart > from about:memory and DirectoryProvider, do any other files need > modifications? I don't believe so, but I guess that remains to be seen. I recommend using the Remote Debugger tools I showed you last week to step through the JavaScript while it is executing and ensuring that FileUtils is being accessed properly, and to see the value of what is being passed to fpFinish. That will be very interesting. [1]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise [2]: https://hg.mozilla.org/mozilla-central/file/494289c72ba3/toolkit/components/jsdownloads/src/Downloads.jsm#l271
Flags: needinfo?(mconley)
Assignee | ||
Comment 33•8 years ago
|
||
In addition to Surabhi's changes, on adding "Cu.import("resource://gre/modules/FileUtils.jsm");" to about:memory.js, I could save the memory report to storage/emulated/0/Download/memory-report.json.gz in the emulator
Comment 34•8 years ago
|
||
(In reply to Rutuja from comment #33) > Created attachment 8741071 [details] > Emulator screenshot > > In addition to Surabhi's changes, on adding > "Cu.import("resource://gre/modules/FileUtils.jsm");" to about:memory.js, I > could save the memory report to > storage/emulated/0/Download/memory-report.json.gz in the emulator Ah, excellent! That sounds like the bug then! Well spotted!
Assignee | ||
Comment 35•8 years ago
|
||
Attachment #8741084 -
Flags: review?(mconley)
Comment 36•8 years ago
|
||
Comment on attachment 8741084 [details] [diff] [review] Added import statement for FileUtils to about:memory.js Review of attachment 8741084 [details] [diff] [review]: ----------------------------------------------------------------- Thanks Rutuja! Looking good. Just a few corrections below. Also, are all tests passing locally for you with this patch? ::: toolkit/components/aboutmemory/content/aboutMemory.js @@ +34,5 @@ > > Cu.import("resource://gre/modules/Services.jsm"); > Cu.import("resource://gre/modules/XPCOMUtils.jsm"); > Cu.import("resource://gre/modules/NetUtil.jsm"); > +Cu.import("resource://gre/modules/Downloads.jsm"); For performance, let's load these lazily using XPCOMUtils.defineLazyModuleGetter - example: https://hg.mozilla.org/mozilla-central/file/494289c72ba3/browser/base/content/browser-places.js#l652 That way, we only load these modules right when we need them. @@ +2013,5 @@ > .getService(Ci.nsIMemoryInfoDumper); > let finishDumping = () => { > updateMainAndFooter("Saved memory reports to " + file.path, HIDE_FOOTER); > } > + Please remove this trailing whitespace. @@ +2031,5 @@ > } catch(ex) { > // This will fail on Android, since there is no Save as file picker there. > // Just save to the default downloads dir if it does. > + Downloads.getSystemDownloadsDirectory().then(function(dirPath) { > + let file = FileUtils.File(dirPath); The indentation here needs fixing. This file is using two-space indentation, so everything from lines 2035 - 2037 will need to be indented 2 more spaces.
Attachment #8741084 -
Flags: review?(mconley) → review-
Assignee | ||
Comment 37•8 years ago
|
||
This is the patch with the required changes. Also, locally on the emulator I get the same response as in the screenshot after saving.
Attachment #8741131 -
Flags: review?(mconley)
Comment 38•8 years ago
|
||
Comment on attachment 8741131 [details] [diff] [review] Added import statement for FileUtils and LazyModuleGetters to about:memory.js Review of attachment 8741131 [details] [diff] [review]: ----------------------------------------------------------------- Almost there! ::: toolkit/components/aboutmemory/content/aboutMemory.js @@ +32,5 @@ > const UNITS_COUNT_CUMULATIVE = Ci.nsIMemoryReporter.UNITS_COUNT_CUMULATIVE; > const UNITS_PERCENTAGE = Ci.nsIMemoryReporter.UNITS_PERCENTAGE; > > +XPCOMUtils.defineLazyModuleGetter(this, "Services", "resource://gre/modules/Services.jsm"); > +XPCOMUtils.defineLazyModuleGetter(this, "XPCOMUtils", "resource://gre/modules/XPCOMUtils.jsm"); Strange that this works - you're using XPCOMUtils on line 35 before you define its lazy getter on 36. Anyhow, the only ones I wanted you to make lazy were Downloads.jsm and FileUtils.jsm. If we were fine with Services.jsm and NetUtils.jsm being loaded immediately, we can keep them like that - but let's load Downloads.jsm and FileUtils.jsm as lazy modules. Also, please try to break it up so that it's less than 80 chars, like XPCOMUtils.defineLazyModuleGetter(this, "Downloads", "resource://gre/modules/Downloads.jsm"); @@ +2029,5 @@ > fp.init(window, "Save Memory Reports", Ci.nsIFilePicker.modeSave); > } catch(ex) { > // This will fail on Android, since there is no Save as file picker there. > // Just save to the default downloads dir if it does. > + Downloads.getSystemDownloadsDirectory().then(function(dirPath) { Nit - Sorry, line 2033 needs to be unindented 2 spaces. We want it to look like: ``` Downloads.getSystemDownloadsDirectory().then(function(dirPath) { let file = FileUtils.File(dirPath); file.append(fp.defaultString); fpFinish(file); }); ``` And the reason we're indenting those lines 2 more spaces is because they're inside a block - in this case, a function block, for the function passed to the getSystemDownloadsDirectory Promise's "then" function.
Attachment #8741131 -
Flags: review?(mconley) → review-
Assignee | ||
Comment 39•8 years ago
|
||
Attachment #8741205 -
Flags: review?(mconley)
Attachment #8741084 -
Attachment is obsolete: true
Attachment #8741131 -
Attachment is obsolete: true
Attachment #8741205 -
Attachment is obsolete: true
Attachment #8741205 -
Flags: review?(mconley)
Assignee | ||
Comment 40•8 years ago
|
||
Attachment #8741206 -
Flags: review?(mconley)
Comment 41•8 years ago
|
||
Comment on attachment 8741206 [details] [diff] [review] Added import statement for FileUtils and LazyModuleGetters for Downloads and FileUtils Review of attachment 8741206 [details] [diff] [review]: ----------------------------------------------------------------- Bah - just noticed something new. Sorry to keep rotating on this. :/ ::: mobile/android/components/DirectoryProvider.js @@ -74,5 @@ > - // synchronous, so just return what the getSystemDownloadsDirectory > - // implementation would have returned. > - let env = Cc["@mozilla.org/process/environment;1"].getService(Ci.nsIEnvironment); > - return new FileUtils.File(env.get("DOWNLOADS_DIRECTORY")); > - } else if (AppConstants.MOZ_B2GDROID && prop === "coreAppsDir") { I just realized that we probably don't want to remove the coreAppsDir block here. Just the DOWNLOAD_DIR one. ::: toolkit/components/aboutmemory/content/aboutMemory.js @@ +34,5 @@ > > Cu.import("resource://gre/modules/Services.jsm"); > Cu.import("resource://gre/modules/XPCOMUtils.jsm"); > Cu.import("resource://gre/modules/NetUtil.jsm"); > +XPCOMUtils.defineLazyModuleGetter(this, "Downloads", Trailing whitespaces, but I can remove these on landing.
Attachment #8741206 -
Flags: review?(mconley) → review-
Attachment #8741206 -
Attachment is obsolete: true
Assignee | ||
Comment 42•8 years ago
|
||
Attachment #8741411 -
Flags: review?(mconley)
Comment 43•8 years ago
|
||
Comment on attachment 8741411 [details] [diff] [review] Added import statement for FileUtils and LazyModuleGetters for Downloads and FileUtils Review of attachment 8741411 [details] [diff] [review]: ----------------------------------------------------------------- This looks fine - I have one more alignment problem, but I can fix it before I land it. In the meantime, going to get margaret sign-off on the DirectoryProvider.js change. ::: toolkit/components/aboutmemory/content/aboutMemory.js @@ +2031,5 @@ > fp.init(window, "Save Memory Reports", Ci.nsIFilePicker.modeSave); > } catch(ex) { > // This will fail on Android, since there is no Save as file picker there. > // Just save to the default downloads dir if it does. > + Downloads.getSystemDownloadsDirectory().then(function(dirPath) { The entire block of 2035-2039 needs to be indented 2 more spaces, since it's inside this catch block. I don't want to harp on this though, so I'll fix this before I land it.
Attachment #8741411 -
Flags: review?(mconley)
Attachment #8741411 -
Flags: review?(margaret.leibovic)
Attachment #8741411 -
Flags: review+
Assignee | ||
Comment 44•8 years ago
|
||
Yes Sure! :)
Comment 45•8 years ago
|
||
Gentle review ping on margaret
Comment 46•8 years ago
|
||
Comment on attachment 8741411 [details] [diff] [review] Added import statement for FileUtils and LazyModuleGetters for Downloads and FileUtils Review of attachment 8741411 [details] [diff] [review]: ----------------------------------------------------------------- Sorry for the slow review, this is fine by me. Thanks for your help!
Attachment #8741411 -
Flags: review?(margaret.leibovic) → review+
Comment 47•8 years ago
|
||
ni? myself to fix alignment in the patch and land on fx-team.
Flags: needinfo?(mconley)
Comment 49•8 years ago
|
||
Hey Rutuja, I don't currently have a virtual Android device set up on my machine. I've rebased your patch and made the indentation changes I had talked about in comment 43. We appear to be passing things on try, but I'd love some confirmation that you can still save the about:memory reports. Would you mind downloading the APK I've built on try and testing it?
Flags: needinfo?(mconley) → needinfo?(rutuja.r.surve)
Comment 50•8 years ago
|
||
Nevermind - I was able to test this, and it appears to work!
Flags: needinfo?(rutuja.r.surve)
Comment 51•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/3b3c1809c8be95512093a3cf19d1a35e99ad145c Bug 1115006 - Use Downloads.getSystemDownloadsDirectory to get download directory for about:memory on Fennec. r=mconley,margaret
Assignee | ||
Comment 52•8 years ago
|
||
Hi! Sorry for replying late.. I indented the catch block by 2 spaces this way: try { fp.init(window, "Save Memory Reports", Ci.nsIFilePicker.modeSave); } catch(ex) { // This will fail on Android, since there is no Save as file picker there. // Just save to the default downloads dir if it does. Downloads.getSystemDownloadsDirectory().then(function(dirPath) { let file = FileUtils.File(dirPath); file.append(fp.defaultString); fpFinish(file); }); On running the code, I could save the reports generated by about:memory.js successfully.
Assignee | ||
Comment 53•8 years ago
|
||
Sorry, I indented it this way: try { fp.init(window, "Save Memory Reports", Ci.nsIFilePicker.modeSave); } catch(ex) { // This will fail on Android, since there is no Save as file picker there. // Just save to the default downloads dir if it does. Downloads.getSystemDownloadsDirectory().then(function(dirPath) { let file = FileUtils.File(dirPath); file.append(fp.defaultString); fpFinish(file); }); It works! :)
Comment 54•8 years ago
|
||
Thanks for testing Rutuja. I ended up formatting it this way: try { fp.init(window, "Save Memory Reports", Ci.nsIFilePicker.modeSave); } catch(ex) { // This will fail on Android, since there is no Save as file picker there. // Just save to the default downloads dir if it does. Downloads.getSystemDownloadsDirectory().then(function(dirPath) { let file = FileUtils.File(dirPath); file.append(fp.defaultString); fpFinish(file); }); return } Check out the commit for specifics: https://hg.mozilla.org/integration/fx-team/rev/3b3c1809c8be95512093a3cf19d1a35e99ad145c Note that it's only necessary to indent the _contents_ of a block, so in this case, just the line starting at "let file =", up until "fpFinish(file);". That's the 2-space convention we have in this file, and in many of the JS files in the Firefox codebase. There are other places where we have 4-space indentation instead - usually best to go with what's already in the file to be consistent. Anyhow, thanks for your great work here. Your patch has been landed on an integration branch and will be closed out once it merges to central.
Assignee | ||
Comment 55•8 years ago
|
||
Okay sure! :)
Comment 56•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/3b3c1809c8be
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Updated•3 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
•