Sync Blob.close() behavior wrt updated spec.

Sync Blob.close() behavior wrt updated spec.

The File API now defines Blobs as having a readability state of
opened/closed, http://dev.w3.org/2006/webapi/FileAPI/#readabilityState
With close() naturally making the Blob enter the 'closed' state,

  http://dev.w3.org/2006/webapi/FileAPI/#close-method

Like the FileReader operations, Blob methods will now throw
InvalidStateError when passed a Blob in such a 'closed' state.

Update the implementation to follow the spec.

R=kinuko@chromium.org
BUG=344820
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=168567

[sof, 2014-02-28 13:11:01]

When you have a moment, please take a look.

[kinuko, 2014-03-03 05:18:45]

lgtm

https://codereview.chromium.org/184473002/diff/1/Source/core/fileapi/Blob.h
File Source/core/fileapi/Blob.h (right):

https://codereview.chromium.org/184473002/diff/1/Source/core/fileapi/Blob.h#n...
Source/core/fileapi/Blob.h:76: }
Oops... sad, is this the common way for adding exception state to a method with
optional args?

[sof, 2014-03-03 09:36:52]

nbarth: when you have some time, could you clarify if there are no IDL encoding
tricks to avoid this?

(Relatedly, I'm seeing some unintuitive code being generated for
'[Default=Undefined] optional's  -- e.g., the generated code for
GamepadList.item() doesn't appear to check arity of 'length' before indexing the
'info' block.)

https://codereview.chromium.org/184473002/diff/1/Source/core/fileapi/Blob.h
File Source/core/fileapi/Blob.h (right):

https://codereview.chromium.org/184473002/diff/1/Source/core/fileapi/Blob.h#n...
Source/core/fileapi/Blob.h:76: }
On 2014/03/03 05:18:46, kinuko wrote:
> Oops... sad, is this the common way for adding exception state to a method
with
> optional args?

It is definitely sad; I don't see a way to express this in IDL (i.e., annotating
the first two optional args with [Default=Undefined] would not be correct.)

nbarth: no way to avoid this situation, correct? i.e., a [RaisesException]
attribute on a method taking optionals will require more than one corresponding
method on the underlying object.

[sof, 2014-03-03 11:47:43]

On 2014/03/03 09:36:52, sof wrote:
> nbarth: when you have some time, could you clarify if there are no IDL
encoding
> tricks to avoid this?
> 
> (Relatedly, I'm seeing some unintuitive code being generated for
> '[Default=Undefined] optional's  -- e.g., the generated code for
> GamepadList.item() doesn't appear to check arity of 'length' before indexing
the
> 'info' block.)
> 

nvm, out-of-bounds accesses are mapped to 'undefined' by
FunctionCallbackInfo<T>::operator[].

[Nils Barth (inactive), 2014-03-04 00:50:28]

On 2014/03/03 11:47:43, sof wrote:
> On 2014/03/03 09:36:52, sof wrote:
> > (Relatedly, I'm seeing some unintuitive code being generated for
> > '[Default=Undefined] optional's  -- e.g., the generated code for
> > GamepadList.item() doesn't appear to check arity of 'length' before indexing
> the
> > 'info' block.)
> > 
> 
> nvm, out-of-bounds accesses are mapped to 'undefined' by
> FunctionCallbackInfo<T>::operator[].

n/p; this is potentially confusing enough that I'll see if an explanatory
comment is in order.

[Nils Barth (inactive), 2014-03-04 01:11:54]

+haraken: WDYT?

Agreed that optional handling is ugly (doesn't play well with
[RaisesException]).
Two complementary proposed solutions below.

https://codereview.chromium.org/184473002/diff/1/Source/core/fileapi/Blob.h
File Source/core/fileapi/Blob.h (right):

https://codereview.chromium.org/184473002/diff/1/Source/core/fileapi/Blob.h#n...
Source/core/fileapi/Blob.h:76: }
On 2014/03/03 09:36:52, sof wrote:
> On 2014/03/03 05:18:46, kinuko wrote:
> > Oops... sad, is this the common way for adding exception state to a method
> with
> > optional args?
> 
> It is definitely sad; I don't see a way to express this in IDL (i.e.,
annotating
> the first two optional args with [Default=Undefined] would not be correct.)
> 
> nbarth: no way to avoid this situation, correct? i.e., a [RaisesException]
> attribute on a method taking optionals will require more than one
corresponding
> method on the underlying object.

No, I don't believe there's currently a cleaner way to do this.

2 thoughts:
1. We could pass ExceptionState in *first*, as that would allow C++ handling of
optional arguments with default values
2. We could implement default values for optional arguments (properly).
In fact, we probably should do both.

To clarify, the issue is that:
* Bindings code generates early calls for optional arguments w/o default values
* Default values can be specified in C++ (as previously here).
* However, this doesn't work if we have [RaisesException], since it is always
passed in, which breaks C++ method resolution.

If we specify default argument values in the IDL (which is clearer),
it would generate a single C++ call (not different ones per different
number of arguments), and we could remove all default values from C++:

[RaisesException]
Blob slice(
　　optional long long start = 0,
　　optional long long end = LLONG_MAX,
　　[TreatNullAs=NullString, TreatUndefinedAs=NullString]
　　optional DOMString contentType = null);

Refs:
// LLONG_MAX = 9223372036854775807
// 0x7FFFFFFFFFFFFFFF is a bit more legible
// needs to be included literally, unless we add some new keywords
// or other handling; might be useful if a common case.

