Closed Bug 934367 Opened 11 years ago Closed 10 years ago

[Filesystem API] Implement createFile method for device storage.

Categories

(Core :: DOM: Device Interfaces, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: xyuan, Assigned: xyuan)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 5 obsolete files)

To be implemented:

interface Directory {
  ...
  Promise<File> createFile(DOMString path,
                           CreateFileOptions options);
  ...
}

dictionary CreateFileOptions {
  CreateIfExistsMode ifExists = "fail";
  (DOMString or Blob or ArrayBuffer or ArrayBufferView) data;
};
A bunch of the foundation work for this will be done by bug 910498, which will "remote" creating a file to the parent. I'm currently in the process of testing with the Camera app.
Assignee: nobody → xyuan
Status: NEW → ASSIGNED
Attached patch createFile v1 (obsolete) — Splinter Review
Implement Directory#createFile. This patch is based on the patches of bug 910412 and bug 934368.
Hi Dave. If you have time, please skim the patch for a preliminary review. Or 
wait until the bugs (bug 910412 and bug 934368) blocking this have been fixed.
I'll ask for a full review then.
Attachment #8384436 - Flags: feedback?(dhylands)
Blocks: 910387
Comment on attachment 8384436 [details] [diff] [review]
createFile v1

Review of attachment 8384436 [details] [diff] [review]:
-----------------------------------------------------------------

There are a few things that need to be addressed, but otherwise it looks ok to me.

There seems to be a bunch of stuff duplicated between this patch and the one from bug 934368

::: dom/filesystem/CreateDirectoryTask.cpp
@@ +98,5 @@
>    }
>  
>    bool ret;
>    nsresult rv = file->Exists(&ret);
> +  TASK_BASE_ENSURE_SUCCESS(rv);

I made this comment in the other review, but I'll repeat it here.

