Paint media element's play button using padding.

Paint media element's play button using padding.

Rather than specify only a box size in CSS, we now specify the box
size and padding to draw the play and pause buttons.  This also
requires the image to be reduced to 32 image pixels square.

In order to handle desktop zoom, the images are now specified with
FloatRect rather than IntRect.

BUG=547955
Committed: https://crrev.com/8879c396bb77660b821a0c0d1dedb1ee2061088b
Cr-Commit-Position: refs/heads/master@{#389780}

[liberato (no reviews please), 2015-10-26 21:56:13]

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

[liberato (no reviews please), 2015-10-26 21:56:13]

fyi -- not really ready for review.

pretty small patch to use padding in the media controls painter.  my concern is
that we're given an intrect, but it would be better to carry the FloatRect along
a bit longer.

doesn't seem easy to get that into the painter, though.  i'm fairly sure that
it's not supposed to care about padding at this point; that should already be
folded in.  i'm still looking at how to do this more properly.

since i wanted to see if the pixel tests still pass anyway for desktop, i
figured i'd send it along in case you have concerns.

thanks
-fl

[philipj_slow, 2015-10-27 10:25:39]

Is this entirely experimental? You're increasing the padding, and I had assumed
that on desktop you would reduce the padding, and also decrease the height of
the controls. What is the intended final outcome on desktop?

[liberato (no reviews please), 2015-10-27 16:54:43]

On 2015/10/27 10:25:39, philipj wrote:
> Is this entirely experimental? You're increasing the padding, and I had
assumed
> that on desktop you would reduce the padding, and also decrease the height of
> the controls. What is the intended final outcome on desktop?

the goal: on desktop, the goal is to have 32px rather than mobile's 48px boxes. 
both have 16px images, so 8px padding for desktop or 16px padding for desktop.

the buttons are sized as border-box, and the button images are painted as a
background.  so, padding was effectively ignored, unless it exceeded the
specified size of the box.  the 12px padding doesn't do much, as long as its <
48/2 .

experimental: no, not really.  i'm still not happy with the mechanism i've used
to get the paint routines to use padding, but the css is correct.  the only
other approach i've found is to modify the input element type to be "image", and
setting the image resource manually.  i'm experimenting with that today.  seems
like something similar happens with 'broken image' images already.

thanks
-fl

[philipj_slow, 2015-10-28 12:33:49]

On 2015/10/27 16:54:43, liberato wrote:
> On 2015/10/27 10:25:39, philipj wrote:
> > Is this entirely experimental? You're increasing the padding, and I had
> assumed
> > that on desktop you would reduce the padding, and also decrease the height
of
> > the controls. What is the intended final outcome on desktop?
> 
> the goal: on desktop, the goal is to have 32px rather than mobile's 48px
boxes. 
> both have 16px images, so 8px padding for desktop or 16px padding for desktop.
> 
> the buttons are sized as border-box, and the button images are painted as a
> background.  so, padding was effectively ignored, unless it exceeded the
> specified size of the box.  the 12px padding doesn't do much, as long as its <
> 48/2 .
> 
> experimental: no, not really.  i'm still not happy with the mechanism i've
used
> to get the paint routines to use padding, but the css is correct.  the only
> other approach i've found is to modify the input element type to be "image",
and
> setting the image resource manually.  i'm experimenting with that today. 
seems
> like something similar happens with 'broken image' images already.
> 
> thanks
> -fl

Given that the goal is to have 32px buttons on desktop and we have
border-sizing:border-box, isn't the only way to do this by changing width and
height? It seem like this could actually be easier by dropping
border-sizing:border-box and setting width/height to 16px?

[philipj_slow, 2015-10-28 12:38:07]

philipj@opera.com changed reviewers:
+ chrishtr@chromium.org, eae@chromium.org

[philipj_slow, 2015-10-28 12:38:07]

https://codereview.chromium.org/1424733003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/MediaControlsPainter.cpp (right):

https://codereview.chromium.org/1424733003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/MediaControlsPainter.cpp:92: static
FloatRect adjustRectForPadding(IntRect rect, const LayoutObject* object)
It's surprising that it would be necessary to write this manually. If you ask
someone on the layout (eae@) or paint teams (chrishtr@) perhaps they can suggest
a better way.

[liberato (no reviews please), 2015-10-28 14:54:44]

On 2015/10/28 12:33:49, philipj wrote:
> On 2015/10/27 16:54:43, liberato wrote:
> > On 2015/10/27 10:25:39, philipj wrote:
> > > Is this entirely experimental? You're increasing the padding, and I had
> > assumed
> > > that on desktop you would reduce the padding, and also decrease the height
> of
> > > the controls. What is the intended final outcome on desktop?
> > 
> > the goal: on desktop, the goal is to have 32px rather than mobile's 48px
> boxes. 
> > both have 16px images, so 8px padding for desktop or 16px padding for
desktop.
> > 
> > the buttons are sized as border-box, and the button images are painted as a
> > background.  so, padding was effectively ignored, unless it exceeded the
> > specified size of the box.  the 12px padding doesn't do much, as long as its
<
> > 48/2 .
> > 
> > experimental: no, not really.  i'm still not happy with the mechanism i've
> used
> > to get the paint routines to use padding, but the css is correct.  the only
> > other approach i've found is to modify the input element type to be "image",
> and
> > setting the image resource manually.  i'm experimenting with that today. 
> seems
> > like something similar happens with 'broken image' images already.
> > 
> > thanks
> > -fl
> 
> Given that the goal is to have 32px buttons on desktop and we have
> border-sizing:border-box, isn't the only way to do this by changing width and
> height? It seem like this could actually be easier by dropping
> border-sizing:border-box and setting width/height to 16px?

easier in that we could just override the padding rather than padding and size
in android?  true, good point.

[philipj_slow, 2015-10-28 15:14:52]

Also for my understanding, is the intention of this CL to just prepare for later
divergence between desktop and android? Since there are no test changes, I
assume so?

[liberato (no reviews please), 2015-10-28 15:27:58]

On 2015/10/28 15:14:52, philipj wrote:
> Also for my understanding, is the intention of this CL to just prepare for
later
> divergence between desktop and android? Since there are no test changes, I
> assume so?

indeed.

[philipj_slow, 2015-10-28 15:31:46]

OK, so I guess you're going to fiddle a bit, ping me again when I should take
another look :)

