Closed
Bug 956646
Opened 10 years ago
Closed 10 years ago
Add renameTo to nsIFile
Categories
(Core :: XPCOM, defect)
Core
XPCOM
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);
Comment 1•10 years ago
|
||
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)
Assignee | ||
Comment 2•10 years ago
|
||
(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)
Assignee | ||
Comment 3•10 years ago
|
||
Assignee | ||
Comment 4•10 years ago
|
||
Assignee | ||
Comment 5•10 years ago
|
||
Assignee | ||
Comment 6•10 years ago
|
||
I don't know how to test moving file across volumes, so this test doesn't cover that :(
Assignee | ||
Comment 7•10 years ago
|
||
Assignee: nobody → xyuan
Attachment #8356453 -
Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8358283 -
Flags: review?(benjamin)
Assignee | ||
Comment 8•10 years ago
|
||
Attachment #8356455 -
Attachment is obsolete: true
Attachment #8358285 -
Flags: review?(benjamin)
Assignee | ||
Comment 9•10 years ago
|
||
Attachment #8356456 -
Attachment is obsolete: true
Attachment #8358286 -
Flags: review?(benjamin)
Assignee | ||
Comment 10•10 years ago
|
||
Attachment #8356458 -
Attachment is obsolete: true
Attachment #8358287 -
Flags: review?(benjamin)
Assignee | ||
Comment 11•10 years ago
|
||
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?
Assignee | ||
Comment 12•10 years ago
|
||
Try for all platforms: https://tbpl.mozilla.org/?tree=Try&rev=825096994765
Comment 13•10 years ago
|
||
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 14•10 years ago
|
||
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 15•10 years ago
|
||
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 16•10 years ago
|
||
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+
Updated•10 years ago
|
Attachment #8358287 -
Flags: review?(benjamin) → review+
Assignee | ||
Comment 17•10 years ago
|
||
Change the IID of nsIFile as the interface has been changed.
Attachment #8358283 -
Attachment is obsolete: true
Attachment #8373109 -
Flags: review+
Assignee | ||
Comment 18•10 years ago
|
||
removing the adjustment behavior at the end
Attachment #8358285 -
Attachment is obsolete: true
Attachment #8373110 -
Flags: review+
Assignee | ||
Comment 19•10 years ago
|
||
remove the adjustment behavior at the end
Attachment #8358286 -
Attachment is obsolete: true
Attachment #8373113 -
Flags: review+
Assignee | ||
Comment 20•10 years ago
|
||
Attachment #8358287 -
Attachment is obsolete: true
Attachment #8373118 -
Flags: review+
Assignee | ||
Comment 22•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=f4754943688d
Assignee | ||
Comment 23•10 years ago
|
||
Update the test file. https://tbpl.mozilla.org/?tree=Try&rev=2e98865d2eff
Attachment #8373118 -
Attachment is obsolete: true
Comment 24•10 years ago
|
||
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.
Assignee | ||
Comment 25•10 years ago
|
||
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)
Comment 26•10 years ago
|
||
(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)
Assignee | ||
Comment 27•10 years ago
|
||
remove the |renameNative| method.
Attachment #8373126 -
Attachment is obsolete: true
Attachment #8373180 -
Flags: review?(VYV03354)
Assignee | ||
Comment 28•10 years ago
|
||
Rename "rename" method to "renameTo" and remove "renameNative" method.
Attachment #8373110 -
Attachment is obsolete: true
Attachment #8373181 -
Flags: review?(VYV03354)
Assignee | ||
Comment 29•10 years ago
|
||
Rename "rename" method to "renameTo" and remove "renameNative" method.
Attachment #8373113 -
Attachment is obsolete: true
Assignee | ||
Comment 30•10 years ago
|
||
Rename "rename" method to "renameTo".
Attachment #8373136 -
Attachment is obsolete: true
Attachment #8373184 -
Flags: review?(VYV03354)
Assignee | ||
Updated•10 years ago
|
Attachment #8373182 -
Flags: review?(VYV03354)
Comment 31•10 years ago
|
||
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+
Updated•10 years ago
|
Attachment #8373181 -
Flags: review?(benjamin)
Attachment #8373181 -
Flags: review?(VYV03354)
Attachment #8373181 -
Flags: feedback+
Comment 32•10 years ago
|
||
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+
Updated•10 years ago
|
Attachment #8373184 -
Flags: review?(benjamin)
Attachment #8373184 -
Flags: review?(VYV03354)
Attachment #8373184 -
Flags: feedback+
Assignee | ||
Comment 33•10 years ago
|
||
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 34•10 years ago
|
||
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-
Updated•10 years ago
|
Attachment #8373180 -
Flags: review?(benjamin) → review+
Assignee | ||
Comment 35•10 years ago
|
||
Fix issues listed in comment 34.
Attachment #8373232 -
Attachment is obsolete: true
Attachment #8373232 -
Flags: review?(benjamin)
Assignee | ||
Comment 36•10 years ago
|
||
Push to try to have a test before review: https://tbpl.mozilla.org/?tree=Try&rev=aa783ce96f71
Assignee | ||
Updated•10 years ago
|
Attachment #8373798 -
Flags: review?(benjamin)
Attachment #8373798 -
Flags: feedback?(VYV03354)
Assignee | ||
Comment 37•10 years ago
|
||
This diff shows the new changes I made in |Part3 v5 Windows-patch.patch| to address comment 34.
Comment 38•10 years ago
|
||
Comment on attachment 8373798 [details] [diff] [review] Part3 v5 Windows-patch.patch Interdiff worked :) https://bugzilla.mozilla.org/attachment.cgi?oldid=8373232&action=interdiff&newid=8373798&headers=1
Attachment #8373798 -
Flags: feedback?(VYV03354) → feedback+
Assignee | ||
Comment 39•10 years ago
|
||
@emk, thanks for your help:-).
Summary: Add rename to nsIFile → Add renameTo to nsIFile
Comment 40•10 years ago
|
||
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)
Assignee | ||
Updated•10 years ago
|
Attachment #8373181 -
Flags: review+
Assignee | ||
Comment 41•10 years ago
|
||
rebased.
Attachment #8373180 -
Attachment is obsolete: true
Attachment #8374659 -
Flags: review+
Assignee | ||
Comment 42•10 years ago
|
||
rebased
Attachment #8373181 -
Attachment is obsolete: true
Attachment #8374660 -
Flags: review+
Assignee | ||
Comment 43•10 years ago
|
||
rebased
Attachment #8373798 -
Attachment is obsolete: true
Attachment #8373798 -
Flags: review?(benjamin)
Attachment #8374662 -
Flags: review+
Assignee | ||
Comment 44•10 years ago
|
||
rebased
Attachment #8373184 -
Attachment is obsolete: true
Attachment #8373184 -
Flags: review?(benjamin)
Attachment #8374663 -
Flags: review+
Assignee | ||
Updated•10 years ago
|
Attachment #8373874 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 45•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/c54489569226 https://hg.mozilla.org/integration/mozilla-inbound/rev/587d8a9a2773 https://hg.mozilla.org/integration/mozilla-inbound/rev/93e51e65dd7f https://hg.mozilla.org/integration/mozilla-inbound/rev/de496ba851d7
Flags: in-testsuite+
Keywords: checkin-needed
Comment 46•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/c54489569226 https://hg.mozilla.org/mozilla-central/rev/587d8a9a2773 https://hg.mozilla.org/mozilla-central/rev/93e51e65dd7f https://hg.mozilla.org/mozilla-central/rev/de496ba851d7
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Assignee | ||
Comment 47•10 years ago
|
||
MDN has been updated: https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/nsIFile#renameTo%28%29
Keywords: dev-doc-complete
You need to log in
before you can comment on or make changes to this bug.
Description
•