Closed Bug 926736 Opened 6 years ago Closed 6 years ago

Returns path string for get directory methods in DownloadIntegration.jsm

Categories

(Toolkit :: Downloads API, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla27
Tracking Status
firefox26 + fixed
firefox27 + fixed

People

(Reporter: raymondlee, Assigned: raymondlee)

References

Details

Attachments

(5 files, 8 obsolete files)

1.47 KB, patch
emorley
: checkin+
Details | Diff | Splinter Review
3.51 KB, patch
emorley
: checkin+
Details | Diff | Splinter Review
20.84 KB, patch
emorley
: checkin+
Details | Diff | Splinter Review
2.40 KB, patch
emorley
: checkin+
Details | Diff | Splinter Review
21.71 KB, patch
Paolo
: checkin+
Details | Diff | Splinter Review
We should also update their callers as well.
Blocks: 851471
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...
(And undo the patch in bug 923940)
Attached patch v1 (obsolete) — Splinter Review
(In reply to :Gavin Sharp (use gavin@gavinsharp.com 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.
Attachment #818207 - Flags: review?(paolo.mozmail)
My question is whether we really need to return path string for those method or we are ok with a clone of file?
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());

?
Attached patch v2 (.clone() version) (obsolete) — Splinter Review
(In reply to :Gavin Sharp (use gavin@gavinsharp.com 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
Attachment #818207 - Attachment is obsolete: true
Attachment #818207 - Flags: review?(paolo.mozmail)
Attachment #818813 - Flags: review?(paolo.mozmail)
Attachment #818813 - Attachment description: v2 → v2 (.clone() version)
Attachment #819018 - Flags: review?(paolo.mozmail)
Attached patch v3 - devtools (obsolete) — Splinter Review
Attached patch v3 - metro (obsolete) — Splinter Review
Attached patch v3 - mozapps (obsolete) — Splinter Review
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.
Attachment #819018 - Flags: review?(paolo.mozmail) → review+
Comment on attachment 818813 [details] [diff] [review]
v2 (.clone() version)

No need to do this, given the other patch.
Attachment #818813 - Flags: review?(paolo.mozmail)
There are no in-tree consumers of this API in Firefox 26, no need to uplift the change there.
(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.
(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
Assignee: nobody → raymond
Attachment #818813 - Attachment is obsolete: true
Attachment #819018 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Comment on attachment 819021 [details] [diff] [review]
v3 - metro

This should be applied first.
https://bugzilla.mozilla.org/attachment.cgi?id=819540
Attachment #819021 - Flags: review?(mbrubeck)
Comment on attachment 819022 [details] [diff] [review]
v3 - mozapps

This should be applied first.
https://bugzilla.mozilla.org/attachment.cgi?id=819540
Attachment #819022 - Flags: review?(paolo.mozmail)
Comment on attachment 819019 [details] [diff] [review]
v3 - devtools

This should be applied first.
https://bugzilla.mozilla.org/attachment.cgi?id=819540
Attachment #819019 - Flags: review?(jwalker)
Blocks: 875731
Blocks: 875648
Attachment #819019 - Flags: review?(jwalker) → review+
Attachment #819022 - Flags: review?(paolo.mozmail) → review+
(In reply to :Gavin Sharp (use gavin@gavinsharp.com 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.
Attached patch v4 - devtools r+Splinter Review
Attachment #819019 - Attachment is obsolete: true
Attached patch v4 - mozapps r+Splinter Review
Attachment #819022 - Attachment is obsolete: true
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.
Blocks: 929355
Depends on: 885319
(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?
Flags: needinfo?(paolo.mozmail)
Comment on attachment 819021 [details] [diff] [review]
v3 - metro

Matt: review ping
Attachment #819021 - Flags: review?(mbrubeck) → review+
Minor update some comments
Attachment #819540 - Attachment is obsolete: true
Attached patch v4 - metro r+Splinter Review
Attachment #819021 - Attachment is obsolete: true
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
Attachment #820727 - Attachment is obsolete: true
Flags: needinfo?(paolo.mozmail)
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
Attachment #820750 - Flags: checkin?
Attachment #820751 - Flags: checkin?
Attachment #820093 - Flags: checkin?
Attachment #820091 - Flags: checkin?
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
Attachment #820752 - Flags: approval-mozilla-aurora?
Blocks: 923940
Attachment #820091 - Flags: checkin? → checkin+
Attachment #820093 - Flags: checkin? → checkin+
Attachment #820750 - Flags: checkin? → checkin+
Attachment #820751 - Flags: checkin? → checkin+
Attachment #820752 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #820752 - Flags: checkin?
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
Attachment #820752 - Flags: checkin?
Comment on attachment 820752 [details] [diff] [review]
v5 - toolkit/component/jsdownload (Aurora) r+

https://hg.mozilla.org/releases/mozilla-aurora/rev/acc49061d197
Attachment #820752 - Flags: checkin+
You need to log in before you can comment on or make changes to this bug.