855952 - [DeviceStorage] support append file operation on b2g device

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

The MediaRecorder would get encoder data in js context during recording.
We need an API which allows application can save fragmental data during recording.
Otherwise, application only can save the whole blob when user stop it. (Or use a timer try to save the whole data...)

Comment 1

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

oh, API has implemented this by
getEditable and add function. close it.

Comment 2

[Alan Huang]

Attachment: WIP, v1

I think this bug needs something like this, an Append API.

Hello Doug,
Can you help to take a look and give some advise? Many thanks.

Comment 3

[Alan Huang]

Comment on attachment 743412 [details] [diff] [review]
WIP, v1

Hello Dave,

I know DeviceStorage has lots of changes after this proposal. Can you help to take a look at this, to see is this a right direction to do this? Thank you.

Comment 4

[Dave Hylands [:dhylands]]

Comment on attachment 743412 [details] [diff] [review]
WIP, v1

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

So overall, it looks reasonable. You'll need to add in the composite file support.

::: dom/devicestorage/nsDeviceStorage.cpp
@@ +673,5 @@
>  
>  nsresult
> +DeviceStorageFile::Append(nsIInputStream* aInputStream)
> +{
> +  if (!aInputStream) {

This will need to also check || !mFile (as per ::Write)

@@ +692,5 @@
> +
> +  if (!outputStream) {
> +    return NS_ERROR_FAILURE;
> +  }
> +

It would be good to factor the common code (from about here to the end of the function) and have Write and Append both call the common code. Perhaps call the new function WriteInternal (and make it private to DeviceStorageFile)

The only real difference is how the file is opened.

@@ +1545,5 @@
> +      NS_DispatchToMainThread(event);
> +      return NS_OK;
> +    }
> +
> +    nsresult rv = mFile->Write(stream);

Shouldn't this be calling Append?

@@ +1548,5 @@
> +
> +    nsresult rv = mFile->Write(stream);
> +
> +    if (NS_FAILED(rv)) {
> +      mFile->mFile->Remove(false);

And we probably don't want to remove the file if the append fails

@@ +2266,5 @@
> +  DeviceStorageTypeChecker* typeChecker = DeviceStorageTypeChecker::CreateOrGet();
> +  if (!typeChecker) {
> +    return NS_ERROR_FAILURE;
> +  }
> +

You'll need to add a similar IsComposite() check as you'll find in AddNamed.

::: dom/ipc/PContent.ipdl
@@ +88,5 @@
>  };
>  
> +struct DeviceStorageAppendParams
> +{
> +  nsString type;

It looks like you haven't picked up the composite file changes. You'll need to add a storageName field here as well.

Comment 5

[Jason Smith [:jsmith]]

Unrelated to media recording API directly, so I'm removing this from that tracker bug.

Comment 6

[Fernando Campo (:fcampo)]

I'd like to shake a little this bug, as it's also needed for exporting contacts (bug 910182 comment 16).

Not sure if it's better to reuse this one or create one specifically for contacts, but the title is descriptive enough, so I'll go with former option.

Hey Alan, any chance that you have some free cycles to keep working on it? :)

Comment 7

[Alan Huang]

Yeah it has been in the backlog for a long time. Let's do this in 1.5 :)

Comment 8

[Gabriele Svelto [:gsvelto]]

We also have bug 859696 tracking this issue, shall we merge the two bugs and DUP one of them?

Comment 9

[Dave Hylands [:dhylands]]

I don't think that bug 859656 is a dup. You could implement the append operation if you had the getEditable operation, but append certainly can't' be used to implement getEditable.

Comment 10

[Lawrence Mandel [:lmandel] (use needinfo)]

Attempting to move noms out of the general component into a component that will give them action. If I've guessed the wrong component, reassign as necessary.

Comment 11

[Bruce Huang [:bhuang] (inactive)]

Moving it out of blocking, but adding priority flag so it gets some traction.

Comment 12

[Alphan Chen [:alchen]]

Attachment: [DeviceStorage] support append file operation

Hi Dave,
here is the update patch for append file operation.
Please have a review.
Thanks.

Comment 13

[Alphan Chen [:alchen]]

Attachment: (0529) Test case for append file operation

Mochitest for append file operation.

Comment 14

[Alphan Chen [:alchen]]

Attachment: (0529v2) Test case for append file operation

Correct one typo.

Comment 15

[Alphan Chen [:alchen]]

Attachment: (0529v3) Test case for append file operation

Refactor the testcase.
And run it on try server first.

Comment 16

[Dave Hylands [:dhylands]]

Comment on attachment 8430580 [details] [diff] [review]
[DeviceStorage] support append file operation

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

This looks good so far, but I'd like to see some refactoring (as mentioned) and you really need to add some tests, and I'd like to see a try run (devicestorage tests are currently running as M2)

Please put DeviceStorage.webidl as a separate patch and request review from :bzbarsky

::: dom/devicestorage/DeviceStorageRequestParent.cpp
@@ +59,5 @@
>      }
>  
> +    case DeviceStorageParams::TDeviceStorageAppendParams:
> +    {
> +      DeviceStorageAddParams p = mParams;

nit: Shouldn't this me AppendParams? (not AddParams)

@@ +267,5 @@
>      }
>  
> +    case DeviceStorageParams::TDeviceStorageAppendParams:
> +    {
> +      DeviceStorageAddParams p = mParams;

nit: Shouldn't this be DeviceStorageAppendParams?

::: dom/devicestorage/nsDeviceStorage.cpp
@@ +1155,5 @@
> +//  bool check = false;
> +//  mFile->Exists(&check);
> +//  if (!check) {
> +//    return NS_ERROR_FILE_TARGET_DOES_NOT_EXIST;
> +//  }

Why is this code commented out?

I'm going to guess that it's because you're allowing the file to be created below. However, if you create the file, you should be generating a "created" IOCompleteEvent. If so, please remove the commented out code.

Since the tail end of this and the tail end of Write are identical, I'd like to see some refactoring.

Perhaps have Write create the file and then call an Append method which takes an input and output stream, and have Append(inputStream) create/open the output stream and then call the Append method which takes an input/output stream.

@@ +2481,4 @@
>    nsRefPtr<DOMRequest> mRequest;
>  };
>  
> +class AppendFileEvent : public nsRunnable

