1258694 - Implement Directory::GetFiles()

Status: RESOLVED FIXED

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

Description

[Andrea Marchesini [:baku]]

Attachment: dir2.patch

Comment 1

[Andrea Marchesini [:baku]]

Attachment: dir2.patch

Comment 2

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

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).

Comment 3

[Andrea Marchesini [:baku]]

> 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.

Comment 4

[Andrea Marchesini [:baku]]

Attachment: dir2.patch

Comment 5

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

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+.

Comment 6

[Andrea Marchesini [:baku]]

Attachment: dir2.patch

Comment 7

[Andrea Marchesini [:baku]]

Attachment: 1258694_getFiles.patch

Comment 8

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

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.

Comment 9

[Andrea Marchesini [:baku]]

> 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.

Comment 10

[Pulsebot]

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

Comment 11

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

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