894840 - Add an openDirectoryPicker() method to HTMLInputElement

Status: RESOLVED FIXED

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

Description

[Jonathan Watt [:jwatt]]

As a starting point for bug 846931, we should add an openDirectoryPicker() method to HTMLInputElement. That way users can start using the functionality with their own user-styled file upload widgets while we work on the functionality and UX for our native implementation.

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: part 1 - rename AsyncClickHandler

This renames AsyncClickHandler to AsyncPickerRunnable, since its use will no longer be restricted to acting on a click.

Comment 2

[Jonathan Watt [:jwatt]]

Attachment: part 2 - refactor to get rid of nsFilePickerShownCallback::mMode

nsFilePickerShownCallback::aMulti duplicates information stored in mFilePicker. This patch gives access to that information. More to the point, it also gets rid of the boolean aMulti argument that doesn't fit with passing a new (third) "directory" state.

Comment 3

[Mounir Lamouri (:mounir)]

Comment on attachment 780371 [details] [diff] [review]
part 1 - rename AsyncClickHandler

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

Someone is working on a patch to simply remove AsyncClickHandler given that we are now calling the async method from nsIFilePicker (and nsIColorPicker only has an async method).

Comment 4

[Mounir Lamouri (:mounir)]

Comment on attachment 780374 [details] [diff] [review]
part 2 - refactor to get rid of nsFilePickerShownCallback::mMode

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

I am not sure I understand that patch. Is the goal to prevent nsFilePickerShownCallback to keep track of the multiple state?
Generally speaking, we don't expose back the parameters passed to the nsIFilePicker::Init() method so I am not sure why we should make an exception for mode.

Also, regarding the code, if we want to add this feature, you might want to add it to nsBaseFilePicker, it would prevent modifying every single implementation.

Comment 5

[Jonathan Watt [:jwatt]]

(In reply to Mounir Lamouri (:mounir) from comment #3)
> Someone is working on a patch to simply remove AsyncClickHandler given that
> we are now calling the async method from nsIFilePicker (and nsIColorPicker
> only has an async method).

For anyone else following along, that's bug 898550. That landed earlier today, so I'm now rebasing and will upload new patches shortly, including the patches that actually implement HTMLInputElement.openDirectoryPicker().

Comment 6

[Jonathan Watt [:jwatt]]

Attachment: part 1 - refactor to get rid of nsFilePickerShownCallback::mMode

Comment 2 describes the purpose of this patch.

Comment 7

[Jonathan Watt [:jwatt]]

Attachment: part 2 - implement openDirectoryPicker()

I've striped out all the nsIFilePicker and other changes that you didn't like, mounir. I've also not finished with the ProgressPromise code etc. so I've separated out this basic implementation while I work on the other things. It probably makes more sense it terms of reviewing to have it it digestible chunks anyway.

Comment 8

[Jonathan Watt [:jwatt]]

Attachment: part 3 - add a .path property to File, and set it as appropriate

Actually I can split this out too.

Comment 9

[Mounir Lamouri (:mounir)]

Comment on attachment 786619 [details] [diff] [review]
part 1 - refactor to get rid of nsFilePickerShownCallback::mMode

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

r=me with the comments applied.

::: content/html/content/src/HTMLInputElement.cpp
@@ +319,5 @@
>    }
>  
> +  int16_t mode;
> +  mFilePicker->GetMode(&mode);
> +  bool multi = mode == static_cast<int16_t>(nsIFilePicker::modeOpenMultiple);

Sad that ::modeOpenMultiple is an enum...

::: widget/nsIFilePicker.idl
@@ +180,5 @@
>    */
>    void open(in nsIFilePickerShownCallback aFilePickerShownCallback);
>  
> + /**
> +  * The picker's mode, as set by the 'mode' argument passed to init().

nit: specify that the value will match the constants above.

@@ +182,5 @@
>  
> + /**
> +  * The picker's mode, as set by the 'mode' argument passed to init().
> +  */
> +  readonly attribute short mode;

Could you make this [infallible] ?

Comment 10

[Mounir Lamouri (:mounir)]

Comment on attachment 786940 [details] [diff] [review]
part 2 - implement openDirectoryPicker()

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

