Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(44)

Issue 2767823002: Media Remoting: Add interstitial elements to media element shadow dom. (Closed)

Created:
3 years, 9 months ago by xjz
Modified:
3 years, 8 months ago
CC:
aboxhall+watch_chromium.org, aboxhall, darktears, apacible+watch_chromium.org, apavlov+blink_chromium.org, blink-reviews, blink-reviews-api_chromium.org, blink-reviews-css, blink-reviews-html_chromium.org, blink-reviews-paint_chromium.org, chromium-reviews, darin-cc_chromium.org, dglazkov+blink, dmazzoni, dmazzoni+watch_chromium.org, dshwang, dtseng+watch_chromium.org, eric.carlson_apple.com, erickung+watch_chromium.org, feature-media-reviews_chromium.org, fs, gasubic, haraken, jam, je_julie, kinuko+watch, miu+watch_chromium.org, mlamouri+watch-blink_chromium.org, nektarios, nektar+watch_chromium.org, rwlbuis, nessy, Srirama, xjz+watch_chromium.org, yuzo+watch_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Media Remoting: Add interstitial elements to media element shadow dom. This CL add the following elements to media element shadow dom when media remoting starts. 1. Add a disable button to allow user stop remoting and fall back to tab mirroring. This is to solve the accessiblity regression caused by media remoting with lacking support for closed captions. 2. Add a background image, cast icon and a text message. These three are currently rendered as a video frame in media renderer. Move them to media element to keep consistent with the disabel button and solve the font renderering issue on Windows 8/10. The old interstitial (rendered as a video frame) will be removed in a later CL. BUG=700572, 697672 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Review-Url: https://codereview.chromium.org/2767823002 Cr-Commit-Position: refs/heads/master@{#464604} Committed: https://chromium.googlesource.com/chromium/src/+/df9b67eb9751650cebee13333ab125bb3f11df75

Patch Set 1 #

Total comments: 8

Patch Set 2 : Addressed miu's comments. #

Patch Set 3 : Modified disable button. Added cast icon and message. #

Total comments: 2

Patch Set 4 : Don't hide media control during media remoting. #

Total comments: 4

Patch Set 5 : Move MediaRemotingInterstitial to HTMLVideoElement. #

Patch Set 6 : Rebase only #

Patch Set 7 : Adjust CSS. Show background image only when poster url is not empty. #

Patch Set 8 : Remove cast text message element. Not assuming remoting interstitial is media control. #

Total comments: 24

Patch Set 9 : Addressed mlamouri's comments. #

Total comments: 15

Patch Set 10 : Addressed comments. Rebased. Style fix. #

Patch Set 11 : Updated with new UX design. #

Total comments: 14

Patch Set 12 : Addressed liberato's comments. Changed to use HTMLDivElement instead of HTMLInputElement. #

Total comments: 34

Patch Set 13 : Addressed comments. #

Patch Set 14 : Rebased. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+502 lines, -21 lines) Patch
M content/app/strings/content_strings.grd View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +7 lines, -1 line 0 comments Download
M content/child/blink_platform_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 2 chunks +6 lines, -0 lines 0 comments Download
M media/base/media_observer.h View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +4 lines, -3 lines 0 comments Download
M media/blink/webmediaplayer_impl.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +1 line, -1 line 0 comments Download
M media/blink/webmediaplayer_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +8 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/core/css/CSSPrimitiveValueMappings.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +3 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/css/CSSValueKeywords.json5 View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +1 line, -0 lines 0 comments Download
M third_party/WebKit/Source/core/css/mediaControls.css View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +81 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/dom/Node.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +1 line, -0 lines 0 comments Download
M third_party/WebKit/Source/core/html/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +4 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/html/HTMLMediaElement.h View 1 2 3 4 5 6 7 8 9 1 chunk +3 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/html/HTMLMediaElement.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 2 chunks +28 lines, -8 lines 0 comments Download
M third_party/WebKit/Source/core/html/HTMLVideoElement.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 4 chunks +14 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/html/HTMLVideoElement.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 5 chunks +35 lines, -1 line 0 comments Download
A third_party/WebKit/Source/core/html/shadow/MediaRemotingElements.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +49 lines, -0 lines 0 comments Download
A third_party/WebKit/Source/core/html/shadow/MediaRemotingElements.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +103 lines, -0 lines 0 comments Download
A third_party/WebKit/Source/core/html/shadow/MediaRemotingInterstitial.h View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +56 lines, -0 lines 0 comments Download
A third_party/WebKit/Source/core/html/shadow/MediaRemotingInterstitial.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +58 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/layout/LayoutMedia.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 2 chunks +11 lines, -4 lines 0 comments Download
M third_party/WebKit/Source/core/paint/MediaControlsPainter.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +3 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/paint/MediaControlsPainter.cpp View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +12 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/paint/ThemePainter.cpp View 1 2 3 4 5 6 7 8 9 1 chunk +2 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/modules/media_controls/MediaControlsImpl.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +3 lines, -1 line 0 comments Download
M third_party/WebKit/Source/platform/ThemeTypes.h View 1 2 3 4 5 6 7 8 9 1 chunk +1 line, -0 lines 0 comments Download
M third_party/WebKit/public/blink_image_resources.grd View 1 2 1 chunk +2 lines, -0 lines 0 comments Download
A third_party/WebKit/public/default_100_percent/blink/mediaremoting_cast.png View 1 2 3 4 5 6 7 8 9 10 Binary file 0 comments Download
A third_party/WebKit/public/default_200_percent/blink/mediaremoting_cast.png View 1 2 3 4 5 6 7 8 9 10 Binary file 0 comments Download
M third_party/WebKit/public/platform/WebLocalizedString.h View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +2 lines, -0 lines 0 comments Download
M third_party/WebKit/public/platform/WebMediaPlayerClient.h View 1 2 3 4 5 6 7 8 9 1 chunk +4 lines, -0 lines 0 comments Download

