Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(640)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 177243018: Prevent 'removetrack' events from firing when all inband text tracks are removed. (Closed)

Created:
6 years, 10 months ago by acolwell GONE FROM CHROMIUM
Modified:
6 years, 9 months ago
Reviewers:
adamk
CC:
blink-reviews, nessy, philipj_slow, gasubic, fs, eric.carlson_apple.com, feature-media-reviews_chromium.org, dglazkov+blink, adamk+blink_chromium.org, vcarbune.chromium
Visibility:
Public.

Description

Prevent 'removetrack' events from firing when all inband text tracks are removed. This patch fixes a bug where 'removetrack' events were being fired when all the inband tracks were being removed. This behavior was trying to implement the "forget the media element's media-resource-specific tracks" algorithm, but it wasn't correct. This patch includes the following changes: - Renames removeAllInbandTracks() to forgetResourceSpecificTracks() - Reorganized logic in prepareForLoad() to match the spec order. The shuffling should not have any user visible behaviour change. - Added forgetResourceSpecificTracks() calls to other places that the spec indicates it should run. - Added code to make sure forgetResourceSpecificTracks() doesn't trigger 'removetrack' events. - Converted the existing media/track/track-remove-track.html test to W3C test & added a new test case that verifies that the event doesn't fire. BUG=249427 TEST=Existing LayoutTests still pass. Added a new case to media/track/track-remove-track.html Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=168357

Patch Set 1 #

Patch Set 2 : Fix expectations #

Total comments: 2

Patch Set 3 : Move the inband track removal loop into TextTrackList. #

Total comments: 2

Patch Set 4 : Fix expectations whitespace. #

Patch Set 5 : Address CR comment and fix layout test crashes. #

Patch Set 6 : Fix compiler errors and rebase #

Patch Set 7 : Remove expectations since testharness.js tests don't need them. #

Patch Set 8 : Add media file used by test #

Patch Set 9 : Add a trackElement error handler to shed some light on trybot errors. #

Patch Set 10 : Add video to the document. #

Patch Set 11 : Add more logging statements to aid trybot debugging. #

Patch Set 12 : Rebase and removed media file. Manually checked in the media file. #

Patch Set 13 : Remove console messages #

Unified diffs Side-by-side diffs Delta from patch set Stats (+166 lines, -75 lines) Patch
M LayoutTests/media/track/track-remove-track.html View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +87 lines, -30 lines 0 comments Download
M LayoutTests/media/track/track-remove-track-expected.txt View 1 2 3 4 5 6 1 chunk +0 lines, -15 lines 0 comments Download
M Source/core/html/HTMLMediaElement.h View 1 2 3 4 1 chunk +3 lines, -1 line 0 comments Download
M Source/core/html/HTMLMediaElement.cpp View 1 2 3 4 5 6 chunks +45 lines, -16 lines 0 comments Download
M Source/core/html/track/InbandTextTrack.h View 1 2 3 4 1 chunk +1 line, -1 line 0 comments Download
M Source/core/html/track/InbandTextTrack.cpp View 1 2 1 chunk +5 lines, -1 line 0 comments Download
M Source/core/html/track/TextTrack.h View 1 2 1 chunk +1 line, -1 line 0 comments Download
M Source/core/html/track/TextTrack.cpp View 1 2 3 4 2 chunks +11 lines, -1 line 0 comments Download
M Source/core/html/track/TextTrackList.h View 1 2 3 4 5 3 chunks +3 lines, -3 lines 0 comments Download
M Source/core/html/track/TextTrackList.cpp View 1 2 3 4 4 chunks +10 lines, -6 lines 0 comments Download

Messages

