Update poster code as per https://html.spec.whatwg.org/#show-poster-flag

Update poster code as per https://html.spec.whatwg.org/#show-poster-flag

Update poster related code as per spec and clean up displayMode()
and setDisplayMode() functions.

BUG=342621

[Srirama, 2015-08-17 17:04:15]

srirama.m@samsung.com changed reviewers:
+ philipj@opera.com

[Srirama, 2015-08-17 17:04:16]

PTAL

[philipj_slow, 2015-08-18 08:24:48]

https://codereview.chromium.org/1297983002/diff/1/Source/core/html/HTMLMediaE...
File Source/core/html/HTMLMediaElement.h (right):

https://codereview.chromium.org/1297983002/diff/1/Source/core/html/HTMLMediaE...
Source/core/html/HTMLMediaElement.h:284: virtual DisplayMode displayMode() const
{ return Unknown; }
Do these need to be virtual and on HTMLMediaElement at all? If there are very
few call sites, maybe they can just cast to HTMLVideoElement instead?

[Srirama, 2015-08-18 08:58:26]

https://codereview.chromium.org/1297983002/diff/1/Source/core/html/HTMLMediaE...
File Source/core/html/HTMLMediaElement.h (right):

https://codereview.chromium.org/1297983002/diff/1/Source/core/html/HTMLMediaE...
Source/core/html/HTMLMediaElement.h:284: virtual DisplayMode displayMode() const
{ return Unknown; }
On 2015/08/18 08:24:48, philipj wrote:
> Do these need to be virtual and on HTMLMediaElement at all? If there are very
> few call sites, maybe they can just cast to HTMLVideoElement instead?

But our intention is to make sure these functions won't execute incase of Audio
element. So if we move these functions completely to HTMLVideoElement, it will
again be same as the current code right. Or you want me to move them and then
add VideoElement check at the call site in HTMLMediaElement?

[philipj_slow, 2015-08-18 09:10:29]

https://codereview.chromium.org/1297983002/diff/1/Source/core/html/HTMLMediaE...
File Source/core/html/HTMLMediaElement.h (right):

https://codereview.chromium.org/1297983002/diff/1/Source/core/html/HTMLMediaE...
Source/core/html/HTMLMediaElement.h:284: virtual DisplayMode displayMode() const
{ return Unknown; }
On 2015/08/18 08:58:26, Srirama wrote:
> On 2015/08/18 08:24:48, philipj wrote:
> > Do these need to be virtual and on HTMLMediaElement at all? If there are
very
> > few call sites, maybe they can just cast to HTMLVideoElement instead?
> 
> But our intention is to make sure these functions won't execute incase of
Audio
> element. So if we move these functions completely to HTMLVideoElement, it will
> again be same as the current code right. Or you want me to move them and then
> add VideoElement check at the call site in HTMLMediaElement?

It looks like the displayMode() getter is only used in HTMLVideoElement, so at
least that doesn't need to be virtual.

There are three call sites to setDisplayMode() in HTMLMediaElement, and I think
it almost corresponds to https://html.spec.whatwg.org/#show-poster-flag in the
spec.

So here's an idea: introduce the show poster flag and update it per spec, then
perhaps the setShowPoster() call can be virtual so that HTMLVideoElement does
something extra before or after if needed.

[Srirama, 2015-08-20 05:36:37]

On 2015/08/18 09:10:29, philipj wrote:
>
https://codereview.chromium.org/1297983002/diff/1/Source/core/html/HTMLMediaE...
> File Source/core/html/HTMLMediaElement.h (right):
> 
>
https://codereview.chromium.org/1297983002/diff/1/Source/core/html/HTMLMediaE...
> Source/core/html/HTMLMediaElement.h:284: virtual DisplayMode displayMode()
const
> { return Unknown; }
> On 2015/08/18 08:58:26, Srirama wrote:
> > On 2015/08/18 08:24:48, philipj wrote:
> > > Do these need to be virtual and on HTMLMediaElement at all? If there are
> very
> > > few call sites, maybe they can just cast to HTMLVideoElement instead?
> > 
> > But our intention is to make sure these functions won't execute incase of
> Audio
> > element. So if we move these functions completely to HTMLVideoElement, it
will
> > again be same as the current code right. Or you want me to move them and
then
> > add VideoElement check at the call site in HTMLMediaElement?
> 
> It looks like the displayMode() getter is only used in HTMLVideoElement, so at
> least that doesn't need to be virtual.
> 
> There are three call sites to setDisplayMode() in HTMLMediaElement, and I
think
> it almost corresponds to https://html.spec.whatwg.org/#show-poster-flag in the
> spec.
> 
> So here's an idea: introduce the show poster flag and update it per spec, then
> perhaps the setShowPoster() call can be virtual so that HTMLVideoElement does
> something extra before or after if needed.

