Bug 38594 - Change IndexedDB to use events instead of callbacks
Summary: Change IndexedDB to use events instead of callbacks
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:
 
Reported: 2010-05-05 10:33 PDT by Jeremy Orlow
Modified: 2010-05-10 03:28 PDT (History)
6 users (show)

See Also:

Attachments
Patch (47.17 KB, patch)
2010-05-05 10:46 PDT, Jeremy Orlow 		no flags Details | Formatted Diff | Diff
Patch (50.65 KB, patch)
2010-05-05 12:36 PDT, Jeremy Orlow 		no flags Details | Formatted Diff | Diff
Patch (87.08 KB, patch)
2010-05-06 10:41 PDT, Jeremy Orlow 		japhet: review+
Details | Formatted Diff | Diff
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Jeremy Orlow 2010-05-05 10:33:49 PDT 
Change IndexedDB to use events instead of callbacks
Comment 1 Jeremy Orlow 2010-05-05 10:46:35 PDT 
Created attachment 55138 [details]
Patch
Comment 2 Jeremy Orlow 2010-05-05 12:36:56 PDT 
Created attachment 55143 [details]
Patch
Comment 3 Jeremy Orlow 2010-05-05 16:14:21 PDT 
I know it's big, but can one of you guys take a look?  I'm planning on starting on the JSC bindings tomorrow.
Comment 4 Jeremy Orlow 2010-05-06 10:41:16 PDT 
Created attachment 55258 [details]
Patch
Comment 5 Jeremy Orlow 2010-05-06 10:43:10 PDT 
Now with JSC bindings and support for build systems other than GYP.

Please take a look?
Comment 6 Jeremy Orlow 2010-05-06 11:13:31 PDT 
+ adam in case he's willing to take a look
Comment 7 Jeremy Orlow 2010-05-06 11:28:49 PDT 
Nate kindly said he'd take a look.

Note: I've only built this on Chromium and mac Safari.  I'll be sure to watch the build bots closely when I actually commit in case I messed something up.
Comment 8 Nate Chapin 2010-05-06 14:56:28 PDT 
Comment on attachment 55258 [details]
Patch

Your WebCore/ChangeLog diff is pretty far out of date in terms of the files listed. Plus a couple of nits inline.

Besides that, LGTM.

> --- a/WebCore/bindings/js/JSEventCustom.cpp
> +++ b/WebCore/bindings/js/JSEventCustom.cpp
> @@ -38,6 +38,7 @@
>  #include "JSCustomEvent.h"
>  #include "JSCompositionEvent.h"
>  #include "JSErrorEvent.h"
> +#include "JSIDBRequest.h"
>  #include "JSKeyboardEvent.h"
>  #include "JSMessageEvent.h"
>  #include "JSMouseEvent.h"

Why is this necessary?

> +    ASSERT(0);
> +    return jsUndefined();
> +}

Nit (here and in V8IDBRequestCustom.cpp): Any reason not to use ASSERT_NOT_REACHED instead?
Comment 9 Eric Seidel (no email) 2010-05-08 23:16:13 PDT 
Attachment 55258 [details] was posted by a committer and has review+, assigning to Jeremy Orlow for commit.
Comment 10 Jeremy Orlow 2010-05-10 02:36:10 PDT 
(In reply to comment #8)
> (From update of attachment 55258 [details])
> Your WebCore/ChangeLog diff is pretty far out of date in terms of the files listed. Plus a couple of nits inline.

Done.

> 
> Besides that, LGTM.
> 
> > --- a/WebCore/bindings/js/JSEventCustom.cpp
> > +++ b/WebCore/bindings/js/JSEventCustom.cpp
> > @@ -38,6 +38,7 @@
> >  #include "JSCustomEvent.h"
> >  #include "JSCompositionEvent.h"
> >  #include "JSErrorEvent.h"
> > +#include "JSIDBRequest.h"
> >  #include "JSKeyboardEvent.h"
> >  #include "JSMessageEvent.h"
> >  #include "JSMouseEvent.h"
> 
> Why is this necessary?

It isn't anymore.
 
> > +    ASSERT(0);
> > +    return jsUndefined();
> > +}
> 
> Nit (here and in V8IDBRequestCustom.cpp): Any reason not to use ASSERT_NOT_REACHED instead?

Oops, changed.



Btw, wasn't there one other problem you noticed when we were chatting on chat?
Comment 11 Jeremy Orlow 2010-05-10 02:56:29 PDT 
Committed r59068: <http://trac.webkit.org/changeset/59068>
Comment 12 WebKit Review Bot 2010-05-10 03:28:54 PDT 
http://trac.webkit.org/changeset/59068 might have broken Qt Linux Release minimal