http://heycam.github.io/webidl/#dfn-optional-argument-default-value

[michaeln, 2014-03-04 02:20:05]

There remain some other points of integration around Blobs that aren't directly
specified the the FileAPI doc.
- structured clone algorithm (postMessage clones its inputs)
- formData.append('fieldName', blob);
- xhr.send(blob)
- webSocket.send(blob)
- [soon'ish] store a closed blob somewhere in an idb value
In the structured clone and idb cases, the blob value maybe nested down in the
object graph.

The spirit of the spec changes are to throw an exception if a closed blob is
used in some way, so probably these other points of integration should be
modified to have that behavior too.

I'm not trying to suggest that everything should be crammed into a single CL,
just pointing out these other points of integration so they don't fall thru the
cracks.

[sof, 2014-03-04 07:25:27]

On 2014/03/04 02:20:05, michaeln wrote:
> There remain some other points of integration around Blobs that aren't
directly
> specified the the FileAPI doc.
> - structured clone algorithm (postMessage clones its inputs)
> - formData.append('fieldName', blob);
> - xhr.send(blob)
> - webSocket.send(blob)
> - [soon'ish] store a closed blob somewhere in an idb value
> In the structured clone and idb cases, the blob value maybe nested down in the
> object graph.
> 
> The spirit of the spec changes are to throw an exception if a closed blob is
> used in some way, so probably these other points of integration should be
> modified to have that behavior too.
> 

When I brought this up a couple of months ago, there was no indication that
XHR et al wanted to start throwing here, but treat a closed Blob like
zero-extent
object instead. That's compatible with how neutered ArrayBufferViews are 
_currently_ handled by XHR.send(), for instance.

But I am keeping tabs on these specs, so when/if things change, we'll just have
to sync.

[sof, 2014-03-04 08:52:55]

https://codereview.chromium.org/184473002/diff/1/Source/core/fileapi/Blob.h
File Source/core/fileapi/Blob.h (right):

https://codereview.chromium.org/184473002/diff/1/Source/core/fileapi/Blob.h#n...
Source/core/fileapi/Blob.h:76: }
On 2014/03/04 01:11:54, Nils Barth wrote:
> On 2014/03/03 09:36:52, sof wrote:
> > On 2014/03/03 05:18:46, kinuko wrote:
> > > Oops... sad, is this the common way for adding exception state to a method
> > with
> > > optional args?
> > 
> > It is definitely sad; I don't see a way to express this in IDL (i.e.,
> annotating
> > the first two optional args with [Default=Undefined] would not be correct.)
> > 
> > nbarth: no way to avoid this situation, correct? i.e., a [RaisesException]
> > attribute on a method taking optionals will require more than one
> corresponding
> > method on the underlying object.
> 
> No, I don't believe there's currently a cleaner way to do this.
> 
> 2 thoughts:
> 1. We could pass ExceptionState in *first*, as that would allow C++ handling
of
> optional arguments with default values
> 2. We could implement default values for optional arguments (properly).
> In fact, we probably should do both.
> 
> To clarify, the issue is that:
> * Bindings code generates early calls for optional arguments w/o default
values
> * Default values can be specified in C++ (as previously here).
> * However, this doesn't work if we have [RaisesException], since it is always
> passed in, which breaks C++ method resolution.
> 
> If we specify default argument values in the IDL (which is clearer),
> it would generate a single C++ call (not different ones per different
> number of arguments), and we could remove all default values from C++:
> 
> [RaisesException]
> Blob slice(
> 　　optional long long start = 0,
> 　　optional long long end = LLONG_MAX,
> 　　[TreatNullAs=NullString, TreatUndefinedAs=NullString]
> 　　optional DOMString contentType = null);
> 

Having default value support would be the way to go here. There's no real
reliance on having these arguments be defaulted on the C++ side.

The default for 'end' is what prevents just using [Default=Undefined] here,
which is something that doesn't appear common for other methods that uses
[RaisesException] and optionals.

> Refs:
> // LLONG_MAX = 9223372036854775807
> // 0x7FFFFFFFFFFFFFFF is a bit more legible
> // needs to be included literally, unless we add some new keywords
> // or other handling; might be useful if a common case.
> 
> http://heycam.github.io/webidl/#dfn-optional-argument-default-value

Yes - WebIDL might want to predefine a set of these if type boundary
default values show up in several specs, I suppose.

[Nils Barth (inactive), 2014-03-04 08:54:58]

On 2014/03/04 08:52:55, sof wrote:
> > Refs:
> > // LLONG_MAX = 9223372036854775807
> > // 0x7FFFFFFFFFFFFFFF is a bit more legible
> > // needs to be included literally, unless we add some new keywords
> > // or other handling; might be useful if a common case.
> > 
> > http://heycam.github.io/webidl/#dfn-optional-argument-default-value
> 
> Yes - WebIDL might want to predefine a set of these if type boundary
> default values show up in several specs, I suppose.

I'll check W3C Bugzilla and open a bug for discussion, to see if there are any
suggestions.

[sof, 2014-03-05 13:48:45]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2014-03-05 13:48:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sigbjornf@opera.com/184473002/20001

[commit-bot: I haz the power, 2014-03-05 20:58:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sigbjornf@opera.com/184473002/20001

[commit-bot: I haz the power, 2014-03-06 04:10:41]

Change committed as 168567