Move deferred loading logic from WebMediaPlayerClientImpl to HTMLMediaElement.

Move deferred loading logic from WebMediaPlayerClientImpl to HTMLMediaElement.

BUG=350571
TEST=All layout tests still pass.

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

[acolwell GONE FROM CHROMIUM, 2014-03-08 18:56:20]

Please take a look. I've uploaded the changes as a series of patches so you can
see how the transformation evolved in simple steps. 

The only surprising change might be the removal of clearWebMediaPlayer() in the
last patch set. As far as I can tell HTMLMediaElement never calls load() on the
same m_player instance so that code was apparently unnecessary. I have asserts
in place to catch this situation, just in case I missed something.

[philipj_slow, 2014-03-08 19:38:40]

Non-owner LGTM with nits. There are of course many things to fix in general, but
this is a necessary first step for getting preload="none" into better shape.

https://codereview.chromium.org/192013002/diff/40001/Source/core/html/HTMLMed...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/192013002/diff/40001/Source/core/html/HTMLMed...
Source/core/html/HTMLMediaElement.cpp:88: using blink::WebMediaPlayer;
Move up one line to keep list sorted.

https://codereview.chromium.org/192013002/diff/40001/Source/core/html/HTMLMed...
Source/core/html/HTMLMediaElement.cpp:881: m_delayingLoad = true;
http://www.whatwg.org/specs/web-apps/current-work/multipage/the-video-element...

There are a bunch of things missing here, if you want to fix them. They were
missing before as well, so if you want to keep this refactoring without
observable changes that's fine too.

https://codereview.chromium.org/192013002/diff/40001/Source/core/html/HTMLMed...
Source/core/html/HTMLMediaElement.cpp:3659: m_player->setPreload(m_preload);
I had to look for a while before I understood why this can't be null. It turns
out we call createMediaPlayer in prepareForLoad() even though me might not need
it. Not in this review, but maybe it should be done at the start of the resource
fetch algorithm instead...

https://codereview.chromium.org/192013002/diff/40001/Source/core/html/HTMLMed...
Source/core/html/HTMLMediaElement.cpp:3674: WebMediaPlayer::LoadType
HTMLMediaElement::loadType() const
Maybe move these functions up together with their relatives, around load() or
loadResource()?

https://codereview.chromium.org/192013002/diff/40001/Source/core/html/HTMLMed...
File Source/core/html/HTMLMediaElement.h (right):

https://codereview.chromium.org/192013002/diff/40001/Source/core/html/HTMLMed...
Source/core/html/HTMLMediaElement.h:511: bool m_delayingLoad;
Merge this with the bitfield above and it becomes free. A different name to
avoid confusion with the spec's delaying-the-load-event flag would also be
great.

https://codereview.chromium.org/192013002/diff/40001/Source/web/WebMediaPlaye...
File Source/web/WebMediaPlayerClientImpl.cpp (left):

https://codereview.chromium.org/192013002/diff/40001/Source/web/WebMediaPlaye...
Source/web/WebMediaPlayerClientImpl.cpp:221: // Tell WebMediaPlayer about the
poster image URL.
Re-indent this comment too? Or just remove it, it's rather useless.

[acolwell GONE FROM CHROMIUM, 2014-03-10 21:53:30]

https://codereview.chromium.org/192013002/diff/40001/Source/core/html/HTMLMed...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/192013002/diff/40001/Source/core/html/HTMLMed...
Source/core/html/HTMLMediaElement.cpp:88: using blink::WebMediaPlayer;
On 2014/03/08 19:38:40, philipj wrote:
> Move up one line to keep list sorted.

Done.

https://codereview.chromium.org/192013002/diff/40001/Source/core/html/HTMLMed...
Source/core/html/HTMLMediaElement.cpp:881: m_delayingLoad = true;
On 2014/03/08 19:38:40, philipj wrote:
>
http://www.whatwg.org/specs/web-apps/current-work/multipage/the-video-element...
> 
> There are a bunch of things missing here, if you want to fix them. They were
> missing before as well, so if you want to keep this refactoring without
> observable changes that's fine too.

I just want to keep this CL as a simple move of existing functionality. I
figured we can do a sweep to bring things into line with the spec in later CLs.
Long-term I'd like to make the code in this file look a lot closer to the spec
text. I'm pretty sure we are aligned on that front. :)

