1289254 - Support dnd for webkitdirectory

Status: RESOLVED FIXED

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

Comment 1

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

Attachment: webkitdirectory_dnd.diff

This makes dnd to work with webkitdirectory only.

dom.webkitBlink.filesystem.enabled requires still the
commented out part in nsFileControlFrame::DnDListener::HandleEvent
to be sorted out.
The patch does anyhow try to fix the caching issues .webkitEntries has.


(I think I have in mind a horrible hack for the dom.webkitBlink.filesystem.enabled case to keep supporting directories as files too. That will be a followup patch if ever happens.)

Comment 2

[Andrea Marchesini [:baku]]

Comment on attachment 8774548 [details] [diff] [review]
webkitdirectory_dnd.diff

Review of attachment 8774548 [details] [diff] [review]:
-----------------------------------------------------------------

Many things:

1. For internal communication between processes: we already have an Object to describe a directory, and it's call Directory. We should use it instead touching something that is meant to be used only for Blob and File.
2. You are also introducing 2 sync methods: 1 was only meant to be used for testing.
3. if we have Directory APIs and Webkit stuff disabled, .files would contain directory objects as File. This seems wrong. Also because there are no APIs able to deal with these 'Files'.
4. By spec .files should not contain directory, right? We should enforce this.

Plus, in general, I really would like to keep BlobImpl/blob/File just for Files or memory blobs and not for directory. Conceptually a blob is some data with a type and a size. A File is a blob +  a name.
If we keep directories as File, what are they? no type, no size (or wrong, platform dependent), no data, maybe a name.

::: dom/html/HTMLInputElement.cpp
@@ +2857,5 @@
>  
> +  if (Preferences::GetBool("dom.webkitBlink.filesystem.enabled", false)) {
> +    HTMLInputElementBinding::ClearCachedWebkitEntriesValue(this);
> +    mEntries.Clear();
> +  }

This seems unrelated. Maybe a separate patch?

