Implement platform deleters per spec.

Implement platform deleters per spec.

Extend the functionality of platform deleter operations slightly to
allow them to implement what WebIDL prescribes,

   http://heycam.github.io/webidl/#delete

That is, if the property name being deleted isn't exposed by the
platform object, any native property should be deleted instead.

Update the two deleters that this currently applies to, Storage and
DOMStringMap (dataset).

This change reflects the outcome of a spec clarification issue raised
via https://www.w3.org/Bugs/Public/show_bug.cgi?id=24096

R=
BUG=329176
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=164081

[sof, 2013-12-17 13:07:05]

Please take a look when you next have a spare moment.

[haraken, 2013-12-17 15:59:19]

https://codereview.chromium.org/106853005/diff/1/Source/core/dom/DOMStringMap.h
File Source/core/dom/DOMStringMap.h (right):

https://codereview.chromium.org/106853005/diff/1/Source/core/dom/DOMStringMap...
Source/core/dom/DOMStringMap.h:80: return true;

Don't we want to return |result| ?

https://codereview.chromium.org/106853005/diff/1/Source/core/storage/Storage.cpp
File Source/core/storage/Storage.cpp (right):

https://codereview.chromium.org/106853005/diff/1/Source/core/storage/Storage....
Source/core/storage/Storage.cpp:81: bool Storage::anonymousNamedDeleter(const
AtomicString& name, bool& result, ExceptionState& exceptionState)

I'm a bit confused. What's the difference between the |result| parameter and the
returned value? Do we need both?

[sof, 2013-12-17 16:20:08]

https://codereview.chromium.org/106853005/diff/1/Source/core/dom/DOMStringMap.h
File Source/core/dom/DOMStringMap.h (right):

https://codereview.chromium.org/106853005/diff/1/Source/core/dom/DOMStringMap...
Source/core/dom/DOMStringMap.h:80: return true;
On 2013/12/17 15:59:20, haraken wrote:
> 
> Don't we want to return |result| ?

4.7.4.1 (the indexed case) in the spec tells us that we shouldn't fall into any
of the later steps, but always handle the indexed deletion. So returning 'true'
here always is appropriate.

Hmm, looking at 4.7.4.1.5 again, I think we have to extend the signature of the
indexed deleter to also take a bool reference, so that the operation's return
value can be propagated back out to the script. I'll do that.