[liberato (no reviews please), 2016-03-16 20:25:24]

eae - per our offline discussion, this CL causes MediaControlsPainter to adjust
for padding.

do you know of a better way to make this adjustment than manually subtracting
the ComputedStyle padding values from the computed rectangle in
MediaControlsPainter.cpp?  it seems like BoxPainter does some non-trivial work
to compute the correct rectangle, so i chose to adjust it rather than asking
Layout / LayoutBox.

is there a better way that Layout / LayoutBox provides? 
LayoutBox::paddingBoxRect seems close, but there's still a missing offset that
the box painter adds.

any advice is appreciated.

thanks
-fl

[liberato (no reviews please), 2016-03-17 21:45:49]

eae: since philipj is ooo for an unknown amount of time, would you mind
reviewing this? the only meaningful code change was the painting with padding
change, for which philipj already suggested you as a reviewer anyway.

this change should not change any use-facing behavior, but it lays almost all of
the groundwork for the desktop version of the material design UI.

it adds padding or trims whitespace around each of the images, as needed, to
128x128, and switches to 8px padding in CSS to accomodate.  It also moves the
android-specific stuff into the existing android css rather than update it in
the desktop CSS, where it was before.

i've compared this CL before / after manually with screen shots, and i haven't
found any differences so far.  i'm continuing to do this, but am fairly
confident that i haven't missed anything major at this point.

thanks
-fl

[philipj_slow, 2016-03-21 07:44:28]

The CQ bit was checked by philipj@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-03-21 07:44:44]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1424733003/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1424733003/100001

[philipj_slow, 2016-03-21 07:45:19]

Sorry for no date, I was away only on Friday. I've started a dry run, and if
everything is green I'll take a closer look at the code.

[commit-bot: I haz the power, 2016-03-21 09:19:35]

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

[commit-bot: I haz the power, 2016-03-21 09:19:36]

Dry run: This issue passed the CQ dry run.

[philipj_slow, 2016-03-21 11:48:36]

