926736 - Returns path string for get directory methods in DownloadIntegration.jsm

Status: RESOLVED FIXED

(Toolkit :: Downloads API, defect)

Description

[Raymond Lee [:raymondlee]]

We should also update their callers as well.

Comment 1

[:Gavin Sharp [email: [e-mail]]]

This seems like an important bug to fix ASAP, to avoid people relying on the existing return value type. At the very least we should return a clone of the file...

Comment 2

[:Gavin Sharp [email: [e-mail]]]

(And undo the patch in bug 923940)

Comment 3

[Raymond Lee [:raymondlee]]

Attachment: v1

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #1)
> This seems like an important bug to fix ASAP, to avoid people relying on the
> existing return value type. At the very least we should return a clone of
> the file...

Return a clone of the file is the quickest fix.

Comment 4

[Raymond Lee [:raymondlee]]

My question is whether we really need to return path string for those method or we are ok with a clone of file?

Comment 5

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 818207 [details] [diff] [review]
v1

nsIFile is a bit overkill and we'd like to eventually remove all uses of it, but this is an OK first step I suppose. I guess it does make things harder to change later once there are more consumers of this API...

>diff --git a/toolkit/components/jsdownloads/src/DownloadIntegration.jsm b/toolkit/components/jsdownloads/src/DownloadIntegration.jsm

>-      this._downloadsDirectory = directory;
>+      this._downloadsDirectory = directory.clone();
>       throw new Task.Result(this._downloadsDirectory);

Shouldn't this be:

>+      this._downloadsDirectory = directory;
>       throw new Task.Result(this._downloadsDirectory.clone());

?

Comment 6

[Raymond Lee [:raymondlee]]

Attachment: v2 (.clone() version)

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #5)
> Comment on attachment 818207 [details] [diff] [review]
> v1
> 
> nsIFile is a bit overkill and we'd like to eventually remove all uses of it,
> but this is an OK first step I suppose. I guess it does make things harder
> to change later once there are more consumers of this API...
>

May be we land the .clone version first so we don't need the patch for bug 923940.  I am going to work on a string path version.
 
> >diff --git a/toolkit/components/jsdownloads/src/DownloadIntegration.jsm b/toolkit/components/jsdownloads/src/DownloadIntegration.jsm
> 
> >-      this._downloadsDirectory = directory;
> >+      this._downloadsDirectory = directory.clone();
> >       throw new Task.Result(this._downloadsDirectory);
> 
> Shouldn't this be:
> 
> >+      this._downloadsDirectory = directory;
> >       throw new Task.Result(this._downloadsDirectory.clone());
> 
> ?

Updated

Comment 7

[Raymond Lee [:raymondlee]]

Attachment: v3 - toolkit/component/jsdownload

Comment 8

[Raymond Lee [:raymondlee]]

Attachment: v3 - devtools

Comment 9

[Raymond Lee [:raymondlee]]

Attachment: v3 - metro

Comment 10

[Raymond Lee [:raymondlee]]

Attachment: v3 - mozapps

Comment 11

[:Paolo Amadini]

Comment on attachment 819018 [details] [diff] [review]
v3 - toolkit/component/jsdownload

Review of attachment 819018 [details] [diff] [review]:
-----------------------------------------------------------------

Nice job! I've a few minor comments below, but I think we should go for this approach directly now.

::: toolkit/components/jsdownloads/src/DownloadIntegration.jsm
@@ +282,5 @@
>  #elifdef XP_UNIX
>  #ifdef ANDROID
>        // Android doesn't have a $HOME directory, and by default we only have
>        // write access to /data/data/org.mozilla.{$APP} and /sdcard
>        let directoryPath = gEnvironment.get("DOWNLOADS_DIRECTORY");

Remove "let".

