Update the Blink/Webkit FileSystem API to the latest spec

RESOLVED FIXED in Firefox 52

Status

()

defect
RESOLVED FIXED
3 years ago
4 months ago

People

(Reporter: baku, Assigned: baku)

Tracking

({dev-doc-complete})

50 Branch
mozilla52
Points:
---

Firefox Tracking Flags

(firefox50 affected, firefox52 fixed)

Details

()

Attachments

(6 attachments, 13 obsolete attachments)

5.59 KB, patch
smaug
: review+
Details | Diff | Splinter Review
10.28 KB, patch
smaug
: review+
Details | Diff | Splinter Review
2.30 KB, patch
smaug
: review+
Details | Diff | Splinter Review
3.06 KB, patch
smaug
: review+
Details | Diff | Splinter Review
11.04 KB, patch
smaug
: review+
Details | Diff | Splinter Review
36.61 KB, patch
smaug
: review+
Details | Diff | Splinter Review
Assignee

Description

3 years ago
No description provided.
Assignee

Comment 2

3 years ago
Posted patch patch 2 - FileError (obsolete) — Splinter Review
Attachment #8768530 - Flags: review?(bugs)
Assignee

Comment 3

3 years ago
Attachment #8768531 - Flags: review?(bugs)
Assignee

Comment 4

3 years ago
Attachment #8768530 - Attachment is obsolete: true
Attachment #8768530 - Flags: review?(bugs)
Attachment #8768538 - Flags: review?(bugs)
Assignee

Comment 5

3 years ago
Attachment #8768538 - Attachment is obsolete: true
Attachment #8768538 - Flags: review?(bugs)
Attachment #8768544 - Flags: review?(bugs)
Assignee

Comment 6

3 years ago
Attachment #8768561 - Flags: review?(bugs)
Assignee

Comment 7

3 years ago
Attachment #8768523 - Attachment is obsolete: true
Attachment #8768523 - Flags: review?(bugs)
Attachment #8768660 - Flags: review?(bugs)
Assignee

Comment 8

3 years ago
Attachment #8768544 - Attachment is obsolete: true
Attachment #8768544 - Flags: review?(bugs)
Attachment #8768662 - Flags: review?(bugs)
Assignee

Updated

3 years ago
Attachment #8768531 - Attachment description: patch 3 - Rename FileSystemFlags to Flags → part 3 - Rename FileSystemFlags to Flags
Assignee

Updated

3 years ago
Attachment #8768561 - Attachment description: patch 4 - USVStrings and callbacks → part 4 - USVStrings and callbacks
Assignee

Comment 9

3 years ago
Posted patch part 5 - getParent() (obsolete) — Splinter Review
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.
Attachment #8768681 - Flags: review?(bugs)
Assignee

Comment 10

3 years ago
Posted patch part 6 - getParent nested (obsolete) — Splinter Review
Attachment #8768684 - Flags: review?(bugs)
Assignee

Comment 11

3 years ago
Attachment #8768732 - Flags: review?(bugs)
Assignee

Comment 12

3 years ago
Attachment #8768734 - Flags: review?(bugs)
Attachment #8768531 - Flags: review?(bugs) → review+
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.
Attachment #8768561 - Flags: review?(bugs) → review-
FWIW, I just pinged jsbell about the callback interface issue in #whatwg
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?
Attachment #8768660 - Flags: review?(bugs) → review-
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.
Attachment #8768662 - Flags: review?(bugs) → review-
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.
Attachment #8768681 - Flags: review?(bugs) → review+
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.
Attachment #8768684 - Flags: review?(bugs)
Attachment #8768732 - Flags: review?(bugs) → review+
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 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 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.
Attachment #8768734 - Flags: review?(bugs) → review-
Assignee

Updated

3 years ago
Attachment #8768531 - Attachment is obsolete: true
Assignee

Updated

3 years ago
Attachment #8768561 - Attachment is obsolete: true
Assignee

Updated

3 years ago
Attachment #8768660 - Attachment is obsolete: true
Assignee

Updated

3 years ago
Attachment #8768662 - Attachment is obsolete: true
Assignee

Updated

3 years ago
Attachment #8768681 - Attachment is obsolete: true
Assignee

Updated

3 years ago
Attachment #8768684 - Attachment is obsolete: true
Assignee

Updated

3 years ago
Attachment #8768732 - Attachment is obsolete: true
Assignee

Updated

3 years ago
Attachment #8768734 - Attachment is obsolete: true
Assignee

Comment 22

3 years ago
Attachment #8806692 - Flags: review?(bugs)
Assignee

Comment 23

3 years ago
Attachment #8806693 - Flags: review?(bugs)
Assignee

Comment 24

3 years ago
Attachment #8806694 - Flags: review?(bugs)
Assignee

Comment 25

3 years ago
Attachment #8806695 - Flags: review?(bugs)
Assignee

Comment 26

3 years ago
Attachment #8806696 - Flags: review?(bugs)
Assignee

Comment 27

3 years ago
Attachment #8806697 - Flags: review?(bugs)
Why can we remove removeRecursively? It was in the interfaces we got from MS. Does Edge have it?
Flags: needinfo?(amarchesini)
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.
Attachment #8806692 - Flags: review?(bugs) → review+
Attachment #8806695 - Flags: review?(bugs) → review+
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.
Attachment #8806696 - Flags: review?(bugs)
Attachment #8806697 - Flags: review?(bugs) → review+
Assignee

Comment 31

3 years ago
> 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.
Flags: needinfo?(amarchesini)
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.
(modified a test I found and print to console whether the method exists http://mozilla.pettay.fi/moztests/dnd/Draganddropafolder.htm)
Attachment #8806696 - Flags: review+
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.
Attachment #8806693 - Flags: review?(bugs) → review-
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?
Attachment #8806694 - Flags: review?(bugs)
Assignee

Comment 36

3 years ago
> 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().
Assignee

Comment 37

3 years ago
Attachment #8806693 - Attachment is obsolete: true
Attachment #8806888 - Flags: review?(bugs)
Assignee

Updated

3 years ago
Attachment #8806694 - Flags: review?(bugs)
Attachment #8806694 - Attachment description: part 3 - getParent - recursive → part 3 - getFile - recursive
Attachment #8806888 - Flags: review?(bugs) → review+
Attachment #8806694 - Flags: review?(bugs) → review+

Comment 38

3 years ago
Pushed by amarchesini@mozilla.com:
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
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!
Flags: needinfo?(amarchesini)
Assignee

Comment 41

3 years ago
> 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.
Flags: needinfo?(amarchesini)
(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.
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.