On 2015/10/28 14:54:44, liberato wrote:
> On 2015/10/28 12:33:49, philipj wrote:
> > On 2015/10/27 16:54:43, liberato wrote:
> > > On 2015/10/27 10:25:39, philipj wrote:
> > > > Is this entirely experimental? You're increasing the padding, and I had
> > > assumed
> > > > that on desktop you would reduce the padding, and also decrease the
height
> > of
> > > > the controls. What is the intended final outcome on desktop?
> > > 
> > > the goal: on desktop, the goal is to have 32px rather than mobile's 48px
> > boxes. 
> > > both have 16px images, so 8px padding for desktop or 16px padding for
> desktop.
> > > 
> > > the buttons are sized as border-box, and the button images are painted as
a
> > > background.  so, padding was effectively ignored, unless it exceeded the
> > > specified size of the box.  the 12px padding doesn't do much, as long as
its
> <
> > > 48/2 .
> > > 
> > > experimental: no, not really.  i'm still not happy with the mechanism i've
> > used
> > > to get the paint routines to use padding, but the css is correct.  the
only
> > > other approach i've found is to modify the input element type to be
"image",
> > and
> > > setting the image resource manually.  i'm experimenting with that today. 
> > seems
> > > like something similar happens with 'broken image' images already.
> > > 
> > > thanks
> > > -fl
> > 
> > Given that the goal is to have 32px buttons on desktop and we have
> > border-sizing:border-box, isn't the only way to do this by changing width
and
> > height? It seem like this could actually be easier by dropping
> > border-sizing:border-box and setting width/height to 16px?
> 
> easier in that we could just override the padding rather than padding and size
> in android?  true, good point.

Did you look into this any further? A smaller the difference in stylesheet
between Android and Desktop sure would be nice.

[philipj_slow, 2016-03-21 11:49:30]

OK, so the bots are happy, I'm sure this works. Curious about feedback from eae@
as you requested.

https://codereview.chromium.org/1424733003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/mediaControlsAndroidNew.css (right):

https://codereview.chromium.org/1424733003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/mediaControlsAndroidNew.css:47: line-height:
48px;
Hmm, why is line-height needed everywhere?

[liberato (no reviews please), 2016-03-21 14:33:33]

also, i rather liked your ooo message.  :)

-fl

https://codereview.chromium.org/1424733003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/mediaControlsAndroidNew.css (right):

https://codereview.chromium.org/1424733003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/mediaControlsAndroidNew.css:47: line-height:
48px;
On 2016/03/21 11:49:30, philipj_UTC7 wrote:
> Hmm, why is line-height needed everywhere?

i doubt that it is, but it was present before in mediaControlsNew.css .  keeping
it makes it clearer that there's no unintended functionality change.

i'll try without, but it's unclear to me that one should change it here.

[philipj_slow, 2016-03-21 14:44:41]

https://codereview.chromium.org/1424733003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/mediaControlsAndroidNew.css (right):

https://codereview.chromium.org/1424733003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/mediaControlsAndroidNew.css:47: line-height:
48px;
On 2016/03/21 14:33:33, liberato wrote:
> On 2016/03/21 11:49:30, philipj_UTC7 wrote:
> > Hmm, why is line-height needed everywhere?
> 
> i doubt that it is, but it was present before in mediaControlsNew.css . 
keeping
> it makes it clearer that there's no unintended functionality change.
> 
> i'll try without, but it's unclear to me that one should change it here.

Ah, I see, you've just preserved what was already there. Cleanup of useless bits
are appreciated, but whether it's done before/after this CL or not at all, I'll
not hold this CL hostage :)

[liberato (no reviews please), 2016-03-21 15:06:17]

i took the middle approach of moving the line-height decls up to the panel.  UI
looks unchanged, including text track.

yay for cleanup.  sorry about the rebase.

-fl

[philipj_slow, 2016-03-22 11:14:03]

So that we don't deadlock, I'm now waiting for a reply to comment #19 and any
discussion you want to have with chrishtr/eae. (May comment further anyway.)

[liberato (no reviews please), 2016-03-22 14:25:12]

https://codereview.chromium.org/1424733003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/mediaControlsAndroidNew.css (right):

https://codereview.chromium.org/1424733003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/mediaControlsAndroidNew.css:47: line-height:
48px;
On 2016/03/21 14:44:40, philipj_UTC7 wrote:
> On 2016/03/21 14:33:33, liberato wrote:
> > On 2016/03/21 11:49:30, philipj_UTC7 wrote:
> > > Hmm, why is line-height needed everywhere?
> > 
> > i doubt that it is, but it was present before in mediaControlsNew.css . 
> keeping
> > it makes it clearer that there's no unintended functionality change.
> > 
> > i'll try without, but it's unclear to me that one should change it here.
> 
> Ah, I see, you've just preserved what was already there. Cleanup of useless
bits
> are appreciated, but whether it's done before/after this CL or not at all,
I'll
> not hold this CL hostage :)

consolidated into the panel.  better than where it is, relatively safe change,
and didn't seem to break the panel or captions.

