1164310 - Implement MS's proposal for a reduced subset of the new FileSystem API

Status: RESOLVED FIXED

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

Description

[Jonathan Watt [:jwatt]]

+++ This bug was initially created as a clone of Bug #846931 +++

Bug 846931 has proceeded on the basis of allowing users to select directories while having the browser then act in a way that is compatible with legacy websites.

To avoid a confusing morphing of that bug I'm spinning of this new bug to cover implementing Microsoft's proposal for a subset of the new FileSystem API, a proposal which would not "just work" with existing webpages but rather would require opt-in from new pages. More info on that proposal can be found here:

https://lists.w3.org/Archives/Public/public-webapps/2015AprJun/0245.html

Comment 1

[brandon]

Thank you for spinning off this bug!

This API would be very useful to us. We currently have uploading functionality (for very large directory hierarchies) that is only supported in Chrome and Opera.

The proposed API would be able to replace the Chrome/Opera one, and it would allow us to support those uploads in Firefox and Edge as well.

Comment 2

[Jonathan Watt [:jwatt]]

Attachment: part 1 - Make the code for bypassing mobile security checks more general so that it can be used on non-mobile

Comment 3

[Jonathan Watt [:jwatt]]

Attachment: part 2 - Implement an abstraction for a rooted filesystem for non-mobile devices

Comment 4

[Jonathan Watt [:jwatt]]

Attachment: part 3 - Allow the DirState of blobs to be set explicitly

Comment 5

[Jonathan Watt [:jwatt]]

Attachment: part 4 - Implement the new HTMLInputElement API including the new Promise returning GetFilesAndDirectories

Comment 6

[Jonathan Watt [:jwatt]]

Attachment: part 5 - Implement new anonymous content and layout pieces for directory picking via input elements

Comment 7

[Jonathan Watt [:jwatt]]

Attachment: part 6 - Implement the new Promise returning DataTransfer.getFilesAndDirectories() API

Comment 8

[Andrea Marchesini [:baku]]

Comment on attachment 8628920 [details] [diff] [review]
part 2 - Implement an abstraction for a rooted filesystem for non-mobile devices

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

lgtm!

