Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2728193003:
Show download button even when preload=none (Closed)
|
Created:
3 years, 9 months ago by steimel Modified:
3 years, 9 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, Srirama Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionShow download button even when preload=none
Currently, the media controls are not reset after the currentSrc of the
video is set. This CL adds a reset call so that the download button is
displayed, along with a corresponding layout test.
BUG=665152
Review-Url: https://codereview.chromium.org/2728193003
Cr-Commit-Position: refs/heads/master@{#455112}
Committed: https://chromium.googlesource.com/chromium/src/+/94aeb8cb883faee716cca33b7601df68a2d8c09c
Patch Set 1 #
Total comments: 4
Patch Set 2 : Hide download button if video is 404. Don't use reset #Patch Set 3 : Fix incorrect download button unit test and rebase #
Total comments: 4
Patch Set 4 : mlamouri feedback #
Messages
Total messages: 29 (21 generated)
steimel@chromium.org changed reviewers: + avayvod@chromium.org, mlamouri@chromium.org
PTAL
The CQ bit was checked by steimel@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: This issue passed the CQ dry run.
Quick comments because it's a quick review over the weekend :) https://codereview.chromium.org/2728193003/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/media/controls/download-button-displays-with-preload-none.html (right): https://codereview.chromium.org/2728193003/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/media/controls/download-button-displays-with-preload-none.html:14: })); What would happen if preload kicks in? Does the download button disappear after we realise the video is a 404? https://codereview.chromium.org/2728193003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/shadow/MediaControls.cpp (right): https://codereview.chromium.org/2728193003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:958: reset(); `reset()` is a nuclear weapon. We don't want to reset the entire controls just to update the download button :)
PTAL https://codereview.chromium.org/2728193003/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/media/controls/download-button-displays-with-preload-none.html (right): https://codereview.chromium.org/2728193003/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/media/controls/download-button-displays-with-preload-none.html:14: })); On 2017/03/04 14:10:01, mlamouri wrote: > What would happen if preload kicks in? Does the download button disappear after > we realise the video is a 404? Fixed to make the button disappear when preload kicks in. However, preload only kicks in if the play button is clicked. If the user only clicks the download button, the download will fail, but there will be no changes to the UI. https://codereview.chromium.org/2728193003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/shadow/MediaControls.cpp (right): https://codereview.chromium.org/2728193003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:958: reset(); On 2017/03/04 14:10:01, mlamouri wrote: > `reset()` is a nuclear weapon. We don't want to reset the entire controls just > to update the download button :) Changed to just directly update the download button's state
The CQ bit was checked by steimel@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_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 steimel@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: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by steimel@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: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
lgtm https://codereview.chromium.org/2728193003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp (right): https://codereview.chromium.org/2728193003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:685: return false; style: you need { }
lgtm https://codereview.chromium.org/2728193003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/shadow/MediaControlsTest.cpp (right): https://codereview.chromium.org/2728193003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/shadow/MediaControlsTest.cpp:405: DownloadActionMetrics::Shown, 1); Strange you don't need to update that. Shouldn't the count be 0?
https://codereview.chromium.org/2728193003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp (right): https://codereview.chromium.org/2728193003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:685: return false; On 2017/03/07 11:23:07, mlamouri wrote: > style: you need { } Done. https://codereview.chromium.org/2728193003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/shadow/MediaControlsTest.cpp (right): https://codereview.chromium.org/2728193003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/shadow/MediaControlsTest.cpp:405: DownloadActionMetrics::Shown, 1); On 2017/03/07 12:50:56, whywhat wrote: > Strange you don't need to update that. Shouldn't the count be 0? Hmm I'm not actually sure what that's measuring exactly, but if I change it to zero then the test fails ¯\_(ツ)_/¯
The CQ bit was checked by steimel@chromium.org
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/2728193003/#ps60001 (title: "mlamouri feedback")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1488900418411560,
"parent_rev": "4f9b45a2186699c5840388c8d13b4c0376064004", "commit_rev":
"94aeb8cb883faee716cca33b7601df68a2d8c09c"}
Message was sent while issue was closed.
Description was changed from ========== Show download button even when preload=none Currently, the media controls are not reset after the currentSrc of the video is set. This CL adds a reset call so that the download button is displayed, along with a corresponding layout test. BUG=665152 ========== to ========== Show download button even when preload=none Currently, the media controls are not reset after the currentSrc of the video is set. This CL adds a reset call so that the download button is displayed, along with a corresponding layout test. BUG=665152 Review-Url: https://codereview.chromium.org/2728193003 Cr-Commit-Position: refs/heads/master@{#455112} Committed: https://chromium.googlesource.com/chromium/src/+/94aeb8cb883faee716cca33b7601... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/94aeb8cb883faee716cca33b7601... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
