If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Support dnd for webkitdirectory

RESOLVED FIXED in Firefox 50

Status

()

Core
DOM
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: smaug, Assigned: smaug)

Tracking

unspecified
mozilla50
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Comment hidden (empty)
(Assignee)

Comment 1

a year ago
Created attachment 8774548 [details] [diff] [review]
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.)
Attachment #8774548 - Flags: review?(amarchesini)
(Assignee)

Updated

a year ago
Blocks: 1289255
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.
Attachment #8774548 - Flags: review?(amarchesini)
(Assignee)

Comment 3

a year ago
(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.
(Assignee)

Comment 4

a year ago
Created attachment 8774672 [details] [diff] [review]
webkitdirectory_dnd.diff

Be horrified.
Attachment #8774548 - Attachment is obsolete: true
Attachment #8774672 - Flags: review?(amarchesini)
(Assignee)

Updated

a year ago
Blocks: 1188880
(Assignee)

Updated

a year ago
Blocks: 1288683
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
Attachment #8774672 - Flags: review?(amarchesini) → review-
(Assignee)

Comment 6

a year ago
Created attachment 8775543 [details] [diff] [review]
webkitdirectory_dnd_2.diff
Attachment #8774672 - Attachment is obsolete: true
Attachment #8775543 - Flags: review?(amarchesini)
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.
Attachment #8775543 - Flags: review?(amarchesini) → review+
(Assignee)

Comment 8

a year ago
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.
(Assignee)

Comment 9

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=02ef7625a85e
(Assignee)

Comment 10

a year ago
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

a year ago
Pushed by opettay@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f1f06933167f
Support dnd for webkitdirectory, r=baku

Comment 12

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f1f06933167f
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.