Implement Directory.getContents() and Directory.path

RESOLVED FIXED in Firefox 42

Status

()

RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: jwatt, Assigned: jwatt)

Tracking

({dev-doc-needed})

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

Firefox Tracking Flags

(firefox42 fixed)

Details

Attachments

(5 attachments, 1 obsolete attachment)

(Assignee)

Description

4 years ago
The implementation of Directory.getContents() and Directory.path can be done as a standalone piece of work separate from the greater directory picker implementation bug, bug 1164310.

Directory objects can be obtained via the device storage API on B2G. This bug is about implementing .getContents() and .path sufficiently completely to be usable on that platform.
(Assignee)

Comment 1

4 years ago
Created attachment 8626508 [details] [diff] [review]
part 1 - Add API and functionality to the BlobImpl classes in order that BlobImpl's that are created from an nsIFile can provide information about whether or not the nsIFile was a directory

Currently when a Blob is created from an nsIFile that is a directory we just treat it as a normal file with zero size. Usually we don't have a direct way to determine whether such a Blob is from an empty file or a directory. This just adds the ability for us to tag such Blobs with the information that they correspond to a directory.
Attachment #8626508 - Flags: review?(amarchesini)
(Assignee)

Comment 2

4 years ago
Created attachment 8626509 [details] [diff] [review]
part 2 - Add support to the FileSystem code for obtaining a listing of a Directory's Directory and File contents via a sequence of Blobs
Attachment #8626509 - Flags: review?(amarchesini)
(Assignee)

Comment 3

4 years ago
Created attachment 8626510 [details] [diff] [review]
part 3 - Implement .getContents() and .path for DOM Directory objects
Attachment #8626510 - Flags: review?(amarchesini)
(Assignee)

Comment 4

4 years ago
Created attachment 8626511 [details] [diff] [review]
part 4 - Add DeviceStorage tests for the new .getContents() and .path API on Directory
Attachment #8626511 - Flags: review?(amarchesini)
(Assignee)

Updated

4 years ago
Depends on: 1176800
Comment on attachment 8626508 [details] [diff] [review]
part 1 - Add API and functionality to the BlobImpl classes in order that BlobImpl's that are created from an nsIFile can provide information about whether or not the nsIFile was a directory

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

I want to check how to propagate isDirectory via IPC for RemoteBlobs with bent.

::: dom/base/File.h
@@ +362,5 @@
>  
>    virtual bool IsDateUnknown() const = 0;
>  
>    virtual bool IsFile() const = 0;
> +  virtual void SetIsDirectory() = 0;

that can go away.

@@ +528,5 @@
>    {
>      return mIsFile;
>    }
>  
> +  virtual void SetIsDirectory() override

Just add:
 mFile->IsDirectory(&mIsDirectory);
in the CTOR of BlobImplFile.

::: dom/ipc/Blob.cpp
@@ +2143,5 @@
>    virtual bool
>    IsFile() const override;
>  
> +  virtual void
> +  SetIsDirectory() override;

we can remove this.
Attachment #8626508 - Flags: review?(amarchesini)
Attachment #8626508 - Flags: review+
Attachment #8626508 - Flags: feedback?(bent.mozilla)
Comment on attachment 8626509 [details] [diff] [review]
part 2 - Add support to the FileSystem code for obtaining a listing of a Directory's Directory and File contents via a sequence of Blobs

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

::: dom/filesystem/GetDirectoryListingTask.cpp
@@ +58,5 @@
> +already_AddRefed<Promise>
> +GetDirectoryListingTask::GetPromise()
> +{
> +  MOZ_ASSERT(NS_IsMainThread(), "Only call on main thread!");
> +  return nsRefPtr<Promise>(mPromise).forget();

What about:

Promise*
GetDirectoryListingTask::GetPromise() const
{
  MOZ_ASSERT(...);
  return mPromise;
}

@@ +114,5 @@
> +  }
> +
> +  // Whether we want to get the root directory.
> +  bool getRoot = mTargetRealPath.IsEmpty();
> +

All of these code lines are equal to GetFileOrDirectoryTask::Work(). We should try to create an helper or move them to the base class.