https://codereview.chromium.org/192013002/diff/40001/Source/core/html/HTMLMed...
Source/core/html/HTMLMediaElement.cpp:3659: m_player->setPreload(m_preload);
On 2014/03/08 19:38:40, philipj wrote:
> I had to look for a while before I understood why this can't be null. It turns
> out we call createMediaPlayer in prepareForLoad() even though me might not
need
> it. Not in this review, but maybe it should be done at the start of the
resource
> fetch algorithm instead...
Agreed. I was surprised about this as well especially since one of the call
sites checks m_player and the other one doesn't. In general, I think we'll
probably want to revisit when m_player is actually created. Just preserving
existing behavior here for now.

https://codereview.chromium.org/192013002/diff/40001/Source/core/html/HTMLMed...
Source/core/html/HTMLMediaElement.cpp:3674: WebMediaPlayer::LoadType
HTMLMediaElement::loadType() const
On 2014/03/08 19:38:40, philipj wrote:
> Maybe move these functions up together with their relatives, around load() or
> loadResource()?

Done.

https://codereview.chromium.org/192013002/diff/40001/Source/core/html/HTMLMed...
File Source/core/html/HTMLMediaElement.h (right):

https://codereview.chromium.org/192013002/diff/40001/Source/core/html/HTMLMed...
Source/core/html/HTMLMediaElement.h:511: bool m_delayingLoad;
On 2014/03/08 19:38:40, philipj wrote:
> Merge this with the bitfield above and it becomes free. A different name to
> avoid confusion with the spec's delaying-the-load-event flag would also be
> great.

Done.

https://codereview.chromium.org/192013002/diff/40001/Source/web/WebMediaPlaye...
File Source/web/WebMediaPlayerClientImpl.cpp (left):

https://codereview.chromium.org/192013002/diff/40001/Source/web/WebMediaPlaye...
Source/web/WebMediaPlayerClientImpl.cpp:221: // Tell WebMediaPlayer about the
poster image URL.
On 2014/03/08 19:38:40, philipj wrote:
> Re-indent this comment too? Or just remove it, it's rather useless.

Done.

[adamk, 2014-03-10 22:01:21]

lgtm for Source/web/

[philipj_slow, 2014-03-12 05:21:01]

PS6 LGTM.

https://codereview.chromium.org/192013002/diff/40001/Source/core/html/HTMLMed...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/192013002/diff/40001/Source/core/html/HTMLMed...
Source/core/html/HTMLMediaElement.cpp:881: m_delayingLoad = true;
On 2014/03/10 21:53:31, acolwell_OOO_til_3-15 wrote:
> On 2014/03/08 19:38:40, philipj wrote:
> >
>
http://www.whatwg.org/specs/web-apps/current-work/multipage/the-video-element...
> > 
> > There are a bunch of things missing here, if you want to fix them. They were
> > missing before as well, so if you want to keep this refactoring without
> > observable changes that's fine too.
> 
> I just want to keep this CL as a simple move of existing functionality. I
> figured we can do a sweep to bring things into line with the spec in later
CLs.
> Long-term I'd like to make the code in this file look a lot closer to the spec
> text. I'm pretty sure we are aligned on that front. :)

Sounds great!

[acolwell GONE FROM CHROMIUM, 2014-03-17 17:36:01]

The CQ bit was checked by acolwell@chromium.org

[commit-bot: I haz the power, 2014-03-17 17:36:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/acolwell@chromium.org/192013002/90001

[commit-bot: I haz the power, 2014-03-17 17:39:06]

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

[commit-bot: I haz the power, 2014-03-17 17:39:07]

Try jobs failed on following builders:
  tryserver.blink on linux_blink_rel

[acolwell GONE FROM CHROMIUM, 2014-03-18 16:18:41]

The CQ bit was checked by acolwell@chromium.org

[commit-bot: I haz the power, 2014-03-18 16:18:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/acolwell@chromium.org/192013002/110001

[commit-bot: I haz the power, 2014-03-18 17:01:53]

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

[commit-bot: I haz the power, 2014-03-18 17:01:57]

Try jobs failed on following builders:
  tryserver.blink on win_blink_rel

[acolwell GONE FROM CHROMIUM, 2014-03-18 18:07:34]

The CQ bit was checked by acolwell@chromium.org

[commit-bot: I haz the power, 2014-03-18 18:07:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/acolwell@chromium.org/192013002/110001

[commit-bot: I haz the power, 2014-03-18 19:15:50]

Change committed as 169465