Closed
Bug 1258095
Opened 9 years ago
Closed 9 years ago
Implement Directory::GetPath() correctly.
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
mozilla48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: baku, Assigned: baku)
Details
Attachments
(3 files, 5 obsolete files)
4.07 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
9.26 KB,
patch
|
Details | Diff | Splinter Review | |
7.68 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
Currently we return what GetName() returns, but that doesn't follow the spec.
Assignee | ||
Comment 1•9 years ago
|
||
Attachment #8732539 -
Flags: review?(bugs)
Comment 2•9 years ago
|
||
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-
Assignee | ||
Comment 3•9 years ago
|
||
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)
Assignee | ||
Comment 4•9 years ago
|
||
Attachment #8733344 -
Flags: review?(bugs)
Assignee | ||
Comment 5•9 years ago
|
||
getFilesAndDirectories() can make a quite big list. Better to have the array fallible.
Attachment #8733348 -
Flags: review?(bugs)
Comment 6•9 years ago
|
||
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•9 years ago
|
||
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 8•9 years ago
|
||
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-
Assignee | ||
Comment 9•9 years ago
|
||
> 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.
Assignee | ||
Comment 10•9 years ago
|
||
> 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•9 years ago
|
||
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-
Assignee | ||
Comment 12•9 years ago
|
||
Attachment #8733340 -
Attachment is obsolete: true
Attachment #8733589 -
Flags: review?(bugs)
Assignee | ||
Comment 13•9 years ago
|
||
Attachment #8733589 -
Attachment is obsolete: true
Attachment #8733589 -
Flags: review?(bugs)
Attachment #8733590 -
Flags: review?(bugs)
Assignee | ||
Updated•9 years ago
|
Attachment #8733590 -
Attachment description: path0.patch → patch 1 - OSFileSystem should have the root == the directory root.
Assignee | ||
Comment 14•9 years ago
|
||
Based on the new patch, if you are ok with it, can you reconsider the review of patch 2?
Updated•9 years ago
|
Attachment #8733590 -
Flags: review?(bugs) → review+
Comment 15•9 years ago
|
||
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
Assignee | ||
Comment 16•9 years ago
|
||
Attachment #8733754 -
Flags: review?(bugs)
Assignee | ||
Comment 17•9 years ago
|
||
> >+ 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.
Assignee | ||
Updated•9 years ago
|
Attachment #8733344 -
Attachment is obsolete: true
Comment 18•9 years ago
|
||
(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 19•9 years ago
|
||
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.
Assignee | ||
Comment 20•9 years ago
|
||
> 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)
Comment 21•9 years ago
|
||
(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?
Assignee | ||
Comment 22•9 years ago
|
||
> 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•9 years ago
|
||
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?
Updated•9 years ago
|
Flags: needinfo?(amarchesini)
Comment 24•9 years ago
|
||
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)
Assignee | ||
Comment 25•9 years ago
|
||
> 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)
Comment 27•9 years ago
|
||
Same with the getter returning the value.
Also explain then in the comment what value it is.
Assignee | ||
Comment 28•9 years ago
|
||
Attachment #8733590 -
Attachment is obsolete: true
Attachment #8735079 -
Flags: review?(bugs)
Updated•9 years ago
|
Attachment #8735079 -
Flags: review?(bugs) → review+
Comment 29•9 years ago
|
||
Comment 30•9 years ago
|
||
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."
Updated•9 years ago
|
Flags: needinfo?(amarchesini)
Assignee | ||
Comment 31•9 years ago
|
||
I thought we already found a good solution for local root handling. Didn't we?
Flags: needinfo?(amarchesini) → needinfo?(bugs)
Comment 33•9 years ago
|
||
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•9 years ago
|
||
baku improved the code and comment in the latest version of patch 1, so the issue in patch 2 got resolved there.
Comment 35•9 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/1910aa3c10fc
https://hg.mozilla.org/mozilla-central/rev/554e58d8b27d
https://hg.mozilla.org/mozilla-central/rev/7f1ca2749ce7
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•