::: toolkit/components/jsdownloads/test/unit/test_DownloadIntegration.js
@@ +131,5 @@
>    // Should return the system downloads directory.
>    Services.prefs.setIntPref(folderListPrefName, 1);
>    let systemDir = yield DownloadIntegration.getSystemDownloadsDirectory();
>    let downloadDir = yield DownloadIntegration.getPreferredDownloadsDirectory();
> +  do_check_neq(downloadDir, null);

I'm not sure about how much these null checks are useful, given that we return an exception if something goes wrong. If we want to keep them to be on the safe side, we could turn them all into empty string checks.

::: toolkit/components/jsdownloads/test/unit/test_Downloads.js
@@ +137,4 @@
>  
>  /**
>   * Tests that the getSystemDownloadsDirectory returns a valid nsFile
>   * download directory object.

These test descriptions need to be updated.

Comment 12

[:Paolo Amadini]

Comment on attachment 818813 [details] [diff] [review]
v2 (.clone() version)

No need to do this, given the other patch.

Comment 13

[:Paolo Amadini]

There are no in-tree consumers of this API in Firefox 26, no need to uplift the change there.

Comment 14

[:Gavin Sharp [email: [e-mail]]]

(In reply to :Paolo Amadini from comment #13)
> There are no in-tree consumers of this API in Firefox 26, no need to uplift
> the change there.

Shouldn't we make the API change in the first-released version, though? We don't want add-ons to rely on nsIFile in 26 and then have to change in 27.

Comment 15

[Raymond Lee [:raymondlee]]

Attachment: v4 - toolkit/component/jsdownload r+

(In reply to :Paolo Amadini from comment #11)
> Comment on attachment 819018 [details] [diff] [review]
> v3 - toolkit/component/jsdownload
> 
> Review of attachment 819018 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Nice job! I've a few minor comments below, but I think we should go for this
> approach directly now.
> 
> ::: toolkit/components/jsdownloads/src/DownloadIntegration.jsm
> @@ +282,5 @@
> >  #elifdef XP_UNIX
> >  #ifdef ANDROID
> >        // Android doesn't have a $HOME directory, and by default we only have
> >        // write access to /data/data/org.mozilla.{$APP} and /sdcard
> >        let directoryPath = gEnvironment.get("DOWNLOADS_DIRECTORY");
> 
> Remove "let".

Removed

> 
> ::: toolkit/components/jsdownloads/test/unit/test_DownloadIntegration.js
> @@ +131,5 @@
> >    // Should return the system downloads directory.
> >    Services.prefs.setIntPref(folderListPrefName, 1);
> >    let systemDir = yield DownloadIntegration.getSystemDownloadsDirectory();
> >    let downloadDir = yield DownloadIntegration.getPreferredDownloadsDirectory();
> > +  do_check_neq(downloadDir, null);
> 
> I'm not sure about how much these null checks are useful, given that we
> return an exception if something goes wrong. If we want to keep them to be
> on the safe side, we could turn them all into empty string checks.

Done.

> 
> ::: toolkit/components/jsdownloads/test/unit/test_Downloads.js
> @@ +137,4 @@
> >  
> >  /**
> >   * Tests that the getSystemDownloadsDirectory returns a valid nsFile
> >   * download directory object.
> 
> These test descriptions need to be updated.

Updated

Comment 16

[Raymond Lee [:raymondlee]]

Comment on attachment 819021 [details] [diff] [review]
v3 - metro

This should be applied first.
https://bugzilla.mozilla.org/attachment.cgi?id=819540

Comment 17

[Raymond Lee [:raymondlee]]

Comment on attachment 819022 [details] [diff] [review]
v3 - mozapps

This should be applied first.
https://bugzilla.mozilla.org/attachment.cgi?id=819540

Comment 18

[Raymond Lee [:raymondlee]]

Comment on attachment 819019 [details] [diff] [review]
v3 - devtools

This should be applied first.
https://bugzilla.mozilla.org/attachment.cgi?id=819540

Comment 19

[:Paolo Amadini]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #14)
> Shouldn't we make the API change in the first-released version, though? We
> don't want add-ons to rely on nsIFile in 26 and then have to change in 27.

There is little use for this API in Firefox 26, considering that generally add-ons would continue to use the nsIDownloadManager getters. However, I don't see any downside to uplift this change as well.

Comment 20

[Raymond Lee [:raymondlee]]

Attachment: v4 - devtools r+

Comment 21

[Raymond Lee [:raymondlee]]

Attachment: v4 - mozapps r+

Comment 22

[:Paolo Amadini]

Raymond, could you create a version of attachment 819540 [details] [diff] [review] rebased on top of the Aurora branch, and run a tryserver build for that? I expect that to succeed given that there are no callers on Aurora.

Comment 23

[Raymond Lee [:raymondlee]]

Attachment: v4 - toolkit/component/jsdownload (Aurora) r+

(In reply to :Paolo Amadini from comment #22)
> Raymond, could you create a version of attachment 819540 [details] [diff] [review]
> [review] rebased on top of the Aurora branch, and run a tryserver build for
> that? I expect that to succeed given that there are no callers on Aurora.

Rebased on top of Aurora branch

Passed try
https://tbpl.mozilla.org/?tree=Try&rev=3ba03b859a28

Paolo: Shall we land this?

Comment 24

[Raymond Lee [:raymondlee]]

Comment on attachment 819021 [details] [diff] [review]
v3 - metro

Matt: review ping

Comment 25

[Raymond Lee [:raymondlee]]

Attachment: v5 - toolkit/component/jsdownload r+

Minor update some comments

Comment 26

[Raymond Lee [:raymondlee]]

Attachment: v4 - metro r+

Comment 27

[Raymond Lee [:raymondlee]]

Attachment: v5 - toolkit/component/jsdownload (Aurora) r+

Minor update some comments

Push to try again
https://tbpl.mozilla.org/?tree=Try&rev=05e3ba3f0154

Paolo: if you are happy with it, we can land this to Aurora

Comment 28

[Raymond Lee [:raymondlee]]

Comment on attachment 820750 [details] [diff] [review]
v5 - toolkit/component/jsdownload r+

This also passed try with other patches in this bug
https://tbpl.mozilla.org/?tree=Try&rev=680682d62021

Comment 29

[:Paolo Amadini]

Comment on attachment 820752 [details] [diff] [review]
v5 - toolkit/component/jsdownload (Aurora) r+

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 825588
User impact if declined: None (but see comment 14 for API consistency)
Testing completed (on m-c, etc.): https://tbpl.mozilla.org/?tree=Try&rev=05e3ba3f0154
Risk to taking this patch (and alternatives if risky): Low, has tests
String or IDL/UUID changes made by this patch: None

Comment 30

[Ed Morley [:emorley]]

https://hg.mozilla.org/integration/fx-team/rev/eafedd45d99e
https://hg.mozilla.org/integration/fx-team/rev/776a19e7928c
https://hg.mozilla.org/integration/fx-team/rev/1e970a11c576
https://hg.mozilla.org/integration/fx-team/rev/27d5b2984cc4

Comment 31

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/eafedd45d99e
https://hg.mozilla.org/mozilla-central/rev/776a19e7928c
https://hg.mozilla.org/mozilla-central/rev/1e970a11c576
https://hg.mozilla.org/mozilla-central/rev/27d5b2984cc4

Comment 32

[:Paolo Amadini]

Comment on attachment 820752 [details] [diff] [review]
v5 - toolkit/component/jsdownload (Aurora) r+

I'll handle this check-in together with some others as soon as more results from the tryserver build come in:

https://tbpl.mozilla.org/?tree=Try&rev=c147baedeece

Comment 33

[:Paolo Amadini]

Comment on attachment 820752 [details] [diff] [review]
v5 - toolkit/component/jsdownload (Aurora) r+

https://hg.mozilla.org/releases/mozilla-aurora/rev/acc49061d197