Switch from WebMediaPlayer::maxTimeSeekable() to seekable()

Switch from WebMediaPlayer::maxTimeSeekable() to seekable()

BUG=417669
Committed: https://crrev.com/b0e6f3fce92c7e2cc3df4a1d10d5508a48d1febc
Cr-Commit-Position: refs/heads/master@{#297395}

[philipj_slow, 2014-09-26 07:44:57]

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

[philipj_slow, 2014-09-26 07:44:58]

PTAL. No need to be gentle, I know this is a bit ugly, I was being conservative
to not introduce any change in behavior.

[scherkus (not reviewing), 2014-09-26 18:53:28]

lgtm w/ question about interval notation because it's been a while since I've
had to think about math :(

https://codereview.chromium.org/599103003/diff/1/content/renderer/media/andro...
File content/renderer/media/android/webmediaplayer_android.cc (right):

https://codereview.chromium.org/599103003/diff/1/content/renderer/media/andro...
content/renderer/media/android/webmediaplayer_android.cc:519: // If we have a
duration then use [0, duration] as the seekable range.
silly math/interval notation question: seeking to duration() is valid so using ]
is correct vs. ), right? (i.e., seekable() represents closed intervals)

https://codereview.chromium.org/599103003/diff/1/media/blink/webmediaplayer_i...
File media/blink/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/599103003/diff/1/media/blink/webmediaplayer_i...
media/blink/webmediaplayer_impl.cc:499: // If we have a duration then use [0,
duration] as the seekable range.
ditto

[philipj_slow, 2014-09-26 20:32:02]

All that's needed to land these patches is platform review on the first patch.
If you have an API owner handy, could you ask for review? (pfeldman is probably
asleep by now)

https://codereview.chromium.org/599103003/diff/1/content/renderer/media/andro...
File content/renderer/media/android/webmediaplayer_android.cc (right):

https://codereview.chromium.org/599103003/diff/1/content/renderer/media/andro...
content/renderer/media/android/webmediaplayer_android.cc:519: // If we have a
duration then use [0, duration] as the seekable range.
On 2014/09/26 18:53:27, scherkus wrote:
> silly math/interval notation question: seeking to duration() is valid so using
]
> is correct vs. ), right? (i.e., seekable() represents closed intervals)

Right, these are inclusive intervals, not only for duration but for any seekable
range the clamping in the spec's seek algorithm make it clear that the end time
of an interval must be reachable by seeking.

[DaleCurtis, 2014-09-26 20:45:24]

lgtm

https://codereview.chromium.org/599103003/diff/1/content/renderer/media/andro...
File content/renderer/media/android/webmediaplayer_android.cc (right):

https://codereview.chromium.org/599103003/diff/1/content/renderer/media/andro...
content/renderer/media/android/webmediaplayer_android.cc:518: 
WDYT about reordering slightly so that:

const double duration = duration();
if (!duration)
  return WebTimeRanges();

<more complex steps>

I find it's best to handle the simple cases at the top, but I don't feel
strongly, so if you disagree, by all means ignore me :)

https://codereview.chromium.org/599103003/diff/1/media/blink/webmediaplayer_i...
File media/blink/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/599103003/diff/1/media/blink/webmediaplayer_i...
media/blink/webmediaplayer_impl.cc:498: 
Ditto.

[philipj_slow, 2014-09-29 09:33:31]

early return

[philipj_slow, 2014-09-29 09:33:44]

https://codereview.chromium.org/599103003/diff/1/content/renderer/media/andro...
File content/renderer/media/android/webmediaplayer_android.cc (right):

https://codereview.chromium.org/599103003/diff/1/content/renderer/media/andro...
content/renderer/media/android/webmediaplayer_android.cc:518: 
On 2014/09/26 20:45:24, DaleCurtis wrote:
> WDYT about reordering slightly so that:
> 
> const double duration = duration();
> if (!duration)
>   return WebTimeRanges();
> 
> <more complex steps>
> 
> I find it's best to handle the simple cases at the top, but I don't feel
> strongly, so if you disagree, by all means ignore me :)

Done.

[DaleCurtis, 2014-09-29 16:52:44]

https://codereview.chromium.org/599103003/diff/20001/content/renderer/media/a...
File content/renderer/media/android/webmediaplayer_android.cc (right):

