Bug 101212 – [V8] IDBKey::toV8(impl) should return null instead of undefined when impl is 0

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Kentaro Hara]

All other toV8(impl) return null when impl is 0.

[Kentaro Hara]

Created attachment 172326 [details]
Patch

[Kentaro Hara]

jsbell, alecflett: would you check if this patch doesn't break compatibility?

[Adam Barth]

Comment on attachment 172326 [details]
Patch

Please wait to hear from jsbell or alecflett before landing.

[Joshua Bell]

Returning undefined in that case is intentional, see:

https://bugs.webkit.org/show_bug.cgi?id=96401

...which contains the spec reference.

[Kentaro Hara]

(In reply to comment #4)
> Returning undefined in that case is intentional, see:
> 
> https://bugs.webkit.org/show_bug.cgi?id=96401
> 
> ...which contains the spec reference.

ok, then I'll add a comment about it.

(The spec sounds strange to me though.)

[Kentaro Hara]

Created attachment 172336 [details]
patch for landing

[Adam Barth]

Comment on attachment 172336 [details]
patch for landing

This probably isn't the best way to implement this requirement.  Instead, the attributes that can return undefined should do so explicitly rather than relying upon toJS/toV8 to do something wonky.

[Kentaro Hara]

Comment on attachment 172336 [details]
patch for landing

Makes sense. Will fix it.

[WebKit Review Bot]

Comment on attachment 172336 [details]
patch for landing

Clearing flags on attachment: 172336

Committed r133483: <http://trac.webkit.org/changeset/133483>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Kentaro Hara]

(In reply to comment #7)
> (From update of attachment 172336 [details])
> This probably isn't the best way to implement this requirement.  Instead, the attributes that can return undefined should do so explicitly rather than relying upon toJS/toV8 to do something wonky.

At first I was thinking about introducing a new IDL attribute [TreatReturnedNullAs=Undefined]. However, as far as I scanned custom bindings, DOM attributes benefited by the IDL attribute are only IDBKeyRange.upper and IDBKeyRange.lower. Maybe it would make sense to just write custom getters for IDBKeyRange.upper and IDBKeyRange.lower?

[Adam Barth]

> At first I was thinking about introducing a new IDL attribute [TreatReturnedNullAs=Undefined]. However, as far as I scanned custom bindings, DOM attributes benefited by the IDL attribute are only IDBKeyRange.upper and IDBKeyRange.lower. Maybe it would make sense to just write custom getters for IDBKeyRange.upper and IDBKeyRange.lower?

We should look in WebIDL, but I think technically the right way to handle this is by adding a ? after the type in the IDL file.

[Kentaro Hara]

(In reply to comment #12)
> We should look in WebIDL, but I think technically the right way to handle this is by adding a ? after the type in the IDL file.

The situation looks a bit different. 'IDBKey?' means that an IDBKey object and null are valid values. (http://dev.w3.org/2006/webapi/WebIDL/#idl-nullable-type) On the other hand, in our situation we want to return undefined.

[Adam Barth]

Does that mean the behavior of this interface cannot be represented in WebIDL?

[Kentaro Hara]

(In reply to comment #14)
> Does that mean the behavior of this interface cannot be represented in WebIDL?

In my understanding, we cannot. (If upper and lower are speced to return null, it is conformed to the WebIDL. In that sense, the spec sounds strange to me.)

It is possible to represent the behavior in the WebKit IDL by introducing [TreatReturnedNullAs=Undefined], but the IDL attribute just benefits only IDBKeyRange.upper and IDBKeyRannge.lower. (Also, if we try to support [TreatReturnedNullAs=*] for all types, it would mess up JSValueToNative() in CodeGenerator*.pm.)

[Adam Barth]

For the time being, I'd recommend going with custom code.

I don't fully understand why these attributes return undefined.  Is there a strong reason for that from the IDB side?  If not, we might want to change the spec to better align with the rest of the platform.

[Kentaro Hara]

(In reply to comment #16)
> I don't fully understand why these attributes return undefined.  Is there a strong reason for that from the IDB side?  If not, we might want to change the spec to better align with the rest of the platform.

Agreed.

jsbell, alecflett: Do you have any idea?

[Alec Flett]

That sounds totally reasonable to me, but I don't know enough of the background - I defer to jsbell@ here but I expect we'll take it up on the list..

[Joshua Bell]

I don't think there's any particular reason to prefer null vs. undefined for these attributes in the spec. Neither are "valid" values for a key.

As alecflett@ says, this will come down to list consensus. 

As far as implementing it, I have webkit.org/b/97375 filed to propose removing all of the custom binding logic around IDBKey, since IDBKey is a WebKit concept not an IDB concept. I was thinking use any/ScriptValue (IDL/C++) and do IDBKey<->ScriptValue conversion in the IDB code itself by calls into the binding utilities. Not sure that's the right direction to go, but it would remove these special cases.

[Joshua Bell]

(In reply to comment #13)

> The situation looks a bit different. 'IDBKey?' means that an IDBKey object and null are valid values. (http://dev.w3.org/2006/webapi/WebIDL/#idl-nullable-type) On the other hand, in our situation we want to return undefined.

And just to re-iterate - the IDB spec doesn't define an IDBKey interface in the IDL; that's entirely a WebKit creation. Wherever WebKitIDLs use IDBKey it is |any| in the IDB spec.

[Adam Barth]

> And just to re-iterate - the IDB spec doesn't define an IDBKey interface in the IDL; that's entirely a WebKit creation. Wherever WebKitIDLs use IDBKey it is |any| in the IDB spec.

Is there a particular reason IDB wants to return undefined here rather than null?

[Joshua Bell]

(In reply to comment #21)
> 
> Is there a particular reason IDB wants to return undefined here rather than null?

No, I don't think so. (I think the IDB spec in general went a little crazy with "undefined"; I seem to recall this was done across the spec at some point about a year ago but I don't recall the rationale or history.)

[Adam Barth]

It might be better if we changed to spec to make these attributes behave like most other attributes in the platform.  I don't know how locked down the current spec is, however.