Bug 83858 - The cuechange event on track element is fired synchronously rather than queued
Summary: The cuechange event on track element is fired synchronously rather than queued
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Anna Cavender
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2012-04-12 21:25 PDT by Anna Cavender
Modified: 2012-04-13 16:05 PDT (History)
6 users (show)

See Also:

Attachments
Patch (9.75 KB, patch)
2012-04-12 21:57 PDT, Anna Cavender 		no flags Details | Formatted Diff | Diff
assert track element and remove fireCueChangeEvent() (12.01 KB, patch)
2012-04-13 13:42 PDT, Anna Cavender 		eric.carlson: review+
annacc: commit-queue-
Details | Formatted Diff | Diff
Show Obsolete (1) 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 Anna Cavender 2012-04-12 21:25:46 PDT 
4.8.10.8 Playing the media resource: http://www.whatwg.org/specs/web-apps/current-work/#playing-the-media-resource "When the current playback position of a media element changes..." specifies how events are to be sorted and filtered before dispatch.

Currently, the cuechange event on track elements is fired synchronously rather than queued along with the other events in the queueing algorithm.
Comment 1 Anna Cavender 2012-04-12 21:57:03 PDT 
Created attachment 137037 [details]
Patch
Comment 2 Eric Carlson 2012-04-13 11:20:13 PDT 
Comment on attachment 137037 [details]
Patch

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

Nice fix, thanks!

> Source/WebCore/html/HTMLMediaElement.cpp:-1197
> -            affectedTracks[i]->fireCueChangeEvent();

It looks like fireCueChangeEvent will not be used after this change. If so, it should be removed.

> Source/WebCore/html/HTMLMediaElement.cpp:1197
> +        if (affectedTracks[i]->trackType() == TextTrack::TrackElement) {
> +            RefPtr<Event> event = Event::create(eventNames().cuechangeEvent, false, false);
> +            event->setTarget(static_cast<LoadableTextTrack*>(affectedTracks[i])->trackElement());

I think it would be safer to put the HTMLTrackElement in a local so you can ASSERT it value before setting the event target.
Comment 3 Radar WebKit Bug Importer 2012-04-13 11:20:52 PDT 
<rdar://problem/11246028>
Comment 4 Anna Cavender 2012-04-13 13:41:14 PDT 
Comment on attachment 137037 [details]
Patch

Thanks Eric!

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

>> Source/WebCore/html/HTMLMediaElement.cpp:-1197
>> -            affectedTracks[i]->fireCueChangeEvent();
> 
> It looks like fireCueChangeEvent will not be used after this change. If so, it should be removed.

Good call.

>> Source/WebCore/html/HTMLMediaElement.cpp:1197
>> +            event->setTarget(static_cast<LoadableTextTrack*>(affectedTracks[i])->trackElement());
> 
> I think it would be safer to put the HTMLTrackElement in a local so you can ASSERT it value before setting the event target.

Another good call.  Thanks.
Comment 5 Anna Cavender 2012-04-13 13:42:03 PDT 
Created attachment 137139 [details]
assert track element and remove fireCueChangeEvent()
Comment 6 Anna Cavender 2012-04-13 14:48:05 PDT 
Committed as http://trac.webkit.org/changeset/114171
Comment 7 Alexey Proskuryakov 2012-04-13 16:05:03 PDT 
*** Bug 83878 has been marked as a duplicate of this bug. ***