Expose the poster image URL to WebMediaPlayer subclasses

Expose the poster image URL to WebMediaPlayer subclasses

This is the Blink change for exposing the poster image to Chrome for Android.

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

[whywhat, 2014-02-04 15:51:58]

lgtm

https://codereview.chromium.org/131763011/diff/1/Source/web/WebMediaPlayerCli...
File Source/web/WebMediaPlayerClientImpl.cpp (right):

https://codereview.chromium.org/131763011/diff/1/Source/web/WebMediaPlayerCli...
Source/web/WebMediaPlayerClientImpl.cpp:247: if (!poster.isEmpty()) {
nit: you don't need the curly braces for a one-line branch.

[philipj_slow, 2014-02-04 16:10:09]

This seems rather odd, why does the platform need the poster URL? I tried to
find the answer in the bug, but
http://code.google.com/p/chromium/issues/detail?id=333260 is a 403...

[whywhat, 2014-02-04 16:15:43]

On 2014/02/04 16:10:09, philipj wrote:
> This seems rather odd, why does the platform need the poster URL? I tried to
> find the answer in the bug, but
> http://code.google.com/p/chromium/issues/detail?id=333260 is a 403...

Hi, we're working on opening the bugs up at the moment.

This is about playing the video remotely (e.g. on a Chromecast) - the UI might
need a poster image to show on a notification when the browser is in the
background, for example.

[acolwell GONE FROM CHROMIUM, 2014-02-04 18:07:49]

On 2014/02/04 16:15:43, whywhat wrote:
> On 2014/02/04 16:10:09, philipj wrote:
> > This seems rather odd, why does the platform need the poster URL? I tried to
> > find the answer in the bug, but
> > http://code.google.com/p/chromium/issues/detail?id=333260 is a 403...
> 
> Hi, we're working on opening the bugs up at the moment.
> 
> This is about playing the video remotely (e.g. on a Chromecast) - the UI might
> need a poster image to show on a notification when the browser is in the
> background, for example.

not lgtm. This does not seem like the right approach to me and this change
doesn't properly handle updating the poster if the application changes it.

[whywhat, 2014-02-04 18:20:14]

On 2014/02/04 18:07:49, acolwell wrote:
> On 2014/02/04 16:15:43, whywhat wrote:
> > On 2014/02/04 16:10:09, philipj wrote:
> > > This seems rather odd, why does the platform need the poster URL? I tried
to
> > > find the answer in the bug, but
> > > http://code.google.com/p/chromium/issues/detail?id=333260 is a 403...
> > 
> > Hi, we're working on opening the bugs up at the moment.
> > 
> > This is about playing the video remotely (e.g. on a Chromecast) - the UI
might
> > need a poster image to show on a notification when the browser is in the
> > background, for example.
> 
> not lgtm. This does not seem like the right approach to me

what would the right approach be? We discussed this approach with Min Qin who's
OOO for another two weeks unfortunately and with michaelbai@.

 and this change
> doesn't properly handle updating the poster if the application changes it.

Where this change is being observed by Blink and how could we handle it?

[acolwell GONE FROM CHROMIUM, 2014-02-04 18:32:26]

On 2014/02/04 18:20:14, whywhat wrote:
> On 2014/02/04 18:07:49, acolwell wrote:
> > On 2014/02/04 16:15:43, whywhat wrote:
> > > On 2014/02/04 16:10:09, philipj wrote:
> > > > This seems rather odd, why does the platform need the poster URL? I
tried
> to
> > > > find the answer in the bug, but
> > > > http://code.google.com/p/chromium/issues/detail?id=333260 is a 403...
> > > 
> > > Hi, we're working on opening the bugs up at the moment.
> > > 
> > > This is about playing the video remotely (e.g. on a Chromecast) - the UI
> might
> > > need a poster image to show on a notification when the browser is in the
> > > background, for example.
> > 
> > not lgtm. This does not seem like the right approach to me
> 
> what would the right approach be? We discussed this approach with Min Qin
who's
> OOO for another two weeks unfortunately and with michaelbai@.

Where is the Chromium code that this call would be passing the poster
information to? I don't fully understand why you need this or what you are
trying to do so I need more context before I can propose a better solution.

> 
>  and this change
> > doesn't properly handle updating the poster if the application changes it.
> 
> Where this change is being observed by Blink and how could we handle it?

Poster attribute updates are handled here
(https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...)
so I'd expect a call down into player() everytime this value changed.

[michaelbai, 2014-02-04 18:43:42]

I don't know why you need to set poster url, but there is method to do so.

WebSettings::setDefaultVideoPosterURL()

[whywhat, 2014-02-04 19:02:30]

On 2014/02/04 18:43:42, michaelbai wrote:
> I don't know why you need to set poster url, but there is method to do so.
> 
> WebSettings::setDefaultVideoPosterURL()

The bug is now open. The Chromium code fetches the bitmap the same way we fetch
favicons and passes it down to the Java UI that uses it as a thumbnaul for media
controls (not exactly but very much alike the notification/lock screen you get
when playing music in the background)

[whywhat, 2014-02-04 19:03:43]

On 2014/02/04 19:02:30, whywhat wrote:
> On 2014/02/04 18:43:42, michaelbai wrote:
> > I don't know why you need to set poster url, but there is method to do so.
> > 
> > WebSettings::setDefaultVideoPosterURL()
> 
> The bug is now open. The Chromium code fetches the bitmap the same way we
fetch
> favicons and passes it down to the Java UI that uses it as a thumbnaul for
media
> controls (not exactly but very much alike the notification/lock screen you get
> when playing music in the background)

The Chromium change is https://codereview.chromium.org/135863004/

[michaelbai, 2014-02-04 19:55:24]

So, Is it ok to download poster twice?

[cimamoglu (inactive), 2014-02-06 22:39:51]

PTAL?

michealbai: yes, for now we're okay with it.

[acolwell GONE FROM CHROMIUM, 2014-02-06 22:53:50]

Overall this change is looking good. I just have a few minor nits.

After looking at the other related CLs though, I'm concerned about the number of
layers that this poster URL has to flow through to get to its final destination.
It makes me wonder why so many layers are needed for Blink to communicate with
some alternate playback controls.

https://codereview.chromium.org/131763011/diff/140001/Source/web/WebMediaPlay...
File Source/web/WebMediaPlayerClientImpl.cpp (right):

https://codereview.chromium.org/131763011/diff/140001/Source/web/WebMediaPlay...
Source/web/WebMediaPlayerClientImpl.cpp:417: if (m_webMediaPlayer &&
!poster.isEmpty()) {
nit: I believe you need to notify m_webMediaPlayer of empty URLs as well since
the web application may clear the poster URL.

https://codereview.chromium.org/131763011/diff/140001/Source/web/WebMediaPlay...
File Source/web/WebMediaPlayerClientImpl.h (right):

https://codereview.chromium.org/131763011/diff/140001/Source/web/WebMediaPlay...
Source/web/WebMediaPlayerClientImpl.h:51: class KURL;
nit: You shouldn't need this since 7 lines up there is an include for KURL.

[cimamoglu (inactive), 2014-02-07 14:36:18]

+jochen for OWNERS review

Aaron, thanks for the comments. PTAL?

https://codereview.chromium.org/131763011/diff/140001/Source/web/WebMediaPlay...
File Source/web/WebMediaPlayerClientImpl.cpp (right):

https://codereview.chromium.org/131763011/diff/140001/Source/web/WebMediaPlay...
Source/web/WebMediaPlayerClientImpl.cpp:417: if (m_webMediaPlayer &&
!poster.isEmpty()) {
On 2014/02/06 22:53:50, acolwell wrote:
> nit: I believe you need to notify m_webMediaPlayer of empty URLs as well since
> the web application may clear the poster URL.

Done.

https://codereview.chromium.org/131763011/diff/140001/Source/web/WebMediaPlay...
File Source/web/WebMediaPlayerClientImpl.h (right):

https://codereview.chromium.org/131763011/diff/140001/Source/web/WebMediaPlay...
Source/web/WebMediaPlayerClientImpl.h:51: class KURL;
On 2014/02/06 22:53:50, acolwell wrote:
> nit: You shouldn't need this since 7 lines up there is an include for KURL.

Done.

[jochen (gone - plz use gerrit), 2014-02-07 14:41:36]

i'm happy to rubberstamp once acolwell@ is happy

https://codereview.chromium.org/131763011/diff/200001/Source/web/WebMediaPlay...
File Source/web/WebMediaPlayerClientImpl.cpp (right):

https://codereview.chromium.org/131763011/diff/200001/Source/web/WebMediaPlay...
Source/web/WebMediaPlayerClientImpl.cpp:417: if (m_webMediaPlayer &&
!poster.isEmpty())
why is it not possible to clear the poster url?

[cimamoglu (inactive), 2014-02-07 14:52:09]

Thanks!

https://codereview.chromium.org/131763011/diff/200001/Source/web/WebMediaPlay...
File Source/web/WebMediaPlayerClientImpl.cpp (right):

https://codereview.chromium.org/131763011/diff/200001/Source/web/WebMediaPlay...
Source/web/WebMediaPlayerClientImpl.cpp:417: if (m_webMediaPlayer &&
!poster.isEmpty())
On 2014/02/07 14:41:36, jochen wrote:
> why is it not possible to clear the poster url?

Good point, missed that! Done.

[acolwell GONE FROM CHROMIUM, 2014-02-11 02:02:49]

lgtm % comment. 

In the future we should figure out a way to make these controls you are working
on directly visible to Blink in some way so that you don't have to go through
the WebMediaPlayer interface. The ability for these controls to query the
HTMLMediaElement itself for the information they need would be a much better
path.

https://codereview.chromium.org/131763011/diff/260001/Source/web/WebMediaPlay...
File Source/web/WebMediaPlayerClientImpl.cpp (right):

https://codereview.chromium.org/131763011/diff/260001/Source/web/WebMediaPlay...
Source/web/WebMediaPlayerClientImpl.cpp:232: WebURL poster =
static_cast<HTMLVideoElement*>(m_client)->posterImageURL();
You can't blindly cast like this. m_client may be an HTMLAudioElement. For now,
I think the least hacky thing you can do is to add a mediaPlayerPosterURL()
method to MediaPlayerClient. HTMLMediaElement should have an implementation that
returns an empty URL and HTMLVideoElement should overload this method with
something that returns the actual poster URL. I'm not a huge fan of this, but
it's the best I can think of at the moment.

[jochen (gone - plz use gerrit), 2014-02-11 10:12:52]

lgtm

[whywhat, 2014-02-12 22:07:02]

The CQ bit was checked by avayvod@chromium.org

[commit-bot: I haz the power, 2014-02-12 22:07:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cimamoglu@chromium.org/131763011/380001

[cimamoglu (inactive), 2014-02-12 22:11:06]

Addressed Aaron's final comment, tried locally and submitted. Thanks a lot to
all the reviewers!

[commit-bot: I haz the power, 2014-02-13 00:27:17]

Change committed as 167054