Bug 101212 - [V8] IDBKey::toV8(impl) should return null instead of undefined when impl is 0
Summary: [V8] IDBKey::toV8(impl) should return null instead of undefined when impl is 0
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kentaro Hara
URL:
Keywords:
Depends on:
Blocks: 101054
  Show dependency treegraph
 
Reported: 2012-11-05 06:21 PST by Kentaro Hara
Modified: 2012-11-05 14:14 PST (History)
5 users (show)

See Also:


Attachments
Patch (6.90 KB, patch)
2012-11-05 06:26 PST, Kentaro Hara
no flags Details | Formatted Diff | Diff
patch for landing (2.42 KB, patch)
2012-11-05 08:04 PST, Kentaro Hara
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kentaro Hara 2012-11-05 06:21:34 PST
All other toV8(impl) return null when impl is 0.
Comment 1 Kentaro Hara 2012-11-05 06:26:06 PST
Created attachment 172326 [details]
Patch
Comment 2 Kentaro Hara 2012-11-05 06:26:47 PST
jsbell, alecflett: would you check if this patch doesn't break compatibility?
Comment 3 Adam Barth 2012-11-05 07:41:52 PST
Comment on attachment 172326 [details]
Patch

Please wait to hear from jsbell or alecflett before landing.
Comment 4 Joshua Bell 2012-11-05 07:51:14 PST
Returning undefined in that case is intentional, see:

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

...which contains the spec reference.
Comment 5 Kentaro Hara 2012-11-05 07:53:17 PST
(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.)
Comment 6 Kentaro Hara 2012-11-05 08:04:19 PST
Created attachment 172336 [details]
patch for landing
Comment 7 Adam Barth 2012-11-05 08:29:45 PST
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.
Comment 8 Kentaro Hara 2012-11-05 08:31:40 PST
Comment on attachment 172336 [details]
patch for landing

Makes sense. Will fix it.
Comment 9 WebKit Review Bot 2012-11-05 08:32:34 PST
Comment on attachment 172336 [details]
patch for landing

Clearing flags on attachment: 172336

Committed r133483: <http://trac.webkit.org/changeset/133483>
Comment 10 WebKit Review Bot 2012-11-05 08:32:38 PST
All reviewed patches have been landed.  Closing bug.
Comment 11 Kentaro Hara 2012-11-05 10:01:10 PST
(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?
Comment 12 Adam Barth 2012-11-05 10:09:24 PST
> 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.
Comment 13 Kentaro Hara 2012-11-05 10:13:05 PST
(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.
Comment 14 Adam Barth 2012-11-05 10:15:41 PST
Does that mean the behavior of this interface cannot be represented in WebIDL?
Comment 15 Kentaro Hara 2012-11-05 10:22:11 PST
(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.)
Comment 16 Adam Barth 2012-11-05 10:25:54 PST
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.
Comment 17 Kentaro Hara 2012-11-05 10:27:20 PST
(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?
Comment 18 Alec Flett 2012-11-05 10:56:34 PST
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..
Comment 19 Joshua Bell 2012-11-05 11:56:49 PST
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.
Comment 20 Joshua Bell 2012-11-05 11:59:14 PST
(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.
Comment 21 Adam Barth 2012-11-05 14:03:55 PST
> 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?
Comment 22 Joshua Bell 2012-11-05 14:07:39 PST
(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.)
Comment 23 Adam Barth 2012-11-05 14:14:19 PST
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.