Bug 106731 - IndexedDB: Fix tests which depend upon V8 event listener behavior
Summary: IndexedDB: Fix tests which depend upon V8 event listener behavior
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Joshua Bell
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-01-12 17:07 PST by Michael Pruett
Modified: 2013-01-15 14:29 PST (History)
7 users (show)

See Also:


Attachments
Patch (3.77 KB, patch)
2013-01-12 17:16 PST, Michael Pruett
no flags Details | Formatted Diff | Diff
Patch (26.26 KB, patch)
2013-01-15 11:54 PST, Joshua Bell
no flags Details | Formatted Diff | Diff
Patch (28.29 KB, patch)
2013-01-15 12:27 PST, Joshua Bell
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Pruett 2013-01-12 17:07:09 PST
Currently two IndexedDB tests fail when run on JSC because they depend upon event listener behavior which differs between V8 and JSC.

V8 sets window.event for all event listeners to the current event and resets this variable to its previous state once the event listener has returned. In JSC by contrast, once window.event is overridden in user code, the user-defined definition persists and will not be reset when calling subsequent event listeners.

Two tests, storage/indexeddb/cursor-advance.html and storage/indexeddb/deletedatabase-delayed-by-open-and-versionchange.html, set window.event and then depend upon the value of this variable to be reset in a subsequent event handler. These tests should be modified to work on JSC.
Comment 1 Michael Pruett 2013-01-12 17:16:09 PST
Created attachment 182477 [details]
Patch
Comment 2 Joshua Bell 2013-01-12 17:28:22 PST
The preamble(evt) pattern is used so that the tests will run in both a window context and in a worker context - workers don't set a global event property. We don't yet have HTML files for running them all in worker contexts, so this patch probably won't fail EWS but we'd like to keep (or update) that pattern. 

Agreed that the tests should be updated, but we'll need to decide what to do for these tests in Workers. 

(I'll be able to look more closely on Monday.)
Comment 3 Joshua Bell 2013-01-15 11:54:47 PST
Created attachment 182821 [details]
Patch
Comment 4 Joshua Bell 2013-01-15 11:56:06 PST
I attached a different approach (that Michael and I discussed on IRC) that uses preamble(evt) consistently. This has the happy side effect of making the tests run in workers, so I added the *-workers.html wrappers for these.

Michael - can you confirm that these work in your in-progress port?
Comment 5 Joshua Bell 2013-01-15 12:27:35 PST
Created attachment 182826 [details]
Patch
Comment 6 Joshua Bell 2013-01-15 12:28:08 PST
Oops, one more try. Thanks Michael!

If Michael gives this the thumbs up, tony@ can you r?
Comment 7 Michael Pruett 2013-01-15 12:32:37 PST
(In reply to comment #6)
> Oops, one more try. Thanks Michael!
> 
> If Michael gives this the thumbs up, tony@ can you r?

This change looks good to me. The tests pass in WebKitGTK+.
Comment 8 Joshua Bell 2013-01-15 14:25:31 PST
Comment on attachment 182826 [details]
Patch

Thanks, tony@ !
Comment 9 WebKit Review Bot 2013-01-15 14:29:23 PST
Comment on attachment 182826 [details]
Patch

Clearing flags on attachment: 182826

Committed r139793: <http://trac.webkit.org/changeset/139793>
Comment 10 WebKit Review Bot 2013-01-15 14:29:27 PST
All reviewed patches have been landed.  Closing bug.