Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2301823002:
Reordering media controls. (Closed)
|
Created:
4 years, 3 months ago by kdsilva Modified:
4 years, 3 months ago 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@media-controls Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionReordering media controls to match the new spec.
BUG=601247
Committed: https://crrev.com/66090fb3f2d49b3a4bcadb4d6aa09d7d9eafa7b9
Cr-Commit-Position: refs/heads/master@{#419196}
Patch Set 1 #Patch Set 2 : Updated tests #Patch Set 3 : rebase #Patch Set 4 : rebased, again #Patch Set 5 : Updating tests. #Patch Set 6 : rebased #Patch Set 7 #Patch Set 8 : updating tests #Patch Set 9 : updated tests #Patch Set 10 : rebased #Patch Set 11 #Patch Set 12 : updating code #Patch Set 13 : fixing tests #Patch Set 14 : fixing android tests #
Total comments: 8
Patch Set 15 : addressed comments #Depends on Patchset: Messages
Total messages: 72 (61 generated)
The CQ bit was checked by kdsilva@google.com 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_rel_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 kdsilva@google.com 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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by kdsilva@google.com 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_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by kdsilva@google.com 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_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by kdsilva@google.com 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_rel_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 kdsilva@google.com 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 checked by kdsilva@google.com 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_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by kdsilva@google.com 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: 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 kdsilva@google.com 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_rel_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 kdsilva@google.com 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 ========== Reordering media controls. BUG= ========== to ========== Reordering media controls to match the new spec. BUG=601247 ==========
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_...)
The CQ bit was checked by kdsilva@google.com 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 checked by kdsilva@google.com 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: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by kdsilva@google.com 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...
kdsilva@google.com changed reviewers: + avayvod@chromium.org
PTAL :)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by kdsilva@google.com 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...
https://codereview.chromium.org/2301823002/diff/250001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/media/remote/CastTestBase.java (right): https://codereview.chromium.org/2301823002/diff/250001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/media/remote/CastTestBase.java:553: Rect fullscreenButton = fullscreenButton(videoRect); hm, isn't this a unending recursion now? you call fsButton from dwnButton and vice versa. https://codereview.chromium.org/2301823002/diff/250001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/media/video-controls-track-selection-menu.html (right): https://codereview.chromium.org/2301823002/diff/250001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/media/video-controls-track-selection-menu.html:8: <video controls style="width: 500px"> nit: comment. https://codereview.chromium.org/2301823002/diff/250001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/shadow/MediaControls.cpp (right): https://codereview.chromium.org/2301823002/diff/250001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:260: // The order in which we append elements to the overflow list does matter. nit: perhaps explain _how_ it matters. That the last appended item will go into the overflow menu first when the element narrows (and the last two appended items go together). https://codereview.chromium.org/2301823002/diff/250001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:719: m_toggleClosedCaptionsButton.get(), nit: forgot to delete this one?
The CQ bit was checked by kdsilva@google.com to run a CQ dry run
https://codereview.chromium.org/2301823002/diff/250001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/media/remote/CastTestBase.java (right): https://codereview.chromium.org/2301823002/diff/250001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/media/remote/CastTestBase.java:553: Rect fullscreenButton = fullscreenButton(videoRect); On 2016/09/16 14:18:55, whywhat wrote: > hm, isn't this a unending recursion now? you call fsButton from dwnButton and > vice versa. Whoops. Fixed. https://codereview.chromium.org/2301823002/diff/250001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/media/video-controls-track-selection-menu.html (right): https://codereview.chromium.org/2301823002/diff/250001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/media/video-controls-track-selection-menu.html:8: <video controls style="width: 500px"> On 2016/09/16 14:18:55, whywhat wrote: > nit: comment. Done. https://codereview.chromium.org/2301823002/diff/250001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/shadow/MediaControls.cpp (right): https://codereview.chromium.org/2301823002/diff/250001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:260: // The order in which we append elements to the overflow list does matter. On 2016/09/16 14:18:55, whywhat wrote: > nit: perhaps explain _how_ it matters. That the last appended item will go into > the overflow menu first when the element narrows (and the last two appended > items go together). Done. https://codereview.chromium.org/2301823002/diff/250001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:719: m_toggleClosedCaptionsButton.get(), On 2016/09/16 14:18:55, whywhat wrote: > nit: forgot to delete this one? Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
avayvod@chromium.org changed reviewers: + foolip@chromium.org
lgtm Philip, your stamp is needed for MediaControls.cpp if you don't mind :)
kdsilva@google.com changed reviewers: + liberato@chromium.org
@liberato, perhaps you could rs MediaControls.cpp?
rs lgtm
On 2016/09/16 16:01:49, kdsilva wrote: > @liberato, perhaps you could rs MediaControls.cpp? yay for philip :)
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 kdsilva@google.com
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 ========== Reordering media controls to match the new spec. BUG=601247 ========== to ========== Reordering media controls to match the new spec. BUG=601247 ==========
Message was sent while issue was closed.
Committed patchset #15 (id:270001)
Message was sent while issue was closed.
Description was changed from ========== Reordering media controls to match the new spec. BUG=601247 ========== to ========== Reordering media controls to match the new spec. BUG=601247 Committed: https://crrev.com/66090fb3f2d49b3a4bcadb4d6aa09d7d9eafa7b9 Cr-Commit-Position: refs/heads/master@{#419196} ==========
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/66090fb3f2d49b3a4bcadb4d6aa09d7d9eafa7b9 Cr-Commit-Position: refs/heads/master@{#419196} |
