Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(4)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 658353002: Remove background color from overlay cast button (Closed)

Created:
6 years, 2 months ago by aberent
Modified:
6 years, 2 months ago
CC:
blink-reviews, nessy, blink-reviews-css, philipj_slow, gasubic, ed+blinkwatch_opera.com, eric.carlson_apple.com, fs, feature-media-reviews_chromium.org, dglazkov+blink, apavlov+blink_chromium.org, darktears, rwlbuis, vcarbune.chromium, rune+blink
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Project:
blink
Visibility:
Public.

Description

Remove background color from overlay cast button The overlay cast button had a semi-transparent CSS background color. Unfortunately the button image is treated in Blink as a background image, so this modified the image, rather than simply providing a background for it. As a result the cast button came out as grey on grey on pale backgrounds. Fix this by making the CSS background color transparent, and modifying the icon image itself to make its background semi-transparent. Note that the icon image is within Chromium, so that change will be a seperate CL. BUG=424180 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=184092

Patch Set 1 #

Total comments: 2

Patch Set 2 : Fix Peter's nit on CSS, and add rendering layout tests. #

Total comments: 8

Patch Set 3 : Fix as suggested by Peter's comments. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+128 lines, -1 line) Patch
M LayoutTests/TestExpectations View 1 2 1 chunk +4 lines, -0 lines 0 comments Download
A LayoutTests/media/video-controls-with-cast-rendering.html View 1 2 1 chunk +41 lines, -0 lines 0 comments Download
A LayoutTests/media/video-overlay-cast-dark-rendering.html View 1 2 1 chunk +41 lines, -0 lines 0 comments Download
A LayoutTests/media/video-overlay-cast-light-rendering.html View 1 2 1 chunk +41 lines, -0 lines 0 comments Download
M Source/core/css/mediaControls.css View 1 2 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 18 (5 generated)
aberent
6 years, 2 months ago (2014-10-17 11:37:41 UTC) #2
Peter Beverloo
There seem to be a bunch of layout tests around with image expectations for verifying ...
6 years, 2 months ago (2014-10-17 11:47:27 UTC) #4
aberent
Also new layout tests added. https://codereview.chromium.org/658353002/diff/1/Source/core/css/mediaControls.css File Source/core/css/mediaControls.css (right): https://codereview.chromium.org/658353002/diff/1/Source/core/css/mediaControls.css#newcode165 Source/core/css/mediaControls.css:165: color: inherit; On 2014/10/17 ...
6 years, 2 months ago (2014-10-17 16:21:03 UTC) #5
aberent
On 2014/10/17 16:21:03, aberent wrote: > Also new layout tests added. > > https://codereview.chromium.org/658353002/diff/1/Source/core/css/mediaControls.css > ...
6 years, 2 months ago (2014-10-17 16:22:30 UTC) #6
aberent
Peter has already done one pass on this. Now looking for a quick review and ...
6 years, 2 months ago (2014-10-20 18:01:51 UTC) #8
Peter Beverloo
Thanks for the tests! One comment and a few further nits (which apply to all ...
6 years, 2 months ago (2014-10-20 19:08:48 UTC) #9
aberent
https://codereview.chromium.org/658353002/diff/20001/LayoutTests/media/video-controls-with-cast-rendering.html File LayoutTests/media/video-controls-with-cast-rendering.html (right): https://codereview.chromium.org/658353002/diff/20001/LayoutTests/media/video-controls-with-cast-rendering.html#newcode1 LayoutTests/media/video-controls-with-cast-rendering.html:1: <script src="media-file.js"></script> On 2014/10/20 19:08:48, Peter Beverloo wrote: > ...
6 years, 2 months ago (2014-10-20 20:13:51 UTC) #10
whywhat
On 2014/10/20 20:13:51, aberent wrote: > https://codereview.chromium.org/658353002/diff/20001/LayoutTests/media/video-controls-with-cast-rendering.html > File LayoutTests/media/video-controls-with-cast-rendering.html (right): > > https://codereview.chromium.org/658353002/diff/20001/LayoutTests/media/video-controls-with-cast-rendering.html#newcode1 > ...
6 years, 2 months ago (2014-10-20 21:45:05 UTC) #11
aberent
Mike, can you give Owner lgtm on this. Philipj, who I would normally ask, doesn't ...
6 years, 2 months ago (2014-10-21 12:56:48 UTC) #13
Mike West
LGTM if Peter's happy with the tests.
6 years, 2 months ago (2014-10-21 13:01:17 UTC) #14
Peter Beverloo
On 2014/10/21 13:01:17, Mike West wrote: > LGTM if Peter's happy with the tests. lgtm!
6 years, 2 months ago (2014-10-21 13:13:12 UTC) #15
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/658353002/40001
6 years, 2 months ago (2014-10-21 13:31:18 UTC) #17
commit-bot: I haz the power
6 years, 2 months ago (2014-10-21 15:07:41 UTC) #18
Message was sent while issue was closed. 
Committed patchset #3 (id:40001) as 184092

Powered by Google App Engine
This is Rietveld 408576698