Someone else than me should review the entire "run this in a different thread" part and maybe the directory traversal (at least, having a double check wouldn't hurt).

There is a couple of small changes I would like to see and questions answered but otherwise, the patch looks pretty good to go as far as I am concerned.

::: content/html/content/src/HTMLInputElement.cpp
@@ +339,5 @@
> +    bool isDir;
> +    aTopDir->IsDirectory(&isDir);
> +    MOZ_ASSERT(isDir);
> +#endif
> +    nsCOMPtr<nsISimpleEnumerator> entries;

nit: empty line after #endif plz

@@ +417,5 @@
> +    if (isDir) {
> +#ifdef DEBUG
> +      nsAutoString leafName;
> +      file->GetLeafName(leafName);
> +      MOZ_ASSERT(leafName != NS_LITERAL_STRING(".."));

Why are you checking that? Could that happen by any chance? If this is just about special values, there is "." too.

@@ +441,5 @@
> +  nsCOMPtr<nsIFile> mTopDir;
> +  nsCOMPtr<nsIFile> mNextFile;
> +  nsCOMArray<nsISimpleEnumerator> mDirEnumeratorStack;
> +};
> +  

nit: trailing ws

@@ +459,5 @@
> +      // Build up list of nsDOMFileFile objects on this dedicated thread:
> +      nsCOMPtr<nsISimpleEnumerator> iter =
> +        new DirPickerRecursiveFileEnumerator(mTopDir);
> +      bool hasMore = true;
> +      nsCOMPtr<nsISupports> tmp;      

nit: trailing ws

@@ +469,5 @@
> +      }
> +      NS_DispatchToMainThread(this);
> +      return NS_OK;
> +    }
> +    // Now back no the main thread, set the list on our HTMLInputElement:

nit: empty line before this comment.

@@ +530,5 @@
> +#ifdef DEBUG
> +    bool isDir;
> +    pickedDir->IsDirectory(&isDir);
> +    MOZ_ASSERT(isDir);
> +#endif

nit: add an empty line before the #ifdef and one after the #endif

@@ +541,5 @@
> +
> +    nsRefPtr<DirPickerBuildFileListTask> event =
> +      new DirPickerBuildFileListTask(mInput.get(), pickedDir.get());
> +    target->Dispatch(event, NS_DISPATCH_NORMAL);
> +    return NS_OK;

return target->Dispatch(event, ...);

