Closed
Bug 934368
Opened 11 years ago
Closed 11 years ago
[Filesystem API] Implement remove and removeDeep methods for device storage.
Categories
(Core :: DOM: Device Interfaces, defect)
Core
DOM: Device Interfaces
Tracking
()
RESOLVED
FIXED
mozilla31
People
(Reporter: xyuan, Assigned: xyuan)
References
(Blocks 1 open bug)
Details
Attachments
(3 files, 7 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
xyuan
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
dhylands
:
review+
|
Details | Diff | Splinter Review |
To be implemented:
interface Directory {
...
Promise<boolean> remove((DOMString or File or Directory) path);
Promise<boolean> removeDeep((DOMString or File or Directory) path);
...
}
Assignee | ||
Comment 1•11 years ago
|
||
Implement Directory#remove and #removeDeep. This patch is based on the patch of bug 910412.
Attachment #8380562 -
Flags: feedback?(dhylands)
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → xyuan
Status: NEW → ASSIGNED
Comment 2•11 years ago
|
||
Comment on attachment 8380562 [details] [diff] [review]
remove v1.patch
Review of attachment 8380562 [details] [diff] [review]:
-----------------------------------------------------------------
I'm not sure about the use of the word deep in describing the removal? Is that used in other places. I'm used to the term recursive. The option on rm is -r for remove recursive.
I understand the use of deep when talking about a graph, but we're talking about a filesystem here, so I would have expected filesystem terminology.
r=me with issues addressed.
::: dom/devicestorage/test/test_fs_remove.html
@@ +168,5 @@
> + case 0:
> + gRoot = r;
> + // Get sub1
> + gPath = "sub1";
> + gRoot.get("sub1").then(getSuccess, cbError);
It seems a bit fragile to me that this switch statement has to have indicies that match the order things appear in the gTest array.
Would it be possible to put each little snippet of code as an optional function in the array itself? This way the function stays with the test case.
::: dom/filesystem/DeviceStorageFilesystem.cpp
@@ +94,5 @@
> return file.forget();
> }
>
> +bool
> +DeviceStorageFilesystem::GetRealPath(nsIDOMFile *aFile, nsAString& aRealPath) const
nit: nsIDOMFile* aFile
@@ +103,5 @@
> +
> + aRealPath.Truncate();
> +
> + nsAutoString filePath;
> + if (NS_FAILED(aFile->GetMozFullPathInternal(filePath))) {
Just wanted to clarify that if this needs to call any filesystem related functions (like realpath or stat or anything like that), then it has to happen in the parent.
If its just doing string manipulations then it can stay in the child.
::: dom/filesystem/DeviceStorageFilesystem.h
@@ +33,5 @@
> virtual already_AddRefed<nsIFile>
> GetLocalFile(const nsAString& aRealPath) const MOZ_OVERRIDE;
>
> + virtual bool
> + GetRealPath(nsIDOMFile *aFile, nsAString& aRealPath) const MOZ_OVERRIDE;
nit: nsIDOMFile* aFile
::: dom/filesystem/FilesystemBase.h
@@ +15,1 @@
> class nsPIDOMWindow; // You need |#include "nsPIDOMWindow.h"| in CPP files.
I don't think that the comment is quite correct, so I'd be inclined to just remove it.
You only need the #include if you're in a cpp file that needs to dereference an nsIDOMFile pointer. That seems to be quite a common paradigm.
@@ +63,5 @@
> + * If succeeded, returns true. Otherwise, returns false and set aRealPath to
> + * empty string.
> + */
> + virtual bool
> + GetRealPath(nsIDOMFile *aFile, nsAString& aRealPath) const = 0;
nit: nsIDOMFile* aFile,
::: dom/filesystem/RemoveTask.cpp
@@ +113,5 @@
> +RemoveTask::Work()
> +{
> + MOZ_ASSERT(FilesystemUtils::IsParentProcess(),
> + "Only call from parent process!");
> + MOZ_ASSERT(!NS_IsMainThread(), "Only call on child thread!");
we've been using child/parent to describe the relationship between the processes. So I would say non-main to not confuse processes/threads.
@@ +186,5 @@
> + if (!dom::WrapNewBindingObject(cx, scopeObj, domError, &val)) {
> + return;
> + }
> + mPromise->MaybeReject(cx, val);
> + }
One side or the other of this if should return early, and then you can remove the else and unindent.
::: dom/ipc/PContent.ipdl
@@ +191,4 @@
> nsString realPath;
> };
>
> +union FilesystemPathOrFileValue
nit: trailing space
@@ +193,5 @@
>
> +union FilesystemPathOrFileValue
> +{
> + nsString;
> + PBlob;
missing names? (maybe they're optional?)
Attachment #8380562 -
Flags: feedback?(dhylands) → feedback+
Assignee | ||
Comment 3•11 years ago
|
||
Thanks for your quick reply:)
(In reply to Dave Hylands [:dhylands] from comment #2)
> Comment on attachment 8380562 [details] [diff] [review]
> remove v1.patch
>
> Review of attachment 8380562 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> I'm not sure about the use of the word deep in describing the removal? Is
> that used in other places. I'm used to the term recursive. The option on rm
> is -r for remove recursive.
>
> I understand the use of deep when talking about a graph, but we're talking
> about a filesystem here, so I would have expected filesystem terminology.
>
> r=me with issues addressed.
>
> ::: dom/devicestorage/test/test_fs_remove.html
> @@ +168,5 @@
> > + case 0:
> > + gRoot = r;
> > + // Get sub1
> > + gPath = "sub1";
> > + gRoot.get("sub1").then(getSuccess, cbError);
>
> It seems a bit fragile to me that this switch statement has to have indicies
> that match the order things appear in the gTest array.
>
> Would it be possible to put each little snippet of code as an optional
> function in the array itself? This way the function stays with the test case.
>
OK.
> ::: dom/filesystem/DeviceStorageFilesystem.cpp
> @@ +94,5 @@
> > return file.forget();
> > }
> >
> > +bool
> > +DeviceStorageFilesystem::GetRealPath(nsIDOMFile *aFile, nsAString& aRealPath) const
>
> nit: nsIDOMFile* aFile
>
> @@ +103,5 @@
> > +
> > + aRealPath.Truncate();
> > +
> > + nsAutoString filePath;
> > + if (NS_FAILED(aFile->GetMozFullPathInternal(filePath))) {
>
> Just wanted to clarify that if this needs to call any filesystem related
> functions (like realpath or stat or anything like that), then it has to
> happen in the parent.
>
> If its just doing string manipulations then it can stay in the child.
This function will only be called in the parent and the member variable mNormalizedLocalRootPath it depends is only available on the parent, so |MOZ_ASSERT(parent)| is used to ensure this function is called on parent.
>
> ::: dom/filesystem/DeviceStorageFilesystem.h
> @@ +33,5 @@
> > virtual already_AddRefed<nsIFile>
> > GetLocalFile(const nsAString& aRealPath) const MOZ_OVERRIDE;
> >
> > + virtual bool
> > + GetRealPath(nsIDOMFile *aFile, nsAString& aRealPath) const MOZ_OVERRIDE;
>
> nit: nsIDOMFile* aFile
>
> ::: dom/filesystem/FilesystemBase.h
> @@ +15,1 @@
> > class nsPIDOMWindow; // You need |#include "nsPIDOMWindow.h"| in CPP files.
>
> I don't think that the comment is quite correct, so I'd be inclined to just
> remove it.
>
> You only need the #include if you're in a cpp file that needs to dereference
> an nsIDOMFile pointer. That seems to be quite a common paradigm.
I'll remove it.
>
> @@ +63,5 @@
> > + * If succeeded, returns true. Otherwise, returns false and set aRealPath to
> > + * empty string.
> > + */
> > + virtual bool
> > + GetRealPath(nsIDOMFile *aFile, nsAString& aRealPath) const = 0;
>
> nit: nsIDOMFile* aFile,
>
> ::: dom/filesystem/RemoveTask.cpp
> @@ +113,5 @@
> > +RemoveTask::Work()
> > +{
> > + MOZ_ASSERT(FilesystemUtils::IsParentProcess(),
> > + "Only call from parent process!");
> > + MOZ_ASSERT(!NS_IsMainThread(), "Only call on child thread!");
>
> we've been using child/parent to describe the relationship between the
> processes. So I would say non-main to not confuse processes/threads.
>
> @@ +186,5 @@
> > + if (!dom::WrapNewBindingObject(cx, scopeObj, domError, &val)) {
> > + return;
> > + }
> > + mPromise->MaybeReject(cx, val);
> > + }
>
> One side or the other of this if should return early, and then you can
> remove the else and unindent.
>
> ::: dom/ipc/PContent.ipdl
> @@ +191,4 @@
> > nsString realPath;
> > };
> >
> > +union FilesystemPathOrFileValue
>
> nit: trailing space
>
> @@ +193,5 @@
> >
> > +union FilesystemPathOrFileValue
> > +{
> > + nsString;
> > + PBlob;
>
> missing names? (maybe they're optional?)
No:-) The union member for .ipdl file has no name.
Assignee | ||
Comment 4•11 years ago
|
||
(In reply to Dave Hylands [:dhylands] from comment #2)
> Comment on attachment 8380562 [details] [diff] [review]
> remove v1.patch
>
> Review of attachment 8380562 [details] [diff] [review]:
-- snip --
>
> ::: dom/filesystem/RemoveTask.cpp
> @@ +113,5 @@
> > +RemoveTask::Work()
> > +{
> > + MOZ_ASSERT(FilesystemUtils::IsParentProcess(),
> > + "Only call from parent process!");
> > + MOZ_ASSERT(!NS_IsMainThread(), "Only call on child thread!");
>
> we've been using child/parent to describe the relationship between the
> processes. So I would say non-main to not confuse processes/threads.
How about worker thread?
Assignee | ||
Comment 5•11 years ago
|
||
We need to resolve boolean value from C++. So I made this patch.
Attachment #8382258 -
Flags: review?(bugs)
Assignee | ||
Comment 6•11 years ago
|
||
Assignee | ||
Comment 7•11 years ago
|
||
Address comments and the interdiff:
https://bugzilla.mozilla.org/attachment.cgi?id=8382259&action=diff
1. Use the term recursive instead of deep.
2. Polish mochitest.
3. Fix nits and rebase.
Attachment #8380562 -
Attachment is obsolete: true
Attachment #8382267 -
Flags: review?(dhylands)
Updated•11 years ago
|
Attachment #8382258 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 8•11 years ago
|
||
Refactor and add permission check tests for "remove" and "removeDeep" functions.
Attachment #8382267 -
Attachment is obsolete: true
Attachment #8382267 -
Flags: review?(dhylands)
Attachment #8385968 -
Flags: review?(dhylands)
Comment 9•11 years ago
|
||
Hey Yuan! Anything I can help with here?
Assignee | ||
Comment 10•11 years ago
|
||
Could help to make the patch review quicker? :-)
Flags: needinfo?(ehsan)
Comment 11•11 years ago
|
||
Dave, do you have an ETA for the review on this patch?
Also, is there anybody else who can review this code? It seems like Dave has been reviewing most of it so far.
Flags: needinfo?(ehsan) → needinfo?(dhylands)
Comment 12•11 years ago
|
||
I'm hoping to review this on Monday. I had a couple of higher priority bugs pop up, which is why these were delayed.
Flags: needinfo?(dhylands)
Comment 13•11 years ago
|
||
Sounds good, thanks very much!
Assignee | ||
Comment 14•11 years ago
|
||
Ehsan and Dave, thank you. Dave gave me quite a lot help with file system related bugs and reviewed a bunch of code as well as bug 980372 and bug 980136.
Comment 15•11 years ago
|
||
Comment on attachment 8385968 [details] [diff] [review]
Part2 v3 Implement remove and removeDeep.patch
Review of attachment 8385968 [details] [diff] [review]:
-----------------------------------------------------------------
Looks reasonable to me.
::: dom/filesystem/CreateDirectoryTask.cpp
@@ +98,5 @@
> }
>
> bool ret;
> nsresult rv = file->Exists(&ret);
> + TASK_BASE_ENSURE_SUCCESS(rv);
I believe that the ENSURE_SUCCESS type macros have been deprecated (because they hide return paths).
https://groups.google.com/forum/#!topic/mozilla.dev.platform/1clMLuuhtWQ
::: dom/filesystem/Directory.cpp
@@ +146,5 @@
> + nsString realPath;
> + nsCOMPtr<nsIDOMFile> file;
> +
> + if (!fs) {
> + goto parameters_check_done;
Shouldn't this set an error?
Otherwise, why would a null-filesystem be acceptable? Perhaps add a comment to explain whats happening here if a null filesystem is allowed.
::: dom/filesystem/FileSystemBase.cpp
@@ +65,5 @@
>
> +bool
> +FileSystemBase::IsSafeDirectory(Directory* aDir) const
> +{
> + return true;
does it make sense to flip this around and assume false, unless the derived class says true?
It seems safer from an implementation standpoint (if somebody forgets to override then it will fail rather than succeed)
Attachment #8385968 -
Flags: review?(dhylands) → review+
Assignee | ||
Comment 16•11 years ago
|
||
(In reply to Dave Hylands [:dhylands] from comment #15)
> Comment on attachment 8385968 [details] [diff] [review]
--snip--
>
> ::: dom/filesystem/Directory.cpp
> @@ +146,5 @@
> > + nsString realPath;
> > + nsCOMPtr<nsIDOMFile> file;
> > +
> > + if (!fs) {
> > + goto parameters_check_done;
>
> Shouldn't this set an error?
Yes, I'll fix it.
>
> Otherwise, why would a null-filesystem be acceptable? Perhaps add a comment
> to explain whats happening here if a null filesystem is allowed.
filesystem is a weak reference. When the content window unloads, filesystem
is destroyed and its weak reference will be null before the directory, so
the code accepts a null pointer here.
Comment 17•11 years ago
|
||
(In reply to Yuan Xulei [:yxl] from comment #16)
> (In reply to Dave Hylands [:dhylands] from comment #15)
> > Comment on attachment 8385968 [details] [diff] [review]
> --snip--
> >
> > ::: dom/filesystem/Directory.cpp
> > @@ +146,5 @@
> > > + nsString realPath;
> > > + nsCOMPtr<nsIDOMFile> file;
> > > +
> > > + if (!fs) {
> > > + goto parameters_check_done;
> >
> > Shouldn't this set an error?
> Yes, I'll fix it.
> >
> > Otherwise, why would a null-filesystem be acceptable? Perhaps add a comment
> > to explain whats happening here if a null filesystem is allowed.
> filesystem is a weak reference. When the content window unloads, filesystem
> is destroyed and its weak reference will be null before the directory, so
> the code accepts a null pointer here.
Then I think a comment is definitely warranted (to explain what you just explained). Anytime "the intuitively obvious" is wrong, I think that a comment should be added.
Assignee | ||
Comment 18•11 years ago
|
||
update commit message.
Attachment #8382258 -
Attachment is obsolete: true
Attachment #8388994 -
Flags: review+
Assignee | ||
Comment 19•11 years ago
|
||
Attachment #8382259 -
Attachment is obsolete: true
Assignee | ||
Comment 20•11 years ago
|
||
Address all of comment 15 and the interdiff:
https://bugzilla.mozilla.org/attachment.cgi?id=8388995&action=diff
Try server result:
https://tbpl.mozilla.org/?tree=Try&rev=74e93e07f85f
Attachment #8385968 -
Attachment is obsolete: true
Attachment #8388997 -
Flags: review?(dhylands)
Comment 21•11 years ago
|
||
(In reply to Yuan Xulei [:yxl] from comment #20)
> Try server result:
> https://tbpl.mozilla.org/?tree=Try&rev=74e93e07f85f
I did some of the retriggering mentioned in bug 934367 and I'm seeing one orange 2's that seems to be related to device storage.
https://tbpl.mozilla.org/php/getParsedLog.php?id=35952644&tree=Try
Comment 22•11 years ago
|
||
I also see an orange M8 on B2G ICS Emulator Debug.
https://tbpl.mozilla.org/php/getParsedLog.php?id=35928071&tree=Try
Updated•11 years ago
|
Attachment #8388997 -
Flags: review?(dhylands)
Comment 23•11 years ago
|
||
So I cleared the review flag for the time being.
Lets get all these intermittent assertions and stuff cleaned up.
Assignee | ||
Comment 24•11 years ago
|
||
rebased
Attachment #8388994 -
Attachment is obsolete: true
Attachment #8391046 -
Flags: review+
Assignee | ||
Comment 25•11 years ago
|
||
rebase based on the patch of bug 910412.
And push to try:
https://tbpl.mozilla.org/?tree=Try&rev=59697268b8ee
Attachment #8388997 -
Attachment is obsolete: true
Assignee | ||
Comment 26•11 years ago
|
||
Comment on attachment 8391047 [details] [diff] [review]
Part2 v5 Implement remove and removeDeep.patch
All try server tests for this bug passed.
Attachment #8391047 -
Flags: review?(dhylands)
Comment 27•11 years ago
|
||
(In reply to Yuan Xulei [:yxl] from comment #26)
> Comment on attachment 8391047 [details] [diff] [review]
> Part2 v3 Implement remove and removeDeep.patch
>
> All try server tests for this bug passed.
It doesn't look like it. All of the Windows machines failed with message like these:
https://tbpl.mozilla.org/php/getParsedLog.php?id=36126697&tree=Try
Assignee | ||
Comment 28•11 years ago
|
||
(In reply to Dave Hylands [:dhylands] from comment #27)
> (In reply to Yuan Xulei [:yxl] from comment #26)
> > Comment on attachment 8391047 [details] [diff] [review]
> > Part2 v3 Implement remove and removeDeep.patch
> >
> > All try server tests for this bug passed.
>
> It doesn't look like it. All of the Windows machines failed with message
> like these:
> https://tbpl.mozilla.org/php/getParsedLog.php?id=36126697&tree=Try
I combined the patch of bug 934367 with that of this bug and the failed tests belonged to bug 934367.
Let's have a clean try:
https://tbpl.mozilla.org/?tree=Try&rev=253a81c2b108
Comment 29•11 years ago
|
||
I'm slightly confused about the patch.
The patch you're asking me to review says: Part2 v3 Implement remove and removeDeep.patch
and the patch it obsoletes is Part2 v4 Implement remove and removeDeep.patch
Certain things in the patch also don't make sense if the v3 one is newer.
Could you double check that you're asking me to review the correct patch?
Assignee | ||
Comment 30•11 years ago
|
||
Comment on attachment 8391047 [details] [diff] [review]
Part2 v5 Implement remove and removeDeep.patch
The version should be v5.
Sorry for my mistake.
Attachment #8391047 -
Attachment description: Part2 v3 Implement remove and removeDeep.patch → Part2 v5 Implement remove and removeDeep.patch
Comment 31•11 years ago
|
||
Comment on attachment 8391047 [details] [diff] [review]
Part2 v5 Implement remove and removeDeep.patch
Review of attachment 8391047 [details] [diff] [review]:
-----------------------------------------------------------------
Looks good to me
Attachment #8391047 -
Flags: review?(dhylands) → review+
Assignee | ||
Updated•11 years ago
|
Keywords: checkin-needed
Comment 32•11 years ago
|
||
https://hg.mozilla.org/integration/b2g-inbound/rev/8c1ce4098df7
https://hg.mozilla.org/integration/b2g-inbound/rev/de5e89c89e37
Flags: in-testsuite+
Keywords: checkin-needed
Comment 33•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/8c1ce4098df7
https://hg.mozilla.org/mozilla-central/rev/de5e89c89e37
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Comment 34•11 years ago
|
||
Comment on attachment 8391047 [details] [diff] [review]
Part2 v5 Implement remove and removeDeep.patch
Review of attachment 8391047 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/filesystem/Directory.cpp
@@ +140,5 @@
> + // Check and get the target path.
> +
> + if (aPath.IsFile()) {
> + file = aPath.GetAsFile();
> + goto parameters_check_done;
"goto" ?
The first thing they taught us in the school was "never use goto" :)
I went through mozilla/dom and it seems that only
dom/bluetooth/bluez uses it.
::: dom/filesystem/Directory.h
@@ +70,4 @@
> // =========== End WebIDL bindings.============
> +
> + FileSystemBase*
> + GetFileSystem() const;
Nit: an empty line missing here
Assignee | ||
Comment 35•11 years ago
|
||
(In reply to Jan Varga [:janv] from comment #34)
> Comment on attachment 8391047 [details] [diff] [review]
> Part2 v5 Implement remove and removeDeep.patch
>
> Review of attachment 8391047 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> ::: dom/filesystem/Directory.cpp
> @@ +140,5 @@
> > + // Check and get the target path.
> > +
> > + if (aPath.IsFile()) {
> > + file = aPath.GetAsFile();
> > + goto parameters_check_done;
>
> "goto" ?
> The first thing they taught us in the school was "never use goto" :)
The abuse of 'goto' makes code hard to understand and maintain. I think that's why we should avoid 'goto'.
In this case, 'goto' makes the code much easier without nested 'if-else's. I can convert 'goto' to a while loop with trick, but that will make the code less readable. So I choose to use 'goto'. :-)
>
> I went through mozilla/dom and it seems that only
> dom/bluetooth/bluez uses it.
>
> ::: dom/filesystem/Directory.h
> @@ +70,4 @@
> > // =========== End WebIDL bindings.============
> > +
> > + FileSystemBase*
> > + GetFileSystem() const;
>
> Nit: an empty line missing here
I'll fix it in bug 935883.
Comment 36•11 years ago
|
||
(In reply to Yuan Xulei [:yxl] from comment #35)
> (In reply to Jan Varga [:janv] from comment #34)
> > Comment on attachment 8391047 [details] [diff] [review]
> > Part2 v5 Implement remove and removeDeep.patch
> >
> > Review of attachment 8391047 [details] [diff] [review]:
> > -----------------------------------------------------------------
> >
> > ::: dom/filesystem/Directory.cpp
> > @@ +140,5 @@
> > > + // Check and get the target path.
> > > +
> > > + if (aPath.IsFile()) {
> > > + file = aPath.GetAsFile();
> > > + goto parameters_check_done;
> >
> > "goto" ?
> > The first thing they taught us in the school was "never use goto" :)
> The abuse of 'goto' makes code hard to understand and maintain. I think
> that's why we should avoid 'goto'.
> In this case, 'goto' makes the code much easier without nested 'if-else's. I
> can convert 'goto' to a while loop with trick, but that will make the code
> less readable. So I choose to use 'goto'. :-)
Well, at the very least, I would add a comment that justifies this exception :)
You need to log in
before you can comment on or make changes to this bug.
Description
•