@@ +157,5 @@
> +  }
> +
> +  for (;;) {
> +    bool hasMore = false;
> +    if (NS_FAILED(entries->HasMoreElements(&hasMore)) || !hasMore) {

NS_WARN_IF

@@ +161,5 @@
> +    if (NS_FAILED(entries->HasMoreElements(&hasMore)) || !hasMore) {
> +      break;
> +    }
> +    nsCOMPtr<nsISupports> supp;
> +    if (NS_FAILED(entries->GetNext(getter_AddRefs(supp)))) {

NS_WARN_IF

@@ +165,5 @@
> +    if (NS_FAILED(entries->GetNext(getter_AddRefs(supp)))) {
> +      break;
> +    }
> +    nsCOMPtr<nsIFile> currFile = do_QueryInterface(supp);
> +    

extra spaces + MOZ_ASSERT(currFile);

@@ +167,5 @@
> +    }
> +    nsCOMPtr<nsIFile> currFile = do_QueryInterface(supp);
> +    
> +    bool isLink, isSpecial, isFile;
> +    currFile->IsSymlink(&isLink);

In theory all of these methods (IsSymlink, IsSpecial and IsDirectory) should fail, right?

@@ +179,5 @@
> +      continue;
> +    }
> +    BlobImplFile* impl = new BlobImplFile(currFile);
> +    if (isDir) {
> +      impl->SetIsDirectory();

Remove this.

@@ +219,5 @@
> +      nsAutoString name;
> +      mTargetBlobImpls[i]->GetName(name);
> +      nsAutoString path(mTargetRealPath);
> +      path.AppendLiteral(FILESYSTEM_DOM_PATH_SEPARATOR);
> +      path.Append(name);

I would add:

#ifdef DEBUG
  nsCOMPtr<nsIFile> file = mFileSystem->GetLocalFile(path);
  bool exist;
  file->Exists(&exist);
  MOZ_ASSERT(exist);
}
#endif

::: dom/filesystem/GetDirectoryListingTask.h
@@ +7,5 @@
> +#ifndef mozilla_dom_GetDirectoryListing_h
> +#define mozilla_dom_GetDirectoryListing_h
> +
> +#include "mozilla/dom/FileSystemTaskBase.h"
> +#include "nsAutoPtr.h"

alphabetic order of the headers.

@@ +31,5 @@
> +  virtual
> +  ~GetDirectoryListingTask();
> +
> +  already_AddRefed<Promise>
> +  GetPromise();

const ?
Attachment #8626509 - Flags: review?(amarchesini) → review+
Comment on attachment 8626510 [details] [diff] [review]
part 3 - Implement .getContents() and .path for DOM Directory objects

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

Follow up for the workers, and maybe check in the spec or in the working group if we have to support Directory in the PostMessages.
Otherwise, without a Constructor() and without postMessages, there are no ways to have a Directory in workers.

::: dom/filesystem/Directory.cpp
@@ +235,5 @@
>    return task->GetPromise();
>  }
>  
> +void
> +Directory::GetPath(nsString& aRetval) const

GetPath(nsAString& aRetval) const

