Bug 44599 – Add IndexedDB objects' constructors to window

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

[Jeremy Orlow]

Add IndexedDB objects' constructors to window

[Jeremy Orlow]

Created attachment 65397 [details]
Patch

[Steve Block]

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?

[Andrei Popescu]

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.

[Jeremy Orlow]

(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.

[Jeremy Orlow]

(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.

[Jeremy Orlow]

Comment on attachment 65397 [details]
Patch

r- until details are sorted out

[Jeremy Orlow]

Created attachment 66369 [details]
Ready for review

[Steve Block]

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.

[Jeremy Orlow]

(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?

[Jeremy Orlow]

(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?

[Steve Block]

> 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

[Jeremy Orlow]

(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.

[Jeremy Orlow]

Created attachment 66485 [details]
with comment + s/Static/ClassMethod/

[Steve Block]

Comment on attachment 66485 [details]
with comment + s/Static/ClassMethod/

r=me

[Jeremy Orlow]

landed

[WebKit Review Bot]

http://trac.webkit.org/changeset/66802 might have broken Leopard Intel Debug (Tests)