Simplify and remove some unused video element code (helpful for ARC-compatibility)
Created attachment 341893 [details] Patch
Comment on attachment 341893 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=341893&action=review > Source/WebCore/platform/graphics/MediaPlayer.h:211 > + virtual Vector<RefPtr<PlatformTextTrack>> outOfBandTrackSources() { return Vector<RefPtr<PlatformTextTrack>>(); } return { }; > Source/WebCore/platform/graphics/MediaPlayer.h:588 > + bool m_visible { false }; Can we swap the order of this line and the one below to achieve better structure packing? > Source/WebCore/platform/graphics/cocoa/WebCoreDecompressionSession.h:76 > + unsigned totalVideoFrames() { return m_totalVideoFrames; } Make this member function const? > Source/WebCore/platform/graphics/cocoa/WebCoreDecompressionSession.h:77 > + unsigned droppedVideoFrames() { return m_droppedVideoFrames; } Ditto. > Source/WebCore/platform/graphics/cocoa/WebCoreDecompressionSession.h:78 > + unsigned corruptedVideoFrames() { return m_corruptedVideoFrames; } Ditto. > Source/WebCore/platform/graphics/cocoa/WebCoreDecompressionSession.h:79 > MediaTime totalFrameDelay() { return m_totalFrameDelay; } Ditto even though it is not part of the patch. > Source/WebKitLegacy/mac/WebView/WebVideoFullscreenController.mm:115 > +// called once each time we enter full screen. So it should have a differena name. differena => different > Source/WebKitLegacy/mac/WebView/WebVideoFullscreenController.mm:-126 > - if (_videoElement->platformMedia().type == PlatformMedia::AVFoundationMediaPlayerType) { Should we assert this?
Comment on attachment 341893 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=341893&action=review >> Source/WebKitLegacy/mac/WebView/WebVideoFullscreenController.mm:-126 >> - if (_videoElement->platformMedia().type == PlatformMedia::AVFoundationMediaPlayerType) { > > Should we assert this? There is no such thing as platformMedia().type any more, so we couldn’t assert *this*, exactly. We can’t assert what type of media player we have without adding functions to the MediaPlayerPrivate class since there are currently no type checking functions. Any MediaPlayerPrivate classes other than MediaPlayerPrivateAVFoundationObjC will return nil from the avPlayer function (renamed to objCAVFoundationAVPlayer in the updated version of my patch to be precise and avoid a name conflict). The old code would silently do nothing if the video element had a different/wrong type and the new code will still do that. It seems a bit like asking for trouble for me to do that when doing this refactoring. On the other hand, I have no idea why it’s OK to just fail to make the "full screening" work due to an unexpected media player type, so it does seem that someone could return and tighten this up a bit. I am also unsure about race conditions when the video element is set before the window is loaded and then the window is loaded later. The code goes out of its way to do nothing if isWindowLoaded is NO, but seems to implicitly assume that it cannot be loaded later. I am unsure this cases is tested. None of this should be affected by my refactoring.
Created attachment 341953 [details] Patch
Comment on attachment 341953 [details] Patch Attachment 341953 [details] did not pass win-ews (win): Output: http://webkit-queues.webkit.org/results/7996193 New failing tests: http/tests/security/contentSecurityPolicy/userAgentShadowDOM/allow-audio.html
Created attachment 341957 [details] Archive of layout-test-results from ews205 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews205 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Committed r232510: <https://trac.webkit.org/changeset/232510>
<rdar://problem/40810185>
Looking at naming conventions in the code, maybe instead of objCAVFoundationAVPlayer I should have named this function something like playerAVFObjC?