Bug 45044 - Add the concept of class methods to bindings (for IndexedDB's IDBKeyRange).
Summary: Add the concept of class methods to bindings (for IndexedDB's IDBKeyRange).
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Jeremy Orlow
URL:
Keywords:
Depends on:
Blocks: 44599
  Show dependency treegraph
 
Reported: 2010-09-01 10:01 PDT by Jeremy Orlow
Modified: 2011-11-21 21:34 PST (History)
8 users (show)

See Also:


Attachments
topic of discussion (10.77 KB, patch)
2010-09-01 10:18 PDT, Jeremy Orlow
no flags Details | Formatted Diff | Diff
Patch (17.47 KB, patch)
2010-09-02 06:24 PDT, Jeremy Orlow
no flags Details | Formatted Diff | Diff
Patch (16.45 KB, patch)
2010-09-02 11:06 PDT, Jeremy Orlow
japhet: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jeremy Orlow 2010-09-01 10:01:07 PDT
Add the concept of "static" methods to bindings (for IndexedDB's IDBKeyRange).
Comment 1 Jeremy Orlow 2010-09-01 10:18:35 PDT
Created attachment 66228 [details]
topic of discussion
Comment 2 Jeremy Orlow 2010-09-01 10:20:28 PDT
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.
Comment 3 Jeremy Orlow 2010-09-01 11:01:51 PDT
If you want to test this patch, you should also patch in this one, btw: https://bugs.webkit.org/show_bug.cgi?id=44599
Comment 4 Jeremy Orlow 2010-09-01 11:10:33 PDT
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.
Comment 5 Jeremy Orlow 2010-09-01 11:18:29 PDT
Oh, don't remember to pass in --enable-indexed-database at runtime.
Comment 6 Jeremy Orlow 2010-09-01 11:38:15 PDT
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.)
Comment 7 Jeremy Orlow 2010-09-02 04:24:41 PDT
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.
Comment 8 Jeremy Orlow 2010-09-02 06:24:56 PDT
Created attachment 66363 [details]
Patch
Comment 9 WebKit Review Bot 2010-09-02 06:27:58 PDT
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.
Comment 10 Darin Adler 2010-09-02 10:41:48 PDT
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.
Comment 11 Jeremy Orlow 2010-09-02 10:54:56 PDT
I'll replace "Static" with "ClassMethod".
Comment 12 Jeremy Orlow 2010-09-02 11:06:49 PDT
Created attachment 66387 [details]
Patch
Comment 13 WebKit Review Bot 2010-09-02 11:11:45 PDT
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.
Comment 14 Jeremy Orlow 2010-09-03 05:39:54 PDT
Nate can you please review?
Comment 15 Nate Chapin 2010-09-03 08:56:49 PDT
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.
Comment 16 Jeremy Orlow 2010-09-03 08:58:06 PDT
(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.
Comment 17 Jeremy Orlow 2010-09-05 08:10:16 PDT
landed