I think that the ENSURE_SUCCESS style macros are being deprecated, so we should put the 2 lines from the macro here instead (so that the return paths aren't hidden).

@@ +103,2 @@
>  
>    if (ret) {

nit: I think this would be clearer if ret were renamed fileExists

@@ +106,3 @@
>    }
>  
>    rv = file->Create(nsIFile::DIRECTORY_TYPE, 0777);

And now that all of the I/O is taking place in the parent, I don't think we need to give these files world permissions anymore.

That's something we should probably open a new bug for and fix this everywhere in device storage.

::: dom/filesystem/CreateFileTask.cpp
@@ +200,5 @@
> +
> +  nsCOMPtr<nsIOutputStream> bufferedOutputStream;
> +  rv = NS_NewBufferedOutputStream(getter_AddRefs(bufferedOutputStream),
> +                                  outputStream,
> +                                  4096 * 4);

This feels like something that should come from a preference so that it can be tuned up or down.

@@ +213,5 @@
> +    rv = mBlobStream->Available(&bufSize);
> +    TASK_BASE_ENSURE_SUCCESS(rv);
> +
> +    uint32_t writeCount =  bufSize;
> +    if (writeCount > UINT32_MAX) {

by definition, I don't see how this can ever be true.

I think that you should be comparing bufSize (which is a unit64_t) to UINT32_MAX.

I think that writeSize would be a better name for writeCount.

@@ +219,5 @@
> +    }
> +
> +    while (bufSize) {
> +      uint32_t wrote;
> +      rv = bufferedOutputStream->WriteFrom(mBlobStream, writeCount, &wrote);

nit: I'd use written rather than wrote.

Also writeCount needs to be max'd at bufSize (think about the 2nd iteration of this loop if bufSize started out at UINT32_MAX + 10). Otherwise this loop will always create files which are multiples of writeCount.

@@ +222,5 @@
> +      uint32_t wrote;
> +      rv = bufferedOutputStream->WriteFrom(mBlobStream, writeCount, &wrote);
> +      TASK_BASE_ENSURE_SUCCESS(rv);
> +      bufSize -= wrote;
> +    }

nit: I'd put the mTargetFile = new nsDOMFileFile(file) and return NS_OK here, and then you can unindent the next block

@@ +224,5 @@
> +      TASK_BASE_ENSURE_SUCCESS(rv);
> +      bufSize -= wrote;
> +    }
> +  } else {
> +	  // Write file content from array data.

nit: indentation

::: dom/filesystem/Directory.cpp
@@ +104,5 @@
> +  bool replace = (aOptions.mIfExists == CreateIfExistsMode::Replace);
> +
> +  // Get the file content.
> +  if (aOptions.mData.WasPassed()) {
> +    auto& data = aOptions.mData.Value();

Is this a typo?

I know the word auto is part of the language, but I've never actually seen it used.

What you wrote is the same thing as:

 int& data = ...

::: dom/filesystem/FilesystemTaskBase.h
@@ +26,5 @@
> +  do {                                                                    \
> +    if (NS_WARN_IF(NS_FAILED(res))) {                                     \
> +      return res;                                                         \
> +    }                                                                     \
> +  } while(0)

This change seems to be duplicated in your other patch that I reviewed as well.
Attachment #8384436 - Flags: feedback?(dhylands) → feedback+
(In reply to Dave Hylands [:dhylands] from comment #3)
---snip---
> ::: dom/filesystem/Directory.cpp
> @@ +104,5 @@
> > +  bool replace = (aOptions.mIfExists == CreateIfExistsMode::Replace);
> > +
> > +  // Get the file content.
> > +  if (aOptions.mData.WasPassed()) {
> > +    auto& data = aOptions.mData.Value();
> 
> Is this a typo?
> 
> I know the word auto is part of the language, but I've never actually seen
> it used.
> 
> What you wrote is the same thing as:
> 
>  int& data = ...

No, not a typo:) The |auto| is for type OwningStringOrBlobOrArrayBufferOrArrayBufferView, which 
is generated by webidl and quite long. I saw the indexedDB's implementation had used auto word
(http://mxr.mozilla.org/mozilla-central/source/dom/indexedDB/IDBObjectStore.cpp#2414) and decide 
to use it to simplify the code.
Attached patch interdiff for comment 3. (obsolete) — Splinter Review
Attached patch createFile v2 (obsolete) — Splinter Review
Fix items of comment 3 except the files world permissions issue (Let's solve it with a new bug) and the interdiff:

https://bugzilla.mozilla.org/attachment.cgi?id=8389065&action=diff
Attachment #8384436 - Attachment is obsolete: true
Attachment #8389066 - Flags: review?(dhylands)
BTW, the above patch depends on the patch of bug 934368.
Comment on attachment 8389066 [details] [diff] [review]
createFile v2

Review of attachment 8389066 [details] [diff] [review]:
-----------------------------------------------------------------

r- since the write loop logic still needs to be fixed.

Also, in light of bug 910412 comment 169 I think that you should get green try runs before requesting review.

Since the problems seem to intermittent, once you get a green try run, you can use the TBPL self-serve options and request that M2 (mochitest 2) be rerun a bunch of times (I'd recommend at least 5, and perhaps 10 times to start with). You'll need to select it for each time (I haven't found a convenient way of requesting N).

I took a look at a bunch of the TBPL failure reports and the OSX platform seems to be where most of these are coming from, so I would focus on that platform. I also saw one Ubuntu VM one as well. Make sure that you ask for debug builds (since the release builds don't do assert failures).

I also highly recommend that you do your local testing using debug builds.

::: dom/filesystem/CreateFileTask.cpp
@@ +230,5 @@
> +    if (NS_WARN_IF(NS_FAILED(rv))) {
> +      return rv;
> +    }
> +
> +    uint32_t writeSize =  bufSize < UINT32_MAX ? bufSize : UINT32_MAX;

This needs to be evaluated inside the loop so that writeSize will be reduced on the last iteration of the loop.
Attachment #8389066 - Flags: review?(dhylands) → review-
I found an easier way to retrigger.

Visit TBPL: (say your try run from bug 934368)
Click on the green 2 beside OSX 10.6 Debug
You'll see some stuff show up at the bottom. Click on the blue + once for each time you want to retrigger. You can click a whole bunch of times fairly quickly.
Attached patch createFile v3 (obsolete) — Splinter Review
Fix the while loop issue of comment 8 and add createFile permission test cases to test_fs_app_permissions.html.

try fails on windows: 
https://tbpl.mozilla.org/?tree=Try&rev=59697268b8ee

It seems there exists text encoding issue on windows. The tests failed to created files with array buffer and array buffer view as data, but succeeded with string and blob as data.

Need further investigation on why windows failed.
Attachment #8389066 - Attachment is obsolete: true
Attached patch createFile v4 (obsolete) — Splinter Review
The try's failure on windows is because when overwriting existing file, we didn't truncate the original data to empty. This patch fixed the issue.

https://tbpl.mozilla.org/?tree=Try&rev=048554852065
Attachment #8391077 - Attachment is obsolete: true
Attachment #8397697 - Attachment description: 0001-Bug-934367-Implement-createFile-for-Directory.patch → createFile v4
Retry and B2G KK builds are still broken:

https://tbpl.mozilla.org/?tree=Try&rev=e2db5bebaec3

I checked other latest try results and they were broken, too:

https://tbpl.mozilla.org/?tree=Try&pusher=cviecco@mozilla.com
https://tbpl.mozilla.org/?tree=Try&pusher=snigdhaagarwal93@gmail.com

So I think it's not my fault to break those builds.
Attachment #8397697 - Flags: review?(dhylands)
Attachment #8397697 - Flags: review?(dhylands) → review+
Depends on: 989997
Attached patch createFile v4Splinter Review
I examined the patch and didn't find any clue that causes the intermittent test failure of bug 989997. After I rebased the patch, the intermittent test failure disappeared. It seems the failure was caused by other gecko code, which was fixed already. 

So I ask to re-land the rebased patch. If any intermittent test failure is found again, please back it out.

The TBPL result https://tbpl.mozilla.org/?tree=Try&rev=ad99281c09b6
Attachment #8397697 - Attachment is obsolete: true
Attachment #8409617 - Flags: review+
Attachment #8389065 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/7010324e6153
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in before you can comment on or make changes to this bug.