::: dom/webidl/HTMLInputElement.webidl
@@ +225,5 @@
>  HTMLInputElement implements MozPhonetic;
>  
>  // Webkit/Blink
>  partial interface HTMLInputElement {
> +  [Pref="dom.webkitBlink.filesystem.enabled", Frozen, Cached, Pure]

unrelated.

::: layout/forms/nsFileControlFrame.cpp
@@ +304,5 @@
> +        for (uint32_t i = 0; i < files->Length(); ++i) {
> +          File* file = files->Item(i);
> +          if (file) {
> +            if (file->Impl() && file->Impl()->IsDirectory()) {
> +              AppendBlobImplAsDirectory(array, file->Impl(), content);

This means that .files could contain a directory, if we don't have webkit/directory APIs enabled.

@@ +391,5 @@
>  
> +  RefPtr<BlobImpl> webkitDir;
> +  nsresult rv =
> +    GetBlobImplForWebkitDirectory(fileList, getter_AddRefs(webkitDir));
> +  // Just check if either there isn't webkitdirectory attribute, or 

extra space.

Comment 3

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

(In reply to Andrea Marchesini (:baku) from comment #2)
> 1. For internal communication between processes: we already have an Object
> to describe a directory, and it's call Directory. We should use it instead
> touching something that is meant to be used only for Blob and File.
But using Directory for this stuff would complicate everything a lot.


> 2. You are also introducing 2 sync methods:
so?

> 1 was only meant to be used for
> testing.
Right. Since I need that information, I removed the comment about testing only.


> 3. if we have Directory APIs and Webkit stuff disabled, .files would contain
> directory objects as File. This seems wrong. Also because there are no APIs
> able to deal with these 'Files'.
.files should contain entries for directories even when webkit stuff is enabled.
If <input> doesn't have webkitdirectory, it should have entries for directories which were dnd'ed there.
That is something I need to fix still.



> 4. By spec .files should not contain directory, right? We should enforce
> this.
What spec? I'm not aware anything saying such. I'm trying to follow us closely as possible what blink is doing, since
we're implementing a blink API.


> Plus, in general, I really would like to keep BlobImpl/blob/File just for
> Files or memory blobs and not for directory. Conceptually a blob is some
> data with a type and a size. A File is a blob +  a name.
> If we keep directories as File, what are they? no type, no size (or wrong,
> platform dependent), no data, maybe a name.
Atm, I don't really care too much about that. What I care is compatibility.







> ::: layout/forms/nsFileControlFrame.cpp
> @@ +304,5 @@
> > +        for (uint32_t i = 0; i < files->Length(); ++i) {
> > +          File* file = files->Item(i);
> > +          if (file) {
> > +            if (file->Impl() && file->Impl()->IsDirectory()) {
> > +              AppendBlobImplAsDirectory(array, file->Impl(), content);
> 
> This means that .files could contain a directory, if we don't have
> webkit/directory APIs enabled.
yes. I don't want to break existing behavior, or implement blink APIs differently to what they have implemented.

Comment 4

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

Attachment: webkitdirectory_dnd.diff

Be horrified.

Comment 5

[Andrea Marchesini [:baku]]

Comment on attachment 8774672 [details] [diff] [review]
webkitdirectory_dnd.diff

Review of attachment 8774672 [details] [diff] [review]:
-----------------------------------------------------------------

For IPC what we should do is this:

1. BlobChild::RemoteBlobImpl should receive isDirectory boolean in the CTOR.

2. about the mystery we should not touch but just add an assertion about IsDirectory() == false.
The reason why I say that is that IDB fails storing a 'directory' File, so it's impossible that we receive some mystery IPC message for something that was a directory.

::: dom/base/File.cpp
@@ +995,5 @@
> +BlobImplFile::IsDirectory() const
> +{
> +  bool isDirectory = false;
> +  if (mFile) {
> +    mFile->IsDirectory(&isDirectory);

This could fail.

::: dom/ipc/Blob.cpp
@@ +2204,5 @@
>  
> +bool
> +BlobChild::
> +RemoteBlobImpl::IsDirectory() const
> +{

this should be a cached value. return mIsDirectory;

@@ +4439,5 @@
>    return true;
>  }
>  
> +bool
> +BlobParent::RecvIsDirectory(bool* aIsDirectory)

We should not need this.

::: dom/ipc/BlobParent.h
@@ +235,5 @@
>    virtual bool
>    RecvGetFilePath(nsString* aFilePath) override;
> +
> +  virtual bool
> +  RecvIsDirectory(bool* aIsDirectory) override;

get rid of this.

::: dom/ipc/PBlob.ipdl
@@ +46,1 @@
>    sync GetFilePath()

keep the comment here.

@@ +46,5 @@
>    sync GetFilePath()
>      returns (nsString filePath);
>  
> +  sync IsDirectory()
> +    returns (bool isDirectory);

to remove

Comment 6

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

Attachment: webkitdirectory_dnd_2.diff

Comment 7

[Andrea Marchesini [:baku]]

Comment on attachment 8775543 [details] [diff] [review]
webkitdirectory_dnd_2.diff

Review of attachment 8775543 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/ipc/Blob.cpp
@@ +1712,5 @@
>    RefPtr<BlobImpl> mSameProcessBlobImpl;
>  
>    const bool mIsSlice;
>  
> +  const bool mIsDirectory;

You can use the enum here as well.

Comment 8

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

I was thinking that, but ended up that since we need it later only as a bool, better to store as bool.
enum is just to ensure sane param passing.

Comment 9

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=02ef7625a85e

Comment 10

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

About Mobile: https://bugs.chromium.org/p/chromium/issues/detail?id=465190
so Chrome at least didn't use to support this stuff on Mobile, but I'm trying to get something to test how data:text/html,<input type=file webkitdirectory> behaves there.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f1f06933167f
Support dnd for webkitdirectory, r=baku

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f1f06933167f