Can you give a bit more clarification, i am a bit confused whether we need to
have both setShowPoster and seDisplayMode or you want to rename setDisplayMode
as setShowPoster after adding show-poster-flag.

[philipj_slow, 2015-08-20 09:09:54]

On 2015/08/20 05:36:37, Srirama wrote:
> On 2015/08/18 09:10:29, philipj wrote:
> >
>
https://codereview.chromium.org/1297983002/diff/1/Source/core/html/HTMLMediaE...
> > File Source/core/html/HTMLMediaElement.h (right):
> > 
> >
>
https://codereview.chromium.org/1297983002/diff/1/Source/core/html/HTMLMediaE...
> > Source/core/html/HTMLMediaElement.h:284: virtual DisplayMode displayMode()
> const
> > { return Unknown; }
> > On 2015/08/18 08:58:26, Srirama wrote:
> > > On 2015/08/18 08:24:48, philipj wrote:
> > > > Do these need to be virtual and on HTMLMediaElement at all? If there are
> > very
> > > > few call sites, maybe they can just cast to HTMLVideoElement instead?
> > > 
> > > But our intention is to make sure these functions won't execute incase of
> > Audio
> > > element. So if we move these functions completely to HTMLVideoElement, it
> will
> > > again be same as the current code right. Or you want me to move them and
> then
> > > add VideoElement check at the call site in HTMLMediaElement?
> > 
> > It looks like the displayMode() getter is only used in HTMLVideoElement, so
at
> > least that doesn't need to be virtual.
> > 
> > There are three call sites to setDisplayMode() in HTMLMediaElement, and I
> think
> > it almost corresponds to https://html.spec.whatwg.org/#show-poster-flag in
the
> > spec.
> > 
> > So here's an idea: introduce the show poster flag and update it per spec,
then
> > perhaps the setShowPoster() call can be virtual so that HTMLVideoElement
does
> > something extra before or after if needed.
> 
> Can you give a bit more clarification, i am a bit confused whether we need to
> have both setShowPoster and seDisplayMode or you want to rename setDisplayMode
> as setShowPoster after adding show-poster-flag.

Well I hope that the spec's show poster flag is all that is needed, but I don't
know that without trying to make the change, it's possible that the spec is
slightly wrong (in which case we will fix it) or that the display state does
something more than the spec tries to do.

[Srirama, 2015-08-20 10:00:09]

On 2015/08/20 09:09:54, philipj wrote:
> On 2015/08/20 05:36:37, Srirama wrote:
> > On 2015/08/18 09:10:29, philipj wrote:
> > >
> >
>
https://codereview.chromium.org/1297983002/diff/1/Source/core/html/HTMLMediaE...
> > > File Source/core/html/HTMLMediaElement.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1297983002/diff/1/Source/core/html/HTMLMediaE...
> > > Source/core/html/HTMLMediaElement.h:284: virtual DisplayMode displayMode()
> > const
> > > { return Unknown; }
> > > On 2015/08/18 08:58:26, Srirama wrote:
> > > > On 2015/08/18 08:24:48, philipj wrote:
> > > > > Do these need to be virtual and on HTMLMediaElement at all? If there
are
> > > very
> > > > > few call sites, maybe they can just cast to HTMLVideoElement instead?
> > > > 
> > > > But our intention is to make sure these functions won't execute incase
of
> > > Audio
> > > > element. So if we move these functions completely to HTMLVideoElement,
it
> > will
> > > > again be same as the current code right. Or you want me to move them and
> > then
> > > > add VideoElement check at the call site in HTMLMediaElement?
> > > 
> > > It looks like the displayMode() getter is only used in HTMLVideoElement,
so
> at
> > > least that doesn't need to be virtual.
> > > 
> > > There are three call sites to setDisplayMode() in HTMLMediaElement, and I
> > think
> > > it almost corresponds to https://html.spec.whatwg.org/#show-poster-flag in
> the
> > > spec.
> > > 
> > > So here's an idea: introduce the show poster flag and update it per spec,
> then
> > > perhaps the setShowPoster() call can be virtual so that HTMLVideoElement
> does
> > > something extra before or after if needed.
> > 
> > Can you give a bit more clarification, i am a bit confused whether we need
to
> > have both setShowPoster and seDisplayMode or you want to rename
setDisplayMode
> > as setShowPoster after adding show-poster-flag.
> 
> Well I hope that the spec's show poster flag is all that is needed, but I
don't
> know that without trying to make the change, it's possible that the spec is
> slightly wrong (in which case we will fix it) or that the display state does
> something more than the spec tries to do.

