Simplify media controls CSS

Currently, mediaControls.css is the base CSS, while
mediaControlsChromium(Android).css are applied as extra style via
RenderTheme::extraMediaControlsStyleSheet().

Merge mediaControlsChromium.css into mediaControls.css and simplify.

With the new base CSS, mediaControlsChromiumAndroid.css can be
simplified a great deal, improving readability and maintainability.

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

[philipj_slow, 2013-06-28 15:12:10]

This was the best I was able to do to preserve each commit, but the explanatory
commit messages are gone. Please see the attachments in the bug for those...

[philipj_slow, 2013-06-28 15:21:56]

On 2013/06/28 15:12:10, philipj wrote:
> This was the best I was able to do to preserve each commit, but the
explanatory
> commit messages are gone. Please see the attachments in the bug for those...

Now I see that the the patch sets are incremental and not at all helpful for the
reviewer. Let me know if there's anything I can do to salvage this. (At Opera we
use http://critic-review.org/ and everything is based around Git commits, so I'm
more than a little confused by this new experience :)

[qinmin, 2013-06-28 15:24:33]

+scherkus

[philipj_slow, 2013-06-28 15:35:22]

... and after discussing this on IRC, it looks like it will be possible to
review in the way I hoped, by using "Delta from patch set" and comparing each
patch set with the previous.

[qinmin, 2013-07-02 03:15:13]

I am ok to replace MediaControls.css with  MediaControlsChromium.css. But wait
for the desktop folks to take a look.

https://codereview.chromium.org/18181010/diff/9001/Source/core/css/mediaContr...
File Source/core/css/mediaControls.css (right):

https://codereview.chromium.org/18181010/diff/9001/Source/core/css/mediaContr...
Source/core/css/mediaControls.css:107: -webkit-appearance: media-mute-button;
this normally appears as the first line in the css, same for all the following
-webkit-appearance

https://codereview.chromium.org/18181010/diff/9001/Source/core/css/mediaContr...
File Source/core/css/mediaControlsChromiumAndroid.css (right):

https://codereview.chromium.org/18181010/diff/9001/Source/core/css/mediaContr...
Source/core/css/mediaControlsChromiumAndroid.css:101: font-style: initial;
there are lots of initial value in the new css, why? can we provide more
specific values?

[philipj_slow, 2013-07-02 08:16:22]

https://codereview.chromium.org/18181010/diff/9001/Source/core/css/mediaContr...
File Source/core/css/mediaControls.css (right):

https://codereview.chromium.org/18181010/diff/9001/Source/core/css/mediaContr...
Source/core/css/mediaControls.css:107: -webkit-appearance: media-mute-button;
On 2013/07/02 03:15:14, qinmin wrote:
> this normally appears as the first line in the css, same for all the following
> -webkit-appearance

Yeah, the editing process I followed resulted in this kind of thing. I'm happy
to prettify the result, both in this file and for Android.

https://codereview.chromium.org/18181010/diff/9001/Source/core/css/mediaContr...
File Source/core/css/mediaControlsChromiumAndroid.css (right):

https://codereview.chromium.org/18181010/diff/9001/Source/core/css/mediaContr...
Source/core/css/mediaControlsChromiumAndroid.css:101: font-style: initial;
On 2013/07/02 03:15:14, qinmin wrote:
> there are lots of initial value in the new css, why? can we provide more
> specific values?

I think that all instances of *: initial can be removed from this file without
ill effect, I've only added them to initially be sure that everything is
precisely the same as before my edits. Will add a cleanup commit.

[philipj_slow, 2013-07-03 13:59:17]

@acolwell, @scherkus, do either of you feel like reviewing this on behalf of
desktop, or who should I ask?

[acolwell GONE FROM CHROMIUM, 2013-07-03 15:03:20]

On 2013/07/03 13:59:17, philipj wrote:
> @acolwell, @scherkus, do either of you feel like reviewing this on behalf of
> desktop, or who should I ask?

Yes. Taking a look at this right now.

[acolwell GONE FROM CHROMIUM, 2013-07-03 16:21:55]

lgtm. Thank you for uploading the patch sequence. It made reviewing the changes
much easier.

https://codereview.chromium.org/18181010/diff/9001/Source/core/css/mediaContr...
File Source/core/css/mediaControlsChromiumAndroid.css (right):

https://codereview.chromium.org/18181010/diff/9001/Source/core/css/mediaContr...
Source/core/css/mediaControlsChromiumAndroid.css:105: -webkit-flex: 1 1;
Isn't this equivalent to 1 1 auto? I think this can be removed.

