Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2222163005:
Updating media controls to give fullscreen button highest priority (Closed)
|
Created:
4 years, 4 months ago by kdsilva Modified:
4 years, 4 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@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUpdating media controls to give fullscreen button highest priority
BUG=635581
Committed: https://crrev.com/16042098554782522c2847df7d449f3db6a1dcce
Cr-Commit-Position: refs/heads/master@{#412513}
Patch Set 1 #Patch Set 2 : Added tests #Patch Set 3 : #
Total comments: 2
Patch Set 4 : Addressed comment #Patch Set 5 : #Patch Set 6 : rebase #
Total comments: 2
Patch Set 7 : Addressed comment #
Messages
Total messages: 35 (25 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: This issue passed the CQ dry run.
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: + mlamouri@chromium.org
https://codereview.chromium.org/2222163005/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/media/fullscreen-controls-visible-last.html (right): https://codereview.chromium.org/2222163005/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/fullscreen-controls-visible-last.html:7: <script src="video-controls-fullscreen.js"></script> Could you move hasFullscreenButton() to media-controls.js. video-controls-fullscreen.js is meant to be used in a few specific tests. All of which include media-controls.js.
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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
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_...)
https://codereview.chromium.org/2222163005/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/media/fullscreen-controls-visible-last.html (right): https://codereview.chromium.org/2222163005/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/fullscreen-controls-visible-last.html:7: <script src="video-controls-fullscreen.js"></script> On 2016/08/10 15:34:24, Mounir Lamouri wrote: > Could you move hasFullscreenButton() to media-controls.js. > video-controls-fullscreen.js is meant to be used in a few specific tests. All of > which include media-controls.js. Done.
mlamouri@chromium.org changed reviewers: + avayvod@chromium.org
Anton, PTAL :)
lgtm https://codereview.chromium.org/2222163005/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/media/fullscreen-controls-visible-last.html (right): https://codereview.chromium.org/2222163005/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/media/fullscreen-controls-visible-last.html:15: assert_true(hasFullscreenButton(video)); nit: keep indentation consistent (here 4 spaces)
rs lgtm
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: This issue passed the CQ dry run.
https://codereview.chromium.org/2222163005/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/media/fullscreen-controls-visible-last.html (right): https://codereview.chromium.org/2222163005/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/media/fullscreen-controls-visible-last.html:15: assert_true(hasFullscreenButton(video)); On 2016/08/16 22:14:05, whywhat wrote: > nit: keep indentation consistent (here 4 spaces) Done.
The CQ bit was checked by kdsilva@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from avayvod@chromium.org, mlamouri@chromium.org Link to the patchset: https://codereview.chromium.org/2222163005/#ps120001 (title: "Addressed comment")
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 #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Updating media controls to give fullscreen button highest priority BUG=635581 ========== to ========== Updating media controls to give fullscreen button highest priority BUG=635581 Committed: https://crrev.com/16042098554782522c2847df7d449f3db6a1dcce Cr-Commit-Position: refs/heads/master@{#412513} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/16042098554782522c2847df7d449f3db6a1dcce Cr-Commit-Position: refs/heads/master@{#412513} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
