When HLS redirects are encountered recreate WebMediaPlayer.

When HLS redirects are encountered recreate WebMediaPlayer.

Not all WebMediaPlayer implementations know how to play HLS, and
if the URL does not contain an HLS identifier originally, a later
redirect might break playback if the wrong implementation is
chosen.

When this occurs instead re-trigger Blink's loader with the
redirected source URL that contains the proper HLS tagging.

It's preferred to do this instead of either adding a shim WMP
implementation that choses the real WMP during load() or making
createWebMediaPlayer() block until redirects complete and then
throwing away the data from those connections.

BUG=618109
TEST=Manually tested espn.go.com and adds a new browser test.

Committed: https://crrev.com/ea27a3ed74612a72945993222b01f6aaddccbfe4
Cr-Commit-Position: refs/heads/master@{#401777}

[DaleCurtis, 2016-06-13 22:36:00]

dalecurtis@chromium.org changed reviewers:
+ foolip@chromium.org, hubbe@chromium.org

[DaleCurtis, 2016-06-13 22:36:01]

Publishing this so I can find it later in case we decide to go this route. For
now I think it's simpler to just blacklist the bad domain from Spitzer playback;
UMA stats show we're 99.89% accurate and this workaround would cause the WMPA
path to do additional network loading that was unnecessary if we implement it.

[DaleCurtis, 2016-06-13 22:36:11]

Alternative: https://codereview.chromium.org/2060353004

[DaleCurtis, 2016-06-13 23:26:24]

content/ folk hate the alternative, so we may need this after all. Please look
over it and provide your thoughts.

[DaleCurtis, 2016-06-15 20:43:40]

foolip, hubbe: Ping.

[hubbe, 2016-06-15 21:07:56]

LGTM % nits

https://codereview.chromium.org/2046253002/diff/40001/media/base/android/medi...
File media/base/android/media_codec_util.h (right):

https://codereview.chromium.org/2046253002/diff/40001/media/base/android/medi...
media/base/android/media_codec_util.h:69: // Test whether a URL contains "m3u8".
(Using exactly the same logic as
This seems like a very questionable thing to do, unless false positives are ok.

https://codereview.chromium.org/2046253002/diff/40001/media/blink/buffered_da...
File media/blink/buffered_data_source.h (right):

https://codereview.chromium.org/2046253002/diff/40001/media/blink/buffered_da...
media/blink/buffered_data_source.h:118: // Returns the post-Initialize() URL
after redirects have been followed.
Please document if it's ok for this to change if additional redirects are
encountered or not.

https://codereview.chromium.org/2046253002/diff/40001/media/blink/buffered_da...
media/blink/buffered_data_source.h:119: virtual GURL GetResponseOriginURL()
const = 0;
GetResponseOriginalURL only makes sense if you know what specific fields in the
webkit response class are.

How about GetUrlAfterRedirects() ?

https://codereview.chromium.org/2046253002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/2046253002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:919: // Reload a
post-redirect URL from a previous WebMediaPlayer instance. This provides a
line
length?

[DaleCurtis, 2016-06-15 23:11:31]

https://codereview.chromium.org/2046253002/diff/40001/media/base/android/medi...
File media/base/android/media_codec_util.h (right):

https://codereview.chromium.org/2046253002/diff/40001/media/base/android/medi...
media/base/android/media_codec_util.h:69: // Test whether a URL contains "m3u8".
(Using exactly the same logic as
On 2016/06/15 at 21:07:56, hubbe wrote:
> This seems like a very questionable thing to do, unless false positives are
ok.

It's incredibly questionable, but false positives are okay for now, they just
mean we'll use WebMediaPlayerAndroid instead of WebMediaPlayerImpl. Long term
we'll use bytestream information instead to select the Mojo MediaPlayer.

https://codereview.chromium.org/2046253002/diff/40001/media/blink/buffered_da...
File media/blink/buffered_data_source.h (right):

https://codereview.chromium.org/2046253002/diff/40001/media/blink/buffered_da...
media/blink/buffered_data_source.h:118: // Returns the post-Initialize() URL
after redirects have been followed.
On 2016/06/15 at 21:07:56, hubbe wrote:
> Please document if it's ok for this to change if additional redirects are
encountered or not.

Done.

https://codereview.chromium.org/2046253002/diff/40001/media/blink/buffered_da...
media/blink/buffered_data_source.h:119: virtual GURL GetResponseOriginURL()
const = 0;
On 2016/06/15 at 21:07:56, hubbe wrote:
> GetResponseOriginalURL only makes sense if you know what specific fields in
the webkit response class are.
> 
> How about GetUrlAfterRedirects() ?

Done.

https://codereview.chromium.org/2046253002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/2046253002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:919: // Reload a
post-redirect URL from a previous WebMediaPlayer instance. This provides a
On 2016/06/15 at 21:07:56, hubbe wrote:
> line
> length?

Blink uses longer line lengths unfortunately, so I've tried to maintain
consistency.

[hubbe, 2016-06-15 23:25:08]

lgtm

[DaleCurtis, 2016-06-16 16:53:56]

dalecurtis@chromium.org changed reviewers:
+ mlamouri@chromium.org

[DaleCurtis, 2016-06-16 16:53:57]

+mlamouri since foolip@ seems out.

[foolip, 2016-06-20 10:30:44]

Last week was BlinkOn, sorry for the delay.

This seems quite tricky to do in a way that's without risk, see comment. Ideas?

https://codereview.chromium.org/2046253002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/2046253002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1446: //
TODO(tguilbert): Remove once http://crbug.com/580626 is fixed.
Will support for HLS be added to WMPI, or what's the path for full unification?

https://codereview.chromium.org/2046253002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1448:
scheduleNextSourceChild();
This will end up calling either loadNextSourceChild() or loadInternal(). For
loadNextSourceChild(), it looks like this won't work, the
resetMediaPlayerAndMediaSource() will ruin it all.

In the loadInternal() case, loadResource() will still run some steps twice that
seem problematic, like  setting m_currentSrc (exposing the redirected URL) and
updating m_muted. That can probably be observed by web content but shouldn't be.

Would a more surgical replacement of the m_webMediaPlayer do the trick, or are
there things that then end up out of sync? I'm thinking maybe a
WebMediaPlayerClient::requestReload(WebURL) that would (in a posted task)
destroy the old WebMediaPlayer and replace it with a new one while changing as
little state as possible. Maybe the end of HTMLMediaElement::startPlayerLoad
could be factored out.

[DaleCurtis, 2016-06-20 18:54:25]

https://codereview.chromium.org/2046253002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/2046253002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1446: //
TODO(tguilbert): Remove once http://crbug.com/580626 is fixed.
On 2016/06/20 at 10:30:44, Philip Jägenstedt wrote:
> Will support for HLS be added to WMPI, or what's the path for full
unification?

We'll be adding support for HLS to WMPI, but it will be accomplished by hosting
an Android MediaPlayer in an out-of-process Mojo shell. Here are some more
details on that flow:

https://docs.google.com/document/d/1DHdFhWPGe1holzQM1Leq5tmFI81s-FQ4wbx1CN8sD...

https://codereview.chromium.org/2046253002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1448:
scheduleNextSourceChild();
On 2016/06/20 at 10:30:44, Philip Jägenstedt wrote:
> This will end up calling either loadNextSourceChild() or loadInternal(). For
loadNextSourceChild(), it looks like this won't work, the
resetMediaPlayerAndMediaSource() will ruin it all.
> 
> In the loadInternal() case, loadResource() will still run some steps twice
that seem problematic, like  setting m_currentSrc (exposing the redirected URL)
and updating m_muted. That can probably be observed by web content but shouldn't
be.
> 
> Would a more surgical replacement of the m_webMediaPlayer do the trick, or are
there things that then end up out of sync? I'm thinking maybe a
WebMediaPlayerClient::requestReload(WebURL) that would (in a posted task)
destroy the old WebMediaPlayer and replace it with a new one while changing as
little state as possible. Maybe the end of HTMLMediaElement::startPlayerLoad
could be factored out.

This is a good idea. I don't think we need to post the task as far as I can
tell. We just need to be careful about making sure this is the last line of the
function when called. Otherwise I've implemented your suggestion and it work
well.

[DaleCurtis, 2016-06-22 01:34:17]

foolip: Ping. Thanks!

[foolip, 2016-06-22 13:10:54]

https://codereview.chromium.org/2046253002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/2046253002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3000:
loadResource(WebMediaPlayerSource(newUrl), contentType);
This will still have the problems with changing m_currentSrc and re-checking for
the muted attribute mentioned, and possibly other consequences that aren't
obvious.

I believe that exposing the redirected URL or a cross-origin resource could be
exposing information that isn't otherwise possible to get at.

I think the nicest fix here would be to match the spec, where the currentSrc
attribute is updated *before* the resource fetch algorithm is invoked. If we
pretend that loadResource is the resource fetch algorithm (it's the closest
thing we have) then it's just a matter of setting m_currentSrc before each call
to loadResource, maybe in a helper that also calls onCurrentSrcChanged.

https://codereview.chromium.org/2046253002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLMediaElement.h (right):

https://codereview.chromium.org/2046253002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLMediaElement.h:351: void
requestReload(const WebURL& newUrl) final;
https://www.chromium.org/blink/coding-style#TOC-Names point 9 would want the
parameter name omitted. Style guide differences between Chromium and Blink are
fun?

[DaleCurtis, 2016-06-22 14:28:35]

https://codereview.chromium.org/2046253002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/2046253002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3000:
loadResource(WebMediaPlayerSource(newUrl), contentType);
On 2016/06/22 at 13:10:54, Philip Jägenstedt wrote:
> This will still have the problems with changing m_currentSrc and re-checking
for the muted attribute mentioned, and possibly other consequences that aren't
obvious.
> 
> I believe that exposing the redirected URL or a cross-origin resource could be
exposing information that isn't otherwise possible to get at.
> 
> I think the nicest fix here would be to match the spec, where the currentSrc
attribute is updated *before* the resource fetch algorithm is invoked. If we
pretend that loadResource is the resource fetch algorithm (it's the closest
thing we have) then it's just a matter of setting m_currentSrc before each call
to loadResource, maybe in a helper that also calls onCurrentSrcChanged.

I'm not sure I follow the differences from what's being done here. Setting
m_currentSrc before calling loadResource() appears equivalent to letting
loadResource() set the URL. Are you proposing removing the
m_currentSrc+onCurrentSourceChanged() from loadResource() in favor of making
callers to loadResource() perform that task on their own ahead of time? I.e.
with a helper SetCurrentSrc() which changes m_currentSrc+onCurrentSrcChanged?

[foolip, 2016-06-22 16:01:52]

https://codereview.chromium.org/2046253002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/2046253002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3000:
loadResource(WebMediaPlayerSource(newUrl), contentType);
On 2016/06/22 14:28:35, DaleCurtis wrote:
> On 2016/06/22 at 13:10:54, Philip Jägenstedt wrote:
> > This will still have the problems with changing m_currentSrc and re-checking
> for the muted attribute mentioned, and possibly other consequences that aren't
> obvious.
> > 
> > I believe that exposing the redirected URL or a cross-origin resource could
be
> exposing information that isn't otherwise possible to get at.
> > 
> > I think the nicest fix here would be to match the spec, where the currentSrc
> attribute is updated *before* the resource fetch algorithm is invoked. If we
> pretend that loadResource is the resource fetch algorithm (it's the closest
> thing we have) then it's just a matter of setting m_currentSrc before each
call
> to loadResource, maybe in a helper that also calls onCurrentSrcChanged.
> 
> I'm not sure I follow the differences from what's being done here. Setting
> m_currentSrc before calling loadResource() appears equivalent to letting
> loadResource() set the URL. Are you proposing removing the
> m_currentSrc+onCurrentSourceChanged() from loadResource() in favor of making
> callers to loadResource() perform that task on their own ahead of time? I.e.
> with a helper SetCurrentSrc() which changes m_currentSrc+onCurrentSrcChanged?

Yeah, I'm suggesting to make the order more like the spec, and a setCurrentSrc
sounds reasonable. There would be one call (this one) to loadResource that
doesn't call setCurrentSrc first, which would be the whole point of such a
change.

[DaleCurtis, 2016-06-22 16:17:41]

https://codereview.chromium.org/2046253002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/2046253002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3000:
loadResource(WebMediaPlayerSource(newUrl), contentType);
On 2016/06/22 at 16:01:52, Philip Jägenstedt wrote:
> On 2016/06/22 14:28:35, DaleCurtis wrote:
> > On 2016/06/22 at 13:10:54, Philip Jägenstedt wrote:
> > > This will still have the problems with changing m_currentSrc and
re-checking
> > for the muted attribute mentioned, and possibly other consequences that
aren't
> > obvious.
> > > 
> > > I believe that exposing the redirected URL or a cross-origin resource
could be
> > exposing information that isn't otherwise possible to get at.
> > > 
> > > I think the nicest fix here would be to match the spec, where the
currentSrc
> > attribute is updated *before* the resource fetch algorithm is invoked. If we
> > pretend that loadResource is the resource fetch algorithm (it's the closest
> > thing we have) then it's just a matter of setting m_currentSrc before each
call
> > to loadResource, maybe in a helper that also calls onCurrentSrcChanged.
> > 
> > I'm not sure I follow the differences from what's being done here. Setting
> > m_currentSrc before calling loadResource() appears equivalent to letting
> > loadResource() set the URL. Are you proposing removing the
> > m_currentSrc+onCurrentSourceChanged() from loadResource() in favor of making
> > callers to loadResource() perform that task on their own ahead of time? I.e.
> > with a helper SetCurrentSrc() which changes
m_currentSrc+onCurrentSrcChanged?
> 
> Yeah, I'm suggesting to make the order more like the spec, and a setCurrentSrc
sounds reasonable. There would be one call (this one) to loadResource that
doesn't call setCurrentSrc first, which would be the whole point of such a
change.

StartPlayerLoad() also uses m_currentSrc though, so you're also proposing that
startPlayerLoad() use a passed in parameter instead? Or are you suggesting we
create some m_previousPlayerSource WebURL which is used by StartPlayerLoad() in
this case.

[foolip, 2016-06-22 18:00:50]

https://codereview.chromium.org/2046253002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/2046253002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3000:
loadResource(WebMediaPlayerSource(newUrl), contentType);
On 2016/06/22 16:17:41, DaleCurtis wrote:
> On 2016/06/22 at 16:01:52, Philip Jägenstedt wrote:
> > On 2016/06/22 14:28:35, DaleCurtis wrote:
> > > On 2016/06/22 at 13:10:54, Philip Jägenstedt wrote:
> > > > This will still have the problems with changing m_currentSrc and
> re-checking
> > > for the muted attribute mentioned, and possibly other consequences that
> aren't
> > > obvious.
> > > > 
> > > > I believe that exposing the redirected URL or a cross-origin resource
> could be
> > > exposing information that isn't otherwise possible to get at.
> > > > 
> > > > I think the nicest fix here would be to match the spec, where the
> currentSrc
> > > attribute is updated *before* the resource fetch algorithm is invoked. If
we
> > > pretend that loadResource is the resource fetch algorithm (it's the
closest
> > > thing we have) then it's just a matter of setting m_currentSrc before each
> call
> > > to loadResource, maybe in a helper that also calls onCurrentSrcChanged.
> > > 
> > > I'm not sure I follow the differences from what's being done here. Setting
> > > m_currentSrc before calling loadResource() appears equivalent to letting
> > > loadResource() set the URL. Are you proposing removing the
> > > m_currentSrc+onCurrentSourceChanged() from loadResource() in favor of
making
> > > callers to loadResource() perform that task on their own ahead of time?
I.e.
> > > with a helper SetCurrentSrc() which changes
> m_currentSrc+onCurrentSrcChanged?
> > 
> > Yeah, I'm suggesting to make the order more like the spec, and a
setCurrentSrc
> sounds reasonable. There would be one call (this one) to loadResource that
> doesn't call setCurrentSrc first, which would be the whole point of such a
> change.
> 
> StartPlayerLoad() also uses m_currentSrc though, so you're also proposing that
> startPlayerLoad() use a passed in parameter instead? Or are you suggesting we
> create some m_previousPlayerSource WebURL which is used by StartPlayerLoad()
in
> this case.

Oh, that's a problem. It's a bit silly that loadResource takes a
WebMediaPlayerSource but then startPlayerLoad has to create one from scratch. If
this new code path could just pass along the WebMediaPlayerSource all the way,
then it could leave m_currentSrc along and there is no problem.

How about letting startPlayerLoad also take a const WebMediaPlayerSource& and
constructing one in executeDeferredLoad? It's a bit brittle, but it will work so
long as deferred loads are only for normal URLs, which is currently the case.

I may have overlooked something more, of course.

[DaleCurtis, 2016-06-22 23:08:29]

Patchset #6 (id:100001) has been deleted

[DaleCurtis, 2016-06-22 23:16:15]

https://codereview.chromium.org/2046253002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/2046253002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3000:
loadResource(WebMediaPlayerSource(newUrl), contentType);
On 2016/06/22 at 18:00:49, Philip Jägenstedt wrote:
> On 2016/06/22 16:17:41, DaleCurtis wrote:
> > On 2016/06/22 at 16:01:52, Philip Jägenstedt wrote:
> > > On 2016/06/22 14:28:35, DaleCurtis wrote:
> > > > On 2016/06/22 at 13:10:54, Philip Jägenstedt wrote:
> > > > > This will still have the problems with changing m_currentSrc and
> > re-checking
> > > > for the muted attribute mentioned, and possibly other consequences that
> > aren't
> > > > obvious.
> > > > > 
> > > > > I believe that exposing the redirected URL or a cross-origin resource
> > could be
> > > > exposing information that isn't otherwise possible to get at.
> > > > > 
> > > > > I think the nicest fix here would be to match the spec, where the
> > currentSrc
> > > > attribute is updated *before* the resource fetch algorithm is invoked.
If we
> > > > pretend that loadResource is the resource fetch algorithm (it's the
closest
> > > > thing we have) then it's just a matter of setting m_currentSrc before
each
> > call
> > > > to loadResource, maybe in a helper that also calls onCurrentSrcChanged.
> > > > 
> > > > I'm not sure I follow the differences from what's being done here.
Setting
> > > > m_currentSrc before calling loadResource() appears equivalent to letting
> > > > loadResource() set the URL. Are you proposing removing the
> > > > m_currentSrc+onCurrentSourceChanged() from loadResource() in favor of
making
> > > > callers to loadResource() perform that task on their own ahead of time?
I.e.
> > > > with a helper SetCurrentSrc() which changes
> > m_currentSrc+onCurrentSrcChanged?
> > > 
> > > Yeah, I'm suggesting to make the order more like the spec, and a
setCurrentSrc
> > sounds reasonable. There would be one call (this one) to loadResource that
> > doesn't call setCurrentSrc first, which would be the whole point of such a
> > change.
> > 
> > StartPlayerLoad() also uses m_currentSrc though, so you're also proposing
that
> > startPlayerLoad() use a passed in parameter instead? Or are you suggesting
we
> > create some m_previousPlayerSource WebURL which is used by StartPlayerLoad()
in
> > this case.
> 
> Oh, that's a problem. It's a bit silly that loadResource takes a
WebMediaPlayerSource but then startPlayerLoad has to create one from scratch. If
this new code path could just pass along the WebMediaPlayerSource all the way,
then it could leave m_currentSrc along and there is no problem.
> 
> How about letting startPlayerLoad also take a const WebMediaPlayerSource& and
constructing one in executeDeferredLoad? It's a bit brittle, but it will work so
long as deferred loads are only for normal URLs, which is currently the case.
> 
> I may have overlooked something more, of course.

I started going down this path and it quickly escalated into a pretty large
change. It ends up needing a couple helper functions that interact with the
loaded URL in subtle ways. I.e., today startPlayerLoad() doesn't use the source
given to loadResource() it filters its own.

Instead I introduced a playerProvidedUrl to the relevant functions which
overrides m_currentSrc updates and provides a succinct comment on why we do
this. I think this is simpler and less likely to have merge conflicts for
getting this back to M52.

[foolip, 2016-06-23 11:22:36]

A bit concerned about how to make this less invasive, WDYT? Tests for this is
also a very good idea, because this code is borderline in need of refactoring,
and this edge case will be easy to break without test.

https://codereview.chromium.org/2046253002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLMediaElement.h (right):

https://codereview.chromium.org/2046253002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLMediaElement.h:351: void
requestReload(const WebURL& newUrl) final;
On 2016/06/22 13:10:54, Philip Jägenstedt wrote:
> https://www.chromium.org/blink/coding-style#TOC-Names point 9 would want the
> parameter name omitted. Style guide differences between Chromium and Blink are
> fun?

Ping.

https://codereview.chromium.org/2046253002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/2046253002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1068:
startPlayerLoad(playerProvidedUrl);
If preload=none, then I think you'll have taken the deferLoad() path above and
the playerProviderUrl will be lost.

I've looked through loadResource to see which steps we really want to run again
and not, and I think that it's actually very little.

An isSafeToLoadURL check should be done again I think, but currently isn't. Or
how does that currently work with redirects, is there something in WMPI that
check redirect URLs in a similar way?

I think that startProgressEventTimer shouldn't be necessary, at least I can't
see what would stop the timer in HTMLMediaElement::requestReload.

And for the rest, I think none of it is needed.

So, could you try to have requestReload call directly into startPlayerLoad with
the new URL, with just a isSafeToLoadURL check first?

[DaleCurtis, 2016-06-23 18:55:57]

Comments addressed. I'm not sure what to do about a test here though. We don't
run layout tests on Android, I'll poke around and see if I can find a way to
write a content browser test for this.

https://codereview.chromium.org/2046253002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLMediaElement.h (right):

https://codereview.chromium.org/2046253002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLMediaElement.h:351: void
requestReload(const WebURL& newUrl) final;
On 2016/06/23 at 11:22:36, Philip Jägenstedt wrote:
> On 2016/06/22 13:10:54, Philip Jägenstedt wrote:
> > https://www.chromium.org/blink/coding-style#TOC-Names point 9 would want the
> > parameter name omitted. Style guide differences between Chromium and Blink
are
> > fun?
> 
> Ping.

Whoops, sorry, done.

https://codereview.chromium.org/2046253002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/2046253002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1068:
startPlayerLoad(playerProvidedUrl);
On 2016/06/23 at 11:22:36, Philip Jägenstedt wrote:
> If preload=none, then I think you'll have taken the deferLoad() path above and
the playerProviderUrl will be lost.
> 
> I've looked through loadResource to see which steps we really want to run
again and not, and I think that it's actually very little.
> 
> An isSafeToLoadURL check should be done again I think, but currently isn't. Or
how does that currently work with redirects, is there something in WMPI that
check redirect URLs in a similar way?
> 
> I think that startProgressEventTimer shouldn't be necessary, at least I can't
see what would stop the timer in HTMLMediaElement::requestReload.
> 
> And for the rest, I think none of it is needed.
> 
> So, could you try to have requestReload call directly into startPlayerLoad
with the new URL, with just a isSafeToLoadURL check first?

This seems to work fine. Done.

[DaleCurtis, 2016-06-23 21:36:56]

Latest patch set adds a test... though most of MediaBrowserTest's are disabled
on Android, so I'm not sure it'll last long :(

[DaleCurtis, 2016-06-23 21:37:35]

Description was changed from

==========
When HLS redirects are encountered recreate WebMediaPlayer.

Not all WebMediaPlayer implementations know how to play HLS, and
if the URL does not contain an HLS identifier originally, a later
redirect might break playback if the wrong implementation is
chosen.

When this occurs instead re-trigger Blink's loader with the
redirected source URL that contains the proper HLS tagging.

It's preferred to do this instead of either adding a shim WMP
implementation that choses the real WMP during load() or making
createWebMediaPlayer() block until redirects complete and then
throwing away the data from those connections.

BUG=618109
TEST=tbd
==========

to

==========
When HLS redirects are encountered recreate WebMediaPlayer.

Not all WebMediaPlayer implementations know how to play HLS, and
if the URL does not contain an HLS identifier originally, a later
redirect might break playback if the wrong implementation is
chosen.

When this occurs instead re-trigger Blink's loader with the
redirected source URL that contains the proper HLS tagging.

It's preferred to do this instead of either adding a shim WMP
implementation that choses the real WMP during load() or making
createWebMediaPlayer() block until redirects complete and then
throwing away the data from those connections.

BUG=618109
TEST=Manually tested espn.go.com and adds a new browser test.
==========

[DaleCurtis, 2016-06-23 22:13:11]

The CQ bit was checked by dalecurtis@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-23 22:14:01]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2046253002/160001

[commit-bot: I haz the power, 2016-06-23 22:23:24]

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

[commit-bot: I haz the power, 2016-06-23 22:23:25]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[foolip, 2016-06-23 23:00:20]

third_party/WebKit lgtm, I'll be OOO until Tuesday so if you need any more
feedback please poke Mounir :)

[DaleCurtis, 2016-06-24 00:11:59]

Thanks foolip@, hubbe@ can you take a nother pass over the new media_browsertest
code?

- dale

[DaleCurtis, 2016-06-24 00:13:54]

The CQ bit was checked by dalecurtis@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-24 00:14:47]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2046253002/180001

[hubbe, 2016-06-24 00:27:17]

lgtm

[DaleCurtis, 2016-06-24 00:44:29]

The CQ bit was unchecked by dalecurtis@chromium.org

[DaleCurtis, 2016-06-24 00:44:37]

The CQ bit was checked by dalecurtis@chromium.org

[DaleCurtis, 2016-06-24 00:44:37]

The patchset sent to the CQ was uploaded after l-g-t-m from foolip@chromium.org
Link to the patchset: https://codereview.chromium.org/2046253002/#ps180001
(title: "Fix compile failures.")

[commit-bot: I haz the power, 2016-06-24 00:45:51]

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

[commit-bot: I haz the power, 2016-06-24 01:41:43]

Committed patchset #9 (id:180001)

[commit-bot: I haz the power, 2016-06-24 01:44:39]

Description was changed from

==========
When HLS redirects are encountered recreate WebMediaPlayer.

Not all WebMediaPlayer implementations know how to play HLS, and
if the URL does not contain an HLS identifier originally, a later
redirect might break playback if the wrong implementation is
chosen.

When this occurs instead re-trigger Blink's loader with the
redirected source URL that contains the proper HLS tagging.

It's preferred to do this instead of either adding a shim WMP
implementation that choses the real WMP during load() or making
createWebMediaPlayer() block until redirects complete and then
throwing away the data from those connections.

BUG=618109
TEST=Manually tested espn.go.com and adds a new browser test.
==========

to

==========
When HLS redirects are encountered recreate WebMediaPlayer.

Not all WebMediaPlayer implementations know how to play HLS, and
if the URL does not contain an HLS identifier originally, a later
redirect might break playback if the wrong implementation is
chosen.

When this occurs instead re-trigger Blink's loader with the
redirected source URL that contains the proper HLS tagging.

It's preferred to do this instead of either adding a shim WMP
implementation that choses the real WMP during load() or making
createWebMediaPlayer() block until redirects complete and then
throwing away the data from those connections.

BUG=618109
TEST=Manually tested espn.go.com and adds a new browser test.

Committed: https://crrev.com/ea27a3ed74612a72945993222b01f6aaddccbfe4
Cr-Commit-Position: refs/heads/master@{#401777}
==========

[commit-bot: I haz the power, 2016-06-24 01:44:41]

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