1284987 - Update the Blink/Webkit FileSystem API to the latest spec

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Comment 1

[Andrea Marchesini [:baku]]

Attachment: patch 1 - Remove DOMFileSystem, RootDirectoryEntry and RootDirectoryReader

Comment 2

[Andrea Marchesini [:baku]]

Attachment: patch 2 - FileError

Comment 3

[Andrea Marchesini [:baku]]

Attachment: part 3 - Rename FileSystemFlags to Flags

Comment 4

[Andrea Marchesini [:baku]]

Attachment: patch 2 - FileError and remove CreateWriter

Comment 5

[Andrea Marchesini [:baku]]

Attachment: patch 2 - FileError and remove CreateWriter and friends

Comment 6

[Andrea Marchesini [:baku]]

Attachment: part 4 - USVStrings and callbacks

Comment 7

[Andrea Marchesini [:baku]]

Attachment: part 1 - Remove DOMFileSystem, RootDirectoryEntry and RootDirectoryReader

Comment 8

[Andrea Marchesini [:baku]]

Attachment: part 2 - FileError and remove CreateWriter and friends

Comment 9

[Andrea Marchesini [:baku]]

Attachment: part 5 - getParent()

This patch is about Entry.getParent(). It doesn't return the correct parent in case we do getFile('subdir/something'). This is covered by a separate patch.

Comment 10

[Andrea Marchesini [:baku]]

Attachment: part 6 - getParent nested

Comment 11

[Andrea Marchesini [:baku]]

Attachment: part 7 - DataTransferItem.webkitGetAsEntry()

Comment 12

[Andrea Marchesini [:baku]]

Attachment: part 8 - Rename DOMFileSystem to Entries.webidl

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8768561 [details] [diff] [review]
part 4 - USVStrings and callbacks

> 
>-callback interface EntryCallback {
>-    void handleEvent(Entry entry);
>-};
>-
>-callback interface VoidCallback {
>-    void handleEvent();
>-};
>+callback EntryCallback = void (Entry entry);
....

>-callback interface EntriesCallback {
>-    void handleEvent(sequence<Entry> entries);
>-};
>+callback EntriesCallback = void (sequence<Entry> entries);
> 
>-callback interface ErrorCallback {
>-    void handleEvent(FileError error);
>-};
>+callback ErrorCallback = void (FileError err);
blink has callback interfaces, not just callbacks. So I see no reason for this change.
 Please file a spec bug, or does blink not really support these
as callback interfaces?




>+++ b/dom/webidl/HTMLCanvasElement.webidl
>@@ -22,17 +22,17 @@ interface HTMLCanvasElement : HTMLElemen
> 
>   [Throws]
>   nsISupports? getContext(DOMString contextId, optional any contextOptions = null);
> 
>   [Throws]
>   DOMString toDataURL(optional DOMString type = "",
>                       optional any encoderOptions);
>   [Throws]
>-  void toBlob(FileCallback _callback,
>+  void toBlob(BlobCallback _callback,
>               optional DOMString type = "",
>               optional any encoderOptions);
> };
> 
> // Mozilla specific bits
> partial interface HTMLCanvasElement {
>   [Pure, SetterThrows]
>            attribute boolean mozOpaque;
>@@ -60,9 +60,9 @@ interface MozCanvasPrintState
>   readonly attribute nsISupports context;
> 
>   // To be called when rendering to the context is done.
>   void done();
> };
> 
> callback PrintCallback = void(MozCanvasPrintState ctx);
> 
>-callback FileCallback = void(Blob file);
>+callback BlobCallback = void (Blob? blob);
I have no idea why this change is being done in this bug but fine.




Since we're going to ship this webkit-blink nonsense API soon, better to make it compatible with blink, and not
some spec-ish. Would be of course good to get the spec to be compatible with blink, and then we could rely on the spec.

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

FWIW, I just pinged jsbell about the callback interface issue in #whatwg

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8768660 [details] [diff] [review]
part 1 - Remove DOMFileSystem, RootDirectoryEntry and RootDirectoryReader