I think that this logic and the logic from WriteFileEvent can be combined into WriteFileEvent.

The only difference seems to be whether the file exists already or not, so you could just add an additional argument to the constructor to determine whether the file should exist already or not exist.

The same should be done with DeviceStorageRequestParent::AppendFileEvent (fold it into WriteFileEvent)

@@ +3684,5 @@
> +}
> +
> +already_AddRefed<DOMRequest>
> +nsDOMDeviceStorage::AppendNamed(nsIDOMBlob* aBlob, const nsAString& aPath,
> +                                ErrorResult& aRv)

I think that AppendNamed and AddNamed are virtually identical, so you should create one common function (perhaps called AddOrAppendNamed) which takes DEVICE_STORAGE_REQUEST_APPEND or DEVICE_STORAGE_REQUEST_CREATE as an argument.

Comment 17

[Alphan Chen [:alchen]]

(In reply to Dave Hylands [:dhylands] from comment #16)
> Comment on attachment 8430580 [details] [diff] [review]
> [DeviceStorage] support append file operation
> 
> Review of attachment 8430580 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks good so far, but I'd like to see some refactoring (as mentioned)
> and you really need to add some tests, and I'd like to see a try run
> (devicestorage tests are currently running as M2)
> 
> Please put DeviceStorage.webidl as a separate patch and request review from
> :bzbarsky
> 
> ::: dom/devicestorage/DeviceStorageRequestParent.cpp
> @@ +59,5 @@
> >      }
> >  
> > +    case DeviceStorageParams::TDeviceStorageAppendParams:
> > +    {
> > +      DeviceStorageAddParams p = mParams;
> 
> nit: Shouldn't this me AppendParams? (not AddParams)
> 
> @@ +267,5 @@
> >      }
> >  
> > +    case DeviceStorageParams::TDeviceStorageAppendParams:
> > +    {
> > +      DeviceStorageAddParams p = mParams;
> 
> nit: Shouldn't this be DeviceStorageAppendParams?
> 
> ::: dom/devicestorage/nsDeviceStorage.cpp
> @@ +1155,5 @@
> > +//  bool check = false;
> > +//  mFile->Exists(&check);
> > +//  if (!check) {
> > +//    return NS_ERROR_FILE_TARGET_DOES_NOT_EXIST;
> > +//  }
> 
> Why is this code commented out?

At first, I comment this out due to duplication.
There is same check in "DeviceStorageRequestParent::AppendFileEvent::CancelableRun()".

I will have new patch with "Append" and "Add" refactory.

Comment 18

[Dave Hylands [:dhylands]]

(In reply to Alphan Chen[:Alphan] from comment #17)
> (In reply to Dave Hylands [:dhylands] from comment #16)
> > ::: dom/devicestorage/nsDeviceStorage.cpp
> > @@ +1155,5 @@
> > > +//  bool check = false;
> > > +//  mFile->Exists(&check);
> > > +//  if (!check) {
> > > +//    return NS_ERROR_FILE_TARGET_DOES_NOT_EXIST;
> > > +//  }
> > 
> > Why is this code commented out?
> 
> At first, I comment this out due to duplication.
> There is same check in
> "DeviceStorageRequestParent::AppendFileEvent::CancelableRun()".
> 
> I will have new patch with "Append" and "Add" refactory.

So my point was, that you shouldn't check it a patch with code that's commented out. The commented-out code should either be removed entirely, or it should be uncommented.

Comment 19

[Alphan Chen [:alchen]]

Attachment: (0609) [DeviceStorage] support append file operation

Here is the patch with the suggestion above.
Please have a review.
Thanks.

Comment 20

[Alphan Chen [:alchen]]

Attachment: (0609) [DeviceStorage] add appendNamed into DeviceStorage webidl

I add appendNamed for append file operation.
Please have a look.
Thanks.

Comment 21

[Alphan Chen [:alchen]]

Attachment: (0609) [DeviceStorage] Test case for append file operation

Here is the try server result with attachment 8436676 [details] [diff] [review], attachment 8436677 [details] [diff] [review] and this testcase.
https://tbpl.mozilla.org/?tree=Try&rev=ce55f6f67b18

Comment 22

[Dave Hylands [:dhylands]]

Comment on attachment 8436678 [details] [diff] [review]
(0609) [DeviceStorage] Test case for append file operation

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

r=me with nits addressed

::: dom/devicestorage/test/test_fs_appendFile.html
@@ +17,5 @@
> +<body>
> +<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=855952">Mozilla Bug 855952</a>
> +<p id="display"></p>
> +<div id="content" style="display: none">
> +  

nit: trailing space

@@ +24,5 @@
> +<script class="testbody" type="text/javascript">
> +
> +var file   = new Blob(["This is a text file."], {type: "text/plain"});
> +var appendFile   = new Blob([" Another text file."], {type: "text/plain"});
> +var filename = "devicestorage/append.asc"

nit: call this gFilename, or just remove it entirely and just pass "devicestorage/append.asc" into the call to addNamed on line 79.

All of the follow on functions should be able to use e.target.result (or a sub-field) to get a full qualified filename.

@@ +40,5 @@
> +
> +function appendSuccess(e) {
> +  ok(true, "appendSuccess was called.");
> +  var storage = navigator.getDeviceStorage("sdcard");
> +  request = storage.delete(filename)

nit: use e.target.result rather than filename.

If you want to clarify, then add:

var filename = e.target.result

@@ +56,5 @@
> +
> +  var storage = navigator.getDeviceStorage("sdcard");
> +  ok(navigator.getDeviceStorage, "Should have getDeviceStorage");
> +
> +  request = storage.appendNamed(appendFile, filename);

nit: rather than reusing filename, use e.target.result

@@ +78,5 @@
> +  // Add a file first
> +  request = storage.addNamed(file, filename);
> +  ok(request, "Should have a non-null request");
> +  request.onsuccess = addSuccess;
> +  request.onerror = addError;  

nit: trailing space

@@ +82,5 @@
> +  request.onerror = addError;  
> +}
> +
> +function runtest() {
> +  var storage = navigator.getDeviceStorage("sdcard");

nit: If you were to store storage as a global (i.e. put these 2 lines just before the call to runtest) then you could just use it rather than re-fetching in all of the other functions (like appendSuccess, addSuccess, addError, addFile)

@@ +84,5 @@
> +
> +function runtest() {
> +  var storage = navigator.getDeviceStorage("sdcard");
> +  ok(navigator.getDeviceStorage, "Should have getDeviceStorage");
> +  

nit: trailing space

Comment 23

[Dave Hylands [:dhylands]]

Comment on attachment 8436676 [details] [diff] [review]
(0609) [DeviceStorage] support append file operation

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

Looks great.

r=me with nits addressed.

::: dom/devicestorage/nsDeviceStorage.cpp
@@ +2450,4 @@
>      if (NS_FAILED(rv)) {
> +      if (mRequestType == DEVICE_STORAGE_REQUEST_CREATE) {
> +        mFile->mFile->Remove(false);
> +      }

nit: I'd move the file removal into the if statement above (if the write fails, remove the file)

@@ +3617,5 @@
> +        (aRequestType == DEVICE_STORAGE_REQUEST_CREATE))
> +    {
> +      return ds->AddOrAppendNamed(aBlob, storagePath,
> +                                  aRequestType, aRv);
> +    } else {

nit: No need for else after a return. Remove the else and unindent the following 2 lines.

@@ +3621,5 @@
> +    } else {
> +      aRv.Throw(NS_ERROR_UNEXPECTED);
> +      return nullptr;
> +    }
> +

nit: I'd probably lose the extra checking here. You can replace the entire if statement above with:

return ds->AddOrAppendNamed(aBlob, storagePath, aRequestType, aRv);

The checking of aRequestType is being done below on line 3639.

@@ +3635,5 @@
>    } else if (!typeChecker->Check(mStorageType, dsf->mFile) ||
>        !typeChecker->Check(mStorageType, aBlob)) {
>      r = new PostErrorEvent(request, POST_ERROR_EVENT_ILLEGAL_TYPE);
>    } else {
> +    if (aRequestType == DEVICE_STORAGE_REQUEST_APPEND) {

nit: I'd probably just make this an else if () { rather than an else { if ()

It probably makes sense to make it look like:

} else if (aRequestType == DEVICE_STORAGE_REQUEST_APPEND || aRequestType == DEVICE_STORAGE_REQUEST_CREATE) {
  r = new DeviceStorageRequest(aRequestType,
		win, mPrincipal, dsf, request, aBlob);
} else {
  aRv.Throw(NS_ERROR_UNEXPECTED);
  return nullptr;
}

Comment 24

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8436677 [details] [diff] [review]
(0609)  [DeviceStorage] add appendNamed into DeviceStorage webidl

How does appendNamed() differ from addNamed()?  Documentation is sorely needed.

Also, in general asking for review of just IDL with no context is not very useful.  It just means I have to go try to find the context myself...

In this case, why do we need to change nsIDOMDeviceStorage.idl?

Comment 25

[Alphan Chen [:alchen]]

(In reply to Boris Zbarsky [:bz] from comment #24)
> Comment on attachment 8436677 [details] [diff] [review]
> (0609)  [DeviceStorage] add appendNamed into DeviceStorage webidl
> 
> How does appendNamed() differ from addNamed()?  Documentation is sorely
> needed.

appendNamed() is used for append context into one existing file.
In some use cases, append operation is useful.

> 
> Also, in general asking for review of just IDL with no context is not very
> useful.  It just means I have to go try to find the context myself...
> 

Sorry about that, I will re-create new patches.

> In this case, why do we need to change nsIDOMDeviceStorage.idl?

In this implementation, I try to let AppendNamed much the same as AddNamed.
There are both idl and webidl for AddNamed case.
What is your opinion? 
Does "remove .idl part for AppendNamed case" make sense to you?

Comment 26

[Alphan Chen [:alchen]]

Attachment: [DeviceStorage] Testcase for append file operation. r=dhylands

Upload a revised version with some fixes.

Comment 27

[Boris Zbarsky [:bzbarsky]]

> appendNamed() is used for append context into one existing file.

Don't tell _me_ that.  Put in in comments in the IDL.  What does it do if there is no existing file?  How does it pick the file to append to?

Basically, document the interface.  That's something that every interface not already documented by a spec should have, and I'm a bit sad that DeviceStorage is totally undocumented right now... but at least document the new thing.

> Does "remove .idl part for AppendNamed case" make sense to you?

Yes, unless you expect people to use it from C++.

Comment 28

[Alphan Chen [:alchen]]

Attachment: [DeviceStorage] support append file operation. r=dhylands

Upload a revised version.

Comment 29

[Alphan Chen [:alchen]]

Attachment: (0610) [DeviceStorage] add appendNamed into DeviceStorage webidl

Hi Boris,
The patch is based on comment 24 and comment 27.
Please have a review.
Thanks.

Comment 30

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8438185 [details] [diff] [review]
(0610) [DeviceStorage] add appendNamed into DeviceStorage webidl

>+   * Append a file inside a given file.

Maybe "append data to a given file"?

>+   * If the file doesn't exist, a POST_ERROR_EVENT_FILE_DOES_NOT_EXIST event will be dispatched.

That's an odd way to write "NotFoundError" in this case...

Also, what will be returned in this situation?

>+   * @parameter aBlob: A Blob object representing the file to append

s/file/data/?

>+   * @parameter aName: A string representing the full name (path + file name) of the file.

"of the file to append data to"?

r=me with that, assuming people have sane ways of getting the full names from somwhere.

Comment 31

[Dave Hylands [:dhylands]]

(In reply to Boris Zbarsky [:bz] from comment #30)
> Comment on attachment 8438185 [details] [diff] [review]
> (0610) [DeviceStorage] add appendNamed into DeviceStorage webidl
> r=me with that, assuming people have sane ways of getting the full names
> from somwhere.

I'll have to make a bug to get everything in the .idl/webidl stuff documented properly - I inherited that way :(

All of the device storage APIs that return paths always try to return fully-qualified paths. By fully qualified I mean /volume-name/path/file  These paths are only usable inside device storage, and you can't use an nsIFile with the same path (so device storage is hiding the physical location).

Comment 32

[Dave Hylands [:dhylands]]

I filed bug 1024029 for getting the remainder of the devicestorage stuff documented.

Comment 33

[Alphan Chen [:alchen]]

> 
> Maybe "append data to a given file"?
> 
> >+   * If the file doesn't exist, a POST_ERROR_EVENT_FILE_DOES_NOT_EXIST event will be dispatched.
> 
> That's an odd way to write "NotFoundError" in this case...

Now "appending data to a non-existing file" is the same behavior as "reading data from a non-existing file".
Do you suggest creating the file if it doesn't exist?

> 
> Also, what will be returned in this situation?
> 
It is a request.onerror case.
Since we share the same base function with "Add" (nsDOMDeviceStorage::AddOrAppendNamed), it will return null when blob file is null and other unexpected situations.

Comment 34

[Boris Zbarsky [:bzbarsky]]

> Do you suggest creating the file if it doesn't exist?

I'm not suggesting changing the behavior, just that it might be clearer to actually write out the "NotFoundError" string so it's clear what's being dispatched.

> It is a request.onerror case.

Don't tell _me_ that.  Tell the IDL documetation reader.  ;)

Comment 35

[Alphan Chen [:alchen]]

Attachment: [DeviceStorage] add appendNamed into DeviceStorage webidl. r=bz

Update webidl patch based on comment 30.

Here is the try server result.
It looks fine.
https://tbpl.mozilla.org/?tree=Try&rev=ca2c65694653

Comment 36

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/705e176f1beb
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc5f910e98da
https://hg.mozilla.org/integration/mozilla-inbound/rev/a485b8a93a3d

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/705e176f1beb
https://hg.mozilla.org/mozilla-central/rev/bc5f910e98da
https://hg.mozilla.org/mozilla-central/rev/a485b8a93a3d

Comment 38

[Boris Zbarsky [:bzbarsky]]

>+   * If the file doesn't exist, a "NotFoundError" event will be dispatched.

On this DeviceStorage object, right?  Is it dispatched before the function returns, or asynchronously?

>+   * In the same time, it is a request.onerror case.

What does that mean, exactly?

>+   * The function will return null when blob file is null and other unexpected
> situations.

Return null instead of throwing?

When does this function throw, and what does it throw?

Comment 39

[Alphan Chen [:alchen]]

(In reply to Boris Zbarsky [:bz] from comment #38)
> >+   * If the file doesn't exist, a "NotFoundError" event will be dispatched.
> 
> On this DeviceStorage object, right?  Is it dispatched before the function
> returns, or asynchronously?
> 
> >+   * In the same time, it is a request.onerror case.
> 
> What does that mean, exactly?

The method is asynchronous and return a DOMRequest object to handle the success or error of the operation.

> 
> >+   * The function will return null when blob file is null and other unexpected
> > situations.
> 
> Return null instead of throwing?
> 
> When does this function throw, and what does it throw?

Except for blob file is null case, they will throw error before return in other cases.
When blob file is null, it just return null without throwing.
In the beginning, the function is return NS_OK. (NS_IMETHODIMP nsDOMDeviceStorage::AddNamed)
Then in the patch of "move DeviceStorage to WebIDL", Ms2ger return null without throwing.
So do you think we need to throw error before return null in this case?





I will update the comment later.

Here is the revised version.
  /*
   * Append data to a given file.
   * The method is asynchronous and return a DOMRequest object to 
   *   handle the success or error of the operation.
   * If the file doesn't exist, the error name will be "NotFoundError".
   * If the file exists, it will be opened with the following permission:
   *                                         "PR_WRONLY|PR_CREATE_FILE|PR_APPEND".
   * @parameter aBlob: A Blob object representing the data to append
   * @parameter aName: A string representing the full name (path + file name) of the file
   *                   to append data to.
   */

Comment 40

[Boris Zbarsky [:bzbarsky]]

> The method is asynchronous and return a DOMRequest object to handle the success or error
> of the operation.

Yes, I know that.  But what does "it is a request.onerror case" mean?  Presumably it's trying to say something about the DOMRequest object that will be returned, but it's not clear what.

The new comment is much better in this regard, thank you.

> So do you think we need to throw error before return null in this case?

That's an API design decision.  How do you expect typical code that uses this API to look?

>   * If the file doesn't exist, the error name will be "NotFoundError".

Ah, so there are no DOM events involved at all, ok.

What is the result of the returned DOMRequest in success cases?

Comment 41

[Alphan Chen [:alchen]]

(In reply to Boris Zbarsky [:bz] from comment #40)

> >Except for blob file is null case, they will throw error before return in other cases.
> >When blob file is null, it just return null without throwing.
> >In the beginning, the function is return NS_OK. (NS_IMETHODIMP nsDOMDeviceStorage::AddNamed)
> >Then in the patch of "move DeviceStorage to WebIDL", Ms2ger return null without throwing.
> > So do you think we need to throw error before return null in this case?
> 
> That's an API design decision.  How do you expect typical code that uses
> this API to look?

Hi Dave, do you have any comment about blob file is null case?
In my opinion, when adding/appending null data, we can throw an error(maybe NS_ERROR_UNEXPECTED) before return.
However, I don't know if there is any concern or reason for today's code. (just return null without throwing)

Comment 42

[Alphan Chen [:alchen]]

(In reply to Boris Zbarsky [:bz] from comment #40)
> 
> >   * If the file doesn't exist, the error name will be "NotFoundError".
> 
> Ah, so there are no DOM events involved at all, ok.
> 
> What is the result of the returned DOMRequest in success cases?
I will add a description for it as below.

(Revise version)
  /*
   * Append data to a given file.
   * The method is asynchronous and return a DOMRequest object to 
   *   handle the success or error of the operation.
   * If the file doesn't exist, the error name will be "NotFoundError".
   * If the file exists, it will be opened with the following permission:
   *                                         "PR_WRONLY|PR_CREATE_FILE|PR_APPEND".
   * The result of the retruned DOMRequest in success cases will be fully-qualified paths.
   * @parameter aBlob: A Blob object representing the data to append
   * @parameter aName: A string representing the full name (path + file name) of the file
   *                   to append data to.
   */

Comment 43

[Dave Hylands [:dhylands]]

(In reply to Alphan Chen[:Alphan] from comment #41)
> (In reply to Boris Zbarsky [:bz] from comment #40)
> 
> > >Except for blob file is null case, they will throw error before return in other cases.
> > >When blob file is null, it just return null without throwing.
> > >In the beginning, the function is return NS_OK. (NS_IMETHODIMP nsDOMDeviceStorage::AddNamed)
> > >Then in the patch of "move DeviceStorage to WebIDL", Ms2ger return null without throwing.
> > > So do you think we need to throw error before return null in this case?
> > 
> > That's an API design decision.  How do you expect typical code that uses
> > this API to look?
> 
> Hi Dave, do you have any comment about blob file is null case?
> In my opinion, when adding/appending null data, we can throw an error(maybe
> NS_ERROR_UNEXPECTED) before return.
> However, I don't know if there is any concern or reason for today's code.
> (just return null without throwing)

This seems unusual. Sending a null blob might be interpreted as write zero bytes, and that returns success immediately.

You could also interpret it as an error, but I think I would treat it like "write zero bytes".