Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2499013003:
Media Controls: don't "cut" controls if fully hidden. (Closed)
|
Created:
4 years, 1 month ago by mlamouri (slow - plz ping) Modified:
4 years, 1 month ago CC:
blink-reviews, blink-reviews-layout_chromium.org, chromium-reviews, eae+blinkwatch, eric.carlson_apple.com, feature-media-reviews_chromium.org, jchaffraix+rendering, leviw+renderwatch, mlamouri+watch-blink_chromium.org, pdr+renderingwatchlist_chromium.org, Srirama, szager+layoutwatch_chromium.org, zoltan1, Zhiqiang Zhang (Slow) Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMedia Controls: don't "cut" controls if fully hidden.
A media element might be outside of the page and can't be accessed and
for some reasons would have the controls enabled. Instead of having a
DCHECK, we should early return and use the default value.
BUG=664097
R=eae@chromium.org
Committed: https://crrev.com/467481c23e2c416c34e5d4a34766d59a5aeefcf2
Cr-Commit-Position: refs/heads/master@{#433219}
Patch Set 1 #Patch Set 2 : add tests #
Total comments: 4
Patch Set 3 : review comments #
Messages
Total messages: 24 (14 generated)
The CQ bit was checked by mlamouri@chromium.org to run a CQ dry run
mlamouri@chromium.org changed reviewers: + ojan@chromium.org - zqzhang@chromium.org
-zqzhang@ (to CC) +ojan@ (I don't own this file, neither does zqzhang@)
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
ojan@chromium.org changed reviewers: + eae@chromium.org
Probably best if someone from the layout team reviews this. +eae, to route to the appropriate reviewer. As to the patch itself, is it possible to write a test for this?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Media Controls: don't "cut" controls if fully hidden. A media element might be outside of the page and can't be accessed and for some reasons would have the controls enabled. Instead of having a DCHECK, we should early return and use the default value. BUG=664097 R=zqzhang@chromium.org ========== to ========== Media Controls: don't "cut" controls if fully hidden. A media element might be outside of the page and can't be accessed and for some reasons would have the controls enabled. Instead of having a DCHECK, we should early return and use the default value. BUG=664097 R=eae@chromium.org ==========
I wasn't able to write a test for this. I usually don't do layout changes so any pointer would be appreciated.
Change itself looks good but this needs a test, please see https://chromium.googlesource.com/chromium/src/+/master/docs/testing/layout_t... for testing instructions.
So sorry about not having a test. PTAL.
The CQ bit was checked by mlamouri@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: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_TIMED_OUT, no build URL) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_TIMED_OUT, no build URL) android_compile_dbg on master.tryserver.chromium.android (JOB_TIMED_OUT, no build URL) android_cronet on master.tryserver.chromium.android (JOB_TIMED_OUT, no build URL) cast_shell_android on master.tryserver.chromium.android (JOB_TIMED_OUT, no build URL)
This is great, thanks for adding the test! I have a couple of suggestions for ways to improve the test but those are *suggestions*, not *requirements*. LGTM https://codereview.chromium.org/2499013003/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/media/controls/overflow-fully-hidden.html (right): https://codereview.chromium.org/2499013003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/controls/overflow-fully-hidden.html:35: var videos = document.querySelectorAll('video'); This doesn't really matter here but please keep in mind that in many cases, including this, querySelectorAll returns a live node list thus this loop is O(n^2). Either cache the length or change the loop to be a for in or a for loop like this to avoid that. for (var videoElement, i = 0; videoElement = videos[i]; i++) { videoElement.src = findMediaFile('video', 'content/test'); } https://codereview.chromium.org/2499013003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/controls/overflow-fully-hidden.html:44: assert_equals(getComputedStyle(controls).width, expectedWidth[i]); Try to avoid data-driven tests like this. They tend to be hard to read and debug. Instead how about something like this: assert_equals(getComputedStyle(mediaControlsButton(videos[0], 'panel')).width, '400px'); assert_equals(getComputedStyle(mediaControlsButton(videos[1], 'panel')).width, '800px'); assert_equals(getComputedStyle(mediaControlsButton(videos[2], 'panel')).width, '1200px'); assert_equals(getComputedStyle(mediaControlsButton(videos[3], 'panel')).width, '600px');
The CQ bit was checked by mlamouri@chromium.org
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/2499013003/#ps40001 (title: "review comments")
https://codereview.chromium.org/2499013003/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/media/controls/overflow-fully-hidden.html (right): https://codereview.chromium.org/2499013003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/controls/overflow-fully-hidden.html:35: var videos = document.querySelectorAll('video'); On 2016/11/17 at 16:40:33, eae wrote: > This doesn't really matter here but please keep in mind that in many cases, including this, querySelectorAll returns a live node list thus this loop is O(n^2). > > Either cache the length or change the loop to be a for in or a for loop like this to avoid that. > > for (var videoElement, i = 0; videoElement = videos[i]; i++) { > videoElement.src = findMediaFile('video', 'content/test'); > } Didn't realise this. Thanks :) https://codereview.chromium.org/2499013003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/controls/overflow-fully-hidden.html:44: assert_equals(getComputedStyle(controls).width, expectedWidth[i]); On 2016/11/17 at 16:40:33, eae wrote: > Try to avoid data-driven tests like this. They tend to be hard to read and debug. Instead how about something like this: > > assert_equals(getComputedStyle(mediaControlsButton(videos[0], 'panel')).width, '400px'); > assert_equals(getComputedStyle(mediaControlsButton(videos[1], 'panel')).width, '800px'); > assert_equals(getComputedStyle(mediaControlsButton(videos[2], 'panel')).width, '1200px'); > assert_equals(getComputedStyle(mediaControlsButton(videos[3], 'panel')).width, '600px'); I actually prefer data-driver tests because it allows for more tests. Though, given the size, I used your suggestion.
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.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Media Controls: don't "cut" controls if fully hidden. A media element might be outside of the page and can't be accessed and for some reasons would have the controls enabled. Instead of having a DCHECK, we should early return and use the default value. BUG=664097 R=eae@chromium.org ========== to ========== Media Controls: don't "cut" controls if fully hidden. A media element might be outside of the page and can't be accessed and for some reasons would have the controls enabled. Instead of having a DCHECK, we should early return and use the default value. BUG=664097 R=eae@chromium.org Committed: https://crrev.com/467481c23e2c416c34e5d4a34766d59a5aeefcf2 Cr-Commit-Position: refs/heads/master@{#433219} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/467481c23e2c416c34e5d4a34766d59a5aeefcf2 Cr-Commit-Position: refs/heads/master@{#433219} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