Total messages: 35 (0 generated)
acolwell GONE FROM CHROMIUM
6 years, 10 months ago (2014-02-25 23:47:11 UTC) #1
adamk
https://codereview.chromium.org/177243018/diff/20001/Source/core/html/HTMLMediaElement.cpp File Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/177243018/diff/20001/Source/core/html/HTMLMediaElement.cpp#newcode2567 Source/core/html/HTMLMediaElement.cpp:2567: for (int i = m_textTracks->length() - 1; i >= ...
6 years, 10 months ago (2014-02-26 00:45:32 UTC) #2
acolwell GONE FROM CHROMIUM
PTAL. I moved the "scary loop" into TextTrackList which allowed it to become rather boring ...
6 years, 10 months ago (2014-02-26 02:52:48 UTC) #3
adamk
Wow, much better, lgtm with style nit https://codereview.chromium.org/177243018/diff/40001/Source/core/html/track/InbandTextTrack.h File Source/core/html/track/InbandTextTrack.h (right): https://codereview.chromium.org/177243018/diff/40001/Source/core/html/track/InbandTextTrack.h#newcode50 Source/core/html/track/InbandTextTrack.h:50: void setTrackList(TextTrackList*); ...
6 years, 10 months ago (2014-02-26 16:54:34 UTC) #4
acolwell GONE FROM CHROMIUM
https://codereview.chromium.org/177243018/diff/40001/Source/core/html/track/InbandTextTrack.h File Source/core/html/track/InbandTextTrack.h (right): https://codereview.chromium.org/177243018/diff/40001/Source/core/html/track/InbandTextTrack.h#newcode50 Source/core/html/track/InbandTextTrack.h:50: void setTrackList(TextTrackList*); On 2014/02/26 16:54:34, adamk wrote: > please ...
6 years, 10 months ago (2014-02-26 19:25:12 UTC) #5
acolwell GONE FROM CHROMIUM
The CQ bit was checked by acolwell@chromium.org
6 years, 10 months ago (2014-02-26 19:27:47 UTC) #6
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/acolwell@chromium.org/177243018/80001
6 years, 10 months ago (2014-02-26 19:28:16 UTC) #7
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
6 years, 10 months ago (2014-02-26 21:30:49 UTC) #8
commit-bot: I haz the power
Retried try job too often on mac_blink_rel for step(s) webkit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_blink_rel&number=20161
6 years, 10 months ago (2014-02-26 21:30:49 UTC) #9
acolwell GONE FROM CHROMIUM
The CQ bit was checked by acolwell@chromium.org
6 years, 10 months ago (2014-02-27 04:11:37 UTC) #10
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/acolwell@chromium.org/177243018/100001
6 years, 10 months ago (2014-02-27 04:12:25 UTC) #11
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
6 years, 10 months ago (2014-02-27 05:40:11 UTC) #12
commit-bot: I haz the power
Retried try job too often on mac_blink_rel for step(s) webkit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_blink_rel&number=20225
6 years, 10 months ago (2014-02-27 05:40:12 UTC) #13
acolwell GONE FROM CHROMIUM
The CQ bit was checked by acolwell@chromium.org
6 years, 9 months ago (2014-02-27 13:26:29 UTC) #14
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/acolwell@chromium.org/177243018/120001
6 years, 9 months ago (2014-02-27 13:26:36 UTC) #15
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
6 years, 9 months ago (2014-02-27 15:35:19 UTC) #16
commit-bot: I haz the power
Retried try job too often on linux_blink_rel for step(s) webkit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blink_rel&number=23419
6 years, 9 months ago (2014-02-27 15:35:19 UTC) #17
acolwell GONE FROM CHROMIUM
The CQ bit was checked by acolwell@chromium.org
6 years, 9 months ago (2014-02-27 18:48:38 UTC) #18
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/acolwell@chromium.org/177243018/140001
6 years, 9 months ago (2014-02-27 18:48:54 UTC) #19
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
6 years, 9 months ago (2014-02-27 20:43:22 UTC) #20
commit-bot: I haz the power
Retried try job too often on mac_blink_rel for step(s) webkit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_blink_rel&number=20352
6 years, 9 months ago (2014-02-27 20:43:22 UTC) #21
acolwell GONE FROM CHROMIUM
The CQ bit was checked by acolwell@chromium.org
6 years, 9 months ago (2014-03-01 18:27:46 UTC) #22
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/acolwell@chromium.org/177243018/200002
6 years, 9 months ago (2014-03-01 18:28:01 UTC) #23
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
6 years, 9 months ago (2014-03-01 20:22:15 UTC) #24
commit-bot: I haz the power
Retried try job too often on win_layout for step(s) webkit_lint http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_layout&number=24152
6 years, 9 months ago (2014-03-01 20:22:16 UTC) #25
acolwell GONE FROM CHROMIUM
The CQ bit was checked by acolwell@chromium.org
6 years, 9 months ago (2014-03-03 19:06:29 UTC) #26
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/acolwell@chromium.org/177243018/200002
6 years, 9 months ago (2014-03-03 19:06:48 UTC) #27
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
6 years, 9 months ago (2014-03-03 20:04:35 UTC) #28
commit-bot: I haz the power
Retried try job too often on win_layout for step(s) webkit_lint http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_layout&number=24798
6 years, 9 months ago (2014-03-03 20:04:36 UTC) #29
acolwell GONE FROM CHROMIUM
The CQ bit was checked by acolwell@chromium.org
6 years, 9 months ago (2014-03-03 20:37:24 UTC) #30
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/acolwell@chromium.org/177243018/200002
6 years, 9 months ago (2014-03-03 20:37:38 UTC) #31
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/acolwell@chromium.org/177243018/200002
6 years, 9 months ago (2014-03-03 23:20:57 UTC) #32
acolwell GONE FROM CHROMIUM
The CQ bit was unchecked by acolwell@chromium.org
6 years, 9 months ago (2014-03-04 00:58:47 UTC) #33
acolwell GONE FROM CHROMIUM
The CQ bit was checked by acolwell@chromium.org
6 years, 9 months ago (2014-03-04 00:58:48 UTC) #34
commit-bot: I haz the power
6 years, 9 months ago (2014-03-04 04:04:52 UTC) #35
Message was sent while issue was closed. 
Change committed as 168357

Powered by Google App Engine
This is Rietveld 408576698