Closed Bug 1258694 Opened 8 years ago Closed 8 years ago

Implement Directory::GetFiles()

Categories

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

47 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: baku, Assigned: baku)

References

Details

(Whiteboard: btpp-active)

Attachments

(1 file, 4 obsolete files)

Attached patch dir2.patch (obsolete) — Splinter Review
      No description provided.
Attachment #8733359 - Flags: review?(bugs)
Attached patch dir2.patch (obsolete) — Splinter Review
Attachment #8733359 - Attachment is obsolete: true
Attachment #8733359 - Flags: review?(bugs)
Attachment #8733595 - Flags: review?(bugs)
Comment on attachment 8733595 [details] [diff] [review]
dir2.patch

I filed https://github.com/WICG/directory-upload/issues/34 since I'm not sure the API is quite right yet.
That would be a simple change to the API and implementation.

>+GetFilesTask::SetSuccessRequestResult(const FileSystemResponseValue& aValue,
>+                                     ErrorResult& aRv)
Nit, align ErrorResult

>+  // We cannot store File objects bacause this object is created
because

>+  // on a different thread and File is not thread-safe.
>+  FallibleTArray<nsString> mTargetData;
But we could store nsIFile. However, looks like the real reason here is that we need to do stuff cross process, no?


>+struct FileSystemGetFilesParams
>+{
>+  nsString filesystem;
totally unclear what filesystem means here. Is is like C:\ on windows? or what
And I must be missing something...
I don't have dom/filesystem/PFileSystemParams.ipdlh
I guess this patch depends on something else.


>+function test_getFiles(aDirectory, aRecursive, aNext) {
>+  aDirectory.getFiles(aRecursive).then(
>+    function(data) {
>+      for (var i = 0; i < data.length; ++i) {
>+        ok (data[i] instanceof File, "File: " + data[i].name);
>+      }
>+    },
>+    function() {
>+      ok(false, "Something when wrong");
>+    }
>+  ).then(aNext);
So we need some test actually making sure recursive==true works and behaves differently to recursive=false.
Like that recursive==true ends up collecting more files in case subdirectories contain some files.
r- especially because of this, but also because PFileSystemParams.ipdlh stuff (but that is probably because there is some other patch waiting for my review in my review queue).
Attachment #8733595 - Flags: review?(bugs) → review-
> r- especially because of this, but also because PFileSystemParams.ipdlh
> stuff (but that is probably because there is some other patch waiting for my
> review in my review queue).

Right, the order of these patches is:

bug 1258095 - getPath() - 3 patches
bug 1258221 - PBackground - 2 patches
bug 1257180 - Directory in workers - 3 patch
This bug.
Attached patch dir2.patch (obsolete) — Splinter Review
Attachment #8733595 - Attachment is obsolete: true
Attachment #8735089 - Flags: review?(bugs)
Comment on attachment 8735089 [details] [diff] [review]
dir2.patch

There are now enough patches that I can't figure out the right order to review them. Could you perhaps ask reviews in the right order and ask a new review once the previous one has got r+.
Attachment #8735089 - Flags: review?(bugs)
Attached patch dir2.patch (obsolete) — Splinter Review
Attachment #8735089 - Attachment is obsolete: true
Whiteboard: btpp-active
Attachment #8736670 - Attachment is obsolete: true
Attachment #8740350 - Flags: review?(bugs)
Comment on attachment 8740350 [details] [diff] [review]
1258694_getFiles.patch

HTMLInputElement.getFiles() will be implemented in some other bug, right?
And make that block the generic Directory meta bug.

>+already_AddRefed<Promise>
>+Directory::GetFiles(bool aRecursiveFlag, ErrorResult& aRv)
>+{
>+  RefPtr<FileSystemBase> fs = GetFileSystem(aRv);
>+  if (NS_WARN_IF(aRv.Failed())) {
>+    return nullptr;
>+  }
>+
>+  RefPtr<GetFilesTaskChild> task =
>+    GetFilesTaskChild::Create(fs, mFile, aRecursiveFlag, aRv);
>+  if (NS_WARN_IF(aRv.Failed())) {
>+    return nullptr;
>+  }
What kind of value aRv might have in these two cases?
Per spec this method might throw only SecurityError or InvalidStateError.
So I think in these two cases InvalidStateError is right thing to throw.
(and we'll add the SecurityError stuff once the HTTPS state handling has been implemented)

>+/* static */ already_AddRefed<GetFilesTaskChild>
>+GetFilesTaskChild::Create(FileSystemBase* aFileSystem,
>+                          nsIFile* aTargetPath,
>+                          bool aRecursiveFlag,
>+                          ErrorResult& aRv)
>+{
>+  MOZ_ASSERT(aFileSystem);
>+  aFileSystem->AssertIsOnOwningThread();
>+
>+  RefPtr<GetFilesTaskChild> task =
>+    new GetFilesTaskChild(aFileSystem, aTargetPath, aRecursiveFlag);
>+
>+  // aTargetPath can be null. In this case SetError will be called.
>+
>+  nsCOMPtr<nsIGlobalObject> globalObject =
>+    do_QueryInterface(aFileSystem->GetParentObject());
>+  if (NS_WARN_IF(!globalObject)) {
>+    aRv.Throw(NS_ERROR_FAILURE);
>+    return nullptr;
>+  }
Would it make more sense to check whether we have global before creating GetFilesTaskChild.

+  if (HasError()) {
+    mPromise->MaybeReject(mErrorValue);
+    mPromise = nullptr;
+    return;
+  }
We should not reject with some random error value but with InvalidStateError

>+  for (unsigned i = 0; i < count; i++) {
>+    nsCOMPtr<nsIFile> path;
>+    NS_ConvertUTF16toUTF8 fullPath(mTargetData[i]);
>+    nsresult rv = NS_NewNativeLocalFile(fullPath, true, getter_AddRefs(path));
>+    if (NS_WARN_IF(NS_FAILED(rv))) {
>+      mPromise->MaybeReject(rv);
We should reject here with InvalidStateError


>+GetFilesTaskParent::ExploreDirectory(nsIFile* aPath)
>+{
>+  MOZ_ASSERT(XRE_IsParentProcess(),
>+             "Only call from parent process!");
>+  MOZ_ASSERT(!NS_IsMainThread(), "Only call on worker thread!");
not a worker but io thread, right? Just clarify the assert comment a bit.

>+    bool isLink, isSpecial, isFile;
>+    if (NS_WARN_IF(NS_FAILED(currFile->IsSymlink(&isLink)) ||
>+                   NS_FAILED(currFile->IsSpecial(&isSpecial))) ||
>+        isLink || isSpecial) {
>+      continue;
>+    }
curious, why links don't work? I would expect them to work, I think.

>+void
>+GetFilesTaskParent::GetPermissionAccessType(nsCString& aAccess) const
>+{
>+  aAccess.AssignLiteral("read");
please use the #define you've added in some other bug.

I wish we didn't copy so much of the task handling. Like GetDirectoryListingTaskParent, GetFilesTaskParent and GetFileOrDirectoryTaskChild they
all have much in common. Perhaps a followup to clean up.

>+  is(aDirectory.name, '/', "directory.name must be '/'");
>+  is(aDirectory.path, '/', "directory.path must be '/'");
So these will change in some other bug where you fix path/name handling right?
Do we keep all the Directory handling bugs blocking the meta bug? Gets hard to remember what all needs to be fixed.

>+function test_getFilesAndDirectories(aDirectory, aRecursive, aNext) {
>+  function checkSubDir(dir) {
>+    return dir.getFilesAndDirectories().then(
>+      function(data) {
>+        for (var i = 0; i < data.length; ++i) {
>+          ok (data[i] instanceof File || data[i] instanceof Directory, "Just Files or Directories");
>+          if (data[i] instanceof Directory) {
>+            isnot(data[i].name, '/', "Subdirectory should be called with the leafname");
>+            isnot(data[i].path, '/', "Subdirectory path should be called with the leafname");
>+            isnot(data[i].path, dir.path, "Subdirectory path should contain the parent path.");
>+            is(data[i].path,dir.path + '/' + data[i].name, "Subdirectory path should be called parentdir.path + '/' + leafname");
So I assume these will need some tweaking too once the root path naming has been fixed
(the stuff you mentioned on IRC).


Random note, not about this bug. I noticed GetDirectoryListingTaskParent::IOWork() may *create* a directory. Why is that? You reviewed that code.
Attachment #8740350 - Flags: review?(bugs) → review+
Depends on: 1257179
Blocks: 1188880
No longer depends on: 1257179
> Random note, not about this bug. I noticed
> GetDirectoryListingTaskParent::IOWork() may *create* a directory. Why is
> that? You reviewed that code.

It's another corner case for DeviceStorage. The 'storage' directories must exist.
I think it's a problem for OSFileStorage and I already removed those mkdir calls in another bug.
It's a problem because, what about if the user selects one directory, then it deletes it, and then it calls getFilesOrDirectories(). We don't want to create any directory for that operation.
https://hg.mozilla.org/mozilla-central/rev/bd1aca23aba6
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.