1261693 - Implement HTMLInputElement::GetFiles

Status: RESOLVED FIXED

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

Description

[Andrea Marchesini [:baku]]

Attachment: getFiles.patch

Comment 1

[Andrea Marchesini [:baku]]

Attachment: 1261693_getFiles.patch

Comment 2

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

Hmm, what is this bug about. We certainly support input.files

Comment 3

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

oh, this is about the upload proposal getFiles()

Comment 4

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

Comment on attachment 8737694 [details] [diff] [review]
1261693_getFiles.patch

You need to call ClearGetFilesHelpers in HTMLInputElement's dtor to ensure the member variables are released in main thread.

>+class GetFilesHelper final : public nsRunnable
>+{
>+public:
>+  static already_AddRefed<GetFilesHelper>
>+  Create(nsIGlobalObject* aGlobal,
>+         const nsTArray<OwningFileOrDirectory>& aFilesOrDirectory,
>+         bool aRecursiveFlag, ErrorResult& aRv)
>+  {
>+    MOZ_ASSERT(aGlobal);
>+
>+    RefPtr<GetFilesHelper> helper = new GetFilesHelper(aGlobal, aRecursiveFlag);
>+
>+    nsTArray<nsString> directoryPaths;
>+
>+    for (uint32_t i = 0; i < aFilesOrDirectory.Length(); ++i) {
>+      const OwningFileOrDirectory& data = aFilesOrDirectory[i];
>+      if (data.IsFile()) {
>+        if (!helper->mFiles.AppendElement(data.GetAsFile(), fallible)) {
>+          aRv.Throw(NS_ERROR_OUT_OF_MEMORY);
>+          return nullptr;
>+        }
>+      } else {
>+        MOZ_ASSERT(data.IsDirectory());
>+
>+        // We just want 1 directory.
>+        MOZ_ASSERT(directoryPaths.IsEmpty());
I don't understand this setup. Why do we have an array for directories if we expect to get just one directory.


>+#ifdef DEBUG
>+        nsAutoString path;
>+        directory->GetPath(path, aRv);
>+        if (NS_WARN_IF(aRv.Failed())) {
>+          return nullptr;
>+        }
>+
>+        MOZ_ASSERT(path.EqualsLiteral(FILESYSTEM_DOM_PATH_SEPARATOR_LITERAL));
>+#endif
Could you perhaps call the variable domPath here to hint that we're playing with the top level dom stuff

>+
>+        nsAutoString fullPath;
>+        aRv = directory->GetFullRealPath(fullPath);
>+        if (NS_WARN_IF(aRv.Failed())) {
>+          return nullptr;
>+        }
>+
>+        directoryPaths.AppendElement(fullPath);
>+      }
>+    }
>+
>+    // No directories to explore.
>+    if (directoryPaths.IsEmpty()) {
>+      helper->mListingCompleted = true;
>+      return helper.forget();
>+    }
>+
>+    MOZ_ASSERT(helper->mFiles.IsEmpty());
>+    helper->SetDirectoryPath(directoryPaths[0]);
so here we just use the first item. Why do we even have the array?


Took a bit time to understand, but yes, almost there.
The top level directory handling could use some good comments. Only one directory is expected?

Comment 5

[Andrea Marchesini [:baku]]

Attachment: 1261693_getFiles.patch

Comment 6

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

Comment on attachment 8753371 [details] [diff] [review]
1261693_getFiles.patch

>+
>+      bool isLink, isSpecial, isFile, isDir;
>+      if (NS_WARN_IF(NS_FAILED(currFile->IsSymlink(&isLink)) ||
>+                     NS_FAILED(currFile->IsSpecial(&isSpecial))) ||
>+          isLink || isSpecial) {
>+        continue;
>+      }
Please file a bug which blocks exposing both blink API and upload proposal to sort out link handling.
I think our link handling for files is broken, and it should be fixed everywhere (but not sure, just think so).
I've mentioned this in some other bug before.




>+  RefPtr<GetFilesHelper> mGetFilesRecursiveHelper;
>+  RefPtr<GetFilesHelper> mGetFilesNonRecursiveHelper;

Could we store these as nsINode properties to reduce the memory usage of non-type=file input elements.
You could do that change as a separate patch or bug.

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/77c60a79313d

Comment 8

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

Had to back this out in https://hg.mozilla.org/integration/mozilla-inbound/rev/e94aa4bc6f1a for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=28474115&repo=mozilla-inbound

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b797131df354
Implement HTMLInputElement::GetFiles, r=smaug

Comment 10

[Carsten Book [:Tomcat]]

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