@@ +795,4 @@
>    NS_ENSURE_SUCCESS(rv, rv);
>  
> +  if (HasAttr(kNameSpaceID_None, nsGkAtoms::accept) &&
> +      !aPickDir /* directories don't have a type */) {

Very likely the widget code already ignore types for directory picking, don't they?

@@ +809,5 @@
>    nsCOMPtr<nsIFilePickerShownCallback> callback =
>      new HTMLInputElement::nsFilePickerShownCallback(this, filePicker);
>  
> +  if (oldFiles.Count() &&
> +      !aPickDir) {

Why not selecting the currently selected directory in the file picker?
Is this disabled by design or simply to be done later?

@@ +2383,5 @@
> +void
> +HTMLInputElement::OpenDirectoryPicker(ErrorResult& aRv)
> +{
> +  if (mType != NS_FORM_INPUT_FILE) {
> +    aRv.Throw(NS_ERROR_FAILURE);

The exception should be INVALID_STATE_ERROR.

::: content/html/content/src/HTMLInputElement.h
@@ +56,2 @@
>     */
> +  nsresult StoreLastUsedDirectory(nsIDocument* aDoc, nsIFile* aDir);

Why not keeping that method as-is and if it is a file, call it again with the parent. If it is a directory, just save it. Should make the callers life easier. If it is not, keep it as is, this is anyway private to HTMLInputElement, not a public API.

@@ +1087,5 @@
>     * picker (color picker or file picker).
>     */
>    nsresult MaybeInitPickers(nsEventChainPostVisitor& aVisitor);
>  
> +  nsresult InitFilePicker(bool aPickDir = false);

Please, please, please, use an enum.

Comment 11

[Mounir Lamouri (:mounir)]

Comment on attachment 786944 [details] [diff] [review]
part 3 - add a .path property to File, and set it as appropriate

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

I think khuey would be a better reviewer for that patch.

Comment 12

[Jonathan Watt [:jwatt]]

Attachment: part 2 - implement openDirectoryPicker()

(In reply to Mounir Lamouri (:mounir) from comment #10)
> Why are you checking that? Could that happen by any chance? If this is just
> about special values, there is "." too.

Debugging code I meant to remove. Deleted.

> Very likely the widget code already ignore types for directory picking,
> don't they?

It does. There's no point in spending cycles working up a filter list. I've adjusted the comment.

> Why not selecting the currently selected directory in the file picker?
> Is this disabled by design or simply to be done later?

The FetchDirectoryAndDisplayPicker() call after the |if| block takes care of this.

> The exception should be INVALID_STATE_ERROR.

That would be more consistent with existing DOM API. Fixed.

> > +  nsresult StoreLastUsedDirectory(nsIDocument* aDoc, nsIFile* aDir);
> 
> Why not keeping that method as-is and if it is a file, call it again with
> the parent. If it is a directory, just save it. Should make the callers life
> easier. If it is not, keep it as is, this is anyway private to
> HTMLInputElement, not a public API.

That would only improve the caller at:

  // Store the last used directory using the content pref service:
  nsCOMPtr<nsIFile> file = DOMFileToLocalFile(newFiles[0]);
  nsCOMPtr<nsIFile> lastUsedDir;
  file->GetParent(getter_AddRefs(lastUsedDir));
  HTMLInputElement::gUploadLastDir->StoreLastUsedDirectory(
    mInput->OwnerDoc(), lastUsedDir.get());

where we'd remove two lines (the third and fourth lines). StoreLastUsedDirectory would then get more complicated, both in terms of code and terms of the concept (may take a file then do a parent lookup, or else may take a dir). So I think this would be a net loss.

Comment 13

[Mounir Lamouri (:mounir)]

Comment on attachment 787584 [details] [diff] [review]
part 2 - implement openDirectoryPicker()

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

r=me with the comments applied :)

::: content/html/content/src/HTMLInputElement.cpp
@@ +326,5 @@
> + * does not guaranteed to walk it depth-first either (since it uses
> + * nsIFile::GetDirectoryEntries, which is not guaranteed to group a directory's
> + * subdirectories at the beginning of the list that it returns).
> + */
> +class DirPickerRecursiveFileEnumerator : public nsISimpleEnumerator

You can mark this class MOZ_FINAL.

@@ +349,5 @@
> +    }
> +  }
> +
> +  virtual ~DirPickerRecursiveFileEnumerator()
> +  {}

You don't need to specify the dtor.

@@ +384,5 @@
> +      mNextFile = nullptr;
> +      return;
> +    }
> +    nsISimpleEnumerator* currentDirEntries =
> +    mDirEnumeratorStack[mDirEnumeratorStack.Count() - 1];

nit: tabulation before mDirEnumeratorStack[...];

@@ +407,5 @@
> +    bool isLink, isSpecial;
> +    file->IsSymlink(&isLink);
> +    file->IsSpecial(&isSpecial);
> +    if (isLink || isSpecial) {
> +      // skip

nit: I think you can get ride of that comment ;)

@@ +463,5 @@
> +        MOZ_ASSERT(domFile);
> +        mFileList.AppendObject(domFile);
> +      }
> +      NS_DispatchToMainThread(this);
> +      return NS_OK;

return NS_DispatchToMainThread(this);

@@ +501,5 @@
> +                             getter_AddRefs(localFile));
> +  return localFile.forget();
> +}
> +
> +} // local namespace

nit: "anonymous namespace"

@@ +539,5 @@
> +    NS_ASSERTION(target, "Must have stream transport service");
> +
> +    nsRefPtr<DirPickerBuildFileListTask> event =
> +      new DirPickerBuildFileListTask(mInput.get(), pickedDir.get());
> +    return target->Dispatch(event, NS_DISPATCH_NORMAL);

Can you add a comment that says that HTMLInputElement::SetFiles() and the "change" event will be taken care of by DirPickerBuildFileListTask?