Thanks for the clarification, i will start implementing as per the spec.

[Srirama, 2015-09-06 12:17:16]

On 2015/08/20 10:00:09, Srirama wrote:
> On 2015/08/20 09:09:54, philipj wrote:
> > On 2015/08/20 05:36:37, Srirama wrote:
> > > On 2015/08/18 09:10:29, philipj wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/1297983002/diff/1/Source/core/html/HTMLMediaE...
> > > > File Source/core/html/HTMLMediaElement.h (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/1297983002/diff/1/Source/core/html/HTMLMediaE...
> > > > Source/core/html/HTMLMediaElement.h:284: virtual DisplayMode
displayMode()
> > > const
> > > > { return Unknown; }
> > > > On 2015/08/18 08:58:26, Srirama wrote:
> > > > > On 2015/08/18 08:24:48, philipj wrote:
> > > > > > Do these need to be virtual and on HTMLMediaElement at all? If there
> are
> > > > very
> > > > > > few call sites, maybe they can just cast to HTMLVideoElement
instead?
> > > > > 
> > > > > But our intention is to make sure these functions won't execute incase
> of
> > > > Audio
> > > > > element. So if we move these functions completely to HTMLVideoElement,
> it
> > > will
> > > > > again be same as the current code right. Or you want me to move them
and
> > > then
> > > > > add VideoElement check at the call site in HTMLMediaElement?
> > > > 
> > > > It looks like the displayMode() getter is only used in HTMLVideoElement,
> so
> > at
> > > > least that doesn't need to be virtual.
> > > > 
> > > > There are three call sites to setDisplayMode() in HTMLMediaElement, and
I
> > > think
> > > > it almost corresponds to https://html.spec.whatwg.org/#show-poster-flag
in
> > the
> > > > spec.
> > > > 
> > > > So here's an idea: introduce the show poster flag and update it per
spec,
> > then
> > > > perhaps the setShowPoster() call can be virtual so that HTMLVideoElement
> > does
> > > > something extra before or after if needed.
> > > 
> > > Can you give a bit more clarification, i am a bit confused whether we need
> to
> > > have both setShowPoster and seDisplayMode or you want to rename
> setDisplayMode
> > > as setShowPoster after adding show-poster-flag.
> > 
> > Well I hope that the spec's show poster flag is all that is needed, but I
> don't
> > know that without trying to make the change, it's possible that the spec is
> > slightly wrong (in which case we will fix it) or that the display state does
> > something more than the spec tries to do.
> 
> Thanks for the clarification, i will start implementing as per the spec.

Sorry for the late response. I was a bit busy with my internal work.
I have gone through the spec and tried to implement by introducing show poster
flag.
But one reason i feel we need m_displayMode (Unknown, Poster, Video) rather than
just a boolean flag is that we are avoiding repaints (call to
updateFromElement()) in setDisplayMode() by tracking this mode.
Do you think we can control this with the boolean flag and get rid of the
display mode?
If you just want to see the results first(atleast layout tests) i can give a try
and remove the displaymode in favor of show poster flag.

[philipj_slow, 2015-09-08 08:22:04]

On 2015/09/06 12:17:16, Srirama wrote:
> Sorry for the late response. I was a bit busy with my internal work.
> I have gone through the spec and tried to implement by introducing show poster
> flag.
> But one reason i feel we need m_displayMode (Unknown, Poster, Video) rather
than
> just a boolean flag is that we are avoiding repaints (call to
> updateFromElement()) in setDisplayMode() by tracking this mode.
> Do you think we can control this with the boolean flag and get rid of the
> display mode?
> If you just want to see the results first(atleast layout tests) i can give a
try
> and remove the displaymode in favor of show poster flag.

Can you describe the case in which the current code avoids repaints?

The spec has a list to determine what to show, under "A video element represents
what is given for the first matching condition in the list below:" in
https://html.spec.whatwg.org/multipage/embedded-content.html#the-video-element

Per this list, the video element can represent either its poster frame (with
intrinsic size of poster), some video frame (with intrinsic size of video), or
transparent black (no intrinsic size). This should correspond to the three
current displayMode states, but if the spec is broken in some way we can have it
fixed.

[Srirama, 2015-09-08 12:58:01]

On 2015/09/08 08:22:04, philipj wrote:
> On 2015/09/06 12:17:16, Srirama wrote:
> > Sorry for the late response. I was a bit busy with my internal work.
> > I have gone through the spec and tried to implement by introducing show
poster
> > flag.
> > But one reason i feel we need m_displayMode (Unknown, Poster, Video) rather
> than
> > just a boolean flag is that we are avoiding repaints (call to
> > updateFromElement()) in setDisplayMode() by tracking this mode.
> > Do you think we can control this with the boolean flag and get rid of the
> > display mode?
> > If you just want to see the results first(atleast layout tests) i can give a
> try
> > and remove the displaymode in favor of show poster flag.
> 
> Can you describe the case in which the current code avoids repaints?
> 
> The spec has a list to determine what to show, under "A video element
represents
> what is given for the first matching condition in the list below:" in
> https://html.spec.whatwg.org/multipage/embedded-content.html#the-video-element
> 
> Per this list, the video element can represent either its poster frame (with
> intrinsic size of poster), some video frame (with intrinsic size of video), or
> transparent black (no intrinsic size). This should correspond to the three
> current displayMode states, but if the spec is broken in some way we can have
it
> fixed.

I ran some random video test case with logs inside setDisplayMode, it is called
with "Video" as mode (and oldMode is also "Video") more than once in which case
we are avoiding the call to layoutObject()->updateFromElement(), i am not sure
if this internally has some performance impact (in
LayoutVideo::updateFromElement() it sets PaintInvalidationFull).
And the current displayMode already corresponds to the spec and it covers the
"show poster flag" cases as well. So isn't a new flag for "show poster"
redundant?
If the objective is to add "show poster" flag as per spec and achieve the same
functionality like displayMode and remove displayMode infavor of "show poster"
flag by fixing spec issues if any in this process, then i will try that.

[philipj_slow, 2015-09-09 10:05:31]

On 2015/09/08 12:58:01, Srirama wrote:
> On 2015/09/08 08:22:04, philipj wrote:
> > On 2015/09/06 12:17:16, Srirama wrote:
> > > Sorry for the late response. I was a bit busy with my internal work.
> > > I have gone through the spec and tried to implement by introducing show
> poster
> > > flag.
> > > But one reason i feel we need m_displayMode (Unknown, Poster, Video)
rather
> > than
> > > just a boolean flag is that we are avoiding repaints (call to
> > > updateFromElement()) in setDisplayMode() by tracking this mode.
> > > Do you think we can control this with the boolean flag and get rid of the
> > > display mode?
> > > If you just want to see the results first(atleast layout tests) i can give
a
> > try
> > > and remove the displaymode in favor of show poster flag.
> > 
> > Can you describe the case in which the current code avoids repaints?
> > 
> > The spec has a list to determine what to show, under "A video element
> represents
> > what is given for the first matching condition in the list below:" in
> >
https://html.spec.whatwg.org/multipage/embedded-content.html#the-video-element
> > 
> > Per this list, the video element can represent either its poster frame (with
> > intrinsic size of poster), some video frame (with intrinsic size of video),
or
> > transparent black (no intrinsic size). This should correspond to the three
> > current displayMode states, but if the spec is broken in some way we can
have
> it
> > fixed.
> 
> I ran some random video test case with logs inside setDisplayMode, it is
called
> with "Video" as mode (and oldMode is also "Video") more than once in which
case
> we are avoiding the call to layoutObject()->updateFromElement(), i am not sure
> if this internally has some performance impact (in
> LayoutVideo::updateFromElement() it sets PaintInvalidationFull).
> And the current displayMode already corresponds to the spec and it covers the
> "show poster flag" cases as well. So isn't a new flag for "show poster"
> redundant?
> If the objective is to add "show poster" flag as per spec and achieve the same
> functionality like displayMode and remove displayMode infavor of "show poster"
> flag by fixing spec issues if any in this process, then i will try that.

Right, my suggestion is to remove the displayMode and just have the show poster
flag. If you find something that is lost by doing that, perhaps a spec change is
needed, or perhaps an additional bit that's used only to fix a perf problem is
warranted.

[Srirama, 2015-09-09 10:13:30]

On 2015/09/09 10:05:31, philipj wrote:
> On 2015/09/08 12:58:01, Srirama wrote:
> > On 2015/09/08 08:22:04, philipj wrote:
> > > On 2015/09/06 12:17:16, Srirama wrote:
> > > > Sorry for the late response. I was a bit busy with my internal work.
> > > > I have gone through the spec and tried to implement by introducing show
> > poster
> > > > flag.
> > > > But one reason i feel we need m_displayMode (Unknown, Poster, Video)
> rather
> > > than
> > > > just a boolean flag is that we are avoiding repaints (call to
> > > > updateFromElement()) in setDisplayMode() by tracking this mode.
> > > > Do you think we can control this with the boolean flag and get rid of
the
> > > > display mode?
> > > > If you just want to see the results first(atleast layout tests) i can
give
> a
> > > try
> > > > and remove the displaymode in favor of show poster flag.
> > > 
> > > Can you describe the case in which the current code avoids repaints?
> > > 
> > > The spec has a list to determine what to show, under "A video element
> > represents
> > > what is given for the first matching condition in the list below:" in
> > >
> https://html.spec.whatwg.org/multipage/embedded-content.html#the-video-element
> > > 
> > > Per this list, the video element can represent either its poster frame
(with
> > > intrinsic size of poster), some video frame (with intrinsic size of
video),
> or
> > > transparent black (no intrinsic size). This should correspond to the three
> > > current displayMode states, but if the spec is broken in some way we can
> have
> > it
> > > fixed.
> > 
> > I ran some random video test case with logs inside setDisplayMode, it is
> called
> > with "Video" as mode (and oldMode is also "Video") more than once in which
> case
> > we are avoiding the call to layoutObject()->updateFromElement(), i am not
sure
> > if this internally has some performance impact (in
> > LayoutVideo::updateFromElement() it sets PaintInvalidationFull).
> > And the current displayMode already corresponds to the spec and it covers
the
> > "show poster flag" cases as well. So isn't a new flag for "show poster"
> > redundant?
> > If the objective is to add "show poster" flag as per spec and achieve the
same
> > functionality like displayMode and remove displayMode infavor of "show
poster"
> > flag by fixing spec issues if any in this process, then i will try that.
> 
> Right, my suggestion is to remove the displayMode and just have the show
poster
> flag. If you find something that is lost by doing that, perhaps a spec change
is
> needed, or perhaps an additional bit that's used only to fix a perf problem is
> warranted.

Ok, i will try that, thanks.

[Srirama, 2015-09-16 15:12:13]

Please have a quick look if it is fine.
probably need to update the commit message and comments referring to
m_displayMode in few places in the code.
Layout tests are fine locally but infact there isn't much coverage for poster
handling in layout tests.

[Srirama, 2015-10-01 09:31:43]

Patchset #4 (id:60001) has been deleted

[philipj_slow, 2015-11-12 20:32:19]

On 2015/09/16 15:12:13, Srirama wrote:
> Please have a quick look if it is fine.
> probably need to update the commit message and comments referring to
> m_displayMode in few places in the code.
> Layout tests are fine locally but infact there isn't much coverage for poster
> handling in layout tests.

Om my, I've totally forgotten this review! I see that all the bots are purple or
red, is this still ready to review? Looks like you have to move it to the
Chromium tree, if you do that (new CL) and make the bots green I will take a
look!

[Srirama, 2015-11-13 05:24:05]

On 2015/11/12 20:32:19, philipj wrote:
> On 2015/09/16 15:12:13, Srirama wrote:
> > Please have a quick look if it is fine.
> > probably need to update the commit message and comments referring to
> > m_displayMode in few places in the code.
> > Layout tests are fine locally but infact there isn't much coverage for
poster
> > handling in layout tests.
> 
> Om my, I've totally forgotten this review! I see that all the bots are purple
or
> red, is this still ready to review? Looks like you have to move it to the
> Chromium tree, if you do that (new CL) and make the bots green I will take a
> look!

I have already moved it to https://codereview.chromium.org/1377353002/ and i
need to get back to it :)