(An observable example of this would be 'delete element.dataset["1"]' in a
strict context where "data-1" isn't a content attribute, I think.)

https://codereview.chromium.org/106853005/diff/1/Source/core/storage/Storage.cpp
File Source/core/storage/Storage.cpp (right):

https://codereview.chromium.org/106853005/diff/1/Source/core/storage/Storage....
Source/core/storage/Storage.cpp:81: bool Storage::anonymousNamedDeleter(const
AtomicString& name, bool& result, ExceptionState& exceptionState)
On 2013/12/17 15:59:20, haraken wrote:
> 
> I'm a bit confused. What's the difference between the |result| parameter and
the
> returned value? Do we need both?

We need to represent three outcomes,

 * property deleted ok.
 * property not deleted (non-configurable or other platform object
restrictions.)
 * property not known (4.7.4.2 does not apply; 4.7.4.3 should happen instead.)

[sof, 2013-12-17 16:24:37]

https://codereview.chromium.org/106853005/diff/1/Source/core/storage/Storage.cpp
File Source/core/storage/Storage.cpp (right):

https://codereview.chromium.org/106853005/diff/1/Source/core/storage/Storage....
Source/core/storage/Storage.cpp:81: bool Storage::anonymousNamedDeleter(const
AtomicString& name, bool& result, ExceptionState& exceptionState)
On 2013/12/17 16:20:08, sof wrote:
> On 2013/12/17 15:59:20, haraken wrote:
> > 
> > I'm a bit confused. What's the difference between the |result| parameter and
> the
> > returned value? Do we need both?
> 
> We need to represent three outcomes,
> 
>  * property deleted ok.
>  * property not deleted (non-configurable or other platform object
> restrictions.)
>  * property not known (4.7.4.2 does not apply; 4.7.4.3 should happen instead.)

Forgot to say/ask...if the preference is to encode this via an enum instead,
then we can definitely switch.

[haraken, 2013-12-17 16:51:04]

https://codereview.chromium.org/106853005/diff/1/Source/core/storage/Storage.cpp
File Source/core/storage/Storage.cpp (right):

https://codereview.chromium.org/106853005/diff/1/Source/core/storage/Storage....
Source/core/storage/Storage.cpp:81: bool Storage::anonymousNamedDeleter(const
AtomicString& name, bool& result, ExceptionState& exceptionState)
On 2013/12/17 16:24:38, sof wrote:
> On 2013/12/17 16:20:08, sof wrote:
> > On 2013/12/17 15:59:20, haraken wrote:
> > > 
> > > I'm a bit confused. What's the difference between the |result| parameter
and
> > the
> > > returned value? Do we need both?
> > 
> > We need to represent three outcomes,
> > 
> >  * property deleted ok.
> >  * property not deleted (non-configurable or other platform object
> > restrictions.)
> >  * property not known (4.7.4.2 does not apply; 4.7.4.3 should happen
instead.)
> 
> Forgot to say/ask...if the preference is to encode this via an enum instead,
> then we can definitely switch.

Thanks for the clarification! enum sounds better to me.

[sof, 2013-12-17 22:02:10]

On 2013/12/17 16:51:04, haraken wrote:
>
https://codereview.chromium.org/106853005/diff/1/Source/core/storage/Storage.cpp
> File Source/core/storage/Storage.cpp (right):
> 
>
https://codereview.chromium.org/106853005/diff/1/Source/core/storage/Storage....
> Source/core/storage/Storage.cpp:81: bool Storage::anonymousNamedDeleter(const
> AtomicString& name, bool& result, ExceptionState& exceptionState)
> On 2013/12/17 16:24:38, sof wrote:
> > On 2013/12/17 16:20:08, sof wrote:
> > > On 2013/12/17 15:59:20, haraken wrote:
> > > > 
> > > > I'm a bit confused. What's the difference between the |result| parameter
> and
> > > the
> > > > returned value? Do we need both?
> > > 
> > > We need to represent three outcomes,
> > > 
> > >  * property deleted ok.
> > >  * property not deleted (non-configurable or other platform object
> > > restrictions.)
> > >  * property not known (4.7.4.2 does not apply; 4.7.4.3 should happen
> instead.)
> > 
> > Forgot to say/ask...if the preference is to encode this via an enum instead,
> > then we can definitely switch.
> 
> Thanks for the clarification! enum sounds better to me.

Done now, adding the enum to V8Binding.h

(Tree isn't quite healthy right now, but will kick off some trybots in due
course.)

[jsbell, 2013-12-18 00:49:33]

https://codereview.chromium.org/106853005/diff/20001/Source/bindings/scripts/...
File Source/bindings/scripts/code_generator_v8.pm (right):

https://codereview.chromium.org/106853005/diff/20001/Source/bindings/scripts/...
Source/bindings/scripts/code_generator_v8.pm:4088: $code .= "        return
v8SetReturnValueBool(info, result == DeleteSuccess);\n";
Sorry if I'm missing something, but doesn't this imply that the deleter may now
return true (if result == DeleteSuccess), false (if result == DeleteReject), or
undefined (if result == DeleteUnknownProperty)?

[sof, 2013-12-18 06:16:59]

https://codereview.chromium.org/106853005/diff/20001/Source/bindings/scripts/...
File Source/bindings/scripts/code_generator_v8.pm (right):

https://codereview.chromium.org/106853005/diff/20001/Source/bindings/scripts/...
Source/bindings/scripts/code_generator_v8.pm:4088: $code .= "        return
v8SetReturnValueBool(info, result == DeleteSuccess);\n";
On 2013/12/18 00:49:34, jsbell wrote:
> Sorry if I'm missing something, but doesn't this imply that the deleter may
now
> return true (if result == DeleteSuccess), false (if result == DeleteReject),
or
> undefined (if result == DeleteUnknownProperty)?
> 
> 

Yes, exactly that.

The v8 interceptor code that's calling will interpret undefined as the property
being unknown to the interceptor layer, and fall into accessing the native
property instead. See JSObject::DeletePropertyWithInterceptor().

[haraken, 2013-12-18 09:55:06]

LGTM. Thanks for improving the IDL conformance!

[commit-bot: I haz the power, 2013-12-18 10:03:14]

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

[commit-bot: I haz the power, 2013-12-18 11:15:16]

Change committed as 164081

[jsbell, 2013-12-18 16:52:30]

On 2013/12/18 06:16:59, sof wrote:
>
https://codereview.chromium.org/106853005/diff/20001/Source/bindings/scripts/...
> File Source/bindings/scripts/code_generator_v8.pm (right):
> 
> The v8 interceptor code that's calling will interpret undefined as the
property
> being unknown to the interceptor layer, and fall into accessing the native
> property instead. See JSObject::DeletePropertyWithInterceptor().

Cool, thanks for clarifying!