Project: chromium Issues People Development process History Sign in
New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.
Issue 313287 Minor <track> and WebVTT issues
Starred by 1 user Reported by phil...@opera.com, Oct 30 2013 Back to list
Status: Fixed
Owner:
Email to this user bounced
Closed: Dec 2013
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug



Sign in to add a comment
An umbrella bug for minor issues I find while working on updating <track> and WebVTT to match the spec.
 
Comment 1 by phil...@opera.com, Oct 30 2013
bugdroid seems to not be commenting as expected, here's two landed reviews it missed:

https://codereview.chromium.org/48113029
https://codereview.chromium.org/51883010
Project Member Comment 3 by bugdroid1@chromium.org, Oct 31 2013
The following revision refers to this bug:
    http://src.chromium.org/viewvc/blink?view=rev&rev=160970

------------------------------------------------------------------------
r160970 | philipj@opera.com | 2013-10-30T21:27:27.224892Z

Changed paths:
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/html/track/LoadableTextTrack.cpp?r1=160970&r2=160969&pathrev=160970
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/loader/TextTrackLoader.cpp?r1=160970&r2=160969&pathrev=160970
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/loader/TextTrackLoader.h?r1=160970&r2=160969&pathrev=160970

Use a TextTrackLoaderClient reference (not pointer)

It's never null, so remove all doubt for future readers of the code.

BUG= 313287 

Review URL: https://codereview.chromium.org/48113029
------------------------------------------------------------------------
Project Member Comment 4 by bugdroid1@chromium.org, Oct 31 2013
The following revision refers to this bug:
    http://src.chromium.org/viewvc/blink?view=rev&rev=160971

------------------------------------------------------------------------
r160971 | philipj@opera.com | 2013-10-30T21:30:18.522534Z

Changed paths:
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/html/track/LoadableTextTrack.cpp?r1=160971&r2=160970&pathrev=160971
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/html/HTMLTrackElement.cpp?r1=160971&r2=160970&pathrev=160971
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/html/HTMLTrackElement.h?r1=160971&r2=160970&pathrev=160971

Simplify HTMLTrackElement::didCompleteLoad

It doesn't need to be virtual and didn't use the first argument.

BUG= 313287 

Review URL: https://codereview.chromium.org/51883010
------------------------------------------------------------------------
Project Member Comment 5 by bugdroid1@chromium.org, Oct 31 2013
The following revision refers to this bug:
    http://src.chromium.org/viewvc/blink?view=rev&rev=160993

------------------------------------------------------------------------
r160993 | philipj@opera.com | 2013-10-31T01:07:43.318227Z

Changed paths:
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/html/track/LoadableTextTrack.h?r1=160993&r2=160992&pathrev=160993

Remove LoadableTextTrackClient

It is completely unused.

BUG= 313287 

Review URL: https://codereview.chromium.org/53413003
------------------------------------------------------------------------
Project Member Comment 6 by bugdroid1@chromium.org, Oct 31 2013
The following revision refers to this bug:
    http://src.chromium.org/viewvc/blink?view=rev&rev=161040

------------------------------------------------------------------------
r161040 | philipj@opera.com | 2013-10-31T09:23:24.717425Z

Changed paths:
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/loader/TextTrackLoader.cpp?r1=161040&r2=161039&pathrev=161040

Handle resource fetch failure in TextTrackLoader::load

It's not clear when this can happen, even after looking at
ResourceFetcher::requestResource I was unable to produce a test
case which would hit this code path. However, given that we are
null-checking already, we might as well handle it correctly.

BUG= 313287 

Review URL: https://codereview.chromium.org/53213002
------------------------------------------------------------------------
Project Member Comment 7 by bugdroid1@chromium.org, Oct 31 2013
The following revision refers to this bug:
    http://src.chromium.org/viewvc/blink?view=rev&rev=161047

------------------------------------------------------------------------
r161047 | philipj@opera.com | 2013-10-31T10:39:55.932734Z

Changed paths:
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/html/HTMLTrackElement.h?r1=161047&r2=161046&pathrev=161047

Correct use of virtual and OVERRIDE in HTMLTrackElement

canLoadUrl was virtual for no reason, and the TextTrackClient
implementation was missing OVERRIDE.

BUG= 313287 

Review URL: https://codereview.chromium.org/52443006
------------------------------------------------------------------------
Project Member Comment 8 by bugdroid1@chromium.org, Oct 31 2013
The following revision refers to this bug:
    http://src.chromium.org/viewvc/blink?view=rev&rev=161048

