Closed Bug 935883 Opened 6 years ago Closed 2 years ago

[Filesystem API] Implement move methods for device storage.

Categories

(Core :: DOM: Device Interfaces, defect)

defect
Not set

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)

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;
};
Depends on: 910412
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.
Depends on: 956646
I filed bug 956646 to add "rename" to nsIFile, which only allows to move a file within the same volume.
Discussion about abortable promise provided by annevk:
  https://twitter.com/jaffathecake/status/413240852333359105 for some discussion on promises that can be terminated.
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.
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)
Blocks: 910387
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?
Yes, quite clear. The patch for review is on the way... :-)
Flags: needinfo?(annevk)
Flags: needinfo?(amarchesini)
Yuan, have you done any progress on AbortableProgressPromise ?
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?
@Jan, not fully implemented yet. I had some higher priority work to do last month and will continue to work on this month.
(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.
Depends on: 899557
Depends on: 1035060
Assignee: xyuan → dxue
(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
Attached patch WIP: 0001.move methods V2 (obsolete) — Splinter Review
Filesystem API Implement move methods base on AbortablePromise(v1).patch
Attachment #8455130 - Attachment is obsolete: true
Attached patch WIP: 0001.move methods V3 (obsolete) — Splinter Review
add test case and return NS_ERROR_ABORT when abort move.
Attachment #8455851 - Attachment is obsolete: true
Depends on: 1038557
Change DOMFile to DOMFileImpl
Attachment #8455920 - Attachment is obsolete: true
Whiteboard: [DeviceStorageFS]
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)
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: 2 years ago
Resolution: --- → INCOMPLETE
You need to log in before you can comment on or make changes to this bug.