Closed Bug 1258095 Opened 8 years ago Closed 8 years ago

Implement Directory::GetPath() correctly.

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)

Details

Attachments

(3 files, 5 obsolete files)

Currently we return what GetName() returns, but that doesn't follow the spec.
Attached patch path.patch (obsolete) — Splinter Review
Attachment #8732539 - Flags: review?(bugs)
Comment on attachment 8732539 [details] [diff] [review]
path.patch

># HG changeset patch
># User Andrea Marchesini <amarchesini@mozilla.com>
># Parent  2865cc0a28056c660cc51fde7f2a9e0d6eadd21b
>
>diff --git a/dom/filesystem/Directory.cpp b/dom/filesystem/Directory.cpp
>--- a/dom/filesystem/Directory.cpp
>+++ b/dom/filesystem/Directory.cpp
>@@ -346,20 +346,30 @@ Directory::RemoveInternal(const StringOr
>   return task->GetPromise();
> }
> 
> void
> Directory::GetPath(nsAString& aRetval, ErrorResult& aRv)
> {
>   if (mType == eDOMRootDirectory) {
>     aRetval.AssignLiteral(FILESYSTEM_DOM_PATH_SEPARATOR_LITERAL);
>-  } else {
>-    // TODO: this should be a bit different...
>-    GetName(aRetval, aRv);
>+    return;
>   }
odd that you need to have the check for root directory here, and not in
GetDOMPath()
The code would be simpler, and less error prone, if you pushed the check there.


>+FileSystemBase::GetDOMPath(nsIFile* aFile, nsAString& aRetval,
>+                           ErrorResult& aRv) const
>+{
>+  MOZ_ASSERT(aFile);
>+

so I would perhaps add a DirectoryType param, and then check its type here and just return
FILESYSTEM_DOM_PATH_SEPARATOR_LITERAL.
And perhaps in that case assert that the path taken from nsIFile is the same as GetLocalRootPath()


So we do have tests for subsubdirectories and such? and for root
Attachment #8732539 - Flags: review?(bugs) → review-
This is the first patch of 2. I need this to fix the real 'root' directory handling.
Attachment #8732539 - Attachment is obsolete: true
Attachment #8733340 - Flags: review?(bugs)
Attached patch patch 2 - GetPath() (obsolete) — Splinter Review
Attachment #8733344 - Flags: review?(bugs)
getFilesAndDirectories() can make a quite big list. Better to have the array fallible.
Attachment #8733348 - Flags: review?(bugs)
Comment on attachment 8733340 [details] [diff] [review]
patch 1 - OSFileSystem should have the root == the directory root.

Hmm, I don't understand this, but actually I don't know understand the old code either. OSFileSystem takes a root dir as param, but here we just pass some random path.
Comment on attachment 8733348 [details] [diff] [review]
patch 3 - FallibleTArray

This is totally unrelated to this bug, so in general should be done in a separate bug. But fine.
Attachment #8733348 - Flags: review?(bugs) → review+
Comment on attachment 8733340 [details] [diff] [review]
patch 1 - OSFileSystem should have the root == the directory root.

So based on the code in OSFileSystem this doesn't look right. But perhaps you're changing the meaning of mLocalRootPath? But I'm not sure.
Also, is this code execute only in parent process. Since the comment says
the path passed to OSFileSystem should be empty in child process.
Attachment #8733340 - Flags: review?(bugs) → review-
> Hmm, I don't understand this, but actually I don't know understand the old
> code either. OSFileSystem takes a root dir as param, but here we just pass
> some random path.

I guess because the name OSFileSystem is a bit misleading. We should call it FileSystemHelper.
The main task of this class is:

1. to keep the 'relative' root (as full path) for the current directory or for anything that is behind it. Based on this path we do validation to be 100% sure that nothing go out of that path.

2. to manage the permission (for DeviceStorage)

3. to help the managing of tasks via IPC.
> So based on the code in OSFileSystem this doesn't look right. But perhaps
> you're changing the meaning of mLocalRootPath? But I'm not sure.

See my previous comment.

> Also, is this code execute only in parent process. Since the comment says
> the path passed to OSFileSystem should be empty in child process.

Well, that comment is wrong. OSFileSystem exists in 2 contexts:

1. where a directory exists and where a directory wants to run some task.
2. where a task in the PBackground thread is actually doing the real I/O job.

I'll change that comment.
Comment on attachment 8733344 [details] [diff] [review]
patch 2 - GetPath()

So actually, it isn't clear to me how this all is supposed to work in child process where, per comments in the code, GetLocalRootPath() returns empty string.
I should have realized that issue before.
Either the comment is totally wrong, or we're very inconsistent.
But anyhow, it isn't clear to me anymore what mLocalRootPath is supposed to point to and in which process.
Attachment #8733344 - Flags: review?(bugs) → review-
Attachment #8733340 - Attachment is obsolete: true
Attachment #8733589 - Flags: review?(bugs)
Attachment #8733589 - Attachment is obsolete: true
Attachment #8733589 - Flags: review?(bugs)
Attachment #8733590 - Flags: review?(bugs)
Attachment #8733590 - Attachment description: path0.patch → patch 1 - OSFileSystem should have the root == the directory root.
Based on the new patch, if you are ok with it, can you reconsider the review of patch 2?
Attachment #8733590 - Flags: review?(bugs) → review+
Comment on attachment 8733344 [details] [diff] [review]
patch 2 - GetPath()

>+FileSystemBase::GetDOMPath(nsIFile* aFile,
>+                           Directory::DirectoryType aType,
>+                           nsAString& aRetval,
>+                           ErrorResult& aRv) const
>+{
>+  MOZ_ASSERT(aFile);
>+
>+  if (aType == Directory::eDOMRootDirectory) {
So can we MOZ_ASSERT here that the path from aFile is the same as GetLocalRootPath()?


>+    aRetval.AssignLiteral(FILESYSTEM_DOM_PATH_SEPARATOR_LITERAL);
>+    return;
>+  }
>+
>+  nsCOMPtr<nsIFile> fileSystemPath;
>+  aRv = NS_NewNativeLocalFile(NS_ConvertUTF16toUTF8(GetLocalRootPath()),
>+                              true, getter_AddRefs(fileSystemPath));
>+  if (NS_WARN_IF(aRv.Failed())) {
>+    return;
>+  }
>+
>+  bool equal = false;
>+
>+#ifdef DEBUG
>+  // It's possible that we are dealing with the root of the DOM filesystem.
>+  // In this case the 2 paths will be equal.
How? We should have returned already above.

>+  aRv = fileSystemPath->Equals(aFile, &equal);
>+  if (NS_WARN_IF(aRv.Failed())) {
>+    return;
>+  }
>+
>+  MOZ_ASSERT(equal || FileSystemUtils::IsDescendantPath(fileSystemPath, aFile));
I think equal should be already false here.

>+  if (parts.IsEmpty()) {
>+    // This is the root of the filesystem
>+    aRetval.AssignLiteral(FILESYSTEM_DOM_PATH_SEPARATOR_LITERAL);
>+    return;
>+  }
So I don't understand how we can come here without  
aType == Directory::eDOMRootDirectory
Attachment #8733754 - Flags: review?(bugs)
> >+  if (aType == Directory::eDOMRootDirectory) {
> So can we MOZ_ASSERT here that the path from aFile is the same as
> GetLocalRootPath()?


I cannot because this is valid only for OSFileSystem. DeviceStorage uses the parent directory.
Attachment #8733344 - Attachment is obsolete: true
(In reply to Andrea Marchesini (:baku) from comment #17)
> > >+  if (aType == Directory::eDOMRootDirectory) {
> > So can we MOZ_ASSERT here that the path from aFile is the same as
> > GetLocalRootPath()?
> 
> 
> I cannot because this is valid only for OSFileSystem. DeviceStorage uses the
> parent directory.

Could you explain this some more. Parent directory of what?
Flags: needinfo?(amarchesini)
Comment on attachment 8733590 [details] [diff] [review]
patch 1 - OSFileSystem should have the root == the directory root.

>+  // This path must be set by the FileSystem implementation immediatelly and
immediately 

>+  // because it will be used for the validation of any FileSystemTaskBase.
Is the 'and' before 'because' some leftover


>+  // The concept of this path is that, any task will never go out of it and this
>+  // must be consider
considered as

 the OS 'root' of the current FileSystem. Different
>+  // Directory object can have different OS 'root' path.
>+  // To be more clear, any path managed by this FileSystem implementation must
>+  // be discendant of this local root path.
>   nsString mLocalRootPath;


So this doesn't explain how devicestorage uses this somehow differently.
> Could you explain this some more. Parent directory of what?

If a Directory object points to /media/sd/photos/2016-03-23 the DeviceStorageFilesystem points to /media/sd/photos.
In general, DeviceStorageFileSystem points always to one of the storage type folder (photos, videos, these...).
Flags: needinfo?(amarchesini)
(In reply to Andrea Marchesini (:baku) from comment #20)
> > Could you explain this some more. Parent directory of what?
> 
> If a Directory object points to /media/sd/photos/2016-03-23 the
> DeviceStorageFilesystem points to /media/sd/photos.

Yes, but how is that different to OSFileSystem when one has selected 
/media/sd/photos from filepicker?

Or are you saying /media/sd/photos/2016-03-23 in DeviceStorage is the DOMRoot?, yet the local root points to /media/sd/photos when one has selected /media/sd/photos/2016-03-23 from a picker?
> Yes, but how is that different to OSFileSystem when one has selected 
> /media/sd/pictures from filepicker?

Scenario: the user selects /media/sd/picture/2016-03-23 from the directory picker.

In case of OSFileSystem:

Directory:
 - localPath = /media/sd/pictures/2016-03-23
 - type: eDOMRootPath
OSFileSystem: probably null. But when created:
 - path: /media/sd/pictures/2016-03-23

In case of DeviceStorageFileSystem:

Directory:
 - localPath = /media/sd/pictures/2016-03-23
 - type: eDOMRootPath
DeviceStorageFileSystem:
 - path: /media/sd/pictures

DeviceStorageFileSytem path points to the path of that storagetype (in this case 'pictures').
in nsDeviceStorage we have 3 storage types: pictures, videos, music. Each one with its own path.
Ok, so we're dangerously inconsistent here, IMO
The comment about mLocalRootPath needs to explain this all, and the name of the variable probably renamed so that it is clear it actually means two different things.

Or, could we not mark /media/sd/pictures/2016-03-23 being eDOMRootPath?
Flags: needinfo?(amarchesini)
Comment on attachment 8733754 [details] [diff] [review]
patch 2 - GetPath()

So clearing this request until the localroot handling it more understandable.
Attachment #8733754 - Flags: review?(bugs)
> The comment about mLocalRootPath needs to explain this all, and the name of
> the variable probably renamed so that it is clear it actually means two
> different things.

Well, I prefer to say that a FileSystemBase is not directly connected with the exposed Directory object.
Of course, Directory::path must be descendant of FileSystemBase::mLocalRootPath always but this is the only thing that matter here.
 
> Or, could we not mark /media/sd/pictures/2016-03-23 being eDOMRootPath?

No, because the workflow is: navigator.getDeviceStorage('pictures').getRoot().then(function(directory) { ...

I'm OK to change the name of mLocalRootPath, but I don't think we should spend to much effort on finding a good name for DeviceStorage behavior if this API will go away eventually.
Flags: needinfo?(amarchesini) → needinfo?(bugs)
Could we call it mLocalOrDeviceStorageRootPath ?
Flags: needinfo?(bugs)
Same with the getter returning the value.

Also explain then in the comment what value it is.
Attachment #8735079 - Flags: review?(bugs) → review+
FYI, patch 2 seems to have landed without an r+ - last comment with the removal of r? was comment 24 "So clearing this request until the localroot handling it more understandable."
Flags: needinfo?(amarchesini)
I thought we already found a good solution for local root handling. Didn't we?
Flags: needinfo?(amarchesini) → needinfo?(bugs)
yeah, the member variable was renamed and all.
Flags: needinfo?(bugs)
smaug: is that a post-facto r+?  Should we land a followup, or back that (or all of them) out until it's resolved?  (I haven't looked at the code really, and it's not my area anyways - just want a crisp answer here).
baku improved the code and comment in the latest version of patch 1, so the issue in patch 2 got resolved there.
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.