Closed Bug 956646 Opened 6 years ago Closed 6 years ago

Add renameTo to nsIFile

Categories

(Core :: XPCOM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: xyuan, Assigned: xyuan)

References

Details

(Keywords: dev-doc-complete)

Attachments

(4 files, 21 obsolete files)

3.99 KB, patch
xyuan
: review+
Details | Diff | Splinter Review
1.40 KB, patch
xyuan
: review+
Details | Diff | Splinter Review
9.41 KB, patch
xyuan
: review+
Details | Diff | Splinter Review
2.69 KB, patch
xyuan
: review+
Details | Diff | Splinter Review
When moving a file to a different volume, "nsIFile.moveTo" simulates the move by using the "copy" and "delete" functions. For bug 935883, we don't want "nsIFile.moveTo" to simulate the move between volumes. See https://bugzilla.mozilla.org/show_bug.cgi?id=935883#c1 for more details.

So I want to add a "rename" method to nsIFile, which doesn't allow move file across volumes. The method is similar to the "moveTo":

    /**
     *  rename
     *
     *  A method has the same function and parameters as the "moveTo" method
     *  except that if this file or directory is moved to a a different volume,
     *  the method will fail and returns an error (NS_ERROR_FILE_ACCESS_DENIED).
     */
    void rename(in nsIFile newParentDir, in AString newName);
I don't think I understand the reasoning in bug 935883, so I'm going to ask some questions:

* what thread is all this code running on?
* Since MoveFile already falls back to copy/remove, why do you need to do it separately in the client code?
* You seem to be saying that if we're moving a directory, the progress API should list each file separately. Does this mean that the "rename" API will not ever be used with directories. I don't think the API should send different progress notifications based on whether the files are on the same volume.
* Since the nsIFile operations are all synchronous, the progress notifications won't make much sense.
Flags: needinfo?(xyuan)
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #1)
> I don't think I understand the reasoning in bug 935883, so I'm going to ask
> some questions:
> 
> * what thread is all this code running on?

A background thread of the chrome process.

> * Since MoveFile already falls back to copy/remove, why do you need to do it
> separately in the client code?

We need to show the moving progress if it is time-consuming, especially when moving a large file or
directory to a different volume, and allow user to abort a time-consuming "move" operation, but "moveTo" doesn't provide such functions.

To move a file or directory within the same volume, it is quite fast and we 
don't need the detail progress information. "moveTo" or "rename" is qualified for such task.

To move across volumes, it may take a lot of time. We want to manually copy the file or directory,
send progress notifications and allow user to abort.

> * You seem to be saying that if we're moving a directory, the progress API
> should list each file separately. Does this mean that the "rename" API will
> not ever be used with directories. 

Not exactly. "rename" will also be used for moving directory within the same volume.

> I don't think the API should send
> different progress notifications based on whether the files are on the same
> volume.
> * Since the nsIFile operations are all synchronous, the progress
> notifications won't make much sense.

