1258490 - Consider to implement file.webkitrelativepath

Status: RESOLVED FIXED

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

Description

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

MS is implementing it too, before implementing upload proposal.
I filed https://github.com/WICG/directory-upload/issues/30 to get it spec'ed.

Comment 1

[Andrea Marchesini [:baku]]

Attachment: webkitreleativepath.patch

Comment 2

[Jonathan Watt [:jwatt]]

Can we hold off on this until we figure out exactly what it is that MS is/has implemented in Edge?

Also, why remove .path?

Comment 3

[Andrea Marchesini [:baku]]

Because it's not used at all anywhere.
We had a setter but it has gone away releases ago and now we have just a getter that returns "" all the time.

Comment 4

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

(In reply to Jonathan Watt [:jwatt] from comment #2)
> Can we hold off on this until we figure out exactly what it is that MS
> is/has implemented in Edge?
Sure. That is what I asked yesterday from MS.
Perhaps you can reach out Ali. He should know more about it.

Comment 5

[Jonathan Watt [:jwatt]]

I've sent Ali an email. Needinfo'ing myself to remember to comment here when he replies.

Comment 6

[Mike Taylor [:miketaylr]]

(I was planning on trying to figure this out next week at the Edge Web Summit in SF)

Testing with http://html5-demos.appspot.com/static/gdd11-modern-web-apps/demos/dir-upload.html, Edge currently supports both webkitdirectory on input elements and webkitrelativepath on File objects.

Comment 7

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

Mike, could you test whether it supports also DataTransferItem.webkitGetAsEntry and such?

Comment 8

[Jonathan Watt [:jwatt]]

Specifically that it contains items of type DirectoryEntry.

Comment 9

[Jonathan Watt [:jwatt]]

So the webkitdirectory and webkitrelativepath stuff apparently shipped back in the November 2015 update for Windows 10. DataTransferItem.webkitGetAsEntry/DirectoryEntry/FileEntry etc. is in testing and will ship later this year. I guess they're doing the DnD stuff because Google Drive supports dragging a directory straight from your file browser into the Google Drive main window.

Comment 10

[Mike Taylor [:miketaylr]]

(Looks like Jonathan got some better info, but here's my comment anyways)

After poking around:

DataTransferItem.prototype.webkitGetAsEntry isn't exposed in my build of Edge (25.10586.0.0)
"webkitEntries" in document.createElement("input") is false.

window.requestFileSystem and window.resolveLocalFileSystemURL are present (unprefixed?)
But window.TEMPORARY and window.PERSISTENT are not there, so I can't test if window.requestFileSystem actually works.

(I also see they've documented their support for webkitdirectory here: https://msdn.microsoft.com/en-us/library/mt574730(v=vs.85).aspx and webkitRelativePath here: https://msdn.microsoft.com/en-us/library/mt488515(v=vs.85).aspx)

Comment 11

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

Comment on attachment 8733458 [details] [diff] [review]
webkitreleativepath.patch


>+++ b/dom/webidl/File.webidl
>@@ -40,18 +40,15 @@ dictionary ChromeFilePropertyBag : FileP
> 
> // Mozilla extensions
> partial interface File {
> 
>   [GetterThrows]
>   readonly attribute Date lastModifiedDate;
> 
>   [GetterThrows, ChromeOnly]
>-  readonly attribute DOMString path;
>-
>-  [GetterThrows, ChromeOnly]
>   readonly attribute DOMString mozFullPath;
> };
> 
> // WebKit extension
> partial interface File {
>   readonly attribute DOMString webkitRelativePath;
> };
This must be wrong patch since this shows we have webkitRelativePath already in File (and even without any Pref. I think it should be added behind some Pref, possibly different Pref than upload directory stuff.)

Clearing request.

Comment 12

[Andrea Marchesini [:baku]]

Attachment: webkitreleativepath.patch

Comment 13

[Jonathan Watt [:jwatt]]

I thought you were going to have File.path for symmetry with Directory.path, and just make File.webkitrelativepath an alias in the WebIDL, or did I misunderstand?

Comment 14

[Andrea Marchesini [:baku]]

Sure, I just reupload the same patch because a part of it was missing.
I'm going to do this alias in a separate patch, same bug.

Comment 15

[Jonathan Watt [:jwatt]]

So all the internal stuff will be 'path' rather than 'webkitrelativepath' in the actual patch that lands?

Comment 16

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

yeah, I think I'd like to see a patch using path for all the internal stuff, and then just in .webidl have some "BinaryName" (or whatever it is called) to map webkitrelativepath to path or some such.
I think .path could still be [ChromeOnly].
and webkitrelativepath should be behind a pref. I think we should file a bug to get .path to some spec.
(and perhaps we want to use it for form submission? depends on what other browsers do with webkitrelativepath and form submission. We after all do have the code there already.)

Comment 17

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

Comment on attachment 8733593 [details] [diff] [review]
webkitreleativepath.patch

>-File::GetPath(nsAString& aPath, ErrorResult& aRv)
>+File::GetWebkitRelativePath(nsAString& aPath) const
> {
>-  mImpl->GetPath(aPath, aRv);
>+  mImpl->GetWebkitRelativePath(aPath);
>+}
So I would try to avoid adding webkit prefixed stuff to C++, but use BinaryName in webidl for that.
And add perhaps also [ChromeOnly] .path or .relativePath.


>       MOZ_ASSERT(mTargetData[i].mType == Directory::FileOrDirectoryPath::eFilePath);
> 
>       RefPtr<File> file =
>         File::CreateFromFile(mFileSystem->GetParentObject(), path);
>       MOZ_ASSERT(file);
> 
>+      nsAutoString filePath;
>+      filePath.Assign(directoryPath);
>+      if (!directoryPath.EqualsLiteral(FILESYSTEM_DOM_PATH_SEPARATOR_LITERAL)) {
Is it guaranteed that filePath doesn't end with '/' here?

>+        filePath.AppendLiteral(FILESYSTEM_DOM_PATH_SEPARATOR_LITERAL);
>+      }
>+
>+      nsAutoString name;
>+      file->GetName(name);
>+      filePath.Append(name);
>+      file->SetWebkitRelativePath(filePath);
>+
We have somewhat similar code in few place.
Could File perhaps have some helper which take directory and name and then does has the right assertions and sets relativepath




>   nsAutoCString filename;
>   nsAutoCString contentType;
>   nsCOMPtr<nsIInputStream> fileStream;
> 
>   if (aBlob) {
>     nsAutoString filename16;
>     RetrieveFileName(aBlob, filename16);
> 
>-    ErrorResult error;
>-    nsAutoString filepath16;
>-    RefPtr<File> file = aBlob->ToFile();
>-    if (file) {
>-      file->GetPath(filepath16, error);
>-      if (NS_WARN_IF(error.Failed())) {
>-        return error.StealNSResult();
>-      }
>-    }
>-
>-    if (!filepath16.IsEmpty()) {
>-      // File.path includes trailing "/"
>-      filename16 = filepath16 + filename16;
>-    }
>-
So in case webkitRelativePath is non-empty, do the browsers supporting it actually pass it in form submission?
If so, we don't actually want to remove this code but access the path

>+partial interface File {
>+  readonly attribute DOMString webkitRelativePath;
So this should be behind some pref and use BinaryName

Comment 18

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

Comment on attachment 8733593 [details] [diff] [review]
webkitreleativepath.patch


>@@ -31,30 +36,37 @@ function test_getFilesAndDirectories(aDi
>         ok (data[i] instanceof File || data[i] instanceof Directory, "Just Files or Directories: " + data[i].name);
>         if (data[i] instanceof Directory) {
>           isnot(data[i].name, '/', "Subdirectory should be called with the leafname");
>           is(data[i].path, '/' + data[i].name, "Subdirectory path should be called '/' + leafname");
>           if (aRecursive) {
>             promises.push(checkSubDir(data[i]));
>           }
>         }
>+
>+        if (data[i] instanceof File) {
>+          is(data[i].webkitRelativePath, '/' + data[i].name, "File.webkitRelativePath should be called '/' + file.name");
Why '/' prefix in the relativePath? I'm not seeing such in other browsers. They seem to report directory/filename.
And if there is no directory, webkitRelativePath is "", at least in Chrome/linux

Comment 19

[Jonathan Watt [:jwatt]]

(In reply to Olli Pettay [:smaug] from comment #16)
> I think we should file a bug to get .path to some spec.

I thought Arun had already added 'path' to File and Directory:

https://w3c.github.io/FileAPI/#dfn-file
https://w3c.github.io/filesystem-api/#idl-def-Directory

but that doesn't seem to be the case.

> (and perhaps we want to use it for form submission? depends on what other
> browsers do with webkitrelativepath and form submission. We after all do
> have the code there already.)

Unless I removed that in bug 1173390 by mistake this should pretty much just work. Chrome includes the path in the "filename" field so that's what we did. So the form data should contain something like:

  Content-Disposition: form-data; name="id-of-input"; filename="path/to/file.txt"

Comment 20

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

Because we never set the path in File objects, it doesn't help that they have currently GetPath which is called during submission. But sounds like we should make that to actually work.

Comment 21

[Jonathan Watt [:jwatt]]

(In reply to Olli Pettay [:smaug] from comment #17)
> So in case webkitRelativePath is non-empty, do the browsers supporting it
> actually pass it in form submission?
> If so, we don't actually want to remove this code but access the path

Ah, yes this. Please include the path.

Comment 22

[Arun Ranganathan]

(In reply to Jonathan Watt [:jwatt] from comment #19)
> (In reply to Olli Pettay [:smaug] from comment #16)
> > I think we should file a bug to get .path to some spec.
> 
> I thought Arun had already added 'path' to File and Directory:
> 
> https://w3c.github.io/FileAPI/#dfn-file


I think it should be added to File.


> https://w3c.github.io/filesystem-api/#idl-def-Directory

It should only be added to Directory if we're going to implement this. Perhaps a better spec to add it to is https://github.com/WICG/directory-upload .

> 
> but that doesn't seem to be the case.


Let's log issues against the relevant specs. I'm not sure I'll be editing going forward (change of professional affiliation, amongst other things).

Comment 23

[Andrea Marchesini [:baku]]

Attachment: relativePath.patch

Comment 24

[Andrea Marchesini [:baku]]

Attachment: patch 1 - webkitRelativePath

Comment 25

[Andrea Marchesini [:baku]]

Attachment: patch 1 - webkitRelativePath

Comment 26

[Andrea Marchesini [:baku]]

Attachment: 1258490_relativePath.patch

Comment 27

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

Comment on attachment 8741340 [details] [diff] [review]
1258490_relativePath.patch

>+    nsAutoString domPath;
>+    domPath.Assign(aDOMPath);
>+    if (!aDOMPath.EqualsLiteral(FILESYSTEM_DOM_PATH_SEPARATOR_LITERAL)) {
>+      domPath.AppendLiteral(FILESYSTEM_DOM_PATH_SEPARATOR_LITERAL);
>+    }
This could use a comment that this is a special case for unix root

>+++ b/dom/webidl/File.webidl
...
>+  [BinaryName="path"]
>+  readonly attribute DOMString webkitRelativePath;
So, this property needs to go behind some pref which we use also for webkitdirectory.
(and need to do the pref check in workers-safe way)


+++ b/dom/html/nsFormSubmission.cpp
@@ -459,31 +459,28 @@ nsFSMultipartFormData::AddNameBlobOrNull
I think we should assert somewhere here that if filepath16 is non-empty, we need to have the pref for webkitrelativepath or pref for
other Directory stuff enabled.

We need some tests that when doing normal file picking (not directory stuff), 
webkitrelativepath is "".


r+ for this, but could you upload a new patch, on top of this one, which fixes those issues and ask review for it.

Comment 28

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

and we need a comment somewhere that the GetPath/SetPath works rather oddly. Either it contains directory path + filename, or it is empty, and some comment that this all to be compatible with webkitrelativepath

Comment 29

[Andrea Marchesini [:baku]]

Attachment: 1258490_relativePath.patch

Comment 30

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

Comment on attachment 8746883 [details] [diff] [review]
1258490_relativePath.patch

>+Directory::WebkitBlinkFileSystemEnabled(JSContext* aCx, JSObject* aObj)
>+{
>+  if (NS_IsMainThread()) {
>+    return Preferences::GetBool("dom.filesystem.webkitBlink.enabled", false) &&
>+           Preferences::GetBool("dom.input.dirpicker", false);
>+  }
>+
>+  workers::WorkerPrivate* workerPrivate =
>+    workers::GetWorkerPrivateFromContext(aCx);
>+  if (!workerPrivate) {
>+    return false;
>+  }
>+
>+  return workerPrivate->WebkitBlinkFileSystemEnabled() &&
>+         workerPrivate->DirectoryPickerEnabled();
>+}
This doesn't look quite right. We want webkitrelativepath to be enabled even without the filesystem stuff.
In other words, as I've said before, we want one pref to enabled webkitdirectory+webkitrelativepath and then a separate thing
for the (readonly) filesystem.
So, rename this method to WebkitBlinkDirectoryPickerEnabled or some such and make it use
dom.webkitBlink.dirPicker.enabled or something.

And, why does this rely on "dom.input.dirpicker"? We need to be able to enable/disable webkit stuff without the new directory upload API stuff.

So, could you upload a new patch fixing the pref handling. Other than that, r+

Comment 31

[Andrea Marchesini [:baku]]

Attachment: 1258490_relativePath.patch

Comment 32

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

Comment on attachment 8748707 [details] [diff] [review]
1258490_relativePath.patch

>+/* static */ bool
>+Directory::WebkitBlinkDirectoryPickerEnabled(JSContext* aCx, JSObject* aObj)
...
>+  return workerPrivate->WebkitBlinkDirectoryPickerEnabled;
>+}
Clearly this code hasn't been tested. Please test.
As far as I see WebkitBlinkDirectoryPickerEnabled is a method, so you're missing ().

Comment 33

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bbe22e01384c

Comment 34

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7bf66811396c

Comment 35

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dba45e1c4bf1

Comment 36

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/bbe22e01384c
https://hg.mozilla.org/mozilla-central/rev/7bf66811396c
https://hg.mozilla.org/mozilla-central/rev/dba45e1c4bf1

Comment 37

[Mike Taylor [:miketaylr]]

Filed spec issue https://github.com/whatwg/compat/issues/53 (would be happy for this to be specced elsewhere, but it should be specced).