Drop all timeline seeks outside of the seekable range.

Drop all timeline seeks outside of the seekable range.

Prevents annoying accidental seeks to the beginning of content for
streaming content; non-streaming content always exposes (0, duration)
as the seekable range so it's unaffected by this change.

BUG=412562
TEST=new layout test.

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=183841

[DaleCurtis, 2014-10-01 00:08:44]

dalecurtis@chromium.org changed reviewers:
+ philipj@opera.com

[philipj_slow, 2014-10-01 11:14:22]

I'm a bit worried about the resulting user experience here. The seekable ranges
aren't exposed in any way, so it's probably going to seem a bit mysterious when
seeking works and not? If the seek handle were clamped as well it might be more
understandable, but I think that it's going to be possible to grab the seek
handle and scrub around where one in fact cannot seek?

Unless we have site exposing using <video controls> for content which is
non-streaming but not seekable, I would be OK with simply not allowing any
seeking at all using these controls. Probably by checking if the end of the last
seekable range is less than duration.

WDYT? One could be more ambitious, but the more code there is the more there is
to test. On that note, this actually ought to be testable using LayoutTests,
look for other tests that click on the timeline to get started.

https://codereview.chromium.org/614263002/diff/1/Source/core/html/shadow/Medi...
File Source/core/html/shadow/MediaControlElements.cpp (left):

https://codereview.chromium.org/614263002/diff/1/Source/core/html/shadow/Medi...
Source/core/html/shadow/MediaControlElements.cpp:422:
mediaElement().controller()->setCurrentTime(time);
Should the same rule not apply to a media element slaved to a controller?
MediaController::seekable() also returns TimeRanges, so it shouldn't be too
messy to deal with both.

https://codereview.chromium.org/614263002/diff/1/Source/core/html/shadow/Medi...
File Source/core/html/shadow/MediaControlElements.cpp (right):

https://codereview.chromium.org/614263002/diff/1/Source/core/html/shadow/Medi...
Source/core/html/shadow/MediaControlElements.cpp:67: static const double
snapPercentageForSeeks = 10 / 100.0;
This is 0.1 and not a percentage :)

https://codereview.chromium.org/614263002/diff/1/Source/core/html/shadow/Medi...
Source/core/html/shadow/MediaControlElements.cpp:439: const double duration =
getFloatingPointAttribute(maxAttr);
It took me a while to realize that the duration is in fact stored in the max
attribute, going directly to mediaElement().duration() would be more clear I
think.

[DaleCurtis, 2014-10-07 21:47:27]

Given that the experience today is that it isn't seekable at all, I wouldn't be
opposed to simply disabling seeks outside of seekable(). This allows dragging
the pointer/scrubber (?) to 0 to work properly.  WDYT?

[philipj_slow, 2014-10-08 07:27:55]

Yeah, only allowing seeking to seekable ranges sounds like a simple and
usable-enough fix.

[DaleCurtis, 2014-10-14 18:06:26]

Finally back from travelling, PTAL!  I tried to have the test verify a seek to
zero on the controls works, but just clicking getBoundingClientRect().left
appears insufficient.  I suspect there's some padding or otherwise which is
uncounted.  Do you know if that can be done in a non-flaky manner?

https://codereview.chromium.org/614263002/diff/1/Source/core/html/shadow/Medi...
File Source/core/html/shadow/MediaControlElements.cpp (left):

https://codereview.chromium.org/614263002/diff/1/Source/core/html/shadow/Medi...
Source/core/html/shadow/MediaControlElements.cpp:422:
mediaElement().controller()->setCurrentTime(time);
On 2014/10/01 11:14:22, philipj wrote:
> Should the same rule not apply to a media element slaved to a controller?
> MediaController::seekable() also returns TimeRanges, so it shouldn't be too
> messy to deal with both.

Done.

[philipj_slow, 2014-10-16 18:52:51]

LGTM with a nit, and the test looks good to me. I didn't consider that testing
the [0,0] case would run into the problem of clicking exactly the right pixel
(if there is one), so I guess that's OK. If you're feeling ambitious you can
make a test that does mouse down in the middle and the drags to the left of the
timeline before mouseup. I'm not sure, but that might just do the right thing.