@@ +250,5 @@
> +already_AddRefed<Promise>
> +Directory::GetContents()
> +{
> +  nsresult error = NS_OK;
> +  nsString realPath;

nsAutoString

@@ +251,5 @@
> +Directory::GetContents()
> +{
> +  nsresult error = NS_OK;
> +  nsString realPath;
> +  ErrorResult aRv; // XXXjwatt

1. rv
2. plus... why this comment?

@@ +254,5 @@
> +  nsString realPath;
> +  ErrorResult aRv; // XXXjwatt
> +  nsRefPtr<GetDirectoryListingTask> task =
> +    new GetDirectoryListingTask(mFileSystem, mPath, aRv);
> +  if (aRv.Failed()) {

NS_WARN_IF

::: dom/webidl/Directory.webidl
@@ +7,1 @@
>  /*

Can you add the URL specs for Directory at the top of this file?

@@ +35,5 @@
>     * The 'data' property contains the new file's content.
>     * @return If succeeds, the promise is resolved with the new created
>     * File object. Otherwise, rejected with a DOM error.
>     */
> +  [Pref="device.storage.enabled", NewObject]

If you want to expose it to workers, you have to write Func="Directory::IsEnabled" instead Pref="..". Then you can implement that function similar to BroadcastChannel::IsEnabled, or MessageChannel::IsEnabled.

@@ +47,5 @@
>     * If path exists, createDirectory must fail.
>     * @return If succeeds, the promise is resolved with the new created
>     * Directory object. Otherwise, rejected with a DOM error.
>     */
> +  [Pref="device.storage.enabled", NewObject]

Same here.
Attachment #8626510 - Flags: review?(amarchesini) → review+
Comment on attachment 8626511 [details] [diff] [review]
part 4 - Add DeviceStorage tests for the new .getContents() and .path API on Directory

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

::: dom/devicestorage/test/test_fs_getContents.html
@@ +27,5 @@
> +// The root directory object.
> +var gRoot = null;
> +
> +function checkContents(contents) {
> +  is(contents.length, 4, "The sub-directory should contain four children");

move this line after 'expected' and do:

is (contents.length, expected.length, ...

@@ +58,5 @@
> +  ok(false, "Should not arrive at handleError! Error: " + e.name);
> +  devicestorage_cleanup();
> +}
> +
> +function createTestFiles(paths, callback) {

This seems == to what we have in test_fs_remove.html. Move it into a helper.js.

@@ +108,5 @@
> +  gStorage.getRoot().then(function(rootDir) {
> +    gRoot = rootDir;
> +    return rootDir.get("sub");
> +  }).then(function(subDir) {
> +    return subDir.getContents();

can you check if you can get data also in the sub directories? 'sub/sub2' for instance?
Attachment #8626511 - Flags: review?(amarchesini) → review+
Comment on attachment 8626508 [details] [diff] [review]
part 1 - Add API and functionality to the BlobImpl classes in order that BlobImpl's that are created from an nsIFile can provide information about whether or not the nsIFile was a directory

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

Yeah, looks like you'll need to add an 'isDirectory' member to FileBlobConstructorParams, and then you'll need to set BlobImplBase::mIsDirectory in the BlobChild::RemoteBlobImpl constructor if that arg is set.
Attachment #8626508 - Flags: feedback?(bent.mozilla) → feedback-
(Assignee)

Comment 10

4 years ago
Ah, I guess my testcase should also be checking instanceof and/or recursing as suggested by baku.

Ben, do you want to see an updated patch, or is it okay to just make those changes?
Flags: needinfo?(bent.mozilla)
I'd be happy to look over an interdiff!
Flags: needinfo?(bent.mozilla)
(Assignee)

Comment 12

4 years ago
Created attachment 8626887 [details] [diff] [review]
part 1b

Using FileBlobConstructorParams.isDirectory appropriately in all the places that we use FileBlobConstructorParams in Blob.cpp has quite a chain of knock-on effects. Seems like this needs an actual review.
Attachment #8626887 - Flags: review?(bent.mozilla)
Comment on attachment 8626887 [details] [diff] [review]
part 1b

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

Most of this looks fine, except you're adding quite a performance footgun (similar to another in blob code that we've been trying to stamp out nearly forever).

::: dom/base/File.h
@@ +603,5 @@
>    NS_DECL_ISUPPORTS_INHERITED
>  
>    BlobImplMemory(void* aMemoryBuffer, uint64_t aLength, const nsAString& aName,
>                   const nsAString& aContentType, int64_t aLastModifiedDate)
> +    : BlobImplBase(aName, aContentType, aLength, aLastModifiedDate, false)

Nit: I think it's always nicer to put the arg name in /* */ comments when using bool literals. E.g.:

  BlobImplBase(... aLastModifiedDate, /* aIsDirectory */ false)

@@ +779,5 @@
>  
>    BlobImplFile(const nsAString& aName, const nsAString& aContentType,
>                 uint64_t aLength, nsIFile* aFile,
>                 int64_t aLastModificationDate)
> +    : BlobImplBase(aName, aContentType, aLength, aLastModificationDate, false)

So this will do synchronous I/O on whatever thread happens to call it. The intent here is to have a constructor that doesn't require any synchronous I/O. Can you force the caller to pass in the aIsDirectory arg rather than filling it in with sync I/O?

In fact, can you do that for as many of these as possible? I hope there are only a few places (maybe only like one?) where we need to create a File object and we only have an nsIFile and are forced to hit the disk to fill in the metadata.
Attachment #8626887 - Flags: review?(bent.mozilla) → review-
(Assignee)

Comment 14

4 years ago
Created attachment 8628604 [details] [diff] [review]
part 1b

Ah, yeah, I pointed this out to baku in Whistler and we agreed to have a tri-state flag to indicate is-dir/isn't-dir/unknown, set the state in the places that matter, and MOZ_ASSERT in IsDirectory() that the state is set. Seems I didn't attach the updated patch after getting back to the UK though. Sorry about that. :(
Attachment #8626887 - Attachment is obsolete: true
Attachment #8628604 - Flags: review?(amarchesini)
Comment on attachment 8628604 [details] [diff] [review]
part 1b

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

::: dom/base/File.h
@@ +55,5 @@
> + * Used to indicate when a Blob/BlobImpl that was created from an nsIFile
> + * (when IsFile() will return true) was from an nsIFile for which
> + * nsIFile::IsDirectory() returned true.
> + */
> +enum BlobDirState : uint32_t {

can we move this into the Blob class or better in BlobImpl?

@@ +203,5 @@
>  
>    static already_AddRefed<File>
>    Create(nsISupports* aParent, const nsAString& aName,
>           const nsAString& aContentType, uint64_t aLength,
> +         int64_t aLastModifiedDate, BlobDirState aDirState);

What about having a defualt value? ..., BlobDirState aDirState = eUnknownIfDir)
Attachment #8628604 - Flags: review?(amarchesini) → review+
(Assignee)

Comment 16

4 years ago
(In reply to Andrea Marchesini (:baku) from comment #15)
> can we move this into the Blob class or better in BlobImpl?

That's what I was doing initially, but I then had to include File.h into various headers that were otherwise very slim since I couldn't forward declare class-nested enums.

> What about having a defualt value? ..., BlobDirState aDirState =
> eUnknownIfDir)

I'll do that assuming it doesn't seem bad for some reason when I write the code.
(Assignee)

Updated

4 years ago
Blocks: 1164310
Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.