That's why I want to replace "moveTo" with manual copy/remove and make it asynchronous :-)
Flags: needinfo?(xyuan)
Attached patch Part1 interface changes (obsolete) — Splinter Review
Attached patch Part3 windows patch (obsolete) — Splinter Review
Attached patch Part4 xpcshell based test (obsolete) — Splinter Review
I don't know how to test moving file across volumes, so this test doesn't cover that :(
Attached patch Part1 v1 Interface-changes.patch (obsolete) — Splinter Review
Assignee: nobody → xyuan
Attachment #8356453 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8358283 - Flags: review?(benjamin)
Attached patch Part2 v1 Unix-patch.patch (obsolete) — Splinter Review
Attachment #8356455 - Attachment is obsolete: true
Attachment #8358285 - Flags: review?(benjamin)
Attached patch Part3 v1 Windows-patch.patch (obsolete) — Splinter Review
Attachment #8356456 - Attachment is obsolete: true
Attachment #8358286 - Flags: review?(benjamin)
Attachment #8356458 - Attachment is obsolete: true
Attachment #8358287 - Flags: review?(benjamin)
Try passed:
  https://tbpl.mozilla.org/?tree=Try&rev=6a92659a460c

@bsmedberg, could you help to review these patch, or suggest someone else?

I didn't implement "rename" methods on OS/2. Do we need to support that old OS?
Comment on attachment 8358283 [details] [diff] [review]
Part1 v1 Interface-changes.patch

I'm unhappy that we need this. I'd much prefer us to have a real file API with consistent progress notifications (OS.File, unless this is JS). But assuming that we can't use OS.File and that this blocks something important, I'll mark it ok.

You need to rev the nsIFile IID.
Attachment #8358283 - Flags: review?(benjamin) → review+
Comment on attachment 8358283 [details] [diff] [review]
Part1 v1 Interface-changes.patch

Also I note that MoveTo currently adjusts the path of the current object if a move completes successfully. I think that behavior is strange and undesirable, but it should be documented for MoveTo. We should not repeat that behavior with Rename(), the localfile should continue to point to the old location.
Comment on attachment 8358285 [details] [diff] [review]
Part2 v1 Unix-patch.patch

r=me removing the adjustment behavior at the end
Attachment #8358285 - Flags: review?(benjamin) → review+
Comment on attachment 8358286 [details] [diff] [review]
Part3 v1 Windows-patch.patch

r=me again with the adjustment removed
Attachment #8358286 - Flags: review?(benjamin) → review+
Attachment #8358287 - Flags: review?(benjamin) → review+
Attached patch Part1 v2 Interface-changes.patch (obsolete) — Splinter Review
Change the IID of nsIFile as the interface has been changed.
Attachment #8358283 - Attachment is obsolete: true
Attachment #8373109 - Flags: review+
Attached patch Part2 v2 Unix-patch.patch (obsolete) — Splinter Review
removing the adjustment behavior at the end
Attachment #8358285 - Attachment is obsolete: true
Attachment #8373110 - Flags: review+
Attached patch Part3 v2 Windows-patch.patch (obsolete) — Splinter Review
remove the adjustment behavior at the end
Attachment #8358286 - Attachment is obsolete: true
Attachment #8373113 - Flags: review+
Attachment #8358287 - Attachment is obsolete: true
Attachment #8373118 - Flags: review+
Attached patch Part1 v2 Interface-changes.patch (obsolete) — Splinter Review
Fix a typo.
Attachment #8373109 - Attachment is obsolete: true
Update the test file.

https://tbpl.mozilla.org/?tree=Try&rev=2e98865d2eff
Attachment #8373118 - Attachment is obsolete: true
Comment on attachment 8373126 [details] [diff] [review]
Part1 v2 Interface-changes.patch

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

::: xpcom/io/nsIFile.idl
@@ +188,5 @@
> +     *  (NS_ERROR_FILE_ACCESS_DENIED).
> +     *  This object will still point to the old location after renaming.
> +     */
> +    void rename(in nsIFile newParentDir, in AString newName);
> +    [noscript] void renameNative(in nsIFile newParentDir, in ACString newName);

Do you really need renameNative()? Please do not add this unless it is absolutely required. People tend to call NS_ConvertUTF16ToUTF8 casually and break the code on non-Western Windows.

Of course you can add nsLocalFileUnix::RenameNative() internally, but please do not expose it.
I don't need renameNative (In reply to Masatoshi Kimura [:emk] from comment #24)
> Comment on attachment 8373126 [details] [diff] [review]
> Part1 v2 Interface-changes.patch
> 
> Review of attachment 8373126 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: xpcom/io/nsIFile.idl
> @@ +188,5 @@
> > +     *  (NS_ERROR_FILE_ACCESS_DENIED).
> > +     *  This object will still point to the old location after renaming.
> > +     */
> > +    void rename(in nsIFile newParentDir, in AString newName);
> > +    [noscript] void renameNative(in nsIFile newParentDir, in ACString newName);
> 
> Do you really need renameNative()? Please do not add this unless it is
> absolutely required. People tend to call NS_ConvertUTF16ToUTF8 casually and
> break the code on non-Western Windows.
I don't need renameNative. I add it just because other methods, such as |moveTo|, |copyToFollowingLinks| 
have native methods. So I should remove the renameNative?
Flags: needinfo?(VYV03354)
(In reply to Yuan Xulei [:yxl] from comment #25)
> I don't need renameNative. I add it just because other methods, such as
> |moveTo|, |copyToFollowingLinks| 
> have native methods. So I should remove the renameNative?

I would like to remove all |*Native| variants in future. Please do not add any new |*Native| variants.
By the way, |renameTo| would be the better name for consistency to other methods.
Flags: needinfo?(VYV03354)
Attached patch Part1 v3 Interface-changes.patch (obsolete) — Splinter Review
remove the |renameNative| method.
Attachment #8373126 - Attachment is obsolete: true
Attachment #8373180 - Flags: review?(VYV03354)
Attached patch Part2 v2 Unix-patch.patch (obsolete) — Splinter Review
Rename "rename" method to "renameTo" and remove "renameNative" method.
Attachment #8373110 - Attachment is obsolete: true
Attachment #8373181 - Flags: review?(VYV03354)
Attached patch Part3 v3 Windows-patch.patch (obsolete) — Splinter Review
Rename "rename" method to "renameTo" and remove "renameNative" method.
Attachment #8373113 - Attachment is obsolete: true
Rename "rename" method to "renameTo".
Attachment #8373136 - Attachment is obsolete: true
Attachment #8373184 - Flags: review?(VYV03354)
Attachment #8373182 - Flags: review?(VYV03354)
Comment on attachment 8373180 [details] [diff] [review]
Part1 v3 Interface-changes.patch

Looks good to me, but an official XPCOM peer should do the final call.
Attachment #8373180 - Flags: review?(benjamin)
Attachment #8373180 - Flags: review?(VYV03354)
Attachment #8373180 - Flags: feedback+
Attachment #8373181 - Flags: review?(benjamin)
Attachment #8373181 - Flags: review?(VYV03354)
Attachment #8373181 - Flags: feedback+
Comment on attachment 8373182 [details] [diff] [review]
Part3 v3 Windows-patch.patch

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

::: xpcom/io/nsLocalFileWin.h
@@ +92,5 @@
>      nsresult CopySingleFile(nsIFile *source, nsIFile* dest,
>                              const nsAString &newName,
>                              bool followSymlinks, bool move,
> +                            bool skipNtfsAclReset = false,
> +                            bool renameOnly = false);

Could you make these gross boolean params to a uint32_t options?
Attachment #8373182 - Flags: review?(benjamin)
Attachment #8373182 - Flags: review?(VYV03354)
Attachment #8373182 - Flags: feedback+
Attachment #8373184 - Flags: review?(benjamin)
Attachment #8373184 - Flags: review?(VYV03354)
Attachment #8373184 - Flags: feedback+
Attached patch Part3 v4 Windows-patch.patch (obsolete) — Splinter Review
Make the boolean params of CopySingleFile and CopyMove to a uint32_t options.
Attachment #8373182 - Attachment is obsolete: true
Attachment #8373182 - Flags: review?(benjamin)
Attachment #8373232 - Flags: review?(benjamin)
Attachment #8373232 - Flags: feedback?(VYV03354)
Comment on attachment 8373232 [details] [diff] [review]
Part3 v4 Windows-patch.patch

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

::: xpcom/io/nsLocalFileWin.cpp
@@ +1787,3 @@
>      nsAutoString filePath;
>  
> +    bool move = (options & Move) || (options & Rename);

options & (Move | Rename)

@@ +1867,5 @@
>      }
>  
>      if (!copyOK)  // CopyFileEx and MoveFileEx return zero at failure.
>          rv = ConvertWinError(GetLastError());
> +    else if (move && (options & SkipNtfsAclReset))

!(options & SkipNtfsAclReset)

@@ +2201,5 @@
> +      return NS_ERROR_FILE_DESTINATION_NOT_DIR;
> +    }
> +  }
> +
> +  uint32_t options = Move;