[liberato (no reviews please), 2016-03-26 21:20:25]

eae: PTAL at the implementation in MediaControlsPainter.cpp. per our email
discussion, i've made it respect CSS padding, but i'd appreciate any guidance
about smarter ways to implement it.  does some method already exist to do this
calculation?

i couldn't find one, but it's easy to get lost in coordinate space conversions
:)

thanks
-fl

[liberato (no reviews please), 2016-03-29 22:30:26]

On 2016/03/21 11:48:36, philipj_UTC7 wrote:
> On 2015/10/28 14:54:44, liberato wrote:
> > On 2015/10/28 12:33:49, philipj wrote:
> > > On 2015/10/27 16:54:43, liberato wrote:
> > > > On 2015/10/27 10:25:39, philipj wrote:
> > > > > Is this entirely experimental? You're increasing the padding, and I
had
> > > > assumed
> > > > > that on desktop you would reduce the padding, and also decrease the
> height
> > > of
> > > > > the controls. What is the intended final outcome on desktop?
> > > > 
> > > > the goal: on desktop, the goal is to have 32px rather than mobile's 48px
> > > boxes. 
> > > > both have 16px images, so 8px padding for desktop or 16px padding for
> > desktop.
> > > > 
> > > > the buttons are sized as border-box, and the button images are painted
as
> a
> > > > background.  so, padding was effectively ignored, unless it exceeded the
> > > > specified size of the box.  the 12px padding doesn't do much, as long as
> its
> > <
> > > > 48/2 .
> > > > 
> > > > experimental: no, not really.  i'm still not happy with the mechanism
i've
> > > used
> > > > to get the paint routines to use padding, but the css is correct.  the
> only
> > > > other approach i've found is to modify the input element type to be
> "image",
> > > and
> > > > setting the image resource manually.  i'm experimenting with that today.

> > > seems
> > > > like something similar happens with 'broken image' images already.
> > > > 
> > > > thanks
> > > > -fl
> > > 
> > > Given that the goal is to have 32px buttons on desktop and we have
> > > border-sizing:border-box, isn't the only way to do this by changing width
> and
> > > height? It seem like this could actually be easier by dropping
> > > border-sizing:border-box and setting width/height to 16px?
> > 
> > easier in that we could just override the padding rather than padding and
size
> > in android?  true, good point.
> 
> Did you look into this any further? A smaller the difference in stylesheet
> between Android and Desktop sure would be nice.

i entirely forgot about this, sorry.  the android css now adds 8px padding and
re-uses the desktop 32px size with content-box.  much nicer.

is this what you had in mind?

also, spoke with eae@ offline.  he suggested adding a TODO to look into
simplifing the MediaControlsPainter logic for now.

thanks
-fl

[eae, 2016-03-29 22:36:38]

LGTM

[philipj_slow, 2016-04-01 08:47:31]

lgtm

https://codereview.chromium.org/1424733003/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/mediaControlsNew.css (right):

https://codereview.chromium.org/1424733003/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/mediaControlsNew.css:149: border-width: 0px;
A pet peeve of mine: 0px==0. There are a bunch of existing cases in this file,
so if you want to fix them all that'd be nice.

[philipj_slow, 2016-04-01 08:48:01]

On 2016/03/29 22:30:26, liberato wrote:
> On 2016/03/21 11:48:36, philipj_UTC7 wrote:
> > On 2015/10/28 14:54:44, liberato wrote:
> > > On 2015/10/28 12:33:49, philipj wrote:
> > > > On 2015/10/27 16:54:43, liberato wrote:
> > > > > On 2015/10/27 10:25:39, philipj wrote:
> > > > > > Is this entirely experimental? You're increasing the padding, and I
> had
> > > > > assumed
> > > > > > that on desktop you would reduce the padding, and also decrease the
> > height
> > > > of
> > > > > > the controls. What is the intended final outcome on desktop?
> > > > > 
> > > > > the goal: on desktop, the goal is to have 32px rather than mobile's
48px
> > > > boxes. 
> > > > > both have 16px images, so 8px padding for desktop or 16px padding for
> > > desktop.
> > > > > 
> > > > > the buttons are sized as border-box, and the button images are painted
> as
> > a
> > > > > background.  so, padding was effectively ignored, unless it exceeded
the
> > > > > specified size of the box.  the 12px padding doesn't do much, as long
as
> > its
> > > <
> > > > > 48/2 .
> > > > > 
> > > > > experimental: no, not really.  i'm still not happy with the mechanism
> i've
> > > > used
> > > > > to get the paint routines to use padding, but the css is correct.  the
> > only
> > > > > other approach i've found is to modify the input element type to be
> > "image",
> > > > and
> > > > > setting the image resource manually.  i'm experimenting with that
today.
> 
> > > > seems
> > > > > like something similar happens with 'broken image' images already.
> > > > > 
> > > > > thanks
> > > > > -fl
> > > > 
> > > > Given that the goal is to have 32px buttons on desktop and we have
> > > > border-sizing:border-box, isn't the only way to do this by changing
width
> > and
> > > > height? It seem like this could actually be easier by dropping
> > > > border-sizing:border-box and setting width/height to 16px?
> > > 
> > > easier in that we could just override the padding rather than padding and
> size
> > > in android?  true, good point.
> > 
> > Did you look into this any further? A smaller the difference in stylesheet
> > between Android and Desktop sure would be nice.
> 
> i entirely forgot about this, sorry.  the android css now adds 8px padding and
> re-uses the desktop 32px size with content-box.  much nicer.
> 
> is this what you had in mind?

