Bug 44599

Summary: Add IndexedDB objects' constructors to window
Product: WebKit Reporter: Jeremy Orlow <jorlow>
Component: New BugsAssignee: Jeremy Orlow <jorlow>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, andreip, bulach, eric, steveblock, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: OS X 10.5   
Bug Depends on: 45044    
Bug Blocks:    
Attachments:
Description Flags
Patch
jorlow: review-
Ready for review
jorlow: review-
with comment + s/Static/ClassMethod/ steveblock: review+

Description Jeremy Orlow 2010-08-25 04:45:27 PDT
Add IndexedDB objects' constructors to window
Comment 1 Jeremy Orlow 2010-08-25 04:52:28 PDT
Created attachment 65397 [details]
Patch
Comment 2 Steve Block 2010-08-25 05:13:32 PDT
Comment on attachment 65397 [details]
Patch

Can you explain why we want these constructors on the window object?

Also, this patch seems to remove IDBkeyRange. I think you should at least mention that in the ChangeLog.

WebCore/bindings/generic/RuntimeEnabledFeatures.h:59

 +      static bool iDBCursorEnabled() { return isIndexedDBEnabled; }
The capitalization here is weird, but I guess you have no control over that?
Comment 3 Andrei Popescu 2010-08-25 05:40:19 PDT
Are you sure that adding the IDB*Constructors gives us access to interface objects. For example, can we still call IDBKeyRange.leftBound(...) ? I think the constructor is just an function and doesn't have any of the interface members. I think you can do IDBKeyRange.prototype.leftBound() but that throws when called that way.
Comment 4 Jeremy Orlow 2010-08-25 05:42:28 PDT
(In reply to comment #2)
> (From update of attachment 65397 [details])
> Can you explain why we want these constructors on the window object?

You need this to access the constant definitions (like IDBDatabaseException.UNKNOWN_ERR).  You often need to access them without access to an instantiated object of that type.  This has been discussed on list extensively, and this is the way forward that was decided on.

Also, this is what's done for the majority of interfaces.

> Also, this patch seems to remove IDBkeyRange. I think you should at least mention that in the ChangeLog.

It's still there, just done differently.
 
> WebCore/bindings/generic/RuntimeEnabledFeatures.h:59
> 
>  +      static bool iDBCursorEnabled() { return isIndexedDBEnabled; }
> The capitalization here is weird, but I guess you have no control over that?

Nope, no control.
Comment 5 Jeremy Orlow 2010-08-25 06:26:46 PDT
(In reply to comment #3)
> Are you sure that adding the IDB*Constructors gives us access to interface objects. For example, can we still call IDBKeyRange.leftBound(...) ? I think the constructor is just an function and doesn't have any of the interface members. I think you can do IDBKeyRange.prototype.leftBound() but that throws when called that way.

Looking into it.  This is probably a spec issue, actually.
Comment 6 Jeremy Orlow 2010-08-25 07:05:09 PDT
Comment on attachment 65397 [details]
Patch

r- until details are sorted out
Comment 7 Jeremy Orlow 2010-09-02 07:42:48 PDT
Created attachment 66369 [details]
Ready for review
Comment 8 Steve Block 2010-09-03 04:33:22 PDT
Comment on attachment 66369 [details]
Ready for review

> [V8Static] IDBKeyRange bound(in IDBKey left, in IDBKey right, in [Optional] boolean openLeft, in [Optional] boolean
So presumably this is what makes these properties available only on window.IDBKeyRange, not on an instance? I assume it doesn't work for JSC? What's the plan for JSC?

I think you should be adding all these new tests to the GTK Skipped list individually - it doesn't allow entire directories to be skipped.
Comment 9 Jeremy Orlow 2010-09-03 04:45:36 PDT
(In reply to comment #8)
> (From update of attachment 66369 [details])
> > [V8Static] IDBKeyRange bound(in IDBKey left, in IDBKey right, in [Optional] boolean openLeft, in [Optional] boolean
> So presumably this is what makes these properties available only on window.IDBKeyRange, not on an instance? 

Yes.

> I assume it doesn't work for JSC? What's the plan for JSC?

As stated in the changeLog, the plan is to implement this at some point.  But given that no one in JSC land seems to care about IndexedDB yet, there's a large amount of work required to serialize JSC SerializedScriptValues to a wire format, that blocks us being able to test IndexedDB on JSC, and implementing this for JSC would just be dead code sitting around, I don't think doing it now makes sense.
 
> I think you should be adding all these new tests to the GTK Skipped list individually - it doesn't allow entire directories to be skipped.

Are you sure?  I haven't heard any complaints from them thus far or ever seen anything about this on webkit-dev.  If so, are they planning on implementing this?
Comment 10 Jeremy Orlow 2010-09-03 04:46:32 PDT
(In reply to comment #9)
> (In reply to comment #8)
> > (From update of attachment 66369 [details] [details])
> > > [V8Static] IDBKeyRange bound(in IDBKey left, in IDBKey right, in [Optional] boolean openLeft, in [Optional] boolean
> > So presumably this is what makes these properties available only on window.IDBKeyRange, not on an instance? 
> 
> Yes.
> 
> > I assume it doesn't work for JSC? What's the plan for JSC?
> 
> As stated in the changeLog, the plan is to implement this at some point.  But given that no one in JSC land seems to care about IndexedDB yet, there's a large amount of work required to serialize JSC SerializedScriptValues to a wire format, that blocks us being able to test IndexedDB on JSC, and implementing this for JSC would just be dead code sitting around, I don't think doing it now makes sense.

Sorry this was stated in the change log for the patch this depends on...which is what implements V8Static/V8ClassMethod.  I guess I could add a fixme here to make it more clear.

> > I think you should be adding all these new tests to the GTK Skipped list individually - it doesn't allow entire directories to be skipped.
> 
> Are you sure?  I haven't heard any complaints from them thus far or ever seen anything about this on webkit-dev.  If so, are they planning on implementing this?
Comment 11 Steve Block 2010-09-03 04:53:59 PDT
> Are you sure?  I haven't heard any complaints from them thus far or ever seen
> anything about this on webkit-dev.
Take a look at the comment at the top of LayoutTests/platform/gtk/Skipped. Maybe skipping directories works, but you're not supposed to do so. That's what I was told when adding Geolocation tests.

> Sorry this was stated in the change log for the patch this depends on...which
> is what implements V8Static/V8ClassMethod.  I guess I could add a fixme here to
> make it more clear.
That would be good
Comment 12 Jeremy Orlow 2010-09-03 05:51:29 PDT
(In reply to comment #11)
> > Are you sure?  I haven't heard any complaints from them thus far or ever seen
> > anything about this on webkit-dev.
> Take a look at the comment at the top of LayoutTests/platform/gtk/Skipped. Maybe skipping directories works, but you're not supposed to do so. That's what I was told when adding Geolocation tests.

I started a thread on webkit-dev about this.  What's there now (a blanket exception for the directory) works fine, so I think it's fine until the policy is made clear.  In which case I can change it in another CL.

> > Sorry this was stated in the change log for the patch this depends on...which
> > is what implements V8Static/V8ClassMethod.  I guess I could add a fixme here to
> > make it more clear.
> That would be good

Next patch will have this.
Comment 13 Jeremy Orlow 2010-09-03 06:03:28 PDT
Created attachment 66485 [details]
with comment + s/Static/ClassMethod/
Comment 14 Steve Block 2010-09-03 06:54:06 PDT
Comment on attachment 66485 [details]
with comment + s/Static/ClassMethod/

r=me
Comment 15 Jeremy Orlow 2010-09-05 08:10:07 PDT
landed
Comment 16 WebKit Review Bot 2010-09-05 09:23:50 PDT
http://trac.webkit.org/changeset/66802 might have broken Leopard Intel Debug (Tests)