Bug 45044 – Add the concept of class methods to bindings (for IndexedDB's IDBKeyRange).

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Jeremy Orlow]

Add the concept of "static" methods to bindings (for IndexedDB's IDBKeyRange).

[Jeremy Orlow]

Created attachment 66228 [details]
topic of discussion

[Jeremy Orlow]

jorlow: japhet dglazkov ...got something to help wake you up  
[5:00pm] jorlow: I'm trying to essentially implement "static" methods in javascript
[5:00pm] jorlow: cause jonas insists on it for IndexedDB
[5:01pm] jorlow: IDBKeyRange is the name of an interface...it has 4 methods on it that we want to be static (cause they're factories for making IDBKeyRanges)
[5:01pm] jorlow: i.e.  IDBKeyRange.rightBound(...)  should return an IDBKeyRange object
[5:01pm] • dglazkov pretends to be completely asleep.
[5:01pm] jorlow: i thought i figured out how to do this
[5:01pm] jorlow: and it even compiles
[5:01pm] jorlow: but it doesn't work
[5:03pm] jorlow: on line 1952 of CodeGeneratorV8.pm I added a check for a custom extAttr which I'm calling V8Static
[5:03pm] jorlow: if it's there, it sets $template = "desc";
[5:03pm] jorlow: which seems like it should then modify the functionTemplate
[5:03pm] jorlow: btw, in window, I obviously have an attribute IDBKeyRangeConstructor IDBKeyRange;
[5:04pm] jorlow: so I thought adding the items to the functionTemplate rather than the prototype would be enough
[5:04pm] jorlow: but it just doesn't work
[5:04pm] jorlow: and i've hit the point where I can't think of anything else to tinker with
[5:05pm] jorlow: i've looked at the generated code by hand and it seems sane
[5:05pm] jorlow: any ideas?


This is what I have.  I believe it should work...but it doesn't.  If you guys have any ideas, I would be extremely appreciative.

[Jeremy Orlow]

If you want to test this patch, you should also patch in this one, btw: https://bugs.webkit.org/show_bug.cgi?id=44599

[Jeremy Orlow]

More background:

What's currently specced doesn't work: http://www.w3.org/TR/IndexedDB/#range-concept  Calling member methods is the only way to get an object.  The normal way to do it would be to put member methods on some other object that's already instantiated.  window.indexedDB returns an IDBFactory object, so that'd be the obvious choice.  Jonas is pretty insistent on us making these "static" methods on IDBKeyRange.  The whole thread has the subject of "[IndexedDB] Constants and interfaces" on public-webapps, but here's the main part:

"""
> We either need to figure out the right language to describe what we intended
> or we need to change it.  As I mentioned, I tried to figure out what was
> needed spec wise, but couldn't.  As far as I can tell, there isn't anything
> else out there trying to do this either, so it's very possible this behavior
> would require a change to the WebIDL spec.  (And blocking on that
> is probably not a good idea given that it's been stagnant for so long and
> has quite a backlog of required changes at this point.)
> Can you (or anyone else) think of a way to spec it?

I think the way to do it is as follows:

* Remove the factory methods from the IDBKeyRange interface. The
factory methods shouldn't be showing up on individual instances
anyway.
* Add prose below the interface description that states that in
javascript, the interface object IDBKeyRange has the following
methods:
 IDBKeyRange only (in any value);
 IDBKeyRange leftBound (in any bound, in optional boolean open);
 IDBKeyRange rightBound (in any bound, in optional boolean open);
 IDBKeyRange bound (in any left, in any right, in optional boolean
openLeft, in optional boolean openRight);
 In other languages the same factory methods are exposed as static
methods in a way that is appropriate for that language. If the
language doesn't support static methods, the methods are exposed on
the IDBFactory interface.
* Make a special note that in javascript, the factory methods are
*not* exposed on the IDBFactory interface.
"""


To test this patch (along with the one I just posted) you should be able to do IDBKeyRange.leftBound().  Right now, IDBKeyRange.leftBound is undefined.  (And so is IDBKeyRange.__proto__.leftBound.)  The attributes seem to work fine though.  I.e. IDBKeyRange.SINGLE works.  And that also works on instantiated objects.

https://cs.corp.google.com/p#chrome/trunk/src/third_party/WebKit/WebCore/bindings/v8/V8Proxy.cpp&q=batchConfigureConstants&sa=N&cd=1&ct=rc is the code for binding constants and what made me think my solution would work.

[Jeremy Orlow]

Oh, don't remember to pass in --enable-indexed-database at runtime.

[Jeremy Orlow]

OK.  Apparently I had some bit of user error building the code.  I have no idea how I triggered it, but it was somehow building stale stuff.  It now works.  At least to the extent that |IDBKeyRange.leftBound| returns a function.  But when I call it like this |IDBKeyRange.leftBound(2)| I get a "TypeError: Illegal invocation" without it hitting a breakpoint I set in V8IDBKeyRange.cpp's leftBoundCallback method.  So I assume now I'm hitting an issue where V8 assumes we always have an instance when we're calling a method.

I'm going to work on this more tomorrow.  If you have suggestions, I'd really appreciate hearing them tho!  (Especially from Mads.)

[Jeremy Orlow]

It looks like the problem was using the defaultSignature.  In the V8 code generator, the extended attribute "V8DoNotCheckSignature" uses "v8::Local<v8::Signature>()" instead of the default one (which seems to essentially be "v8::Signature::New(" . $className . "::GetRawTemplate())").

Now I think all I need to do is change the generated code to assume it's static and not an instance.

[Jeremy Orlow]

Created attachment 66363 [details]
Patch

[WebKit Review Bot]

Attachment 66363 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/bindings/scripts/test/V8/V8TestObj.cpp:1005:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 1 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

The term “static” seems wrong for this. That terminology in C++ is quite quirky and specific to C syntax, and I suggest using a more traditional term, such as class method or something along those lines, in IDL.

[Jeremy Orlow]

I'll replace "Static" with "ClassMethod".

[Jeremy Orlow]

Created attachment 66387 [details]
Patch

[WebKit Review Bot]

Attachment 66387 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/bindings/scripts/test/V8/V8TestObj.cpp:1005:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jeremy Orlow]

Nate can you please review?

[Nate Chapin]

Comment on attachment 66387 [details]
Patch

Ok

> +        // Class methods within JavaScript (like what's used for IDBKeyRange).
> +        [V8ClassMethod] void classMethod();
> +        [V8ClassMethod] long classMethodWithOptional(in [Optional] long arg);

Is there a good reason for the attribute name to have V8 in it? I know it's V8-only at the moment, but if it's going to be used across all bindings eventually, we might want to just drop the 'V8' now.

[Jeremy Orlow]

(In reply to comment #15)
> (From update of attachment 66387 [details])
> Ok
> 
> > +        // Class methods within JavaScript (like what's used for IDBKeyRange).
> > +        [V8ClassMethod] void classMethod();
> > +        [V8ClassMethod] long classMethodWithOptional(in [Optional] long arg);
> 
> Is there a good reason for the attribute name to have V8 in it? I know it's V8-only at the moment, but if it's going to be used across all bindings eventually, we might want to just drop the 'V8' now.

Makes sense.  Will do when landing.

[Jeremy Orlow]

landed