https://codereview.chromium.org/599103003/diff/20001/content/renderer/media/a...
content/renderer/media/android/webmediaplayer_android.cc:530: const double
duration = this->duration();
Avoid this-> with current_duration or something? It's unusual style for chromium
code.

https://codereview.chromium.org/599103003/diff/20001/content/renderer/media/a...
content/renderer/media/android/webmediaplayer_android.cc:534: WebTimeRanges
ranges(static_cast<size_t>(1));
Does 1 hit a compiler error somewhere? 1u should work if so.

[philipj_slow, 2014-09-29 19:51:46]

https://codereview.chromium.org/599103003/diff/20001/content/renderer/media/a...
File content/renderer/media/android/webmediaplayer_android.cc (right):

https://codereview.chromium.org/599103003/diff/20001/content/renderer/media/a...
content/renderer/media/android/webmediaplayer_android.cc:530: const double
duration = this->duration();
On 2014/09/29 16:52:43, DaleCurtis wrote:
> Avoid this-> with current_duration or something? It's unusual style for
chromium
> code.

I don't like this style either, I was just going with the name you suggested :)
I'll use something different.

https://codereview.chromium.org/599103003/diff/20001/content/renderer/media/a...
content/renderer/media/android/webmediaplayer_android.cc:534: WebTimeRanges
ranges(static_cast<size_t>(1));
On 2014/09/29 16:52:43, DaleCurtis wrote:
> Does 1 hit a compiler error somewhere? 1u should work if so.

Yes, it's ambiguous because WebVector has so many constructors. I tried 1u and
that didn't work, but I'll see if there's something less ugly that
static_casting to be done.

[philipj_slow, 2014-09-30 08:07:48]

no this-> or static_cast<size_t>

[philipj_slow, 2014-09-30 08:09:29]

I'll go ahead and land this. As always, if anything is unpalatable don't
hesitate to say so and I'll fix it in a follow-up.

https://codereview.chromium.org/599103003/diff/20001/content/renderer/media/a...
File content/renderer/media/android/webmediaplayer_android.cc (right):

https://codereview.chromium.org/599103003/diff/20001/content/renderer/media/a...
content/renderer/media/android/webmediaplayer_android.cc:530: const double
duration = this->duration();
On 2014/09/29 19:51:46, philipj wrote:
> On 2014/09/29 16:52:43, DaleCurtis wrote:
> > Avoid this-> with current_duration or something? It's unusual style for
> chromium
> > code.
> 
> I don't like this style either, I was just going with the name you suggested
:)
> I'll use something different.

seekable_end it is.

https://codereview.chromium.org/599103003/diff/20001/content/renderer/media/a...
content/renderer/media/android/webmediaplayer_android.cc:534: WebTimeRanges
ranges(static_cast<size_t>(1));
On 2014/09/29 19:51:46, philipj wrote:
> On 2014/09/29 16:52:43, DaleCurtis wrote:
> > Does 1 hit a compiler error somewhere? 1u should work if so.
> 
> Yes, it's ambiguous because WebVector has so many constructors. I tried 1u and
> that didn't work, but I'll see if there's something less ugly that
> static_casting to be done.

I went with the WebVector(const U* values, size_t size) constructor instead.

[philipj_slow, 2014-09-30 08:09:56]

The CQ bit was checked by philipj@opera.com

[commit-bot: I haz the power, 2014-09-30 08:10:53]

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

[commit-bot: I haz the power, 2014-09-30 08:28:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-30 08:28:07]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...)
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)

[philipj_slow, 2014-09-30 08:53:23]

compile on android

[philipj_slow, 2014-09-30 08:53:29]

The CQ bit was checked by philipj@opera.com

[commit-bot: I haz the power, 2014-09-30 08:54:14]

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

[commit-bot: I haz the power, 2014-09-30 09:52:01]

Committed patchset #4 (id:60001) as 27726d24b7e455b1c887978cfb4aa2e5e341bda3

[commit-bot: I haz the power, 2014-09-30 09:52:50]

Patchset 4 (id:??) landed as
https://crrev.com/b0e6f3fce92c7e2cc3df4a1d10d5508a48d1febc
Cr-Commit-Position: refs/heads/master@{#297395}

[DaleCurtis, 2014-09-30 17:22:34]

still lgtm, thanks!