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

Issue 607493002: Introduce WebMediaPlayer::seekable() to replace maxTimeSeekable() (Closed)

Created:
6 years, 2 months ago by philipj_slow
Modified:
6 years, 2 months ago
CC:
blink-reviews, jamesr, blink-reviews-html_chromium.org, philipj_slow, gasubic, fs, eric.carlson_apple.com, abarth-chromium, feature-media-reviews_chromium.org, dglazkov+blink, nessy, vcarbune.chromium
Project:
blink
Visibility:
Public.

Description

Introduce WebMediaPlayer::seekable() to replace maxTimeSeekable() TEST=LayoutTests/media/video-seekable.html Update the test to check that seekable end time is between 5 and 7 seconds, as content/test.* are around 6 seconds long. This should catch grossly incorrect implementations of seekable. Also wait only for loadedmetadata, as that should be enough to determine the seekable ranges, and an implementation waiting until canplaythrough should fail the test. BUG=417669 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=182821

Patch Set 1 #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+22 lines, -12 lines) Patch
M LayoutTests/media/video-seekable.html View 1 chunk +4 lines, -3 lines 0 comments Download
M LayoutTests/media/video-seekable-expected.txt View 1 chunk +3 lines, -2 lines 0 comments Download
M Source/core/html/HTMLMediaElement.cpp View 1 chunk +4 lines, -6 lines 0 comments Download
M public/platform/WebMediaPlayer.h View 1 chunk +11 lines, -1 line 2 comments Download

Messages

Total messages: 12 (3 generated)
philipj_slow
PTAL. Let's keep discussion in this CL and I'll send out the two remaining CLs ...
6 years, 2 months ago (2014-09-25 13:10:07 UTC) #2
DaleCurtis
+100, this will make exposing better seekable() ranges in the future easier.
6 years, 2 months ago (2014-09-25 17:36:00 UTC) #3
DaleCurtis
lgtm
6 years, 2 months ago (2014-09-25 17:36:05 UTC) #4
scherkus (not reviewing)
lgtm! https://codereview.chromium.org/607493002/diff/1/public/platform/WebMediaPlayer.h File public/platform/WebMediaPlayer.h (right): https://codereview.chromium.org/607493002/diff/1/public/platform/WebMediaPlayer.h#newcode117 public/platform/WebMediaPlayer.h:117: virtual WebTimeRanges seekable() const I assume we'll be ...
6 years, 2 months ago (2014-09-26 01:19:27 UTC) #5
philipj_slow
It looks like we have agreement, so I'll send out the other two CLs as ...
6 years, 2 months ago (2014-09-26 07:39:26 UTC) #6
philipj_slow
pfeldman@, can you please review public/platform?
6 years, 2 months ago (2014-09-26 07:41:43 UTC) #8
pfeldman
lgtm
6 years, 2 months ago (2014-09-29 09:03:07 UTC) #9
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/607493002/1
6 years, 2 months ago (2014-09-29 09:20:27 UTC) #11
commit-bot: I haz the power
6 years, 2 months ago (2014-09-29 09:42:48 UTC) #12
Message was sent while issue was closed.
Committed patchset #1 (id:1) as 182821

Powered by Google App Engine
This is Rietveld 408576698