[webkit-reviews] review canceled: [Bug 112971] [chromium] Move ownership of compositor VideoLayer to WebMediaPlayer : [Attachment 194354] Patch

[bugzilla-daemon at webkit.org]

Dana Jansens <danakj at chromium.org> has canceled Dana Jansens
<danakj at chromium.org>'s request for review:
Bug 112971: [chromium] Move ownership of compositor VideoLayer to
WebMediaPlayer
https://bugs.webkit.org/show_bug.cgi?id=112971

Attachment 194354: Patch
https://bugs.webkit.org/attachment.cgi?id=194354&action=review

------- Additional Comments from Dana Jansens <danakj at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=194354&action=review


>> Source/Platform/chromium/public/WebVideoFrame.h:36
>> +#endif
> 
> Can you please just pick one header to #define this in and set it there? 
Maybe WebLayer.h ?

stream_texture_factory_impl_android.cc only includes WebStreamTextureClient.h
and WGC3D.h. But I can just put it in WebVideoLayer.h and
WebStreamTextureClient.h.  Both of these are getting removed anyways, so it's
easy to clean up.

>> Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:276
>> +void WebMediaPlayerClientImpl::setWebLayer(WebLayer* layer)
> 
> I still think we need a synthetic style recalc here, even if the old code
didn't have it.  Without that there's nothing to make sure we get into
RenderLayerBacking to call setContentsToMedia() and hook the PlatformLayer into
the compositing tree

I tried to figure out how this could happen, but with no luck. It always does a
layout or something and you get a composited layer. Anyway, I've added it.

I was trying stuff like this:

<video style="width: 100px; height: 70px" id=v loop></video>
<script>
var v = document.getElementById('v');
v.src =
'http://promodj.com/download/3972016/Baauer%20%E2%80%93%20Harlem%20Shake%20(Sou
ndRus%20remix)%20(promodj.com).mp3';
window.setTimeout(function() {document.getElementById('v').src =
'http://goo.gl/AD9aO';}, 2000);
</script>


Also, when the MediaPlayer is being shut down,
m_mediaPlayer->mediaPlayerClient() is NULL, so we can't use that to find the
HTMLMediaElement* anymore there. So I've guarded it on that being not null.

>> Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:356
>> +	return m_videoLayer;
> 
> Hmm, I think it's reasonable to ask for the platformLayer() for a <video>
element that is composited but doesn't have composited content.  What happens
if you have a page with a <video style="-webkit-transform:translateZ(0)"
src="..."> that isn't in composited video playback?

Ya, I managed to hit this assert, thanks! I removed it.

>> Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:703
>> +	    m_mediaPlayer->mediaPlayerClient())->document()->frame();
> 
> Could any of these be null?  Since this is a public API function I don't
think we can make assumptions about the state.

The frame is also grabbed in WebMediaPlayerClientImpl::loadInternal(). So it is
assumed valid at that time. I can cache the value there instead of making the
same assumption in a new codepath.