Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2417003002:
[MediaControl] Hide overflow menu on window resize (Closed)
|
Created:
4 years, 2 months ago by Zhiqiang Zhang (Slow) Modified:
4 years, 2 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, posciak+watch_chromium.org, nessy, Srirama, vcarbune.chromium Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[MediaControl] Hide overflow menu on window resize
BUG=655646
Committed: https://crrev.com/554c3f7915afd590dfd16fb7d1e7d0ae16defb16
Cr-Commit-Position: refs/heads/master@{#425676}
Patch Set 1 #
Total comments: 7
Patch Set 2 : not polling #
Messages
Total messages: 25 (17 generated)
Description was changed from ========== [MediaControl] Hide overflow menu when resize BUG=655646 ========== to ========== [MediaControl] Hide overflow menu on window resize BUG=655646 ==========
zqzhang@chromium.org changed reviewers: + mlamouri@chromium.org
Thanks for fixing this! :) Code looks good but I have a couple of comments on the test. https://codereview.chromium.org/2417003002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/media/video-controls-overflow-menu-hide-on-resize.html (right): https://codereview.chromium.org/2417003002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/media/video-controls-overflow-menu-hide-on-resize.html:7: background-color: blue; is changing the background-color required? https://codereview.chromium.org/2417003002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/media/video-controls-overflow-menu-hide-on-resize.html:42: // Resizing should make the overflow list invisible nit: s/make the overflow list invisible/hide the overflow list./ https://codereview.chromium.org/2417003002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/media/video-controls-overflow-menu-hide-on-resize.html:47: setTimeout(pollForControlsHidden, 100); I'm a bit worried about the setTimeout. Does the following works: ``` window.onresize = t.step_func_done(setTimeout(_ => { assert_equals(getComputedStyle(overflowList).display, "none"); }, 0)); ```
https://codereview.chromium.org/2417003002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/media/video-controls-overflow-menu-hide-on-resize.html (right): https://codereview.chromium.org/2417003002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/media/video-controls-overflow-menu-hide-on-resize.html:47: setTimeout(pollForControlsHidden, 100); On 2016/10/14 18:35:23, mlamouri wrote: > I'm a bit worried about the setTimeout. Does the following works: > ``` > window.onresize = t.step_func_done(setTimeout(_ => { > assert_equals(getComputedStyle(overflowList).display, "none"); > }, 0)); > ``` I'm not sure whether MediaControlsWindowEventListener or this test script listener receives the signal first. Do you have ideas? Did some search, but not sure the order is definite.
Patchset #3 (id:40001) has been deleted
Patchset #2 (id:20001) has been deleted
The CQ bit was checked by zqzhang@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...
Patchset #2 (id:60001) has been deleted
The CQ bit was checked by zqzhang@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...
Patchset #2 (id:80001) has been deleted
The CQ bit was checked by zqzhang@chromium.org to run a CQ dry run
https://codereview.chromium.org/2417003002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/media/video-controls-overflow-menu-hide-on-resize.html (right): https://codereview.chromium.org/2417003002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/media/video-controls-overflow-menu-hide-on-resize.html:7: background-color: blue; On 2016/10/14 18:35:23, mlamouri wrote: > is changing the background-color required? Done. https://codereview.chromium.org/2417003002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/media/video-controls-overflow-menu-hide-on-resize.html:42: // Resizing should make the overflow list invisible On 2016/10/14 18:35:23, mlamouri wrote: > nit: s/make the overflow list invisible/hide the overflow list./ Done. https://codereview.chromium.org/2417003002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/media/video-controls-overflow-menu-hide-on-resize.html:47: setTimeout(pollForControlsHidden, 100); On 2016/10/14 18:35:23, mlamouri wrote: > I'm a bit worried about the setTimeout. Does the following works: > ``` > window.onresize = t.step_func_done(setTimeout(_ => { > assert_equals(getComputedStyle(overflowList).display, "none"); > }, 0)); > ``` Seems like it works if I just use window.onresize = t.step_func_done(_ => { assert_equals(...); }); Though I'm not sure if it will break someday (as well as other similar overflow menu tests)
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by zqzhang@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 ========== [MediaControl] Hide overflow menu on window resize BUG=655646 ========== to ========== [MediaControl] Hide overflow menu on window resize BUG=655646 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== [MediaControl] Hide overflow menu on window resize BUG=655646 ========== to ========== [MediaControl] Hide overflow menu on window resize BUG=655646 Committed: https://crrev.com/554c3f7915afd590dfd16fb7d1e7d0ae16defb16 Cr-Commit-Position: refs/heads/master@{#425676} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/554c3f7915afd590dfd16fb7d1e7d0ae16defb16 Cr-Commit-Position: refs/heads/master@{#425676} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
