Use Downloads.getSystemDownloadsDirectory to get the download directory for about:memory

RESOLVED FIXED in Firefox 48

Status

()

Firefox for Android
Download Manager
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: Paolo, Assigned: Rutuja, Mentored)

Tracking

Trunk
Firefox 48
Points:
---

Firefox Tracking Flags

(firefox48 fixed)

Details

(Whiteboard: [good first bug][lang=js])

Attachments

(3 attachments, 6 obsolete attachments)

(Reporter)

Description

4 years ago
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

4 years ago
Mentor: margaret.leibovic, paolo.mozmail

Comment 1

4 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

4 years ago
Created attachment 8543410 [details] [diff] [review]
1115006.patch
Attachment #8543410 - Flags: review?(margaret.leibovic)

Comment 3

4 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

4 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

4 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

4 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

4 years ago
Assignee: nobody → san13692

Comment 7

4 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

4 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

4 years ago
Created attachment 8557490 [details] [diff] [review]
1115006_1.patch

I tried to implement promise function.Please give feedback.Thanks
Attachment #8543410 - Attachment is obsolete: true
Attachment #8557490 - Flags: feedback?(margaret.leibovic)

Comment 11

4 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

4 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

4 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

4 years ago
nop everything went alright and was built and installed

Comment 15

4 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

4 years ago
Can i generate the log in the device itself?Because i cant run it in terminal

Comment 17

4 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

4 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

4 years ago
i am installing by ./mach install

Comment 20

4 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

4 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

4 years ago
Created attachment 8561057 [details] [diff] [review]
1115006_3.patch

patch with all the suggested changes:)
Attachment #8557490 - Attachment is obsolete: true
Attachment #8561057 - Flags: review?(margaret.leibovic)

Comment 23

4 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-
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)

Comment 25

2 years ago
Sure go ahead!!
Flags: needinfo?(jalpreetnanda)

Updated

2 years ago
Assignee: san13692 → jalpreetnanda
Cool! Can I get some more information regarding what exactly has to be done? Thanks!
Flags: needinfo?(jalpreetnanda)

Comment 27

2 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)
Hey Jalpreet - are you still working on this?
Flags: needinfo?(jalpreetnanda)
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)
Assignee: jalpreetnanda → nobody
Assignee: nobody → rutuja.r.surve
Mentor: mconley
(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

2 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)
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

2 years ago
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
(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

2 years ago
Created attachment 8741084 [details] [diff] [review]
Added import statement for FileUtils to about:memory.js
Attachment #8741084 - Flags: review?(mconley)
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

2 years ago
Created attachment 8741131 [details] [diff] [review]
Added import statement for FileUtils and LazyModuleGetters to about:memory.js

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 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

2 years ago
Created attachment 8741205 [details] [diff] [review]
Added import statement for FileUtils and LazyModuleGetters for Downloads and FileUtils
Attachment #8741205 - Flags: review?(mconley)
(Assignee)

Updated

2 years ago
Attachment #8741084 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8741131 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8741205 - Attachment is obsolete: true
Attachment #8741205 - Flags: review?(mconley)
(Assignee)

Comment 40

2 years ago
Created attachment 8741206 [details] [diff] [review]
Added import statement for FileUtils and LazyModuleGetters for Downloads and FileUtils
Attachment #8741206 - Flags: review?(mconley)
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-
(Assignee)

Updated

2 years ago
Attachment #8741206 - Attachment is obsolete: true
(Assignee)

Comment 42

2 years ago
Created attachment 8741411 [details] [diff] [review]
Added import statement for FileUtils and LazyModuleGetters for Downloads and FileUtils
Attachment #8741411 - Flags: review?(mconley)
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

2 years ago
Yes Sure! :)
Gentle review ping on margaret

Comment 46

2 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+
ni? myself to fix alignment in the patch and land on fx-team.
Flags: needinfo?(mconley)
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)
Nevermind - I was able to test this, and it appears to work!
Flags: needinfo?(rutuja.r.surve)
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

2 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

2 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! :)
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

2 years ago
Okay sure! :)

Comment 56

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3b3c1809c8be
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
You need to log in before you can comment on or make changes to this bug.