------------------------------------------------------------------------
r161048 | philipj@opera.com | 2013-10-31T10:55:37.311176Z

Changed paths:
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/loader/TextTrackLoader.cpp?r1=161048&r2=161047&pathrev=161048
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/html/track/LoadableTextTrack.h?r1=161048&r2=161047&pathrev=161048
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/loader/TextTrackLoader.h?r1=161048&r2=161047&pathrev=161048
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/html/track/LoadableTextTrack.cpp?r1=161048&r2=161047&pathrev=161048

Remove TextTrackLoaderClient::shouldLoadCues and ::cueLoadingStarted

Their only implementations (in TextTrackLoader) do nothing useful,
so just remove them until needed.

Also sprinkle some OVERRIDE in the neighborhood.

BUG= 313287 

Review URL: https://codereview.chromium.org/53533003
------------------------------------------------------------------------
Comment 9 by phil...@opera.com, Oct 31 2013
Labels: Cr-Blink-Text-Track
Comment 10 by phil...@opera.com, Oct 31 2013
Labels: -Cr-Internals-Media-Track
Project Member Comment 11 by bugdroid1@chromium.org, Nov 1 2013
The following revision refers to this bug:
    http://src.chromium.org/viewvc/blink?view=rev&rev=161172

------------------------------------------------------------------------
r161172 | philipj@opera.com | 2013-11-01T16:02:46.396906Z

Changed paths:
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/html/track/TextTrackCue.h?r1=161172&r2=161171&pathrev=161172
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/rendering/RenderTextTrackCue.cpp?r1=161172&r2=161171&pathrev=161172
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/html/shadow/MediaControlElements.cpp?r1=161172&r2=161171&pathrev=161172
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/html/track/TextTrackCue.cpp?r1=161172&r2=161171&pathrev=161172

Remove virtual where not needed in TextTrackCue

Remove TextTrackCue::cueType() since it always returned CueType::WebVTT
and was never overriden. CueType::Generic was for TextTrackCueGeneric
which was removed in r158977.

Remove TextTrackCue::videoSizeDidChange(...) since it did nothing and
was never overridden. This looks like unfinished logic for moving cues
when the controls are shown and hidden, and could be revived in
< http://crbug.com/314066 >.

BUG= 313287 

Review URL: https://codereview.chromium.org/50903009
------------------------------------------------------------------------
Project Member Comment 12 by bugdroid1@chromium.org, Nov 1 2013
The following revision refers to this bug:
    http://src.chromium.org/viewvc/blink?view=rev&rev=161197

------------------------------------------------------------------------
r161197 | philipj@opera.com | 2013-11-01T20:44:34.421849Z

Changed paths:
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/html/track/LoadableTextTrack.h?r1=161197&r2=161196&pathrev=161197

Make LoadableTextTrack::clearClient not virtual

When TextTrack::clearClient and LoadableTextTrack::clearClient were
added in r99984, both had the virtual keyword and nothing seemed odd.

In r135410 the virtual keyword was dropped from TextTrack::clearClient
because "it is unnecessary", but LoadableTextTrack::clearClient was left
virtual even though it now shadowed (instead of overriding) TextTrack.

The HTMLTrackElement destructor calls LoadableTextTrack::clearClient, so
there's no risk of LoadableTextTrack::m_trackElement becoming stale.

It would be nice to sort this out even more, perhaps by refactoring
LoadableTextTrack into a TextTrackCueProvider or something that doesn't
inherit from TextTrack, but until then add a comment to calm the reader.

BUG= 313287 

Review URL: https://codereview.chromium.org/54963004
------------------------------------------------------------------------
Project Member Comment 13 by bugdroid1@chromium.org, Nov 9 2013
The following revision refers to this bug:
    http://src.chromium.org/viewvc/blink?view=rev&rev=161661

------------------------------------------------------------------------
r161661 | philipj@opera.com | 2013-11-09T00:01:36.597666Z

Changed paths:
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/html/HTMLMediaElement.cpp?r1=161661&r2=161660&pathrev=161661

Replace redundant <track> runtime checks with asserts

The IDL as RuntimeEnabled=VideoTrack for these two cases.

BUG= 313287 

Review URL: https://codereview.chromium.org/61763017
------------------------------------------------------------------------
Comment 14 by phil...@opera.com, Dec 9 2013
Status: Fixed
Closing this now, it's served its purpose.
Components: -Blink>Text>Track Blink>Media>Track
Renamed Blink>Text>Track to Blink>Media>Track. Moving these bugs to the new component.
Sign in to add a comment