Messages

Total messages: 74 (48 generated)
xjz
PTAL. Thanks!
3 years, 9 months ago (2017-03-22 00:37:34 UTC) #4
miu
lgtm % nits: https://codereview.chromium.org/2767823002/diff/1/third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp File third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp (right): https://codereview.chromium.org/2767823002/diff/1/third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp#newcode1112 third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:1112: MouseEventsListener(MediaRemotingDisableButtonElement& element) nit: explicit https://codereview.chromium.org/2767823002/diff/1/third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp#newcode1133 third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:1133: ...
3 years, 9 months ago (2017-03-22 21:26:49 UTC) #5
xjz
mlamouri@: This CL adds an overlay disable button on media element for media remoting. The ...
3 years, 9 months ago (2017-03-22 22:25:09 UTC) #7
mlamouri (slow - plz ping)
This is a large UI change: did you get any feedback from UX? Also, it ...
3 years, 9 months ago (2017-03-23 16:02:29 UTC) #8
xjz
On 2017/03/23 16:02:29, mlamouri wrote: > This is a large UI change: did you get ...
3 years, 9 months ago (2017-03-23 18:07:56 UTC) #9
miu
On 2017/03/23 18:07:56, xjz wrote: > On 2017/03/23 16:02:29, mlamouri wrote: > > This is ...
3 years, 9 months ago (2017-03-24 21:51:53 UTC) #10
xjz
Modified the disable button as the UX designer suggested. Also added other interstitial elements. miu@: ...
3 years, 8 months ago (2017-04-02 00:07:33 UTC) #14
miu
Comments on PS3: Are we sure we want to use the PNG format for the ...
3 years, 8 months ago (2017-04-03 21:44:41 UTC) #15
sandersd (OOO until July 31)
media/ lgtm.
3 years, 8 months ago (2017-04-03 21:48:05 UTC) #16
xjz
On 2017/04/03 21:44:41, miu wrote: > Comments on PS3: > > Are we sure we ...
3 years, 8 months ago (2017-04-03 23:38:07 UTC) #17
xjz
Addressed miu's comment in PS4. Also, as suggested by the UX designer, now keep showing ...
3 years, 8 months ago (2017-04-03 23:39:52 UTC) #18
kinuko
lgtm % nits (A bit hand-wavy for UI related code but per your earlier comments ...
3 years, 8 months ago (2017-04-04 03:18:05 UTC) #19
mlamouri (slow - plz ping)
Could you move the implementation of the remoting interstitial to HTMLVideoElement? https://codereview.chromium.org/2767823002/diff/60001/third_party/WebKit/Source/core/html/shadow/MediaControlElements.h File third_party/WebKit/Source/core/html/shadow/MediaControlElements.h (right): ...
3 years, 8 months ago (2017-04-04 13:19:44 UTC) #20
xjz
Thanks for reviewing. Addressed kinuko's and mlamouri's comments in PS#5. mlamouri: Moved MediaRemotingInterstitial to HTMLVideoElement. ...
3 years, 8 months ago (2017-04-04 20:56:27 UTC) #22
mlamouri (slow - plz ping)
I had another pass but I'm afraid I still have a few comments. https://codereview.chromium.org/2767823002/diff/160001/third_party/WebKit/Source/core/css/mediaControls.css File ...
3 years, 8 months ago (2017-04-07 13:18:41 UTC) #35
xjz
Addressed mlamouri's comments. PTAL again. Thanks! https://codereview.chromium.org/2767823002/diff/160001/third_party/WebKit/Source/core/css/mediaControls.css File third_party/WebKit/Source/core/css/mediaControls.css (right): https://codereview.chromium.org/2767823002/diff/160001/third_party/WebKit/Source/core/css/mediaControls.css#newcode148 third_party/WebKit/Source/core/css/mediaControls.css:148: video::-internal-media-remoting-interstitial { On ...
3 years, 8 months ago (2017-04-07 23:07:02 UTC) #36
mlamouri (slow - plz ping)
https://codereview.chromium.org/2767823002/diff/160001/third_party/WebKit/Source/core/html/shadow/MediaRemotingElements.cpp File third_party/WebKit/Source/core/html/shadow/MediaRemotingElements.cpp (right): https://codereview.chromium.org/2767823002/diff/160001/third_party/WebKit/Source/core/html/shadow/MediaRemotingElements.cpp#newcode72 third_party/WebKit/Source/core/html/shadow/MediaRemotingElements.cpp:72: document().addEventListener(EventTypeNames::click, m_listener, true); On 2017/04/07 at 23:07:01, xjz wrote: ...
3 years, 8 months ago (2017-04-10 13:44:22 UTC) #41
xjz
Thanks for reviewing. Addressed comments, and rebased. PTAL. https://codereview.chromium.org/2767823002/diff/160001/third_party/WebKit/Source/core/html/shadow/MediaRemotingElements.cpp File third_party/WebKit/Source/core/html/shadow/MediaRemotingElements.cpp (right): https://codereview.chromium.org/2767823002/diff/160001/third_party/WebKit/Source/core/html/shadow/MediaRemotingElements.cpp#newcode72 third_party/WebKit/Source/core/html/shadow/MediaRemotingElements.cpp:72: document().addEventListener(EventTypeNames::click, ...
3 years, 8 months ago (2017-04-11 04:29:45 UTC) #45
miu
liberato: Could you PTAL at the Blink changes? Ideally, we'd like to wrap this up ...
3 years, 8 months ago (2017-04-12 20:45:52 UTC) #54
liberato (no reviews please)
lg*m, minus some nits. however, looking at this i realized that it's been far too ...
3 years, 8 months ago (2017-04-12 21:57:44 UTC) #55
xjz
Thanks liberato@ for reviewing. Addressed your comments. Also, changed to use HTMLDivElement instead of HTMLInputElement ...
3 years, 8 months ago (2017-04-13 00:08:48 UTC) #59
miu
PS12 lgtm % nit: https://codereview.chromium.org/2767823002/diff/300001/third_party/WebKit/Source/core/html/HTMLVideoElement.h File third_party/WebKit/Source/core/html/HTMLVideoElement.h (right): https://codereview.chromium.org/2767823002/diff/300001/third_party/WebKit/Source/core/html/HTMLVideoElement.h#newcode136 third_party/WebKit/Source/core/html/HTMLVideoElement.h:136: MediaRemotingStatus GetMediaRemotingStatus() const { style ...
3 years, 8 months ago (2017-04-13 01:32:10 UTC) #60
mlamouri (slow - plz ping)
lgtm with comments applied. https://codereview.chromium.org/2767823002/diff/300001/third_party/WebKit/Source/core/css/mediaControls.css File third_party/WebKit/Source/core/css/mediaControls.css (right): https://codereview.chromium.org/2767823002/diff/300001/third_party/WebKit/Source/core/css/mediaControls.css#newcode164 third_party/WebKit/Source/core/css/mediaControls.css:164: transition: opacity .2s cubic-bezier (0.4, ...
3 years, 8 months ago (2017-04-13 17:20:52 UTC) #63
xjz
Thanks mlamouri for reviewing. Addressed the comments. https://codereview.chromium.org/2767823002/diff/300001/third_party/WebKit/Source/core/css/mediaControls.css File third_party/WebKit/Source/core/css/mediaControls.css (right): https://codereview.chromium.org/2767823002/diff/300001/third_party/WebKit/Source/core/css/mediaControls.css#newcode164 third_party/WebKit/Source/core/css/mediaControls.css:164: transition: opacity ...
3 years, 8 months ago (2017-04-13 19:12:12 UTC) #66
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2767823002/340001
3 years, 8 months ago (2017-04-13 21:39:32 UTC) #71
commit-bot: I haz the power
3 years, 8 months ago (2017-04-13 23:29:23 UTC) #74
Message was sent while issue was closed.
Committed patchset #14 (id:340001) as
https://chromium.googlesource.com/chromium/src/+/df9b67eb9751650cebee13333ab1...

Powered by Google App Engine
This is Rietveld 408576698