>+++ b/dom/webidl/DOMFileSystem.webidl
>@@ -9,27 +9,16 @@ interface Entry {
>     readonly attribute boolean isFile;
>     readonly attribute boolean isDirectory;
> 
>     [GetterThrows]
>     readonly attribute DOMString name;
> 
>     [GetterThrows]
>     readonly attribute DOMString fullPath;
>-
>-    readonly attribute DOMFileSystem filesystem;
We've been told Edge will have filesystem and since blink has it too, why we should remove it?

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8768662 [details] [diff] [review]
part 2 - FileError and remove CreateWriter and friends

MS said they implemented createWriter. I see no reason to have different subset than what they have.

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8768681 [details] [diff] [review]
part 5 - getParent()

The spec has been updated now and callback are callback interfaces so this needs some minor changes related to callback calling.
Looks like blink does have getParent so the new method is fine.

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8768684 [details] [diff] [review]
part 6 - getParent nested

I'm lost why this is needed. The previous patch added link to parent. Why that can't be reused.
And in
 GetEntryHelper::GetEntryHelper(DirectoryEntry* aParentEntry,
+                               Directory* aDirectory,
+                               nsTArray<nsString>& aParts,
What all those arguments mean?

Please explain what this patch is doing.

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8768734 [details] [diff] [review]
part 8 - Rename DOMFileSystem to Entries.webidl

Given that we shouldn't remove DOMFileSystem, I don't think we should do this.

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8768732 [details] [diff] [review]
part 7 - DataTransferItem.webkitGetAsEntry()

So this isn't just about the spec but part of the API we must have implemented to ship the webkit stuff.
Could you possibly move this patch to a separate bug?

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8768734 [details] [diff] [review]
part 8 - Rename DOMFileSystem to Entries.webidl

and DOMFileSystem is nicer filename for this all too than Entries, which is very generic.

Comment 22

[Andrea Marchesini [:baku]]

Attachment: part 1 - USVString

Comment 23

[Andrea Marchesini [:baku]]

Attachment: part 2 - GetParent - no recursive

Comment 24

[Andrea Marchesini [:baku]]

Attachment: part 3 - getFile - recursive

Comment 25

[Andrea Marchesini [:baku]]

Attachment: part 4 - DOMException

Comment 26

[Andrea Marchesini [:baku]]

Attachment: part 5 - remove old methods

Comment 27

[Andrea Marchesini [:baku]]

Attachment: part 6 - BlobCallback

Comment 28

[Olli Pettay [:smaug][[e-mail]]]

Why can we remove removeRecursively? It was in the interfaces we got from MS. Does Edge have it?

Comment 29

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8806692 [details] [diff] [review]
part 1 - USVString

I don't understand why the spec uses USVString, but shouldn't really matter here.

Comment 30

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8806696 [details] [diff] [review]
part 5 - remove old methods

What do other browsers do there? Does blink throw an exception? Does Edge have this method. Can't review this without some more information.

Comment 31

[Andrea Marchesini [:baku]]

> What do other browsers do there? Does blink throw an exception? Does Edge
> have this method. Can't review this without some more information.

At the moment, blink is not implementing the Entries API. This means that they still have this method as many others we didn't implement. About Edge, I don't know. I asked on github when Chrome will implement this API completely and I'm waiting for an answer.

I'm OK with removing this method in a follow up.

Comment 32

[Olli Pettay [:smaug][[e-mail]]]

Web pages rely on the API blink implements, so we must not break that.

I tested in Edge and they don't seem to have the method.

Comment 33

[Olli Pettay [:smaug][[e-mail]]]

(modified a test I found and print to console whether the method exists http://mozilla.pettay.fi/moztests/dnd/Draganddropafolder.htm)

Comment 34

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8806693 [details] [diff] [review]
part 2 - GetParent - no recursive

 
>+function test_fileEntry_getParent() {
>+  fileEntry.getParent(function(entry) {
>+    ok(false, "This is wrong.");
>+  }, function() {
>+    ok(true, "Top level FileEntry doesn't have a parent.");
>+    next();
>+  });
>+}

I don't understand this. Why getParent fails here?

>+function test_directoryEntry_getParent() {
>+  directoryEntry.getParent(function(entry) {
>+    ok(false, "This is wrong.");
>+  }, function() {
>+    ok(true, "Top level DirectoryEntry doesn't have a parent.");
>+    next();
>+  });
>+}
>+
It should return the top level directory itself, so something is wrong here.

Comment 35

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8806694 [details] [diff] [review]
part 3 - getFile - recursive

I don't understand what this patch is about.
Why we need anything recursive?

Comment 36

[Andrea Marchesini [:baku]]

> I don't understand what this patch is about.
> Why we need anything recursive?

because you can do: entry.getFile("foo/bar/a/file.txt", success); and when we call success(entry), you must be able to do: entry.getParent().

Comment 37

[Andrea Marchesini [:baku]]

Attachment: part 2 - GetParent - no recursive

Comment 38

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d3d7be05ed66
Entries API - part 1 - DOMString to USVString, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/aed436c3876f
Entries API - part 2 - FileSystemEntry.getParent, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/78cd683522c2
Entries API - part 3 - FileSystemEntry.getParent recursion, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/508a6a6fb534
Entries API - part 4 - Use of DOMException, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/89b07c9ea3ce
Entries API - part 5 - Get rid of remove methods, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/552ad078563f
Entries API - part 6 - BlobCallback renamed, r=smaug

Comment 39

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/d3d7be05ed66
https://hg.mozilla.org/mozilla-central/rev/aed436c3876f
https://hg.mozilla.org/mozilla-central/rev/78cd683522c2
https://hg.mozilla.org/mozilla-central/rev/508a6a6fb534
https://hg.mozilla.org/mozilla-central/rev/89b07c9ea3ce
https://hg.mozilla.org/mozilla-central/rev/552ad078563f

Comment 40

[Chris Mills [:cmills]]

I have gone through the current documentation and made the changes actioned in this bug.

1. DOMString -> USVString changes:

https://developer.mozilla.org/en-US/docs/Web/API/File/webkitRelativePath
https://developer.mozilla.org/en-US/docs/Web/API/FileSystem
https://developer.mozilla.org/en-US/docs/Web/API/FileSystem/name
https://developer.mozilla.org/en-US/docs/Web/API/FileSystemDirectoryEntry/getFile
https://developer.mozilla.org/en-US/docs/Web/API/FileSystemDirectoryEntry/getDirectory
https://developer.mozilla.org/en-US/docs/Web/API/FileSystemEntry
https://developer.mozilla.org/en-US/docs/Web/API/FileSystemEntry/name

2. DOMError -> DOMException change on error callback — I found mentions of it on the following pages, and changed it:

https://developer.mozilla.org/en-US/docs/Web/API/FileSystemEntry
https://developer.mozilla.org/en-US/docs/Web/API/FileSystemDirectoryEntry

3. Removal of removeRecursively():

https://developer.mozilla.org/en-US/docs/Web/API/FileSystemDirectoryEntry
https://developer.mozilla.org/en-US/docs/Web/API/FileSystemDirectoryEntry/removeRecursively

4. Blob to File changes in FileSystemFileEntry — I've not made any changesa s a result of this; in the file() page we talk about the callbacks quite generally and talk about the items as files already. The example here might need updating though — we create a Blob object and write it to a log file:

https://developer.mozilla.org/en-US/docs/Web/API/FileSystemFileEntry

thoughts?

5. toBlob() changes — this is documented here, but I'm not sure what changes need to be made. Is this all Moz-only functionality, and is there a spec or description I can look at?

https://developer.mozilla.org/en-US/docs/Web/API/HTMLCanvasElement/toBlob

6. I also added a note to the Fx52 release notes:

https://developer.mozilla.org/en-US/Firefox/Releases/52#DOM_HTML_DOM

Could you give me a quick tech review? Thanks!

Comment 41

[Andrea Marchesini [:baku]]

> 3. Removal of removeRecursively():

This has been removed completely. Maybe we should remove it from the documentation too?
 
> https://developer.mozilla.org/en-US/docs/Web/API/FileSystemFileEntry

1. remove CreateWriter - this test is not valid.
2. we could have a test for file(), but maybe we can just avoid having tests.

> 5. toBlob() changes — this is documented here, but I'm not sure what changes

The change is just internal. You don't need to change anything here.

> https://developer.mozilla.org/en-US/Firefox/Releases/52#DOM_HTML_DOM

Cool!

We should monitor what chrome does and when they update their implementation, we can have the 'final' documentation where remove()/moveTo()/copyTo()/and so on are gone.

Comment 42

[Chris Mills [:cmills]]

(In reply to Andrea Marchesini [:baku] from comment #41)
> > 3. Removal of removeRecursively():
> 
> This has been removed completely. Maybe we should remove it from the
> documentation too?
>  
> > https://developer.mozilla.org/en-US/docs/Web/API/FileSystemFileEntry
> 
> 1. remove CreateWriter - this test is not valid.

So for removeRecursively and createWriter - I see they've been removed from the spec, but can we assume they've been removed from all browsers? Until then, we should just leave them in and mark them as obsolete.

> 2. we could have a test for file(), but maybe we can just avoid having tests.
> 
> > 5. toBlob() changes — this is documented here, but I'm not sure what changes
> 
> The change is just internal. You don't need to change anything here.
> 
> > https://developer.mozilla.org/en-US/Firefox/Releases/52#DOM_HTML_DOM
> 
> Cool!
> 
> We should monitor what chrome does and when they update their
> implementation, we can have the 'final' documentation where
> remove()/moveTo()/copyTo()/and so on are gone.

Definitely. We'll keep our eyes open on this.