Bug 130704 - Prevent 'removetrack' events from firing when all inband text tracks are removed.
Summary: Prevent 'removetrack' events from firing when all inband text tracks are remo...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Brent Fulgham
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2014-03-24 17:43 PDT by Brent Fulgham
Modified: 2014-03-31 09:39 PDT (History)
11 users (show)

See Also:


Attachments
Patch (17.62 KB, patch)
2014-03-24 17:56 PDT, Brent Fulgham
eric.carlson: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brent Fulgham 2014-03-24 17:43:48 PDT
This patch fixes a bug where the 'removetrack' event was being fired when following the W3C logic to "forget the media element's media-resource-specific track" algorithm.

It is based on the Blink work done in <https://codereview.chromium.org/177243018/>, though it diverges a bit due to the drift in code since the fork.
Comment 1 Radar WebKit Bug Importer 2014-03-24 17:44:35 PDT
<rdar://problem/16414074>
Comment 2 Brent Fulgham 2014-03-24 17:56:29 PDT
Created attachment 227711 [details]
Patch
Comment 3 Eric Carlson 2014-03-24 19:38:40 PDT
Comment on attachment 227711 [details]
Patch

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

> Source/WebCore/html/HTMLMediaElement.h:293
> +    void removeTextTrack(TextTrack*, bool scheduleEvent = true);

Is the default value used by any of the call sites?
Comment 4 Brent Fulgham 2014-03-24 19:53:43 PDT
(In reply to comment #3)
> (From update of attachment 227711 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=227711&action=review
> 
> > Source/WebCore/html/HTMLMediaElement.h:293
> > +    void removeTextTrack(TextTrack*, bool scheduleEvent = true);
> 
> Is the default value used by any of the call sites?

Yes, mainly in the MediaSources.cpp file. I wanted to make sure the behavior of these locations was unchanged.

Do we need to suppress the remove track event for audio or video track handling? I don't think the WebVTT spec says anything about those operations.
Comment 5 Eric Carlson 2014-03-25 07:55:49 PDT
(In reply to comment #4)
> (In reply to comment #3)
> > (From update of attachment 227711 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=227711&action=review
> > 
> > > Source/WebCore/html/HTMLMediaElement.h:293
> > > +    void removeTextTrack(TextTrack*, bool scheduleEvent = true);
> > 
> > Is the default value used by any of the call sites?
> 
> Yes, mainly in the MediaSources.cpp file. I wanted to make sure the behavior of these locations was unchanged.
> 
You changed HTMLMediaElement::didRemoveTextTrack to pass true explicitly - removeTextTrack(textTrack.get(), true). We should probably be consistent one way or the other.

> Do we need to suppress the remove track event for audio or video track handling? I don't think the WebVTT spec says anything about those operations.

The WebVTT spec wouldn't, but the HTML spec might. I can look later.
Comment 6 Brent Fulgham 2014-03-25 09:51:28 PDT
(In reply to comment #5)
> You changed HTMLMediaElement::didRemoveTextTrack to pass true explicitly - removeTextTrack(textTrack.get(), true). We should probably be consistent one way or the other.

I reverted that line. Now all uses that expect the event to be triggered are called without the argument. We only pass the argument in case where we want to suppress the event.
Comment 7 Brent Fulgham 2014-03-25 09:52:58 PDT
Committed r166238: <http://trac.webkit.org/changeset/166238>
Comment 8 Alexey Proskuryakov 2014-03-31 09:39:41 PDT
The test is very flaky, filed bug 130971.