Closed
Bug 935883
Opened 11 years ago
Closed 6 years ago
[Filesystem API] Implement move methods for device storage.
Categories
(Core :: DOM: Device Interfaces, defect)
Core
DOM: Device Interfaces
Tracking
()
RESOLVED
INCOMPLETE
People
(Reporter: xyuan, Assigned: dxue)
References
(Depends on 1 open bug, Blocks 1 open bug)
Details
(Whiteboard: [DeviceStorageFS])
Attachments
(1 file, 3 obsolete files)
33.39 KB,
patch
|
Details | Diff | Splinter Review |
To be implemented: interface Directory { ... AbortableProgressPromise move((DOMString or File or Directory) path, (DOMString or Directory or DestinationDict) dest); ... } callback VoidAnyCallback = void (optional any value); interface AbortableProgressPromise : Promise { void abort(); AbortableProgressPromise progress(VoidAnyCallback callback); }; dictionary DestinationDict { Directory dir; DOMString name; };
Updated•11 years ago
|
Keywords: dev-doc-needed
Reporter | ||
Comment 1•10 years ago
|
||
If the source file or directory is moved within the same file system volume (disks or partitions). We just need a single operation and it is very fast. `AbortableProgressPromise` will only report the source as the progress value. However if the source is moved to a different volumes. We need to simulate the move by using the |copy| and |remove| functions. That is, it will copy the old file or directory to the new location, and then delete the old one. If the source is a directory, `AbortableProgressPromise` will report every file under the directory as the progress value. I use nsIFile (http://mxr.mozilla.org/mozilla-central/source/xpcom/io/nsIFile.idl) to implement "move". nsIFile doesn't provide a way to detect if a file or directory can be moved without copying. So I'm going to extend nsIFile with such feature.
Reporter | ||
Comment 2•10 years ago
|
||
I filed bug 956646 to add "rename" to nsIFile, which only allows to move a file within the same volume.
Reporter | ||
Comment 3•10 years ago
|
||
Discussion about abortable promise provided by annevk: https://twitter.com/jaffathecake/status/413240852333359105 for some discussion on promises that can be terminated.
Reporter | ||
Comment 4•10 years ago
|
||
Discussed with Jonas about AbortableProgressPromise: https://groups.google.com/d/msg/mozilla.dev.webapi/FMxxAwbdSCg/PB4ESj2NkDgJ He said we need something like this: AbortableProgressPromise is a subclass of Promise callback VoidAnyCallback = void (optional any value); interface AbortableProgressPromise : AbortablePromise { void abort(); AbortableProgressPromise progress(VoidAnyCallback); }; The progress() function always returns 'this'. I.e. x.progress() === x; This way you can write: doAsyncStuff().progress(...).then(...); or doAsyncStuff().then(...); Both do the same thing, except the first allows you to get callbacks to be informed about progress.
Reporter | ||
Comment 5•10 years ago
|
||
I'm going to implement AbortableProgressPromise by C++ with the following pattern for this bug. var directory = ...; // Abortable progress promise var promise = directory.move("a", "b"); promise.progress(function cbProgress(path) { console.log(path + ' was moved.'); }).then(function cbSuccess() { console.log('Finished moving file.'); }, function cbError(e) { console.log(' }); // AbortableProgressPromise inherits Promise function AbortableProgressPromise(abortable) : Promise { this._abortable = abortable; } AbortableProgressPromise.prototype.abort = function() { if (!this._abortable) { this._abortable.abort(); } }; AbortableProgressPromise.prototype.progress = function(callback) { this._progress = callback; }; AbortableProgressPromise.prototype._notify = function(val) { if (this._progress) { this._progress(val); } }; directory.move = function(src, dest) { var aborted = false; var promise = new AbortableProgressPromise(function abortable() { aborted = true; }); runInBackground(function() { // Assume "src" is a directory and there are two files under it and we want to // move src to a new directory "dest". nsIFile.move(src + "/file1", dest + "/file1"); promise._notify(src + "/file1"); if (aborted) { promise.reject('aborted'); return; } nsIFile.move(src + "/file2", dest + "/file2"); promise._notify(src + "/file2"); if (aborted) { promise.reject('aborted'); return; } nsIFile.move(src, dest); promise._notify(src); promise.resolve(); }); return promise; }; My friends, what are your thoughts? Does it make sense?
Flags: needinfo?(annevk)
Flags: needinfo?(amarchesini)
Yuan: Your proposal for implementation of the move function looks mostly right. AbortableProgressPromise doesn't look quite right though. The signature would be something like this: p = new AbortableProgressPromise(function(resolve, reject, progress) { ... }, function() { alert("promise aborted") }); So the first argument works just like with a normal promise. It's a function that is synchronously called by the constructor and which provides callbacks that can be used to reject or resolve the promise. But here it also provides a callback that can be used to notify about progress. The second argument is a callback that is called when p.abort() is called, unless the promise has already been resolved or rejected. Most likely the promise should automatically be rejected with a DOMException of type AbortError before the callback is called. I.e. p.abort() should first reject the promise and then call the callback. The implementation of the move function looks right other than the type of error that is used when the promise is rejected. We should use a DOMException of type AbortError. Does that sound ok?
Reporter | ||
Comment 7•10 years ago
|
||
Yes, quite clear. The patch for review is on the way... :-)
Updated•10 years ago
|
Flags: needinfo?(annevk)
Updated•10 years ago
|
Flags: needinfo?(amarchesini)
Comment 8•10 years ago
|
||
Yuan, have you done any progress on AbortableProgressPromise ?
Comment 9•10 years ago
|
||
I'm not sure AbortableProgressPromise is the direction we should go in here; in IRC chats with folks like Domenic, it seems that he thinks we shouldn't be able to have an abort on a promise, just failure. That said, there isn't an precedent here. If we have a good implementation, maybe we can argue the point. Jonas?
Reporter | ||
Comment 10•10 years ago
|
||
@Jan, not fully implemented yet. I had some higher priority work to do last month and will continue to work on this month.
Comment 11•10 years ago
|
||
(In reply to Yuan Xulei [:yxl] from comment #10) > @Jan, not fully implemented yet. I had some higher priority work to do last > month and will continue to work on this month. Ok, but see comment 9 My original intend was to update FileHandle in IndexedDB to use promises, but we've decided to not do that. We're going to just rename some objects, see bug 1006485.
Assignee | ||
Comment 12•10 years ago
|
||
Assignee: xyuan → dxue
Reporter | ||
Comment 13•10 years ago
|
||
(In reply to DongShengXue from comment #12) > Created attachment 8455130 [details] [diff] [review] > WIP: 0001.Filesystem API Implement move methods Push the patch to try server: https://tbpl.mozilla.org/?tree=Try&rev=4d454a19b298
Assignee | ||
Comment 14•10 years ago
|
||
Filesystem API Implement move methods base on AbortablePromise(v1).patch
Attachment #8455130 -
Attachment is obsolete: true
Assignee | ||
Comment 15•10 years ago
|
||
add test case and return NS_ERROR_ABORT when abort move.
Attachment #8455851 -
Attachment is obsolete: true
Assignee | ||
Comment 16•10 years ago
|
||
Change DOMFile to DOMFileImpl
Attachment #8455920 -
Attachment is obsolete: true
Updated•9 years ago
|
Whiteboard: [DeviceStorageFS]
Comment 17•8 years ago
|
||
Is this still being worked on? AbortablePromise is causing some annoying problems for moving promises into SpiderMonkey. We basically need to either reimplement it from scratch in SpiderMonkey or back it out until it's really needed (and then reimplement it from scratch in SpiderMonkey). Given the radio silence here for two years now, plus the fact that other UAs are pushing back on the concept of abortable promises, my vote is that we back it out, then reimplement it if it becomes necessary...
Flags: needinfo?(jonas)
I definitely think it's fine to get rid of abortable promises.
Flags: needinfo?(jonas)
Comment 19•8 years ago
|
||
Awesome. Filed bug 1242054.
Comment 20•6 years ago
|
||
Cleaning up Device Interfaces component, and mass-marking old FxOS bugs as incomplete. If any of these bugs are still valid, please let me know.
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → INCOMPLETE
Updated•6 years ago
|
Keywords: dev-doc-needed
You need to log in
before you can comment on or make changes to this bug.
Description
•