Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2219673004:
Adjust MediaControls for effective zoom when dropping controls. (Closed)
|
Created:
4 years, 4 months ago by liberato (no reviews please) Modified:
4 years, 4 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@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdjust MediaControls for effective zoom when dropping controls.
When computing how many controls fit in the media control panel, the
effective zoom was ignored. This caused the control width to appear
to change as they zoomed, even though the available space in the bar
also changed proportionally. The effect was that zooming would
change which controls were visible in the panel.
This CL adjusts for the ComputedStyle's effective zoom.
BUG=630466
TEST=media-controls-fit-properly-while-zoomed.html
Committed: https://crrev.com/5d215e472cf4e2e831aa80b9eed4eb82beffdece
Cr-Commit-Position: refs/heads/master@{#410644}
Patch Set 1 #
Total comments: 2
Patch Set 2 : reorganized conditionals #
Messages
Total messages: 24 (16 generated)
The CQ bit was checked by liberato@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by liberato@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Adjust MediaControls for effective zoom when dropping controls. When computing how many controls fit in the media control panel, the effective zoom was ignored. This caused the control width to appear to change as they zoomed, even though the available space in the bar also changed proportionally. The effect was that zooming would change which controls were visible in the panel. This CL adjusts for the ComputedStyle's effective zoom. BUG=630466 TEST=media-controls-fit-properly-while-zoomed.html ========== to ========== Adjust MediaControls for effective zoom when dropping controls. When computing how many controls fit in the media control panel, the effective zoom was ignored. This caused the control width to appear to change as they zoomed, even though the available space in the bar also changed proportionally. The effect was that zooming would change which controls were visible in the panel. This CL adjusts for the ComputedStyle's effective zoom. BUG=630466 TEST=media-controls-fit-properly-while-zoomed.html ==========
liberato@chromium.org changed reviewers: + mlamouri@chromium.org
thanks -fl
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
lgtm It seems that you will have to rebase media/video-zoom-controls.html https://codereview.chromium.org/2219673004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/shadow/MediaControls.cpp (right): https://codereview.chromium.org/2219673004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:711: if (const ComputedStyle* style = m_playButton->layoutObject()->style()) nit: it's not super obvious that `style` is actually being set here. I don't know if this is against the coding style so feel free to ignore the comment but my first impression was "why are these two conditions not bundled?". I clearly didn't notice the assignment.
thanks! -fl https://codereview.chromium.org/2219673004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/shadow/MediaControls.cpp (right): https://codereview.chromium.org/2219673004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:711: if (const ComputedStyle* style = m_playButton->layoutObject()->style()) On 2016/08/08 09:53:16, Mounir Lamouri wrote: > nit: it's not super obvious that `style` is actually being set here. I don't > know if this is against the coding style so feel free to ignore the comment but > my first impression was "why are these two conditions not bundled?". I clearly > didn't notice the assignment. good point. i don't think that it's against the style guide, but i also agree that it isn't clear inside a nested check like this. i bundled them back into one conditional and moved the assignment inside.
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/2219673004/#ps20001 (title: "reorganized conditionals")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by mlamouri@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Adjust MediaControls for effective zoom when dropping controls. When computing how many controls fit in the media control panel, the effective zoom was ignored. This caused the control width to appear to change as they zoomed, even though the available space in the bar also changed proportionally. The effect was that zooming would change which controls were visible in the panel. This CL adjusts for the ComputedStyle's effective zoom. BUG=630466 TEST=media-controls-fit-properly-while-zoomed.html ========== to ========== Adjust MediaControls for effective zoom when dropping controls. When computing how many controls fit in the media control panel, the effective zoom was ignored. This caused the control width to appear to change as they zoomed, even though the available space in the bar also changed proportionally. The effect was that zooming would change which controls were visible in the panel. This CL adjusts for the ComputedStyle's effective zoom. BUG=630466 TEST=media-controls-fit-properly-while-zoomed.html ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Adjust MediaControls for effective zoom when dropping controls. When computing how many controls fit in the media control panel, the effective zoom was ignored. This caused the control width to appear to change as they zoomed, even though the available space in the bar also changed proportionally. The effect was that zooming would change which controls were visible in the panel. This CL adjusts for the ComputedStyle's effective zoom. BUG=630466 TEST=media-controls-fit-properly-while-zoomed.html ========== to ========== Adjust MediaControls for effective zoom when dropping controls. When computing how many controls fit in the media control panel, the effective zoom was ignored. This caused the control width to appear to change as they zoomed, even though the available space in the bar also changed proportionally. The effect was that zooming would change which controls were visible in the panel. This CL adjusts for the ComputedStyle's effective zoom. BUG=630466 TEST=media-controls-fit-properly-while-zoomed.html Committed: https://crrev.com/5d215e472cf4e2e831aa80b9eed4eb82beffdece Cr-Commit-Position: refs/heads/master@{#410644} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/5d215e472cf4e2e831aa80b9eed4eb82beffdece Cr-Commit-Position: refs/heads/master@{#410644} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
