Bug 54331 - Finish up implementing the new event model in IndexedDB
Summary: Finish up implementing the new event model in IndexedDB
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: 2011-02-11 18:57 PST by Jeremy Orlow
Modified: 2011-02-14 17:41 PST (History)
12 users (show)

See Also:


Attachments
Patch (496.61 KB, patch)
2011-02-11 18:59 PST, Jeremy Orlow
no flags Details | Formatted Diff | Diff
patch (510.00 KB, patch)
2011-02-14 10:34 PST, Jeremy Orlow
no flags Details | Formatted Diff | Diff
another (532.88 KB, patch)
2011-02-14 15:46 PST, Jeremy Orlow
japhet: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jeremy Orlow 2011-02-11 18:57:01 PST
Finish up implementing the new event model in IndexedDB
Comment 1 Jeremy Orlow 2011-02-11 18:59:42 PST
Created attachment 82224 [details]
Patch
Comment 2 Jeremy Orlow 2011-02-11 19:00:57 PST
Nate, I know it's big, but don't worry...it's mostly mechanical layout test changes.  Please don't delay too long in reviewing as keeping this patch up to date will be a super bitch.
Comment 3 WebKit Review Bot 2011-02-11 19:31:05 PST
Attachment 82224 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7886848
Comment 4 Early Warning System Bot 2011-02-11 21:47:34 PST
Attachment 82224 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7870862
Comment 5 Collabora GTK+ EWS bot 2011-02-11 23:53:43 PST
Attachment 82224 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/7884786
Comment 6 WebKit Review Bot 2011-02-12 00:34:18 PST
Attachment 82224 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7875832
Comment 7 WebKit Review Bot 2011-02-12 06:21:43 PST
Attachment 82224 [details] did not build on mac:
Build output: http://queues.webkit.org/results/7875884
Comment 8 Jeremy Orlow 2011-02-14 10:34:42 PST
Created attachment 82334 [details]
patch
Comment 9 WebKit Review Bot 2011-02-14 14:07:33 PST
Attachment 82334 [details] did not build on mac:
Build output: http://queues.webkit.org/results/7911643
Comment 10 Jeremy Orlow 2011-02-14 15:46:00 PST
Created attachment 82374 [details]
another
Comment 11 Nate Chapin 2011-02-14 16:31:03 PST
Comment on attachment 82374 [details]
another

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

SO MUCH RED! :)

Just a couple of nits, and a caveat that I skimmed the ~500KB of test expectations changes.

> Source/WebCore/storage/IDBEventDispatcher.cpp:83
> +    // FIXME: "...However, we also wanted to integrate the window.onerror feature in
> +    //        HTML5. So after we've fired an "error" event, if .preventDefault() was
> +    //        never called on the event, we fire an error event on the window (can't
> +    //        remember if this happens before or after we abort the transaction).
> +    //        This is a separate event, which for example means that even if you
> +    //        attach a capturing "error" handler on window, you won't see any events
> +    //        unless an error really went unhandled. And you also can't call
> +    //        .preventDefault on the error event fired on the window in order to
> +    //        prevent the transaction from being aborted. It's purely there for
> +    //        error reporting and distinctly different from the event propagating to
> +    //        the window.
> +    //        
> +    //        This is similar to how "error" events are handled in workers.
> +    //        
> +    //        (I think that so far webkit hasn't implemented the window.onerror
> +    //        feature yet, so you probably don't want to fire the separate error
> +    //        event on the window until that has been implemented)."
> +

[citation needed]

(Sorry for not complaining when I approved it for IDBEvent.cpp).

> Source/WebCore/storage/IDBEventDispatcher.h:31
> +#ifndef IDBEventDispatcher_h
> +#define IDBEvent_h
> +

#define mismatch
Comment 12 Jeremy Orlow 2011-02-14 17:19:19 PST
Committed r78525: <http://trac.webkit.org/changeset/78525>
Comment 13 WebKit Review Bot 2011-02-14 17:41:04 PST
http://trac.webkit.org/changeset/78525 might have broken EFL Linux Release (Build)