Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
456323002:
[WIP] Re-implement MediaControls in Blink-in-JS (Closed)
|
Created:
6 years, 4 months ago by hajimehoshi Modified:
6 years, 2 months ago CC:
arv+blink, blink-reviews, blink-reviews-bindings_chromium.org, blink-reviews-dom_chromium.org, blink-reviews-html_chromium.org, Inactive, dglazkov+blink, eae+blinkwatch, eric.carlson_apple.com, feature-media-reviews_chromium.org, fs, gasubic, rwlbuis, sof, nessy, vcarbune.chromium Base URL:
svn://svn.chromium.org/blink/trunk Project:
blink Visibility:
Public. |
Description[WIP] Re-implement MediaControls in Blink-in-JS
BUG=399821
Patch Set 1 #
Total comments: 15
Patch Set 2 : #Patch Set 3 : #Patch Set 4 : #Patch Set 5 : #Patch Set 6 : #Patch Set 7 : #Patch Set 8 : #Patch Set 9 : #
Total comments: 9
Patch Set 10 : #
Total comments: 3
Messages
Total messages: 29 (0 generated)
https://codereview.chromium.org/456323002/diff/1/Source/core/dom/Element.idl File Source/core/dom/Element.idl (right): https://codereview.chromium.org/456323002/diff/1/Source/core/dom/Element.idl#... Source/core/dom/Element.idl:110: [RaisesException, OnlyExposedToPrivateScript] createUserAgentShadowRoot(); TODO: This change and the changes around HTMLVideoElement won't be needed.
Although this is a work-in-progress, I believe we're going a right way. https://codereview.chromium.org/456323002/diff/1/Source/core/dom/Element.cpp File Source/core/dom/Element.cpp (right): https://codereview.chromium.org/456323002/diff/1/Source/core/dom/Element.cpp#... Source/core/dom/Element.cpp:1700: PassRefPtrWillBeRawPtr<ShadowRoot> Element::createUserAgentShadowRoot(ExceptionState& exceptionState) As discussed offline, we won't need this. https://codereview.chromium.org/456323002/diff/1/Source/core/html/HTMLVideoEl... File Source/core/html/HTMLVideoElement.cpp (right): https://codereview.chromium.org/456323002/diff/1/Source/core/html/HTMLVideoEl... Source/core/html/HTMLVideoElement.cpp:60: //RELEASE_ASSERT(!classObject.IsEmpty()); I guess this won't be needed. https://codereview.chromium.org/456323002/diff/1/Source/core/html/HTMLVideoEl... Source/core/html/HTMLVideoElement.cpp:70: ScriptForbiddenScope::AllowUserAgentScript script; What is this for? https://codereview.chromium.org/456323002/diff/1/Source/core/html/HTMLVideoEl... File Source/core/html/HTMLVideoElement.js (right): https://codereview.chromium.org/456323002/diff/1/Source/core/html/HTMLVideoEl... Source/core/html/HTMLVideoElement.js:8: function reflectAttribute(prototype, attributeName, propertyName) { We want to share this function with HTMLMarqueeElement.js. I'll create PrivateScriptUtilities.js so that you can add this kind of functions to the common js file. https://codereview.chromium.org/456323002/diff/1/Source/core/html/shadow/Medi... File Source/core/html/shadow/MediaControls.js (right): https://codereview.chromium.org/456323002/diff/1/Source/core/html/shadow/Medi... Source/core/html/shadow/MediaControls.js:149: // TODO Nit: Blink uses FIXME instead of TODO.
https://codereview.chromium.org/456323002/diff/1/Source/core/html/HTMLMediaEl... File Source/core/html/HTMLMediaElement.idl (right): https://codereview.chromium.org/456323002/diff/1/Source/core/html/HTMLMediaEl... Source/core/html/HTMLMediaElement.idl:99: [OnlyExposedToPrivateScript] boolean hasAudio(); Why is this needed? We should avoid introducing OnlyExposedToPrivateScript back doors when implementing these features. https://codereview.chromium.org/456323002/diff/1/Source/core/html/shadow/Medi... File Source/core/html/shadow/MediaControls.js (right): https://codereview.chromium.org/456323002/diff/1/Source/core/html/shadow/Medi... Source/core/html/shadow/MediaControls.js:36: this.transitionTimer_ = setTimeout(kFadeOutDuration, this.transitionTimerFired); Why not listen for the transition end event? https://codereview.chromium.org/456323002/diff/1/Source/core/html/shadow/Medi... Source/core/html/shadow/MediaControls.js:49: }; Should we use a web animation instead of a CSS transition?
https://codereview.chromium.org/456323002/diff/1/Source/core/html/HTMLMediaEl... File Source/core/html/HTMLMediaElement.idl (right): https://codereview.chromium.org/456323002/diff/1/Source/core/html/HTMLMediaEl... Source/core/html/HTMLMediaElement.idl:99: [OnlyExposedToPrivateScript] boolean hasAudio(); On 2014/08/11 18:04:12, abarth wrote: > Why is this needed? We should avoid introducing OnlyExposedToPrivateScript back > doors when implementing these features. If things were hooked up just a little bit more internally, it should be possible to use the audioTracks and videoTracks lists instead.
Thank you! https://codereview.chromium.org/456323002/diff/1/Source/core/html/HTMLMediaEl... File Source/core/html/HTMLMediaElement.idl (right): https://codereview.chromium.org/456323002/diff/1/Source/core/html/HTMLMediaEl... Source/core/html/HTMLMediaElement.idl:99: [OnlyExposedToPrivateScript] boolean hasAudio(); On 2014/08/11 19:40:15, philipj wrote: > On 2014/08/11 18:04:12, abarth wrote: > > Why is this needed? We should avoid introducing OnlyExposedToPrivateScript > back > > doors when implementing these features. > > If things were hooked up just a little bit more internally, it should be > possible to use the audioTracks and videoTracks lists instead. Hmm, I wasn't sure how to get the same results of hasAudio/hasVideo by audioTracks/videoTracks. I moved them to MediaControls.idl once. MediaControls.idl is only for internal usage, so this seems much better than defining them here. https://codereview.chromium.org/456323002/diff/1/Source/core/html/HTMLVideoEl... File Source/core/html/HTMLVideoElement.js (right): https://codereview.chromium.org/456323002/diff/1/Source/core/html/HTMLVideoEl... Source/core/html/HTMLVideoElement.js:8: function reflectAttribute(prototype, attributeName, propertyName) { On 2014/08/11 11:09:33, haraken wrote: > > We want to share this function with HTMLMarqueeElement.js. I'll create > PrivateScriptUtilities.js so that you can add this kind of functions to the > common js file. > I've removed HTMLVideoElement.js. PrivateScriptUtilities.js is not needed so far. https://codereview.chromium.org/456323002/diff/1/Source/core/html/shadow/Medi... File Source/core/html/shadow/MediaControls.js (right): https://codereview.chromium.org/456323002/diff/1/Source/core/html/shadow/Medi... Source/core/html/shadow/MediaControls.js:36: this.transitionTimer_ = setTimeout(kFadeOutDuration, this.transitionTimerFired); On 2014/08/11 18:04:12, abarth wrote: > Why not listen for the transition end event? I'm just copying implementation from C++. Now I still can't confirm this transition's behavior because I've not implemented the other methods and event handlers. I'll replace this as you suggested. I added a comment for now. https://codereview.chromium.org/456323002/diff/1/Source/core/html/shadow/Medi... Source/core/html/shadow/MediaControls.js:49: }; On 2014/08/11 18:04:12, abarth wrote: > Should we use a web animation instead of a CSS transition? Same as above.
https://codereview.chromium.org/456323002/diff/1/Source/core/html/HTMLMediaEl... File Source/core/html/HTMLMediaElement.idl (right): https://codereview.chromium.org/456323002/diff/1/Source/core/html/HTMLMediaEl... Source/core/html/HTMLMediaElement.idl:99: [OnlyExposedToPrivateScript] boolean hasAudio(); On 2014/08/12 06:54:09, hajimehoshi wrote: > On 2014/08/11 19:40:15, philipj wrote: > > On 2014/08/11 18:04:12, abarth wrote: > > > Why is this needed? We should avoid introducing OnlyExposedToPrivateScript > > back > > > doors when implementing these features. > > > > If things were hooked up just a little bit more internally, it should be > > possible to use the audioTracks and videoTracks lists instead. > > Hmm, I wasn't sure how to get the same results of hasAudio/hasVideo by > audioTracks/videoTracks. > > I moved them to MediaControls.idl once. MediaControls.idl is only for internal > usage, so this seems much better than defining them here. AudioVideoTracks is still status=experimental, so I guess we can't depend on it. If it were enabled, HTMLMediaElement::createPlaceholderTracksIfNecessary() ought to be enough to create the tracks if they are there, even if they won't contain any other useful info.
I found a problem about image resources. AFAIK, the current parts of a media control, like a play button, are rendered by special logic (RenderMediaControls::renderPlayMediaButton in this case) with some image resources (IDR_MEDIAPLAYER_PLAY_BUTTON in this case). The problems is that it seems to be impossible to access the image resource files from CSS side in Blink-in-JS. Even though I used a style 'background-image: url(...)', the origin of the path would be the origin of the site the user is accessing, which doesn't make sense. Adding special search paths only for Blink-in-JS seems bad in a sense that it would create a backdoor. My idea is creating an API of JavaScript to get image data-URL by image resource names, which wouldn't taint CSS-world. What do you think?
On 2014/08/15 at 06:01:15, hajimehoshi wrote: > My idea is creating an API of JavaScript to get image data-URL by image resource names, which wouldn't taint CSS-world. What do you think? That's worth a try. How did Apple solve this problem in WebKit?
On 2014/08/15 06:18:08, abarth wrote: > On 2014/08/15 at 06:01:15, hajimehoshi wrote: > > My idea is creating an API of JavaScript to get image data-URL by image > resource names, which wouldn't taint CSS-world. What do you think? > > That's worth a try. How did Apple solve this problem in WebKit? Thanks! Maybe WebKit adopts a similar way. Each part is finally rendered by WKDrawMediaUIPart with part id, and this implementation is in WebKitSystemInterface: https://github.com/WebKit/webkit/blob/master/Source/WebCore/rendering/RenderM...
On 2014/08/15 06:54:22, hajimehoshi wrote: > On 2014/08/15 06:18:08, abarth wrote: > > > > That's worth a try. How did Apple solve this problem in WebKit? > > Thanks! > > Maybe WebKit adopts a similar way. Each part is finally rendered by > WKDrawMediaUIPart with part id, and this implementation is in > WebKitSystemInterface: > https://github.com/WebKit/webkit/blob/master/Source/WebCore/rendering/RenderM... That code is obsolete in trunk and will be removed shortly. FWIW, we currently use SVG for our media controls images: http://trac.webkit.org/browser/trunk/Source/WebCore/Modules/mediacontrols/med...
On 2014/08/15 16:26:41, eric.carlson wrote: > That code is obsolete in trunk and will be removed shortly. FWIW, we currently > use SVG for our media controls images: > http://trac.webkit.org/browser/trunk/Source/WebCore/Modules/mediacontrols/med... Thanks! @hajimehoshi: Looks like WebKit is using data URLs as well. Let's keep the resources in the resource bundle and retrieve data URLs via a private API as you suggested.
Now the basic features are almost working (like media/video-controls.html), but there are still many thing to do: * Pass layout tests. * Use WebAnimation. * Consider AXObject. Probably this requires another backdoor. * Remove RenderMediaControls.cpp or other rendering logic for MediaControls. This is the main purpose of this CL. The final change would be bigger anyway. Would it be possible for you check if the direction of this CL is right?
https://codereview.chromium.org/456323002/diff/200001/Source/core/html/shadow... File Source/core/html/shadow/MediaControls.idl (right): https://codereview.chromium.org/456323002/diff/200001/Source/core/html/shadow... Source/core/html/shadow/MediaControls.idl:40: [OnlyExposedToPrivateScript] void updateTextTrackContainerDisplay(HTMLElement textTrackContainer); This is way too many private APIs. Why do you need private APIs? Aren't there sufficient public APIs on the video element?
How close can you come with zero private APIs?
https://codereview.chromium.org/456323002/diff/200001/Source/core/html/shadow... File Source/core/html/shadow/MediaControls.idl (right): https://codereview.chromium.org/456323002/diff/200001/Source/core/html/shadow... Source/core/html/shadow/MediaControls.idl:40: [OnlyExposedToPrivateScript] void updateTextTrackContainerDisplay(HTMLElement textTrackContainer); On 2014/08/23 05:48:33, abarth wrote: > This is way too many private APIs. Why do you need private APIs? Aren't there > sufficient public APIs on the video element? As far as I look at the implementation, it seems hard to avoid adding these private APIs if you keep HTMLMediaElement in C++ and move MediaControlsElement to JavaScript. HTMLMediaElement and MediaControlsElement need to interact with each other, and we need a private API to proxy the interaction. One way to address the issue would be to move (a part of) HTMLMediaElement to JavaScript as well and decrease the interaction that crosses the boundary between C++ and JavaScript. How hard would it be?
https://codereview.chromium.org/456323002/diff/200001/Source/core/html/shadow... File Source/core/html/shadow/MediaControls.idl (right): https://codereview.chromium.org/456323002/diff/200001/Source/core/html/shadow... Source/core/html/shadow/MediaControls.idl:40: [OnlyExposedToPrivateScript] void updateTextTrackContainerDisplay(HTMLElement textTrackContainer); On 2014/08/23 05:48:33, abarth wrote: > This is way too many private APIs. Why do you need private APIs? Aren't there > sufficient public APIs on the video element? As far as I look at the implementation, it seems hard to avoid adding these private APIs if you keep HTMLMediaElement in C++ and move MediaControlsElement to JavaScript. HTMLMediaElement and MediaControlsElement need to interact with each other, and we need a private API to proxy the interaction. One way to address the issue would be to move (a part of) HTMLMediaElement to JavaScript as well and decrease the interaction that crosses the boundary between C++ and JavaScript. How hard would it be?
https://codereview.chromium.org/456323002/diff/200001/Source/core/html/shadow... File Source/core/html/shadow/MediaControls.idl (right): https://codereview.chromium.org/456323002/diff/200001/Source/core/html/shadow... Source/core/html/shadow/MediaControls.idl:40: [OnlyExposedToPrivateScript] void updateTextTrackContainerDisplay(HTMLElement textTrackContainer); On 2014/08/25 03:59:28, haraken wrote: > On 2014/08/23 05:48:33, abarth wrote: > > This is way too many private APIs. Why do you need private APIs? Aren't > there > > sufficient public APIs on the video element? > > As far as I look at the implementation, it seems hard to avoid adding these > private APIs if you keep HTMLMediaElement in C++ and move MediaControlsElement > to JavaScript. HTMLMediaElement and MediaControlsElement need to interact with > each other, and we need a private API to proxy the interaction. > > One way to address the issue would be to move (a part of) HTMLMediaElement to > JavaScript as well and decrease the interaction that crosses the boundary > between C++ and JavaScript. How hard would it be? That would be quite hard. Now MediaElement uses WebMediaPlayer and MediaController. For example, if I tried to implement HTMLMediaElement::hasAudio to Blink-in-JS, another backdoor would be needed since hasAudio uses WebMediaPlayer. Another example is togglePlayState. This accesses MediaController, so how can I re-implement this in Blink-in-JS? I basically agree to haraken-san's idea. In the long term, some (or all?) of implementation of HTMLMediaElement should be moved to Blink-in-JS and the above backdoors should be disappeared. In the short term, can we allow those backdoors temporarily? Of course I'll continue to work on this.
https://codereview.chromium.org/456323002/diff/200001/Source/core/html/shadow... File Source/core/html/shadow/MediaControls.idl (right): https://codereview.chromium.org/456323002/diff/200001/Source/core/html/shadow... Source/core/html/shadow/MediaControls.idl:40: [OnlyExposedToPrivateScript] void updateTextTrackContainerDisplay(HTMLElement textTrackContainer); On 2014/08/25 06:34:01, hajimehoshi wrote: > On 2014/08/25 03:59:28, haraken wrote: > > On 2014/08/23 05:48:33, abarth wrote: > > > This is way too many private APIs. Why do you need private APIs? Aren't > > there > > > sufficient public APIs on the video element? > > > > As far as I look at the implementation, it seems hard to avoid adding these > > private APIs if you keep HTMLMediaElement in C++ and move MediaControlsElement > > to JavaScript. HTMLMediaElement and MediaControlsElement need to interact with > > each other, and we need a private API to proxy the interaction. > > > > One way to address the issue would be to move (a part of) HTMLMediaElement to > > JavaScript as well and decrease the interaction that crosses the boundary > > between C++ and JavaScript. How hard would it be? > > That would be quite hard. Now MediaElement uses WebMediaPlayer and > MediaController. For example, if I tried to implement HTMLMediaElement::hasAudio > to Blink-in-JS, another backdoor would be needed since hasAudio uses > WebMediaPlayer. Another example is togglePlayState. This accesses > MediaController, so how can I re-implement this in Blink-in-JS? > > I basically agree to haraken-san's idea. In the long term, some (or all?) of > implementation of HTMLMediaElement should be moved to Blink-in-JS and the above > backdoors should be disappeared. > > In the short term, can we allow those backdoors temporarily? Of course I'll > continue to work on this. I'm not really happy to introduce ad-hoc backdoors without having a big picture. Shall we spend more time investigating where we should cut the boundary between Blink-in-JS and C++? For example, I think MediaController is something that should be moved to Blink-in-JS with MediaControlsElement. That way you can avoid adding backdoors to MediaController. In my ideal world, we want to move the entire MediaControlsElement, RenderMediaControls, MediaController and a part of HTMLMediaElement associated with MediaControlsElement. That way, the only backdoors we'll need would just be: - Lifecycle callbacks of HTMLMediaElement (i.e., createdCallback, attachedCallback etc). - Interaction between HTMLMediaElement and WebMediaPlayer. - createUserAgentShadowRoot(). Either way, let's spend more time investigating the ideal boundary and clarify what backdoors are needed (i.e., what APIs are missing from the web platform). A part of the goals of this project is to understand missing APIs of the web platform. Thus it's more important to understand it rather than trying to make it workable somehow.
https://codereview.chromium.org/456323002/diff/200001/Source/core/html/shadow... File Source/core/html/shadow/MediaControls.idl (right): https://codereview.chromium.org/456323002/diff/200001/Source/core/html/shadow... Source/core/html/shadow/MediaControls.idl:40: [OnlyExposedToPrivateScript] void updateTextTrackContainerDisplay(HTMLElement textTrackContainer); On 2014/08/25 06:53:49, haraken wrote: > On 2014/08/25 06:34:01, hajimehoshi wrote: > > On 2014/08/25 03:59:28, haraken wrote: > > > On 2014/08/23 05:48:33, abarth wrote: > > > > This is way too many private APIs. Why do you need private APIs? Aren't > > > there > > > > sufficient public APIs on the video element? > > > > > > As far as I look at the implementation, it seems hard to avoid adding these > > > private APIs if you keep HTMLMediaElement in C++ and move > MediaControlsElement > > > to JavaScript. HTMLMediaElement and MediaControlsElement need to interact > with > > > each other, and we need a private API to proxy the interaction. > > > > > > One way to address the issue would be to move (a part of) HTMLMediaElement > to > > > JavaScript as well and decrease the interaction that crosses the boundary > > > between C++ and JavaScript. How hard would it be? > > > > That would be quite hard. Now MediaElement uses WebMediaPlayer and > > MediaController. For example, if I tried to implement > HTMLMediaElement::hasAudio > > to Blink-in-JS, another backdoor would be needed since hasAudio uses > > WebMediaPlayer. Another example is togglePlayState. This accesses > > MediaController, so how can I re-implement this in Blink-in-JS? > > > > I basically agree to haraken-san's idea. In the long term, some (or all?) of > > implementation of HTMLMediaElement should be moved to Blink-in-JS and the > above > > backdoors should be disappeared. > > > > In the short term, can we allow those backdoors temporarily? Of course I'll > > continue to work on this. > > I'm not really happy to introduce ad-hoc backdoors without having a big picture. > Shall we spend more time investigating where we should cut the boundary between > Blink-in-JS and C++? > > For example, I think MediaController is something that should be moved to > Blink-in-JS with MediaControlsElement. That way you can avoid adding backdoors > to MediaController. > > In my ideal world, we want to move the entire MediaControlsElement, > RenderMediaControls, MediaController and a part of HTMLMediaElement associated > with MediaControlsElement. That way, the only backdoors we'll need would just > be: > > - Lifecycle callbacks of HTMLMediaElement (i.e., createdCallback, > attachedCallback etc). > - Interaction between HTMLMediaElement and WebMediaPlayer. > - createUserAgentShadowRoot(). > > Either way, let's spend more time investigating the ideal boundary and clarify > what backdoors are needed (i.e., what APIs are missing from the web platform). A > part of the goals of this project is to understand missing APIs of the web > platform. Thus it's more important to understand it rather than trying to make > it workable somehow. Sure, MediaController seems to be relatively independent from other C++ classes and worth moving to Blink-in-JS, so let me start with MediaController.
https://codereview.chromium.org/456323002/diff/200001/Source/core/html/shadow... File Source/core/html/shadow/MediaControls.idl (right): https://codereview.chromium.org/456323002/diff/200001/Source/core/html/shadow... Source/core/html/shadow/MediaControls.idl:40: [OnlyExposedToPrivateScript] void updateTextTrackContainerDisplay(HTMLElement textTrackContainer); On 2014/08/25 07:11:27, hajimehoshi wrote: > On 2014/08/25 06:53:49, haraken wrote: > > On 2014/08/25 06:34:01, hajimehoshi wrote: > > > On 2014/08/25 03:59:28, haraken wrote: > > > > On 2014/08/23 05:48:33, abarth wrote: > > > > > This is way too many private APIs. Why do you need private APIs? > Aren't > > > > there > > > > > sufficient public APIs on the video element? > > > > > > > > As far as I look at the implementation, it seems hard to avoid adding > these > > > > private APIs if you keep HTMLMediaElement in C++ and move > > MediaControlsElement > > > > to JavaScript. HTMLMediaElement and MediaControlsElement need to interact > > with > > > > each other, and we need a private API to proxy the interaction. > > > > > > > > One way to address the issue would be to move (a part of) HTMLMediaElement > > to > > > > JavaScript as well and decrease the interaction that crosses the boundary > > > > between C++ and JavaScript. How hard would it be? > > > > > > That would be quite hard. Now MediaElement uses WebMediaPlayer and > > > MediaController. For example, if I tried to implement > > HTMLMediaElement::hasAudio > > > to Blink-in-JS, another backdoor would be needed since hasAudio uses > > > WebMediaPlayer. Another example is togglePlayState. This accesses > > > MediaController, so how can I re-implement this in Blink-in-JS? > > > > > > I basically agree to haraken-san's idea. In the long term, some (or all?) of > > > implementation of HTMLMediaElement should be moved to Blink-in-JS and the > > above > > > backdoors should be disappeared. > > > > > > In the short term, can we allow those backdoors temporarily? Of course I'll > > > continue to work on this. > > > > I'm not really happy to introduce ad-hoc backdoors without having a big > picture. > > Shall we spend more time investigating where we should cut the boundary > between > > Blink-in-JS and C++? > > > > For example, I think MediaController is something that should be moved to > > Blink-in-JS with MediaControlsElement. That way you can avoid adding backdoors > > to MediaController. > > > > In my ideal world, we want to move the entire MediaControlsElement, > > RenderMediaControls, MediaController and a part of HTMLMediaElement associated > > with MediaControlsElement. That way, the only backdoors we'll need would just > > be: > > > > - Lifecycle callbacks of HTMLMediaElement (i.e., createdCallback, > > attachedCallback etc). > > - Interaction between HTMLMediaElement and WebMediaPlayer. > > - createUserAgentShadowRoot(). > > > > Either way, let's spend more time investigating the ideal boundary and clarify > > what backdoors are needed (i.e., what APIs are missing from the web platform). > A > > part of the goals of this project is to understand missing APIs of the web > > platform. Thus it's more important to understand it rather than trying to make > > it workable somehow. > > Sure, MediaController seems to be relatively independent from other C++ classes > and worth moving to Blink-in-JS, so let me start with MediaController. Moving MediaController to Blink-in-JS doesn't sound like a good idea to me, to be implemented properly that is eventually going to need some bits in C++. See https://groups.google.com/a/chromium.org/d/msg/blink-dev/MVcoNSPs1UQ/LIF-fvu2... As for all of the private APIs, e.g. playbackStarted, these bits should be rewritten to use the standard events fired on HTMLMediaElement, play and/or playing.
https://codereview.chromium.org/456323002/diff/200001/Source/core/html/shadow... File Source/core/html/shadow/MediaControls.idl (right): https://codereview.chromium.org/456323002/diff/200001/Source/core/html/shadow... Source/core/html/shadow/MediaControls.idl:40: [OnlyExposedToPrivateScript] void updateTextTrackContainerDisplay(HTMLElement textTrackContainer); On 2014/08/25 08:16:07, philipj wrote: > On 2014/08/25 07:11:27, hajimehoshi wrote: > > On 2014/08/25 06:53:49, haraken wrote: > > > On 2014/08/25 06:34:01, hajimehoshi wrote: > > > > On 2014/08/25 03:59:28, haraken wrote: > > > > > On 2014/08/23 05:48:33, abarth wrote: > > > > > > This is way too many private APIs. Why do you need private APIs? > > Aren't > > > > > there > > > > > > sufficient public APIs on the video element? > > > > > > > > > > As far as I look at the implementation, it seems hard to avoid adding > > these > > > > > private APIs if you keep HTMLMediaElement in C++ and move > > > MediaControlsElement > > > > > to JavaScript. HTMLMediaElement and MediaControlsElement need to > interact > > > with > > > > > each other, and we need a private API to proxy the interaction. > > > > > > > > > > One way to address the issue would be to move (a part of) > HTMLMediaElement > > > to > > > > > JavaScript as well and decrease the interaction that crosses the > boundary > > > > > between C++ and JavaScript. How hard would it be? > > > > > > > > That would be quite hard. Now MediaElement uses WebMediaPlayer and > > > > MediaController. For example, if I tried to implement > > > HTMLMediaElement::hasAudio > > > > to Blink-in-JS, another backdoor would be needed since hasAudio uses > > > > WebMediaPlayer. Another example is togglePlayState. This accesses > > > > MediaController, so how can I re-implement this in Blink-in-JS? > > > > > > > > I basically agree to haraken-san's idea. In the long term, some (or all?) > of > > > > implementation of HTMLMediaElement should be moved to Blink-in-JS and the > > > above > > > > backdoors should be disappeared. > > > > > > > > In the short term, can we allow those backdoors temporarily? Of course > I'll > > > > continue to work on this. > > > > > > I'm not really happy to introduce ad-hoc backdoors without having a big > > picture. > > > Shall we spend more time investigating where we should cut the boundary > > between > > > Blink-in-JS and C++? > > > > > > For example, I think MediaController is something that should be moved to > > > Blink-in-JS with MediaControlsElement. That way you can avoid adding > backdoors > > > to MediaController. > > > > > > In my ideal world, we want to move the entire MediaControlsElement, > > > RenderMediaControls, MediaController and a part of HTMLMediaElement > associated > > > with MediaControlsElement. That way, the only backdoors we'll need would > just > > > be: > > > > > > - Lifecycle callbacks of HTMLMediaElement (i.e., createdCallback, > > > attachedCallback etc). > > > - Interaction between HTMLMediaElement and WebMediaPlayer. > > > - createUserAgentShadowRoot(). > > > > > > Either way, let's spend more time investigating the ideal boundary and > clarify > > > what backdoors are needed (i.e., what APIs are missing from the web > platform). > > A > > > part of the goals of this project is to understand missing APIs of the web > > > platform. Thus it's more important to understand it rather than trying to > make > > > it workable somehow. > > > > Sure, MediaController seems to be relatively independent from other C++ > classes > > and worth moving to Blink-in-JS, so let me start with MediaController. > > Moving MediaController to Blink-in-JS doesn't sound like a good idea to me, to > be implemented properly that is eventually going to need some bits in C++. See > https://groups.google.com/a/chromium.org/d/msg/blink-dev/MVcoNSPs1UQ/LIF-fvu2... Thank you for the information! > As for all of the private APIs, e.g. playbackStarted, these bits should be > rewritten to use the standard events fired on HTMLMediaElement, play and/or > playing. Even though I rewrite these private API to use standard events, they just will be replaced with methods like onplaybackstarted, right? It seems that the number of methods won't change.
https://codereview.chromium.org/456323002/diff/220001/Source/core/html/shadow... File Source/core/html/shadow/MediaControls.idl (right): https://codereview.chromium.org/456323002/diff/220001/Source/core/html/shadow... Source/core/html/shadow/MediaControls.idl:54: }; I added the reasons why these private API are needed so far. IIUC, HTMLMediaElement, WebMediaPlayer, MediaController are quite difficult to implement Blink-in-JS. I was wondering if you could advise me how to treat these APIs...
https://codereview.chromium.org/456323002/diff/200001/Source/core/html/shadow... File Source/core/html/shadow/MediaControls.idl (right): https://codereview.chromium.org/456323002/diff/200001/Source/core/html/shadow... Source/core/html/shadow/MediaControls.idl:40: [OnlyExposedToPrivateScript] void updateTextTrackContainerDisplay(HTMLElement textTrackContainer); On 2014/08/25 09:11:18, hajimehoshi wrote: > On 2014/08/25 08:16:07, philipj wrote: > > On 2014/08/25 07:11:27, hajimehoshi wrote: > > > On 2014/08/25 06:53:49, haraken wrote: > > > > On 2014/08/25 06:34:01, hajimehoshi wrote: > > > > > On 2014/08/25 03:59:28, haraken wrote: > > > > > > On 2014/08/23 05:48:33, abarth wrote: > > > > > > > This is way too many private APIs. Why do you need private APIs? > > > Aren't > > > > > > there > > > > > > > sufficient public APIs on the video element? > > > > > > > > > > > > As far as I look at the implementation, it seems hard to avoid adding > > > these > > > > > > private APIs if you keep HTMLMediaElement in C++ and move > > > > MediaControlsElement > > > > > > to JavaScript. HTMLMediaElement and MediaControlsElement need to > > interact > > > > with > > > > > > each other, and we need a private API to proxy the interaction. > > > > > > > > > > > > One way to address the issue would be to move (a part of) > > HTMLMediaElement > > > > to > > > > > > JavaScript as well and decrease the interaction that crosses the > > boundary > > > > > > between C++ and JavaScript. How hard would it be? > > > > > > > > > > That would be quite hard. Now MediaElement uses WebMediaPlayer and > > > > > MediaController. For example, if I tried to implement > > > > HTMLMediaElement::hasAudio > > > > > to Blink-in-JS, another backdoor would be needed since hasAudio uses > > > > > WebMediaPlayer. Another example is togglePlayState. This accesses > > > > > MediaController, so how can I re-implement this in Blink-in-JS? > > > > > > > > > > I basically agree to haraken-san's idea. In the long term, some (or > all?) > > of > > > > > implementation of HTMLMediaElement should be moved to Blink-in-JS and > the > > > > above > > > > > backdoors should be disappeared. > > > > > > > > > > In the short term, can we allow those backdoors temporarily? Of course > > I'll > > > > > continue to work on this. > > > > > > > > I'm not really happy to introduce ad-hoc backdoors without having a big > > > picture. > > > > Shall we spend more time investigating where we should cut the boundary > > > between > > > > Blink-in-JS and C++? > > > > > > > > For example, I think MediaController is something that should be moved to > > > > Blink-in-JS with MediaControlsElement. That way you can avoid adding > > backdoors > > > > to MediaController. > > > > > > > > In my ideal world, we want to move the entire MediaControlsElement, > > > > RenderMediaControls, MediaController and a part of HTMLMediaElement > > associated > > > > with MediaControlsElement. That way, the only backdoors we'll need would > > just > > > > be: > > > > > > > > - Lifecycle callbacks of HTMLMediaElement (i.e., createdCallback, > > > > attachedCallback etc). > > > > - Interaction between HTMLMediaElement and WebMediaPlayer. > > > > - createUserAgentShadowRoot(). > > > > > > > > Either way, let's spend more time investigating the ideal boundary and > > clarify > > > > what backdoors are needed (i.e., what APIs are missing from the web > > platform). > > > A > > > > part of the goals of this project is to understand missing APIs of the web > > > > platform. Thus it's more important to understand it rather than trying to > > make > > > > it workable somehow. > > > > > > Sure, MediaController seems to be relatively independent from other C++ > > classes > > > and worth moving to Blink-in-JS, so let me start with MediaController. > > > > Moving MediaController to Blink-in-JS doesn't sound like a good idea to me, to > > be implemented properly that is eventually going to need some bits in C++. See > > > https://groups.google.com/a/chromium.org/d/msg/blink-dev/MVcoNSPs1UQ/LIF-fvu2... > > Thank you for the information! > > > As for all of the private APIs, e.g. playbackStarted, these bits should be > > rewritten to use the standard events fired on HTMLMediaElement, play and/or > > playing. > > Even though I rewrite these private API to use standard events, they just will > be replaced with methods like onplaybackstarted, right? It seems that the number > of methods won't change. If MediaControls.js does mediaElement.addEventListener("play", /* update the play button, etc */) no additional API should be needed. I haven't done anything with Blink-in-JS so possibly I'm not understanding something.
https://codereview.chromium.org/456323002/diff/220001/Source/core/html/shadow... File Source/core/html/shadow/MediaControls.cpp (right): https://codereview.chromium.org/456323002/diff/220001/Source/core/html/shadow... Source/core/html/shadow/MediaControls.cpp:581: void MediaControls::setCurrentTime(double time) I think this can be implemented in JS as (this.mediaElement_ || this.mediaElement_.controller).currentTime = time.
https://codereview.chromium.org/456323002/diff/200001/Source/core/html/shadow... File Source/core/html/shadow/MediaControls.idl (right): https://codereview.chromium.org/456323002/diff/200001/Source/core/html/shadow... Source/core/html/shadow/MediaControls.idl:40: [OnlyExposedToPrivateScript] void updateTextTrackContainerDisplay(HTMLElement textTrackContainer); On 2014/08/25 09:47:54, philipj wrote: > On 2014/08/25 09:11:18, hajimehoshi wrote: > > On 2014/08/25 08:16:07, philipj wrote: > > > On 2014/08/25 07:11:27, hajimehoshi wrote: > > > > On 2014/08/25 06:53:49, haraken wrote: > > > > > On 2014/08/25 06:34:01, hajimehoshi wrote: > > > > > > On 2014/08/25 03:59:28, haraken wrote: > > > > > > > On 2014/08/23 05:48:33, abarth wrote: > > > > > > > > This is way too many private APIs. Why do you need private APIs? > > > > Aren't > > > > > > > there > > > > > > > > sufficient public APIs on the video element? > > > > > > > > > > > > > > As far as I look at the implementation, it seems hard to avoid > adding > > > > these > > > > > > > private APIs if you keep HTMLMediaElement in C++ and move > > > > > MediaControlsElement > > > > > > > to JavaScript. HTMLMediaElement and MediaControlsElement need to > > > interact > > > > > with > > > > > > > each other, and we need a private API to proxy the interaction. > > > > > > > > > > > > > > One way to address the issue would be to move (a part of) > > > HTMLMediaElement > > > > > to > > > > > > > JavaScript as well and decrease the interaction that crosses the > > > boundary > > > > > > > between C++ and JavaScript. How hard would it be? > > > > > > > > > > > > That would be quite hard. Now MediaElement uses WebMediaPlayer and > > > > > > MediaController. For example, if I tried to implement > > > > > HTMLMediaElement::hasAudio > > > > > > to Blink-in-JS, another backdoor would be needed since hasAudio uses > > > > > > WebMediaPlayer. Another example is togglePlayState. This accesses > > > > > > MediaController, so how can I re-implement this in Blink-in-JS? > > > > > > > > > > > > I basically agree to haraken-san's idea. In the long term, some (or > > all?) > > > of > > > > > > implementation of HTMLMediaElement should be moved to Blink-in-JS and > > the > > > > > above > > > > > > backdoors should be disappeared. > > > > > > > > > > > > In the short term, can we allow those backdoors temporarily? Of course > > > I'll > > > > > > continue to work on this. > > > > > > > > > > I'm not really happy to introduce ad-hoc backdoors without having a big > > > > picture. > > > > > Shall we spend more time investigating where we should cut the boundary > > > > between > > > > > Blink-in-JS and C++? > > > > > > > > > > For example, I think MediaController is something that should be moved > to > > > > > Blink-in-JS with MediaControlsElement. That way you can avoid adding > > > backdoors > > > > > to MediaController. > > > > > > > > > > In my ideal world, we want to move the entire MediaControlsElement, > > > > > RenderMediaControls, MediaController and a part of HTMLMediaElement > > > associated > > > > > with MediaControlsElement. That way, the only backdoors we'll need would > > > just > > > > > be: > > > > > > > > > > - Lifecycle callbacks of HTMLMediaElement (i.e., createdCallback, > > > > > attachedCallback etc). > > > > > - Interaction between HTMLMediaElement and WebMediaPlayer. > > > > > - createUserAgentShadowRoot(). > > > > > > > > > > Either way, let's spend more time investigating the ideal boundary and > > > clarify > > > > > what backdoors are needed (i.e., what APIs are missing from the web > > > platform). > > > > A > > > > > part of the goals of this project is to understand missing APIs of the > web > > > > > platform. Thus it's more important to understand it rather than trying > to > > > make > > > > > it workable somehow. > > > > > > > > Sure, MediaController seems to be relatively independent from other C++ > > > classes > > > > and worth moving to Blink-in-JS, so let me start with MediaController. > > > > > > Moving MediaController to Blink-in-JS doesn't sound like a good idea to me, > to > > > be implemented properly that is eventually going to need some bits in C++. > See > > > > > > https://groups.google.com/a/chromium.org/d/msg/blink-dev/MVcoNSPs1UQ/LIF-fvu2... > > > > Thank you for the information! > > > > > As for all of the private APIs, e.g. playbackStarted, these bits should be > > > rewritten to use the standard events fired on HTMLMediaElement, play and/or > > > playing. > > > > Even though I rewrite these private API to use standard events, they just will > > be replaced with methods like onplaybackstarted, right? It seems that the > number > > of methods won't change. > > If MediaControls.js does mediaElement.addEventListener("play", /* update the > play button, etc */) no additional API should be needed. I haven't done anything > with Blink-in-JS so possibly I'm not understanding something. I see, but I'm afraid your suggested way doesn't work because these events would be exposed to not only Blink-in-JS but users. I think API with OnlyExposedToPrivateScript will be inevitable. Seeing the current situation, it seems impossible to implement MediaControleElement in Blink-in-JS without a number of backdoors. MediaControls can't be separated off the other classes. As for the other classes, HTMLMediaElement uses WebMediaPlayer, and MediaController is being developed in C++ as Phillip said, so both are also difficult to implement in Blink-in-JS. Adam, what do you think? https://codereview.chromium.org/456323002/diff/220001/Source/core/html/shadow... File Source/core/html/shadow/MediaControls.cpp (right): https://codereview.chromium.org/456323002/diff/220001/Source/core/html/shadow... Source/core/html/shadow/MediaControls.cpp:581: void MediaControls::setCurrentTime(double time) On 2014/08/25 09:53:27, philipj wrote: > I think this can be implemented in JS as (this.mediaElement_ || > this.mediaElement_.controller).currentTime = time. Thanks! I didn't realize that MediaController was already accessible.
On 2014/08/26 at 04:33:52, hajimehoshi wrote: > Adam, what do you think? I think we should abandon this work if it requires backdoors.
On 2014/08/26 04:42:10, abarth wrote: > On 2014/08/26 at 04:33:52, hajimehoshi wrote: > > Adam, what do you think? > > I think we should abandon this work if it requires backdoors. In order to avoid backdoors, I think we need to move MediaControlsElement, MediaController and (a part of) HTMLMediaElement in one go. Then the only backdoors we need would be just to allow the private script talk with WebMediaPlayer (i.e., embedder). That world makes sense to me. However, if now is not a good timing to move MediaController to private scripts, I agree that it's better to suspend this work at the moment unfortunately.
On 2014/08/26 04:51:17, haraken wrote: > On 2014/08/26 04:42:10, abarth wrote: > > On 2014/08/26 at 04:33:52, hajimehoshi wrote: > > > Adam, what do you think? > > > > I think we should abandon this work if it requires backdoors. > > In order to avoid backdoors, I think we need to move MediaControlsElement, > MediaController and (a part of) HTMLMediaElement in one go. Then the only > backdoors we need would be just to allow the private script talk with > WebMediaPlayer (i.e., embedder). That world makes sense to me. > > However, if now is not a good timing to move MediaController to private scripts, > I agree that it's better to suspend this work at the moment unfortunately. I agree too. I'll suspend this work for now. Thank you for your comments!
Can you close this review, please? |