[philipj_slow, 2013-07-04 06:43:21]

Great, I'll also add some cleanup to address the issues raised.

[philipj_slow, 2013-07-04 08:40:13]

@qinmin, @acolwell, these last three changes should address both of your issues,
please have a look. Note that I left the border-radius: 0, so that it can be
addressed in https://code.google.com/p/chromium/issues/detail?id=251244 instead.

[philipj_slow, 2013-07-04 08:40:25]

https://codereview.chromium.org/18181010/diff/9001/Source/core/css/mediaContr...
File Source/core/css/mediaControlsChromiumAndroid.css (right):

https://codereview.chromium.org/18181010/diff/9001/Source/core/css/mediaContr...
Source/core/css/mediaControlsChromiumAndroid.css:105: -webkit-flex: 1 1;
On 2013/07/03 16:21:55, acolwell wrote:
> Isn't this equivalent to 1 1 auto? I think this can be removed.

It's actually equivlant to 1 1 0, tested both with getComputedStyle and
according to the spec: http://www.w3.org/TR/css3-flexbox/#flex

However, I'm not sure that the difference is intentional, I will try removing it
and looking for differences.

[philipj_slow, 2013-07-09 06:27:39]

In case my last comment confused anyone, the changes have already been done and
are ready for review. BTW, is there any testing (other than screenshots) that
should be done for this change?

[acolwell GONE FROM CHROMIUM, 2013-07-09 15:01:06]

On 2013/07/09 06:27:39, philipj wrote:
> In case my last comment confused anyone, the changes have already been done
and
> are ready for review. BTW, is there any testing (other than screenshots) that
> should be done for this change?

LGTM. As long as all the existing layout tests still pass I think you're good to
go. I think the LayoutTests that are pixel tests should detect any major changes
introduced since most of them have the controls displayed.

[philipj_slow, 2013-07-10 06:58:22]

OK, judging by
http://dev.chromium.org/developers/testing/try-server-usage#TOC-My-patch-is-i...
I should try {linux,mac,win}_layout. I've requested those now, let's see what
happens :)

[philipj_slow, 2013-07-10 08:55:49]

It looks like I failed, the trybot results are just red links to this very page.
If layout tests are run as part of the commit bot, can I just try committing
instead? On that note, I guess @abarth or someone needs to rubber stamp this
now?

[qinmin, 2013-07-10 14:55:28]

On 2013/07/10 08:55:49, philipj wrote:
> It looks like I failed, the trybot results are just red links to this very
page.
> If layout tests are run as part of the commit bot, can I just try committing
> instead? On that note, I guess @abarth or someone needs to rubber stamp this
> now?

you can run layout tests locally. Since all the bots are failing, i suppose you
can easily reproduce the problem on the platform you are developing.

[philipj_slow, 2013-07-11 09:34:40]

On 2013/07/10 14:55:28, qinmin wrote:
> On 2013/07/10 08:55:49, philipj wrote:
> > It looks like I failed, the trybot results are just red links to this very
> page.
> > If layout tests are run as part of the commit bot, can I just try committing
> > instead? On that note, I guess @abarth or someone needs to rubber stamp this
> > now?
> 
> you can run layout tests locally. Since all the bots are failing, i suppose
you
> can easily reproduce the problem on the platform you are developing.

I've run the test locally now with and without my change and the results are
exactly the same, no new failures. I don't think trybot actually did anything,
because it went red immediately.

[philipj_slow, 2013-07-11 09:36:12]

@abarth, can you rubber stamp this? If there's someone else with your powers who
is a better go-to person for media reviews going forward, do tell :)

[abarth-chromium, 2013-07-11 17:17:08]

rs=me LGTM

[commit-bot: I haz the power, 2013-07-12 06:17:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/philipj@opera.com/18181010/38001

[commit-bot: I haz the power, 2013-07-12 06:39:13]

Retried try job too often on linux_layout_rel for step(s) webkit_python_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_layo...

[commit-bot: I haz the power, 2013-07-12 06:48:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/philipj@opera.com/18181010/38001

[commit-bot: I haz the power, 2013-07-12 07:54:09]

Retried try job too often on win_layout_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_layout...

[commit-bot: I haz the power, 2013-07-12 08:08:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/philipj@opera.com/18181010/38001

[commit-bot: I haz the power, 2013-07-12 09:52:06]

Change committed as 154102