Bug 80207

Summary: Fix IndexedDB build with JSC
Product: WebKit Reporter: Benjamin Poulain <benjamin>
Component: WebCore Misc.Assignee: Benjamin Poulain <benjamin>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, dgrogan, donggwan.kim, efidler, haraken, japhet, jsbell, kling, levin+threading, s.choi, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 80802    
Attachments:
Description Flags
Patch
none
Patch
none
Patch for landing
none
Patch for landing none

Benjamin Poulain
Reported 2012-03-02 22:24:00 PST
I wanted to try IndexedDB, turns out it does not even compile :(
Attachments
Patch (21.22 KB, patch)
2012-03-02 22:26 PST, Benjamin Poulain 		no flags
DetailsFormatted DiffDiff
Patch (35.70 KB, patch)
2012-03-05 23:01 PST, Benjamin Poulain 		no flags
DetailsFormatted DiffDiff
Patch for landing (36.39 KB, patch)
2012-03-12 20:35 PDT, Benjamin Poulain 		no flags
DetailsFormatted DiffDiff
Patch for landing (36.36 KB, patch)
2012-03-12 21:01 PDT, Benjamin Poulain 		no flags
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Benjamin Poulain
Comment 1 2012-03-02 22:26:40 PST
Created attachment 129995 [details] Patch
Benjamin Poulain
Comment 2 2012-03-05 23:01:51 PST
Created attachment 130300 [details] Patch
Benjamin Poulain
Comment 3 2012-03-08 17:13:12 PST
Do you guys want me to break the patch in obvious && non-obvious fixes maybe? :)
Adam Barth
Comment 4 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?
Benjamin Poulain
Comment 5 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.
Joshua Bell
Comment 6 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.
Joshua Bell
Comment 7 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 ?
Benjamin Poulain
Comment 8 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.
Adam Barth
Comment 9 2012-03-09 15:07:44 PST
Comment on attachment 130300 [details] Patch Oops. Sorry I missed that. Custom bindings go in bindings.
Adam Barth
Comment 10 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.
Benjamin Poulain
Comment 11 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.
Adam Barth
Comment 12 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. :)
Adam Barth
Comment 13 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?
Joshua Bell
Comment 14 2012-03-10 00:34:00 PST
Looks good to me. Much appreciated tidying. The exciting patches are yet to come.
Benjamin Poulain
Comment 15 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.
Benjamin Poulain
Comment 16 2012-03-12 20:35:01 PDT
Created attachment 131502 [details] Patch for landing
Benjamin Poulain
Comment 17 2012-03-12 21:01:53 PDT
Created attachment 131505 [details] Patch for landing
WebKit Review Bot
Comment 18 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>
WebKit Review Bot
Comment 19 2012-03-12 22:39:22 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.