Closed Bug 1263992 Opened 8 years ago Closed 8 years ago

remove Directory::DirectoryType enum

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)

References

Details

(Whiteboard: btpp-active)

Attachments

(2 files)

      No description provided.
Whiteboard: btpp-active
Attachment #8740505 - Flags: review?(bugs)
Blocks: 1257179
Blocks: 1188880
No longer blocks: 1257179
Could you hint a bit why we want this. Originally we were using some magical string value, then moved to enum and not drop the whole thing.
Flags: needinfo?(amarchesini)
It doesn't say anywhere in the spec that the path of the root directory must be '/'. It was my wrong interpretation of the spec.
I also discussed this with jwatt on IRC and he confirmed that what we should return '/' + name for the root directory as path.
Flags: needinfo?(amarchesini)
And that is the only reason why we have the type?
Comment on attachment 8740505 [details] [diff] [review]
1263992_enum.patch


> GetFileOrDirectoryTaskParent::GetFileOrDirectoryTaskParent(FileSystemBase* aFileSystem,
>                                                            const FileSystemGetFileOrDirectoryParams& aParam,
>                                                            FileSystemRequestParent* aParent)
>   : FileSystemTaskParentBase(aFileSystem, aParam, aParent)
>   , mIsDirectory(false)
>@@ -245,42 +237,29 @@ GetFileOrDirectoryTaskParent::IOWork()
>   // Whether we want to get the root directory.
>   bool exists;
>   nsresult rv = mTargetPath->Exists(&exists);
>   if (NS_WARN_IF(NS_FAILED(rv))) {
>     return rv;
>   }
> 
>   if (!exists) {
>-    if (mType == Directory::eNotDOMRootDirectory) {
>-      return NS_ERROR_DOM_FILE_NOT_FOUND_ERR;
>-    }
>-
>-    // If the root directory doesn't exit, create it.
>-    rv = mTargetPath->Create(nsIFile::DIRECTORY_TYPE, 0777);
>-    if (NS_WARN_IF(NS_FAILED(rv))) {
>-      return rv;
>-    }
Ok, so for devicestorage we do still create directories in CreateDirectoryTaskParent::IOWork()
But but... this code was explicitly added for devicestorage, so that root is created when needed.
Why can we remove it now? Was it originally for the case like testing where root might not be there?

>-          is(data[i].path, '/' + data[i].name, "Subdirectory path should be called '/' + leafname");
>+          is(data[i].path, aDirectory.path + '/' + data[i].name, "Subdirectory path should be called '/' + leafname");
Please update also the description of the check.


r+ for the patch, but please don't land before explaining why we can remove that mTargetPath->Create.
Attachment #8740505 - Flags: review?(bugs) → review+
We have those create() because of DeviceStorage. Basically it can happen that the root directory for some deviceStorage type doesn't exist. But I don't really like to have Create() there. It seems a huge bug.

Now, for our test suite those Create()s are not needed but I don't know if for the real device, they are, somehow.
In any case we are not going to ship new devices soon and in case we have problems, we can introduce a similar functionality. But if this happen, it will not be in Directory tasks. They must be into DeviceStorageFilesystem or in any other DeviceStorage code.

I would like to land the code as I proposed (plus your comments of course). What do you think about?
Flags: needinfo?(bugs)
Well, if we explicitly start breaking DeviceStorage implementation, we should remove the whole API (and I don't know whether that is possible). Otherwise we shouldn't regress its behavior.

So, safer option would be to detect that we're using devicestorage API, and in that case allow the creation of the directories.
Flags: needinfo?(bugs)
Attachment #8741436 - Flags: review?(bugs)
Comment on attachment 8741436 [details] [diff] [review]
1263992_create.patch

>+  virtual bool
>+  ShouldCreateDirectory() override
>+  {
>+    MOZ_CRASH("This should not be called.");
Could you explain why.
(like this being non-PBackgroun/IOWork object or something.)
Attachment #8741436 - Flags: review?(bugs) → review+
https://hg.mozilla.org/mozilla-central/rev/e41a5ba6a469
https://hg.mozilla.org/mozilla-central/rev/d03c5ce224dd
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.