Bug 55414 - Fix race condition in 4 media tests
Summary: Fix race condition in 4 media tests
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: Tony Gentilcore
URL:
Keywords:
Depends on:
Blocks: 54355
  Show dependency treegraph
 
Reported: 2011-02-28 14:55 PST by Tony Gentilcore
Modified: 2011-02-28 18:03 PST (History)
8 users (show)

See Also:


Attachments
Patch (7.35 KB, patch)
2011-02-28 15:22 PST, Tony Gentilcore
no flags Details | Formatted Diff | Diff
Patch for landing (7.42 KB, patch)
2011-02-28 15:46 PST, Tony Gentilcore
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tony Gentilcore 2011-02-28 14:55:24 PST
Fix race condition in 4 media tests
Comment 1 Tony Gentilcore 2011-02-28 15:22:41 PST
Created attachment 84134 [details]
Patch
Comment 2 Darin Adler 2011-02-28 15:28:05 PST
Comment on attachment 84134 [details]
Patch

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

Looks OK as long as the waitForEvent function is only ever used on events that are unique enough.

> LayoutTests/media/video-test.js:3
> -var mediaElement = null;
> +var mediaElement = document;

This is pretty strange; expedient but pretty unclear! Probably needs a comment.
Comment 3 Tony Gentilcore 2011-02-28 15:36:08 PST
(In reply to comment #2)
> (From update of attachment 84134 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=84134&action=review
> 
> Looks OK as long as the waitForEvent function is only ever used on events that are unique enough.

They are all unique to media elements except for 'error.' I don't think this is a big problem as other errors on the page should be unexpected. But if you think it is a good idea, I could also check the target of the event and only dispatch the callback if it is a media element.

> 
> > LayoutTests/media/video-test.js:3
> > -var mediaElement = null;
> > +var mediaElement = document;
> 
> This is pretty strange; expedient but pretty unclear! Probably needs a comment.

It ends up being strange because of these special "video" and "mediaElement" global variables. Would be much more clear if they were arguments to the method, but that is more refactoring that I want to do here. Will add a comment.
Comment 4 Tony Gentilcore 2011-02-28 15:46:56 PST
Created attachment 84136 [details]
Patch for landing
Comment 5 WebKit Commit Bot 2011-02-28 17:31:52 PST
Comment on attachment 84136 [details]
Patch for landing

Clearing flags on attachment: 84136

Committed r79949: <http://trac.webkit.org/changeset/79949>
Comment 6 WebKit Commit Bot 2011-02-28 17:31:57 PST
All reviewed patches have been landed.  Closing bug.
Comment 7 WebKit Review Bot 2011-02-28 18:03:32 PST
http://trac.webkit.org/changeset/79949 might have broken SnowLeopard Intel Release (Build)