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

Issue 242113012: Make text track and cue wrapper objects dependent on their media element. (Closed)

Created:
6 years, 8 months ago by sof
Modified:
6 years, 8 months ago
CC:
blink-reviews, nessy, arv+blink, philipj_slow, gasubic, fs, eric.carlson_apple.com, watchdog-blink-watchlist_google.com, feature-media-reviews_chromium.org, dglazkov+blink, Inactive, ojan, vcarbune.chromium, adamk+blink_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Visibility:
Public.

Description

Make text track and cue wrapper objects dependent on their media element. If a TextTrack or VTTCue (TextTrackCue) object is added to a media element, indirectly by way of addCue() or addTextTrack(), arrange for their wrapper objects to be registered as dependents of the media element's wrapper so as to keep these wrapper objects alive. This is done so that when we later need to return back a wrapper for the underlying track/cue object, the same wrapper object is returned. R=acolwell@chromium.org BUG=358109 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=172065

Patch Set 1 #

Patch Set 2 : Consistently use a Node-returning owner() #

Total comments: 8

Patch Set 3 : Tidy up code in places #

Unified diffs Side-by-side diffs Delta from patch set Stats (+118 lines, -5 lines) Patch
M LayoutTests/media/track/text-track-cue-is-reachable-expected.txt View 1 chunk +1 line, -1 line 0 comments Download
M LayoutTests/media/track/text-track-is-reachable-expected.txt View 1 chunk +1 line, -1 line 0 comments Download
A LayoutTests/media/track/track-cue-gc-wrapper.html View 1 2 1 chunk +63 lines, -0 lines 0 comments Download
A LayoutTests/media/track/track-cue-gc-wrapper-expected.txt View 1 chunk +33 lines, -0 lines 0 comments Download
M Source/core/html/track/TextTrack.h View 1 1 chunk +2 lines, -1 line 0 comments Download
M Source/core/html/track/TextTrack.cpp View 1 2 1 chunk +6 lines, -1 line 0 comments Download
M Source/core/html/track/TextTrack.idl View 1 1 chunk +3 lines, -1 line 0 comments Download
M Source/core/html/track/TextTrackCue.h View 1 1 chunk +2 lines, -0 lines 0 comments Download
M Source/core/html/track/TextTrackCue.cpp View 1 2 1 chunk +5 lines, -0 lines 0 comments Download
M Source/core/html/track/TextTrackCue.idl View 1 1 chunk +1 line, -0 lines 0 comments Download
M Source/core/html/track/vtt/VTTCue.idl View 1 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 10 (0 generated)
sof
Please take a look.
6 years, 8 months ago (2014-04-20 18:35:05 UTC) #1
acolwell GONE FROM CHROMIUM
lgtm % nits https://codereview.chromium.org/242113012/diff/20001/LayoutTests/media/track/track-cue-gc-wrapper.html File LayoutTests/media/track/track-cue-gc-wrapper.html (right): https://codereview.chromium.org/242113012/diff/20001/LayoutTests/media/track/track-cue-gc-wrapper.html#newcode3 LayoutTests/media/track/track-cue-gc-wrapper.html:3: <head> nit: 4 space indent tags ...
6 years, 8 months ago (2014-04-21 16:51:16 UTC) #2
sof
Thanks for the quick review. https://codereview.chromium.org/242113012/diff/20001/LayoutTests/media/track/track-cue-gc-wrapper.html File LayoutTests/media/track/track-cue-gc-wrapper.html (right): https://codereview.chromium.org/242113012/diff/20001/LayoutTests/media/track/track-cue-gc-wrapper.html#newcode3 LayoutTests/media/track/track-cue-gc-wrapper.html:3: <head> On 2014/04/21 16:51:16, ...
6 years, 8 months ago (2014-04-21 18:35:00 UTC) #3
sof
The CQ bit was checked by sigbjornf@opera.com
6 years, 8 months ago (2014-04-21 19:03:59 UTC) #4
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sigbjornf@opera.com/242113012/40001
6 years, 8 months ago (2014-04-21 19:04:11 UTC) #5
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
6 years, 8 months ago (2014-04-21 19:06:06 UTC) #6
commit-bot: I haz the power
Try jobs failed on following builders: tryserver.blink on win_blink_rel
6 years, 8 months ago (2014-04-21 19:06:07 UTC) #7
sof
The CQ bit was checked by sigbjornf@opera.com
6 years, 8 months ago (2014-04-21 19:08:40 UTC) #8
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sigbjornf@opera.com/242113012/40001
6 years, 8 months ago (2014-04-21 19:08:49 UTC) #9
commit-bot: I haz the power
6 years, 8 months ago (2014-04-21 19:39:54 UTC) #10
Message was sent while issue was closed.
Change committed as 172065

Powered by Google App Engine
This is Rietveld 408576698