Bug 80207 - Fix IndexedDB build with JSC
Summary: Fix IndexedDB build with JSC
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Benjamin Poulain
URL:
Keywords:
Depends on:
Blocks: 80802
  Show dependency treegraph
 
Reported: 2012-03-02 22:24 PST by Benjamin Poulain
Modified: 2012-03-20 09:35 PDT (History)
11 users (show)

See Also:


Attachments
Patch (21.22 KB, patch)
2012-03-02 22:26 PST, Benjamin Poulain
no flags Details | Formatted Diff | Diff
Patch (35.70 KB, patch)
2012-03-05 23:01 PST, Benjamin Poulain
no flags Details | Formatted Diff | Diff
Patch for landing (36.39 KB, patch)
2012-03-12 20:35 PDT, Benjamin Poulain
no flags Details | Formatted Diff | Diff
Patch for landing (36.36 KB, patch)
2012-03-12 21:01 PDT, Benjamin Poulain
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Benjamin Poulain 2012-03-02 22:24:00 PST
I wanted to try IndexedDB, turns out it does not even compile :(
Comment 1 Benjamin Poulain 2012-03-02 22:26:40 PST
Created attachment 129995 [details]
Patch
Comment 2 Benjamin Poulain 2012-03-05 23:01:51 PST
Created attachment 130300 [details]
Patch
Comment 3 Benjamin Poulain 2012-03-08 17:13:12 PST
Do you guys want me to break the patch in obvious && non-obvious fixes maybe? :)
Comment 4 Adam Barth 2012-03-08 17:26:59 PST
Comment on attachment 130300 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=130300&action=review

I was hoping some of the IDB folks would review your patch, but this looks fine to me.

> Source/WebCore/bindings/js/SerializedScriptValue.h:66
> -class SerializedScriptValue : public RefCounted<SerializedScriptValue> {
> +class SerializedScriptValue :
> +#if ENABLE(INDEXED_DATABASE)
> +    public ThreadSafeRefCounted<SerializedScriptValue> {
> +#else
> +    public RefCounted<SerializedScriptValue> {
> +#endif

Should we just make this always ThreadSafeRefCounted ?  Having this ifdef seems scary.

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3058
> +    if ($globalObject) {

Do we need to run-bindings-tests --reset-results after this change to the code generator?
Comment 5 Benjamin Poulain 2012-03-08 17:32:23 PST
Thanks for the review.

> > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3058
> > +    if ($globalObject) {
> 
> Do we need to run-bindings-tests --reset-results after this change to the code generator?

IndexedDB has our first static functions so I do not think so. I am unfamiliar with run-bindings-tests so I'd better test that myself before landing.
Comment 6 Joshua Bell 2012-03-09 14:30:32 PST
How does this relate to https://bugs.webkit.org/show_bug.cgi?id=45110 ? Looks like a duplicate effort.

CC'ing the IDB folks (myself and dgrogan). I'm happy to take a look.
Comment 7 Joshua Bell 2012-03-09 14:33:34 PST
Comment on attachment 130300 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=130300&action=review

> Source/WebCore/Modules/indexeddb/JSIDBVersionChangeRequestCustom.cpp:1
> +/*

Shouldn't this file be in Source/WebCore/bindings/js ?
Comment 8 Benjamin Poulain 2012-03-09 14:40:33 PST
(In reply to comment #7)
> (From update of attachment 130300 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=130300&action=review
> 
> > Source/WebCore/Modules/indexeddb/JSIDBVersionChangeRequestCustom.cpp:1
> > +/*
> 
> Shouldn't this file be in Source/WebCore/bindings/js ?

Good question. I did not put it in the binding because that would be spreading files outside the module.

I did not follow all emails regarding the modularization. I guess the modularization of binding must be documented somewhere.
Comment 9 Adam Barth 2012-03-09 15:07:44 PST
Comment on attachment 130300 [details]
Patch

Oops.  Sorry I missed that.  Custom bindings go in bindings.
Comment 10 Adam Barth 2012-03-09 15:08:21 PST
If you look at this diagram:

https://docs.google.com/drawings/d/10WlCj2J3arxf4cDGRKteNinaP755iFnmYtYtnNSCQOY/edit?authkey=CP6plYAI

You can see that bindings code is allowed to depend on every module.
Comment 11 Benjamin Poulain 2012-03-09 15:11:23 PST
(In reply to comment #10)
> If you look at this diagram:
> 
> https://docs.google.com/drawings/d/10WlCj2J3arxf4cDGRKteNinaP755iFnmYtYtnNSCQOY/edit?authkey=CP6plYAI
> 
> You can see that bindings code is allowed to depend on every module.

My bad, I believed the modules were self-contained.

I will make an update. The code generator has also changed a bit since this patch.
Comment 12 Adam Barth 2012-03-09 15:17:04 PST
> My bad, I believed the modules were self-contained.

My dream is that we'll reduce/remove custom bindings over time.  :)
Comment 13 Adam Barth 2012-03-09 19:42:39 PST
Comment on attachment 130300 [details]
Patch

Actually, I think r+/cq- is a more accurate state for this patch.  jsbell: Any comments before Benjamin lands this patch?
Comment 14 Joshua Bell 2012-03-10 00:34:00 PST
Looks good to me. Much appreciated tidying. The exciting patches are yet to come.
Comment 15 Benjamin Poulain 2012-03-12 17:17:58 PDT
I have been busy with other stuff. I'll try to update and land this tonight or tomorrow.
Comment 16 Benjamin Poulain 2012-03-12 20:35:01 PDT
Created attachment 131502 [details]
Patch for landing
Comment 17 Benjamin Poulain 2012-03-12 21:01:53 PDT
Created attachment 131505 [details]
Patch for landing
Comment 18 WebKit Review Bot 2012-03-12 22:39:16 PDT
Comment on attachment 131505 [details]
Patch for landing

Clearing flags on attachment: 131505

Committed r110539: <http://trac.webkit.org/changeset/110539>
Comment 19 WebKit Review Bot 2012-03-12 22:39:22 PDT
All reviewed patches have been landed.  Closing bug.