Rename
(I can't believe it passed the test.)
Attachment #8373232 - Flags: feedback?(VYV03354) → feedback-
Attachment #8373180 - Flags: review?(benjamin) → review+
Attached patch Part3 v5 Windows-patch.patch (obsolete) — Splinter Review
Fix issues listed in comment 34.
Attachment #8373232 - Attachment is obsolete: true
Attachment #8373232 - Flags: review?(benjamin)
Push to try to have a test before review:

https://tbpl.mozilla.org/?tree=Try&rev=aa783ce96f71
Attachment #8373798 - Flags: review?(benjamin)
Attachment #8373798 - Flags: feedback?(VYV03354)
Attached patch Part2 diff between v4 and v5 (obsolete) — Splinter Review
This diff shows the new changes I made in |Part3 v5 Windows-patch.patch| to address comment 34.
@emk, thanks for your help:-).
Summary: Add rename to nsIFile → Add renameTo to nsIFile
Comment on attachment 8373181 [details] [diff] [review]
Part2 v2 Unix-patch.patch

I don't think I need to re-review the rest.
Attachment #8373181 - Flags: review?(benjamin)
Attachment #8373181 - Flags: review+
rebased.
Attachment #8373180 - Attachment is obsolete: true
Attachment #8374659 - Flags: review+
rebased
Attachment #8373181 - Attachment is obsolete: true
Attachment #8374660 - Flags: review+
rebased
Attachment #8373798 - Attachment is obsolete: true
Attachment #8373798 - Flags: review?(benjamin)
Attachment #8374662 - Flags: review+
rebased
Attachment #8373184 - Attachment is obsolete: true
Attachment #8373184 - Flags: review?(benjamin)
Attachment #8374663 - Flags: review+
Attachment #8373874 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.