1258095 - Implement Directory::GetPath() correctly.

Status: RESOLVED FIXED

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

Description

[Andrea Marchesini [:baku]]

Currently we return what GetName() returns, but that doesn't follow the spec.

Comment 1

[Andrea Marchesini [:baku]]

Attachment: path.patch

Comment 2

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

Comment on attachment 8732539 [details] [diff] [review]
path.patch

># HG changeset patch
># User Andrea Marchesini <[e-mail]>
># 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

Comment 3

[Andrea Marchesini [:baku]]

Attachment: patch 1 - OSFileSystem should have the root == the directory root.

This is the first patch of 2. I need this to fix the real 'root' directory handling.

Comment 4

[Andrea Marchesini [:baku]]

Attachment: patch 2 - GetPath()

Comment 5

[Andrea Marchesini [:baku]]

Attachment: patch 3 - FallibleTArray

getFilesAndDirectories() can make a quite big list. Better to have the array fallible.

Comment 6

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

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 7

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

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.

Comment 8

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

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.

Comment 9

[Andrea Marchesini [:baku]]

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

Comment 10

[Andrea Marchesini [:baku]]

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

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

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.

Comment 12

[Andrea Marchesini [:baku]]

Attachment: patch 1 - OSFileSystem should have the root == the directory root.

Comment 13

[Andrea Marchesini [:baku]]

Attachment: patch 1 - OSFileSystem should have the root == the directory root.

Comment 14

[Andrea Marchesini [:baku]]

Based on the new patch, if you are ok with it, can you reconsider the review of patch 2?

Comment 15

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

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

Comment 16

[Andrea Marchesini [:baku]]

Attachment: patch 2 - GetPath()

Comment 17

[Andrea Marchesini [:baku]]

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

Comment 18

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

(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?

Comment 19

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

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.

Comment 20

[Andrea Marchesini [:baku]]

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

Comment 21

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

(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?

Comment 22

[Andrea Marchesini [:baku]]

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

Comment 23

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

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?

Comment 24

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

Comment on attachment 8733754 [details] [diff] [review]
patch 2 - GetPath()

So clearing this request until the localroot handling it more understandable.

Comment 25

[Andrea Marchesini [:baku]]

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

Comment 26

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

Could we call it mLocalOrDeviceStorageRootPath ?

Comment 27

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

Same with the getter returning the value.

Also explain then in the comment what value it is.

Comment 28

[Andrea Marchesini [:baku]]

Attachment: patch 1 - OSFileSystem should have the root == the directory root.

Comment 29

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1910aa3c10fc
https://hg.mozilla.org/integration/mozilla-inbound/rev/554e58d8b27d
https://hg.mozilla.org/integration/mozilla-inbound/rev/7f1ca2749ce7

Comment 30

[Randell Jesup [:jesup] (needinfo me)]

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

Comment 31

[Andrea Marchesini [:baku]]

I thought we already found a good solution for local root handling. Didn't we?

Comment 32

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

yeah, the member variable was renamed and all.

Comment 33

[Randell Jesup [:jesup] (needinfo me)]

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

Comment 34

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

baku improved the code and comment in the latest version of patch 1, so the issue in patch 2 got resolved there.

Comment 35

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

https://hg.mozilla.org/mozilla-central/rev/1910aa3c10fc
https://hg.mozilla.org/mozilla-central/rev/554e58d8b27d
https://hg.mozilla.org/mozilla-central/rev/7f1ca2749ce7