::: dom/filesystem/OSFileSystem.cpp
@@ +36,5 @@
> +
> +void
> +OSFileSystem::Init(nsPIDOMWindow* aWindow)
> +{
> +  MOZ_ASSERT(NS_IsMainThread(), "Only call on main thread!");

MOZ_ASSERT(!mWindow, "No duple Init() calls"); Could it make sense?

@@ +45,5 @@
> +nsPIDOMWindow*
> +OSFileSystem::GetWindow() const
> +{
> +  MOZ_ASSERT(NS_IsMainThread(), "Only call on main thread!");
> +  if (!mWindow) {

this is not needed.
I think you can just do:

return mWindow;

@@ +63,5 @@
> +{
> +  // The concept of "safe files" is specific to the Device Storage API where
> +  // files are only "safe" if they're of a type that is appropriate for the
> +  // area of device storage that is being used.
> +  MOZ_ASSERT(false, "Don't use OSFileSystem with the Device Storage API");

MOZ_CRASH

@@ +73,5 @@
> +{
> +  // The concept of "safe directory" is specific to the Device Storage API
> +  // where a directory is only "safe" if it belongs to the area of device
> +  // storage that it is being used with.
> +  MOZ_ASSERT(false, "Don't use OSFileSystem with the Device Storage API");

MOZ_CRASH

::: dom/filesystem/OSFileSystem.h
@@ +43,5 @@
> +} // namespace dom
> +} // namespace mozilla
> +
> +#endif // mozilla_dom_OSFileSystem_h
> +

extra line at the bottom of this file.

Comment 9

[Andrea Marchesini [:baku]]

Comment on attachment 8628925 [details] [diff] [review]
part 4 - Implement the new HTMLInputElement API including the new Promise returning GetFilesAndDirectories

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

::: dom/html/HTMLInputElement.cpp
@@ +3564,5 @@
> +    if (target &&
> +        target->GetParent() == this &&
> +        target->IsRootOfNativeAnonymousSubtree() &&
> +        target->HasAttr(kNameSpaceID_None, nsGkAtoms::directory)) {
> +      MOZ_ASSERT(Preferences::GetBool("dom.input.dirpicker", false),

I really would like to see a test for this :)

@@ +4827,5 @@
> +
> +bool
> +HTMLInputElement::IsFilesAndDirectoriesSupported() const
> +{
> +  // This method is supposed to return true if a file and directory picker

80chars?

@@ +4838,5 @@
> +void
> +HTMLInputElement::ChooseDirectory(ErrorResult& aRv)
> +{
> +  if (mType != NS_FORM_INPUT_FILE) {
> +    aRv.Throw(NS_ERROR_DOM_INVALID_STATE_ERR);

return;

@@ +4853,5 @@
> +static already_AddRefed<OSFileSystem>
> +MakeOrReuseFileSystem(const nsAString& aNewLocalRootPath,
> +                      OSFileSystem* aFS,
> +                      nsPIDOMWindow* aWindow)
> +{

MOZ_ASSERT(aWindow);

@@ +4891,5 @@
> +    p->MaybeReject(NS_ERROR_OUT_OF_MEMORY);
> +    return p.forget();
> +  }
> +
> +  nsPIDOMWindow* window = OwnerDoc()->GetInnerWindow(); // XXXjwatt correct thing?

why this comment?
I guess yes, you want to use the inner window.

Comment 10

[Andrea Marchesini [:baku]]

Comment on attachment 8629016 [details] [diff] [review]
part 6 - Implement the new  Promise returning DataTransfer.getFilesAndDirectories() API

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

::: dom/events/DataTransfer.cpp
@@ +8,5 @@
>  #include "mozilla/BasicEvents.h"
>  
>  #include "DataTransfer.h"
>  
> +#include "mozilla/dom/Directory.h"

why here on top and not with the other mozilla/dom/ headers?

@@ +833,5 @@
> +static already_AddRefed<OSFileSystem>
> +MakeOrReuseFileSystem(const nsAString& aNewLocalRootPath,
> +                      OSFileSystem* aFS,
> +                      nsPIDOMWindow* aWindow)
> +{

MOZ_ASSERT(aWindow);

@@ +904,5 @@
> +      nsDependentSubstring basename = Substring(path, leafSeparatorIndex);
> +      fs = MakeOrReuseFileSystem(dirname, fs, window);
> +      filesAndDirsSeq[i].SetAsDirectory() = new Directory(fs, basename);
> +    } else {
> +      filesAndDirsSeq[i].SetAsFile() = mFiles->Item(i);

This is equal to what I just reviewed in the previous patch. Correct?
Wondering if we can unify these 2 blocks (this and HTMLInputElement::GetFilesAndDirectories.

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d9f93c3850e9
https://hg.mozilla.org/integration/mozilla-inbound/rev/4216422f8500
https://hg.mozilla.org/integration/mozilla-inbound/rev/0a945cb91184
https://hg.mozilla.org/integration/mozilla-inbound/rev/c6a6d9d77dd5
https://hg.mozilla.org/integration/mozilla-inbound/rev/728bc2eed910
https://hg.mozilla.org/integration/mozilla-inbound/rev/c09134abb278
https://hg.mozilla.org/integration/mozilla-inbound/rev/c986072df4cb

Comment 14

[Pulsebot]

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

Comment 15

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

https://hg.mozilla.org/mozilla-central/rev/d9f93c3850e9
https://hg.mozilla.org/mozilla-central/rev/4216422f8500
https://hg.mozilla.org/mozilla-central/rev/0a945cb91184
https://hg.mozilla.org/mozilla-central/rev/c6a6d9d77dd5
https://hg.mozilla.org/mozilla-central/rev/728bc2eed910
https://hg.mozilla.org/mozilla-central/rev/c09134abb278
https://hg.mozilla.org/mozilla-central/rev/c986072df4cb

Comment 16

[Jet Villegas (inactive)]

\o/

Comment 17

[Carsten Book [:Tomcat]]

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

Comment 18

[Florian Bender]

Is this pref-controlled / en-/disabled by default?

Comment 19

[Jonathan Watt [:jwatt]]

It's controlled by the pref dom.input.dirpicker which is on in Nightly only, for now.

Comment 20

[silverwind]

Is the drag an drop part of this finished? I seem to get a File when dropping a directory. The <input> part is working great, though!

Comment 21

[silverwind]

To further clarify: When dropping a single directory event.dataTransfer.getFilesAndDirectories() seems to return a File instead of an expected Directory. I can file a bug if you like.

Comment 22

[Jonathan Watt [:jwatt]]

Seems to work for me. Can you file a bug please?

Comment 23

[Oliver Frietsch]

Sorry for maybe asking a newbie question about the release process, but how can I find out when this will be in Firefox _stable_? I've tested Nightly 45 and really liked this feature, but Firefox 42 does not ship it despite this ticket has a flag "status-firefox42: fixed".

Comment 24

[Boris Zbarsky [:bzbarsky]]

That's a good question, not a newbie one.  ;)

This bug implemented the functionality behind a preference ("dom.input.dirpicker" to be precise).  This preference is currently only set to true by default in nightly builds.

Bug 1188880 (blocked by this bug) tracks enabling in release builds.  Note that it has several unresolved dependencies blocking it.

Comment 25

[[e-mail]]

It's currently not possible to upload a folder into Firefox to upload it to cloud storage (Google Drive, One Drive, Sirv, Dropbox or others). When will this be possible? The need for folder uploading is growing and has already received many votes on this related thread:

https://bugzilla.mozilla.org/show_bug.cgi?id=876480

It should be possible to upload folders by both drag and drop and file browsing.

Comment 26

[Tristan Miller]

I can verify that uploading folders via drag-and-drop now works in Firefox 51.0.  However, it doesn't work in SeaMonkey 2.46.  Are there plans for this functionality to be added to SeaMonkey or should I open a new issue?

Comment 27

[[e-mail]]

Well done to everyone that worked on this - folder uploading in Firefox is fantastic, really helpful!