@@ +794,5 @@
>  
> +  // Native directory pickers ignore file type filters, so we don't spend
> +  // cycles adding them for FILE_PICKER_DIRECTORY.
> +  if (HasAttr(kNameSpaceID_None, nsGkAtoms::accept) &&
> +      !(aType == FILE_PICKER_DIRECTORY)) {

aType != FILE_PICKER_DIRECTORY

@@ +809,5 @@
>    nsCOMPtr<nsIFilePickerShownCallback> callback =
>      new HTMLInputElement::nsFilePickerShownCallback(this, filePicker);
>  
> +  if (oldFiles.Count() &&
> +      !(aType == FILE_PICKER_DIRECTORY)) {

aType != FILE_PICKER_DIRECTORY

::: content/html/content/src/HTMLInputElement.h
@@ +1089,5 @@
>    nsresult MaybeInitPickers(nsEventChainPostVisitor& aVisitor);
>  
> +  enum FilePickerType {
> +    FILE_PICKER_FILE,
> +    FILE_PICKER_DIRECTORY

FOR YOUR INFORMATION, NOTHING FORCES YOU TO USE C-STYLE MACRO UPPERCASE FOR ENUMS. JUST SO YOU KNOW.

(To be more useful, we sometimes do things like eFilePickerFile / eFilePickerDirectory but there is no strong convention for that so do whatever you like.)

Comment 14

[Jonathan Watt [:jwatt]]

(In reply to Mounir Lamouri (:mounir) from comment #13)
> FOR YOUR INFORMATION, NOTHING FORCES YOU TO USE C-STYLE MACRO UPPERCASE FOR
> ENUMS. JUST SO YOU KNOW.

Just following the when-in-Rome rule and sticking with the convention used for the existing enum in the file (which you apparently wrote ;) ).

Comment 15

[Jonathan Watt [:jwatt]]

Attachment: part 2 - implement openDirectoryPicker() [r=mounir]

Kyle, can you review the use of the stream transport service? The directory tree scanning can take a long time, so we want it off on its own dedicated thread.

Comment 16

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 786944 [details] [diff] [review]
part 3 - add a .path property to File, and set it as appropriate

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

path should be implemented for Files on worker threads too.  http://hg.mozilla.org/mozilla-central/rev/be390eb890c9 ought to give you some idea what to do (note that the JS rooting stuff has changed since then, so you'll want to crib from the current source).

::: content/base/public/nsIDOMFile.idl
@@ +65,5 @@
>  interface nsIDOMFile : nsIDOMBlob
>  {
>    readonly attribute DOMString name;
>  
> +  readonly attribute DOMString path;

This needs an IID change.

::: content/html/content/src/HTMLInputElement.cpp
@@ +360,5 @@
>  
>      if (!mNextFile) {
>        return NS_ERROR_FAILURE;
>      }
>      nsCOMPtr<nsIDOMFile> domFile = new nsDOMFileFile(mNextFile);

Change this to nsRefPtr<nsDOMFileFile>

@@ +370,5 @@
> +    mNextFile->GetLeafName(leafName);
> +    int32_t length = path.Length() - leafName.Length() - 1; // -1 for "/"
> +    if (length > 0) {
> +      static_cast<nsDOMFileFile*>(domFile.get())->
> +        SetPath(Substring(path, 0, uint32_t(length)));

Then you won't need this super-ugly static cast.

Comment 17

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Is there a spec for this?

Comment 18

[Jonathan Watt [:jwatt]]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #17)
> Is there a spec for this?

No, this is experimental and behind a pref, for now.

Comment 19

[Jonathan Watt [:jwatt]]

Attachment: part 3 - add a .path property to File, and set it as appropriate

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #16)
> path should be implemented for Files on worker threads too. 
> http://hg.mozilla.org/mozilla-central/rev/be390eb890c9 ought to give you
> some idea what to do (note that the JS rooting stuff has changed since then,
> so you'll want to crib from the current source).

Thanks. I cribbed from GetPath rather than GetLastModifiedDate though, since I'm dealing with a string.

> This needs an IID change.

Oops, yes it does.

> Change this to nsRefPtr<nsDOMFileFile>
> Then you won't need this super-ugly static cast.

Done, although that means I need to cast to avoid ambiguity further down. I guess it's a bit better that way.

Comment 20

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

One use case that I realized is not fulfilled by the current API is something like this:

Enable a website to use a <input type=file multiple> to render a reasonable UI on both mobile and desktop.

This means that on desktop they would want to render both a "pick file" and a "pick directory" button, whereas on mobile they would only want to render a "pick files using intents/activities" button. This part we can automatically take care of once we build UI for directory picking.

What can't be handled by the website though is showing the user progress notifications once the user has clicked the "pick directory" button. Since the website didn't call openDirectoryPicker() it doesn't get access ProgressPromise.

Though maybe all of this isn't a problem until we actually implement the "pick directory" button. Which is a separate bug for sure.

Comment 21

[Jonathan Watt [:jwatt]]

Thanks. Mounir and I discussed this previously, and I'll target progress events at the element, both in the case that the user uses the buttons and in the case that openDirectoryPicker() is called.

Comment 22

[Jonathan Watt [:jwatt]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c3b9c515d846
https://hg.mozilla.org/integration/mozilla-inbound/rev/02cccbd90395

Comment 23

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/c3b9c515d846
https://hg.mozilla.org/mozilla-central/rev/02cccbd90395

Comment 24

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 788095 [details] [diff] [review]
part 2 - implement openDirectoryPicker() [r=mounir]

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

::: content/html/content/src/HTMLInputElement.cpp
@@ +313,5 @@
>  
> +namespace {
> +
> +/**
> + * This enumerator returns nsDOMFileFile objects after wrapping a singe nsIFile

single

@@ +318,5 @@
> + * representing a directory. It enumerates the files under that directory and
> + * its subdirectories as a flat list of files, ignoring/skipping over symbolic
> + * links.
> + *
> + * The enumaration involves I/O, so this class should NOT be used on the main

enumeration, s/should/must/

@@ +322,5 @@
> + * The enumaration involves I/O, so this class should NOT be used on the main
> + * thread or else the main thread could be blocked for a very long time.
> + *
> + * This enumerator does not walk the directory tree breadth-first, but it also
> + * does not guaranteed to walk it depth-first either (since it uses

is not

@@ +333,5 @@
> +  NS_DECL_ISUPPORTS
> +
> +  DirPickerRecursiveFileEnumerator(nsIFile* aTopDir)
> +    : mTopDir(aTopDir)
> +  {

This needs a threading assertion too, I think.

@@ +338,5 @@
> +#ifdef DEBUG
> +    bool isDir;
> +    aTopDir->IsDirectory(&isDir);
> +    MOZ_ASSERT(isDir);
> +#endif

Put these in a separate scope.

@@ +342,5 @@
> +#endif
> +
> +    nsCOMPtr<nsISimpleEnumerator> entries;
> +    nsresult ok = mTopDir->GetDirectoryEntries(getter_AddRefs(entries));
> +    if (NS_SUCCEEDED(ok) && entries) {

if (NS_SUCCEEDED(mTopDir->Blah(...)) &&
    entries) {

@@ +358,5 @@
> +
> +    if (!mNextFile) {
> +      return NS_ERROR_FAILURE;
> +    }
> +    nsCOMPtr<nsIDOMFile> domFile = new nsDOMFileFile(mNextFile);

Make this either an nsDOMFileFile (to match the concrete type) or an nsISupports (to match the outparam).

@@ +374,5 @@
> +
> +private:
> +
> +  void
> +  LookupAndCacheNext()

This function recurses ... how much directory nesting or symlinks can we handle before we blow out the stack?  Seems like we may need a better approach.

@@ +412,5 @@
> +    bool isDir;
> +    file->IsDirectory(&isDir);
> +    if (isDir) {
> +      nsCOMPtr<nsISimpleEnumerator> subDirEntries;
> +      nsresult ok = file->GetDirectoryEntries(getter_AddRefs(subDirEntries));

don't redeclare an nsresult, just use rv.

@@ +423,5 @@
> +#ifdef DEBUG
> +    bool isFile;
> +    file->IsFile(&isFile);
> +    MOZ_ASSERT(isFile);
> +#endif

Again, braces around debug only test code.

@@ +431,5 @@
> +
> +private:
> +  nsCOMPtr<nsIFile> mTopDir;
> +  nsCOMPtr<nsIFile> mNextFile;
> +  nsCOMArray<nsISimpleEnumerator> mDirEnumeratorStack;

Please use nsTArray<nsCOMPtr<T> > in new code.

@@ +476,5 @@
> +                                                false);
> +  }
> +
> +private:
> +  nsRefPtr<HTMLInputElement> mInput;

There's no guarantee that the dtor of this class will run on the main thread.  mInput could be released on the background thread which will cause very sad times.  You need to explicitly clear it in places that are guaranteed to be on the main thread.

@@ +478,5 @@
> +
> +private:
> +  nsRefPtr<HTMLInputElement> mInput;
> +  nsCOMPtr<nsIFile> mTopDir;
> +  nsCOMArray<nsIDOMFile> mFileList;

nsTArray<nsCOMPtr<nsIDOMFile> >

@@ +492,5 @@
> +  }
> +
> +  nsCOMPtr<nsIFile> localFile;
> +  rv = NS_NewNativeLocalFile(NS_ConvertUTF16toUTF8(path), true,
> +                             getter_AddRefs(localFile));

If you're not going to check rv don't bother with the assignment.

NS_ENSURE_SUCCESS(rv, nullptr) might make sense here though.

@@ +523,5 @@
> +#ifdef DEBUG
> +    bool isDir;
> +    pickedDir->IsDirectory(&isDir);
> +    MOZ_ASSERT(isDir);
> +#endif

Moar braces.

@@ +526,5 @@
> +    MOZ_ASSERT(isDir);
> +#endif
> +
> +    HTMLInputElement::gUploadLastDir->StoreLastUsedDirectory(
> +      mInput->OwnerDoc(), pickedDir.get());

.get() should not be needed here, I think.

@@ +578,5 @@
> +  nsCOMPtr<nsIFile> file = DOMFileToLocalFile(newFiles[0]);
> +  nsCOMPtr<nsIFile> lastUsedDir;
> +  file->GetParent(getter_AddRefs(lastUsedDir));
> +  HTMLInputElement::gUploadLastDir->StoreLastUsedDirectory(
> +    mInput->OwnerDoc(), lastUsedDir.get());

same here.

Comment 25

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 789242 [details] [diff] [review]
part 3 - add a .path property to File, and set it as appropriate

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

Should probably have a main thread test too.

::: content/html/content/src/HTMLInputElement.cpp
@@ +359,5 @@
>      if (!mNextFile) {
>        return NS_ERROR_FAILURE;
>      }
> +    nsRefPtr<nsDOMFileFile> domFile = new nsDOMFileFile(mNextFile);
> +    nsAutoCString relDescriptor;

No need for an autostring to pass to a getter.  Just use nsCString here.

@@ +365,5 @@
> +    NS_ENSURE_SUCCESS(rv, rv);
> +    NS_ConvertUTF8toUTF16 path(relDescriptor);
> +    nsAutoString leafName;
> +    mNextFile->GetLeafName(leafName);
> +    int32_t length = path.Length() - leafName.Length() - 1; // -1 for "/"

Assert that the numbers here are sane (that leafName.Length is <= path.Length, I think).  Also assert that the result is sane (>= -1, I think).

@@ +367,5 @@
> +    nsAutoString leafName;
> +    mNextFile->GetLeafName(leafName);
> +    int32_t length = path.Length() - leafName.Length() - 1; // -1 for "/"
> +    if (length > 0) {
> +      domFile.get()->SetPath(Substring(path, 0, uint32_t(length)));

the .get() should not be necessary here.

@@ +369,5 @@
> +    int32_t length = path.Length() - leafName.Length() - 1; // -1 for "/"
> +    if (length > 0) {
> +      domFile.get()->SetPath(Substring(path, 0, uint32_t(length)));
> +    }
> +    *aResult = static_cast<nsIDOMFile*>(domFile.forget().get());

I guess we never implemented the forget overload that lets you forget to a base class :-/

Comment 26

[Jonathan Watt [:jwatt]]

Attachment: patch - convert HTMLInputElement to use nsTArray<nsCOMPtr<x> >

Comment 27

[Mounir Lamouri (:mounir)]

Comment on attachment 791214 [details] [diff] [review]
patch - convert HTMLInputElement to use nsTArray<nsCOMPtr<x> >

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

r=me with explicitly calling IsEmpty() instead of using .Length().

::: content/html/content/src/HTMLInputElement.cpp
@@ +364,5 @@
>          mInput->OwnerDoc(), domFile);
>      }
>    }
>  
> +  if (!newFiles.Length()) {

Please do:
if (newFiles.IsEmpty()) {

@@ +585,5 @@
>  
>    nsCOMPtr<nsIFilePickerShownCallback> callback =
>      new HTMLInputElement::nsFilePickerShownCallback(this, filePicker);
>  
> +  if (oldFiles.Length()) {

if (!oldFiles.IsEmpty()) {

@@ +1254,5 @@
>        return NS_OK;
>  
>      case VALUE_MODE_FILENAME:
>        if (nsContentUtils::IsCallerChrome()) {
> +        if (mFiles.Length()) {

if (!mFiles.IsEmpty()) {

@@ +1262,5 @@
>            aValue.Truncate();
>          }
>        } else {
>          // Just return the leaf name
> +        if (mFiles.Length() == 0 || NS_FAILED(mFiles[0]->GetName(aValue))) {

if (mFiles.IsEmpty() || ...) {

@@ +2065,5 @@
>    }
>  
>    nsXPIDLString value;
>  
> +  if (mFiles.Length() == 0) {

if (mFiles.IsEmpty()) {

@@ +4571,4 @@
>        aFormSubmission->AddNameFilePair(name, files[i], NullString());
>      }
>  
> +    if (files.Length() == 0) {

if (files.IsEmpty()) {

@@ +4607,5 @@
>          inputState->SetChecked(mChecked);
>        }
>        break;
>      case VALUE_MODE_FILENAME:
> +      if (mFiles.Length()) {

if (!mFiles.IsEmpty()) {

@@ +5248,5 @@
>        return IsValueEmpty();
>      case VALUE_MODE_FILENAME:
>      {
> +      const nsTArray<nsCOMPtr<nsIDOMFile> >& files = GetFilesInternal();
> +      return !files.Length();

return files.IsEmpty();

Comment 28

[Jonathan Watt [:jwatt]]

Attachment: patch - implement openDirectoryPicker()

Thanks for the thorough review!

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #24)
> This function recurses ... how much directory nesting or symlinks can we
> handle before we blow out the stack?  Seems like we may need a better
> approach.

Good point. It's not impossible that we could blow the stack, so I've changed to an iterative loop implementation.


> @@ +476,5 @@
> > +                                                false);
> > +  }
> > +
> > +private:
> > +  nsRefPtr<HTMLInputElement> mInput;
> 
> There's no guarantee that the dtor of this class will run on the main
> thread.  mInput could be released on the background thread which will cause
> very sad times.  You need to explicitly clear it in places that are
> guaranteed to be on the main thread.

DirPickerBuildFileListTask is created and dies on the main thread (its Run() method posts instances of itself back to the main thread to finish things off), and it holds a strong ref to the HTMLInputElement from the point that it's created on the main thread to the point that it dies on the main thread.

I've added an ifdef DEBUG dtor asserting that the dtor is called on the main thread.


> @@ +358,5 @@
> > +
> > +    if (!mNextFile) {
> > +      return NS_ERROR_FAILURE;
> > +    }
> > +    nsCOMPtr<nsIDOMFile> domFile = new nsDOMFileFile(mNextFile);
> 
> Make this either an nsDOMFileFile (to match the concrete type) or an
> nsISupports (to match the outparam).

Again the problem is that the conversion to nsISupports is ambigous, so for either of those options I need a cast. I've gone with nsRefPtr<nsDOMFileFile> and a cast to nsIDOMFile.

Comment 29

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 791435 [details] [diff] [review]
patch - implement openDirectoryPicker()

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

::: content/html/content/src/HTMLInputElement.cpp
@@ +457,5 @@
> +#ifdef DEBUG
> +  ~DirPickerBuildFileListTask()
> +  {
> +    MOZ_ASSERT(NS_IsMainThread(),
> +               "Having mInput die off the main thread would be bad!");

This assertion does not hold.

Nothing guarantees that the event has not already run on the main thread by the time NS_DispatchToMainThread returns on the background thread.  Once the event is posted to the main thread's event queue an untimely context switch could cause it to run and be released on the main thread before enough code can run on the background thread to return from NS_DispatchToMainThread and release the reference that thread's event queue holds to the runnable.

See Bug 700685 for an example of what this looks like in the wild.

Comment 30

[Jonathan Watt [:jwatt]]

Attachment: patch - implement openDirectoryPicker()

Doh! Right.

Comment 31

[Jonathan Watt [:jwatt]]

Hmm, hold on. HTMLInputElement does not use threadsafe AddRef/Release, so nulling out ThreadSafeAutoRefCnt::mInput isn't enough. Would changing HTMLInputElement from NS_DECL_ISUPPORTS_INHERITED to NS_DECL_THREADSAFE_ISUPPORTS be acceptable, Kyle? We don't have any other element types that use thread safe refcounting as far as I can see, but I think it's probably okay.

Comment 32

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to Jonathan Watt [:jwatt] from comment #31)
> Hmm, hold on. HTMLInputElement does not use threadsafe AddRef/Release, so
> nulling out ThreadSafeAutoRefCnt::mInput isn't enough. Would changing
> HTMLInputElement from NS_DECL_ISUPPORTS_INHERITED to
> NS_DECL_THREADSAFE_ISUPPORTS be acceptable, Kyle? We don't have any other
> element types that use thread safe refcounting as far as I can see, but I
> think it's probably okay.

You should always end up addreffing/releaseing HTMLInputElement on the main thread, so it should not matter that it does not have threadsafe refcounting.  You can't switch it to use threadsafe refcounting because it participates in cycle collection.  I haven't reviewed the patch yet but the idea is to null out mInput in code that is running on the main thread.

Comment 33

[Jonathan Watt [:jwatt]]

Oh, right. The patch is good for review then.

Comment 34

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 791502 [details] [diff] [review]
patch - implement openDirectoryPicker()

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

::: content/html/content/src/HTMLInputElement.cpp
@@ +318,5 @@
> + * nsIFile representing a directory. It enumerates the files under that
> + * directory and its subdirectories as a flat list of files, ignoring/skipping
> + * over symbolic links.
> + *
> + * The enumaration involves I/O, so this class must NOT be used on the main

*enumeration with an 'e'

@@ +388,5 @@
> +        break;
> +      }
> +
> +      nsISimpleEnumerator* currentDirEntries =
> +        mDirEnumeratorStack[mDirEnumeratorStack.Length() - 1];

mDirEnumeratorStack.LastElement()

@@ +459,5 @@
> +  {
> +    MOZ_ASSERT(NS_IsMainThread(),
> +               "Having mInput die off the main thread would be bad!");
> +  }
> +#endif

Again, this assertion is false (and also irrelevant, since you null out mInput).

@@ +470,5 @@
> +      bool hasMore = true;
> +      nsCOMPtr<nsISupports> tmp;
> +      while (NS_SUCCEEDED(iter->HasMoreElements(&hasMore)) && hasMore) {
> +        iter->GetNext(getter_AddRefs(tmp));
> +        nsCOMPtr<nsIDOMFile> domFile = do_QueryInterface(tmp);

This may be the only use of nsISimpleIterator::GetNext in the tree that does it correctly!

@@ +477,5 @@
> +      }
> +      return NS_DispatchToMainThread(this);
> +    }
> +
> +    // Now back no the main thread, set the list on our HTMLInputElement:

*on

@@ +483,5 @@
> +      return NS_OK;
> +    }
> +    // The text control frame (if there is one) isn't going to send a change
> +    // event because it will think this is done by a script.
> +    // So, we can safely send one by ourself.

Why does it think that?  Seems weird.

Comment 35

[Jonathan Watt [:jwatt]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9641c1b620c6
https://hg.mozilla.org/integration/mozilla-inbound/rev/be3663c7c734
https://hg.mozilla.org/integration/mozilla-inbound/rev/e5e6c3bb26b3

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #34)
> Why does it think that?  Seems weird.

I'm not sure. :| I copied that comment from the other part of the file.

Comment 36

[[e-mail]]

(In reply to Kyle Huey from comment #24)
> (From update of attachment 788095 [details] [diff] [review])
> > +  nsCOMArray<nsISimpleEnumerator> mDirEnumeratorStack;
> 
> Please use nsTArray<nsCOMPtr<T> > in new code.

What does it buy you, apart from code bloat and extra typing for > >? (nsCOMArray is more efficient and exposes roughly the same API.)

Comment 37

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/9641c1b620c6
https://hg.mozilla.org/mozilla-central/rev/be3663c7c734
https://hg.mozilla.org/mozilla-central/rev/e5e6c3bb26b3

Comment 38

[David Bruant]

Not sure I understand what happened in this bug...
Is this function expected to be exposed to web content? Since HTMLInputElement is a standard interface, that's what the bug summury suggests.
If so, is it part of any standard? plans to be?

Comment 39

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Yes, no and yes. Jonathan started a thread on the whatwg mailing list about with use cases and a proposed solution.

Comment 40

[Jason Smith [:jsmith]]

Note - this might have caused bug 928166 potentially, which is currently causing Twitter to crash when you add a photo to a tweet.