Yes, thank you, that's very nice!

[liberato (no reviews please), 2016-04-05 04:27:27]

The CQ bit was checked by liberato@chromium.org

[liberato (no reviews please), 2016-04-05 04:27:27]

The patchset sent to the CQ was uploaded after l-g-t-m from philipj@opera.com,
eae@chromium.org
Link to the patchset: https://codereview.chromium.org/1424733003/#ps200001
(title: "rebased.")

[commit-bot: I haz the power, 2016-04-05 04:27:43]

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

[commit-bot: I haz the power, 2016-04-05 07:03:06]

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

[commit-bot: I haz the power, 2016-04-05 07:03:07]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[liberato (no reviews please), 2016-04-25 16:00:08]

@philipj: this reverts the content-box,padding change back to border-box, to
work around quirks mode.

since you're marked _slow, i'll land tomorrow morning PST absent any feedback. 
it's the same(*) CSS as before the content-box change that you already saw,
anyway.

thanks
-fl

(*) the padding is now on the other side of the width/height lines in
androidNew, but otherwise identical.

[philipj_slow, 2016-04-26 14:48:19]

On 2016/04/25 16:00:08, liberato wrote:
> @philipj: this reverts the content-box,padding change back to border-box, to
> work around quirks mode.
> 
> since you're marked _slow, i'll land tomorrow morning PST absent any feedback.

> it's the same(*) CSS as before the content-box change that you already saw,
> anyway.
> 
> thanks
> -fl
> 
> (*) the padding is now on the other side of the width/height lines in
> androidNew, but otherwise identical.

LGTM. It would be fun to know which quirk this was and if there's a workaround
in CSS, but if you investigate I probably won't be around to learn the answer :)

[liberato (no reviews please), 2016-04-26 14:50:57]

The CQ bit was checked by liberato@chromium.org

[liberato (no reviews please), 2016-04-26 14:50:57]

The patchset sent to the CQ was uploaded after l-g-t-m from eae@chromium.org
Link to the patchset: https://codereview.chromium.org/1424733003/#ps260001
(title: "rebased.")

[commit-bot: I haz the power, 2016-04-26 14:51:13]

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

[commit-bot: I haz the power, 2016-04-26 14:59:18]

Committed patchset #14 (id:260001)

[commit-bot: I haz the power, 2016-04-26 15:01:13]

Description was changed from

==========
Paint media element's play button using padding.

Rather than specify only a box size in CSS, we now specify the box
size and padding to draw the play and pause buttons.  This also
requires the image to be reduced to 32 image pixels square.

In order to handle desktop zoom, the images are now specified with
FloatRect rather than IntRect.

BUG=547955
==========

to

==========
Paint media element's play button using padding.

Rather than specify only a box size in CSS, we now specify the box
size and padding to draw the play and pause buttons.  This also
requires the image to be reduced to 32 image pixels square.

In order to handle desktop zoom, the images are now specified with
FloatRect rather than IntRect.

BUG=547955

Committed: https://crrev.com/8879c396bb77660b821a0c0d1dedb1ee2061088b
Cr-Commit-Position: refs/heads/master@{#389780}
==========

[commit-bot: I haz the power, 2016-04-26 15:01:15]

Patchset 14 (id:??) landed as
https://crrev.com/8879c396bb77660b821a0c0d1dedb1ee2061088b
Cr-Commit-Position: refs/heads/master@{#389780}