Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1948363004:
Use media controls play button's width rather than hard-coded 48px. (Closed)
|
Created:
4 years, 7 months ago by liberato (no reviews please) Modified:
4 years, 7 months ago Reviewers:
mlamouri (slow - plz ping) CC:
blink-reviews, blink-reviews-html_chromium.org, chromium-reviews, dglazkov+blink, eric.carlson_apple.com, feature-media-reviews_chromium.org, fs, gasubic, mlamouri+watch-blink_chromium.org, nessy, vcarbune.chromium Base URL:
https://chromium.googlesource.com/chromium/src.git@crbug585078_playbutton Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUse media controls play button's width rather than hard-coded 48px.
Previously, when computing which controls fit on the media panel, we
would use 48px for the android UI. Desktop uses 32px. Since the
play button is always visible, and thus will have a computed style
once layout happens, we'll now use that.
BUG=609851
Committed: https://crrev.com/b2ca18bc9b7f46aff95987ffbc612cb3c7ae1eb2
Cr-Commit-Position: refs/heads/master@{#393540}
Patch Set 1 #
Total comments: 1
Patch Set 2 : rebased. #Messages
Total messages: 22 (10 generated)
Description was changed from ========== Use media controls play button's width rather than hard-coded 48px. Previously, when computing which controls fit on the media panel, we would use 48px for the android UI. Desktop uses 32px. Since the play button is always visible, and thus will have a computed style once layout happens, we'll now use that. BUG=609851 ========== to ========== Use media controls play button's width rather than hard-coded 48px. Previously, when computing which controls fit on the media panel, we would use 48px for the android UI. Desktop uses 32px. Since the play button is always visible, and thus will have a computed style once layout happens, we'll now use that. BUG=609851 ==========
liberato@chromium.org changed reviewers: + mlamouri@chromium.org
i wasn't able to figure out how to do this without causing a layout sometimes. now that the play button is visible, it's simple :) thanks -fl
https://codereview.chromium.org/1948363004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/shadow/MediaControls.cpp (right): https://codereview.chromium.org/1948363004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:712: : 48; `minimumWidth` is being used by all elements in `elements`. They will have a different size than the play button. Is there a reason why we use the play button size for those?
On 2016/05/09 12:41:39, Mounir Lamouri wrote: > https://codereview.chromium.org/1948363004/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/html/shadow/MediaControls.cpp (right): > > https://codereview.chromium.org/1948363004/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:712: : 48; > `minimumWidth` is being used by all elements in `elements`. They will have a > different size than the play button. Is there a reason why we use the play > button size for those? everything else is supposed to be the same size, excluding the sliders. either way, checking them at runtime is harder since, i think, that the computed style is only available if the element is shown. we can guarantee that only for the play button. not sure if there's a better way to get the size here.
This is really confusing that we assume that everything has the size of the play button given that it's not always the case. Though, the logic was assuming that already so lgtm :)
On 2016/05/10 09:06:45, Mounir Lamouri wrote: > This is really confusing that we assume that everything has the size of the play > button given that it's not always the case. Though, the logic was assuming that > already so lgtm :) agreed, it's weird. i wasn't able to find a way to get the actual sizes without forcing a layout way back when i wrote the original code. i'll cq now but am happy to revisit.
The CQ bit was checked by liberato@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1948363004/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1948363004/1
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-simulator on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by liberato@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mlamouri@chromium.org Link to the patchset: https://codereview.chromium.org/1948363004/#ps20001 (title: "rebased.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1948363004/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1948363004/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by liberato@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1948363004/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1948363004/20001
Message was sent while issue was closed.
Description was changed from ========== Use media controls play button's width rather than hard-coded 48px. Previously, when computing which controls fit on the media panel, we would use 48px for the android UI. Desktop uses 32px. Since the play button is always visible, and thus will have a computed style once layout happens, we'll now use that. BUG=609851 ========== to ========== Use media controls play button's width rather than hard-coded 48px. Previously, when computing which controls fit on the media panel, we would use 48px for the android UI. Desktop uses 32px. Since the play button is always visible, and thus will have a computed style once layout happens, we'll now use that. BUG=609851 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Use media controls play button's width rather than hard-coded 48px. Previously, when computing which controls fit on the media panel, we would use 48px for the android UI. Desktop uses 32px. Since the play button is always visible, and thus will have a computed style once layout happens, we'll now use that. BUG=609851 ========== to ========== Use media controls play button's width rather than hard-coded 48px. Previously, when computing which controls fit on the media panel, we would use 48px for the android UI. Desktop uses 32px. Since the play button is always visible, and thus will have a computed style once layout happens, we'll now use that. BUG=609851 Committed: https://crrev.com/b2ca18bc9b7f46aff95987ffbc612cb3c7ae1eb2 Cr-Commit-Position: refs/heads/master@{#393540} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/b2ca18bc9b7f46aff95987ffbc612cb3c7ae1eb2 Cr-Commit-Position: refs/heads/master@{#393540} |
