The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in Firefox 42

Status

()

Core
DOM
RESOLVED FIXED
2 years ago
18 days ago

People

(Reporter: jwatt, Assigned: jwatt)

Tracking

(Depends on: 1 bug, Blocks: 1 bug, {dev-doc-needed, devrel-needed})

Trunk
mozilla42
dev-doc-needed, devrel-needed
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox40 affected, firefox41 affected, firefox42 fixed)

Details

(URL)

Attachments

(6 attachments)

(Assignee)

Description

2 years ago
+++ 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
(Assignee)

Updated

2 years ago
Depends on: 1173320
(Assignee)

Updated

2 years ago
Depends on: 1173390

Comment 1

2 years ago
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.

Updated

2 years ago
See Also: → bug 846931
(Assignee)

Updated

2 years ago
Depends on: 1177688
(Assignee)

Comment 2

2 years ago
Created attachment 8628919 [details] [diff] [review]
part 1 - Make the code for bypassing mobile security checks more general so that it can be used on non-mobile
Attachment #8628919 - Flags: review?(amarchesini)
(Assignee)

Comment 3

2 years ago
Created attachment 8628920 [details] [diff] [review]
part 2 - Implement an abstraction for a rooted filesystem for non-mobile devices
Attachment #8628920 - Flags: review?(amarchesini)
(Assignee)

Comment 4

2 years ago
Created attachment 8628921 [details] [diff] [review]
part 3 - Allow the DirState of blobs to be set explicitly
Attachment #8628921 - Flags: review?(amarchesini)
(Assignee)

Comment 5

2 years ago
Created attachment 8628925 [details] [diff] [review]
part 4 - Implement the new HTMLInputElement API including the new Promise returning GetFilesAndDirectories
Attachment #8628925 - Flags: review?(amarchesini)
(Assignee)

Comment 6

2 years ago
Created attachment 8628959 [details] [diff] [review]
part 5 - Implement new anonymous content and layout pieces for directory picking via input elements
Attachment #8628959 - Flags: review?(dholbert)
(Assignee)

Updated

2 years ago
Attachment #8628959 - Flags: review?(dholbert) → review?(tnikkel)
(Assignee)

Comment 7

2 years ago
Created attachment 8629016 [details] [diff] [review]
part 6 - Implement the new  Promise returning DataTransfer.getFilesAndDirectories() API
(Assignee)

Updated

2 years ago
Attachment #8629016 - Attachment is patch: true
Attachment #8629016 - Flags: review?(amarchesini)
Attachment #8628959 - Flags: review?(tnikkel) → review+
Attachment #8628919 - Flags: review?(amarchesini) → review+
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.
Attachment #8628920 - Flags: review?(amarchesini) → review+
Attachment #8628921 - Flags: review?(amarchesini) → review+
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.
Attachment #8628925 - Flags: review?(amarchesini) → review+
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.
Attachment #8629016 - Flags: review?(amarchesini) → review+
Duplicate of this bug: 846931

Updated

2 years ago
Duplicate of this bug: 876480

Comment 13

2 years ago
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

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/a82eb6004f08
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
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox42: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla42

Comment 16

2 years ago
\o/
https://hg.mozilla.org/mozilla-central/rev/a82eb6004f08

Comment 18

2 years ago
Is this pref-controlled / en-/disabled by default?
Flags: needinfo?(jwatt)
(Assignee)

Comment 19

2 years ago
It's controlled by the pref dom.input.dirpicker which is on in Nightly only, for now.
Flags: needinfo?(jwatt)

Comment 20

2 years ago
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!
Flags: needinfo?(jwatt)

Comment 21

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

Comment 22

2 years ago
Seems to work for me. Can you file a bug please?
Flags: needinfo?(jwatt)

Updated

2 years ago
Depends on: 1188818
(Assignee)

Updated

2 years ago
Blocks: 1188880
Depends on: 1190036
Keywords: devrel-needed

Comment 23

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

11 months ago
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

18 days ago
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?
Well done to everyone that worked on this - folder uploading in Firefox is fantastic, really helpful!
You need to log in before you can comment on or make changes to this bug.