https://codereview.chromium.org/614263002/diff/1/Source/core/html/shadow/Medi...
File Source/core/html/shadow/MediaControlElements.cpp (left):

https://codereview.chromium.org/614263002/diff/1/Source/core/html/shadow/Medi...
Source/core/html/shadow/MediaControlElements.cpp:422:
mediaElement().controller()->setCurrentTime(time);
On 2014/10/14 18:06:26, DaleCurtis wrote:
> On 2014/10/01 11:14:22, philipj wrote:
> > Should the same rule not apply to a media element slaved to a controller?
> > MediaController::seekable() also returns TimeRanges, so it shouldn't be too
> > messy to deal with both.
> 
> Done.

We appear to have miscommunicated :) I mean that the MediaController::seekable()
is also checked in the else branch. My rule of thumb is that a MediaController
with only a single HTMLMediaElement should behave like the HTMLMediaElement
itself, which can be done in this case.

[DaleCurtis, 2014-10-16 19:51:50]

https://codereview.chromium.org/614263002/diff/1/Source/core/html/shadow/Medi...
File Source/core/html/shadow/MediaControlElements.cpp (left):

https://codereview.chromium.org/614263002/diff/1/Source/core/html/shadow/Medi...
Source/core/html/shadow/MediaControlElements.cpp:422:
mediaElement().controller()->setCurrentTime(time);
On 2014/10/16 18:52:51, philipj wrote:
> On 2014/10/14 18:06:26, DaleCurtis wrote:
> > On 2014/10/01 11:14:22, philipj wrote:
> > > Should the same rule not apply to a media element slaved to a controller?
> > > MediaController::seekable() also returns TimeRanges, so it shouldn't be
too
> > > messy to deal with both.
> > 
> > Done.
> 
> We appear to have miscommunicated :) I mean that the
MediaController::seekable()
> is also checked in the else branch. My rule of thumb is that a MediaController
> with only a single HTMLMediaElement should behave like the HTMLMediaElement
> itself, which can be done in this case.

Sorry I still don't understand. :| Are you saying I should check if a media
controller only represents a single element and call it directly in that case?
If so, how can I tell if the controller only represents one element?

The new code always uses the controller if it's present, otherwise it uses the
media element directly.

[philipj_slow, 2014-10-16 20:03:19]

https://codereview.chromium.org/614263002/diff/1/Source/core/html/shadow/Medi...
File Source/core/html/shadow/MediaControlElements.cpp (left):

https://codereview.chromium.org/614263002/diff/1/Source/core/html/shadow/Medi...
Source/core/html/shadow/MediaControlElements.cpp:422:
mediaElement().controller()->setCurrentTime(time);
On 2014/10/16 19:51:50, DaleCurtis wrote:
> On 2014/10/16 18:52:51, philipj wrote:
> > On 2014/10/14 18:06:26, DaleCurtis wrote:
> > > On 2014/10/01 11:14:22, philipj wrote:
> > > > Should the same rule not apply to a media element slaved to a
controller?
> > > > MediaController::seekable() also returns TimeRanges, so it shouldn't be
> too
> > > > messy to deal with both.
> > > 
> > > Done.
> > 
> > We appear to have miscommunicated :) I mean that the
> MediaController::seekable()
> > is also checked in the else branch. My rule of thumb is that a
MediaController
> > with only a single HTMLMediaElement should behave like the HTMLMediaElement
> > itself, which can be done in this case.
> 
> Sorry I still don't understand. :| Are you saying I should check if a media
> controller only represents a single element and call it directly in that case?
> If so, how can I tell if the controller only represents one element?
> 
> The new code always uses the controller if it's present, otherwise it uses the
> media element directly.

Oops, sloppy reading on my part! You've already done what I asked for, I just
didn't notice because I was expecting the indentation level to change. Sorry.

[DaleCurtis, 2014-10-16 21:15:28]

Thanks for review!

[DaleCurtis, 2014-10-16 21:15:31]

The CQ bit was checked by dalecurtis@chromium.org

[commit-bot: I haz the power, 2014-10-16 21:15:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/614263002/20001

[commit-bot: I haz the power, 2014-10-16 23:17:11]

Committed patchset #2 (id:20001) as 183841