Removed clientHeight() from MediaControlsPainter.

Removed clientHeight() from MediaControlsPainter.

Relayout during paint asserts.  There were a few cases where
the layout was actually dirty during paint, causing the
clientHeight() to relayout and crash.

Instead, we now cache the last known layout size in the media
element, and use it to compute things during paint.  It would be
much nicer if we could use CSS for this, but it doesn't seem to
be possible for the media shadow controls.  They all paint as
background images.

This may also be related to crbug.com/519495 .

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

[liberato (no reviews please), 2015-08-17 22:27:33]

liberato@chromium.org changed reviewers:
+ philipj@opera.com

[liberato (no reviews please), 2015-08-17 22:35:53]

The android media controls UI can occasionally trigger layout during paint,
which crashes.  This substitutes a cached value instead, using the same update
mechanism that MediaControls::notifyNewPanelWidth() does.

The alternative is to use LayoutBox::borderBoxRect.  Since we're already doing
something equally awful anyway, I chose the former.

-fl

[liberato (no reviews please), 2015-08-17 22:35:55]

The android media controls UI can occasionally trigger layout during paint,
which crashes.  This substitutes a cached value instead, using the same update
mechanism that MediaControls::notifyNewPanelWidth() does.

The alternative is to use LayoutBox::borderBoxRect.  Since we're already doing
something equally awful anyway, I chose the former.

-fl

[DaleCurtis, 2015-08-17 22:51:32]

To channel philipj pre-emptively, can we add a layout test for this? :)

[liberato (no reviews please), 2015-08-17 22:55:47]

On 2015/08/17 22:51:32, DaleCurtis wrote:
> To channel philipj pre-emptively, can we add a layout test for this? :)

i'd love to, but i haven't managed to repro it locally yet.  otherwise, i would
have.  :(

-fl

[fs, 2015-08-18 07:39:41]

fs@opera.com changed reviewers:
+ fs@opera.com

[fs, 2015-08-18 07:39:41]

On 2015/08/17 22:35:55, liberato wrote:
> The android media controls UI can occasionally trigger layout during paint,
> which crashes.  This substitutes a cached value instead, using the same update
> mechanism that MediaControls::notifyNewPanelWidth() does.
> 
> The alternative is to use LayoutBox::borderBoxRect.  Since we're already doing
> something equally awful anyway, I chose the former.

I actually think using borderBoxRect would be preferable.

https://codereview.chromium.org/1296333002/diff/20001/Source/core/paint/Media...
File Source/core/paint/MediaControlsPainter.cpp (right):

https://codereview.chromium.org/1296333002/diff/20001/Source/core/paint/Media...
Source/core/paint/MediaControlsPainter.cpp:183: if (mediaElement->layoutObject()
&& mediaElement->layoutObject()->style())
I'm not sure in what cases mediaElement wouldn't have a LayoutObject here seems
unlikely. The style() null-check is unnecessary (you can use styleRef() here
instead).

[liberato (no reviews please), 2015-08-18 16:26:42]

thanks, fs!  i've switched to asking the LayoutBox for the size, and now i
understand a tiny bit more about layout as a side-effect!  yay!

-fl

https://codereview.chromium.org/1296333002/diff/20001/Source/core/paint/Media...
File Source/core/paint/MediaControlsPainter.cpp (right):

https://codereview.chromium.org/1296333002/diff/20001/Source/core/paint/Media...
Source/core/paint/MediaControlsPainter.cpp:183: if (mediaElement->layoutObject()
&& mediaElement->layoutObject()->style())
On 2015/08/18 07:39:41, fs wrote:
> I'm not sure in what cases mediaElement wouldn't have a LayoutObject here
seems
> unlikely. The style() null-check is unnecessary (you can use styleRef() here
> instead).

all of this code has been removed.

https://codereview.chromium.org/1296333002/diff/40001/Source/core/paint/Media...
File Source/core/paint/MediaControlsPainter.cpp (right):

https://codereview.chromium.org/1296333002/diff/40001/Source/core/paint/Media...
Source/core/paint/MediaControlsPainter.cpp:176: const LayoutBox* box =
mediaElement->layoutObject()->enclosingBox();
That was much easier than i expected.  gets rid of zoom, too. 
enclosingBox::frameRect() is used for the paint rects directly.

I could replace the existing current use of clientHeight()-via-callback (see
first CL) in MediaControls with this, couldn't I?

[fs, 2015-08-18 16:41:19]

LGTM w/ additional suggestion.

https://codereview.chromium.org/1296333002/diff/40001/Source/core/paint/Media...
File Source/core/paint/MediaControlsPainter.cpp (right):

https://codereview.chromium.org/1296333002/diff/40001/Source/core/paint/Media...
Source/core/paint/MediaControlsPainter.cpp:176: const LayoutBox* box =
mediaElement->layoutObject()->enclosingBox();
On 2015/08/18 16:26:41, liberato wrote:
> That was much easier than i expected.  gets rid of zoom, too. 
> enclosingBox::frameRect() is used for the paint rects directly.
> 
> I could replace the existing current use of clientHeight()-via-callback (see
> first CL) in MediaControls with this, couldn't I?

You mean the stuff in MediaControls::notifyPanelWidthChanged? Seems likely if
so.

https://codereview.chromium.org/1296333002/diff/40001/Source/core/paint/Media...
Source/core/paint/MediaControlsPainter.cpp:179: int mediaHeight =
ceil(box->size().height().toDouble());
You could consider using pixelSnappedHeight() here instead. (And if you want the
client* metric, then that is available on LayoutBox as well.)

[liberato (no reviews please), 2015-08-18 18:18:01]

The CQ bit was checked by liberato@chromium.org

[liberato (no reviews please), 2015-08-18 18:18:01]

The patchset sent to the CQ was uploaded after l-g-t-m from fs@opera.com
Link to the patchset: https://codereview.chromium.org/1296333002/#ps60001
(title: "switched to pixelSnappedHeight()")

[commit-bot: I haz the power, 2015-08-18 18:18:13]

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

[commit-bot: I haz the power, 2015-08-18 19:09:50]

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

[commit-bot: I haz the power, 2015-08-18 19:09:51]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[liberato (no reviews please), 2015-08-18 23:52:37]

The CQ bit was checked by liberato@chromium.org

[commit-bot: I haz the power, 2015-08-18 23:52:44]

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

[commit-bot: I haz the power, 2015-08-19 00:53:52]

Committed patchset #4 (id:60001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=200770