Bug 186258 – Simplify and remove some unused video element code (helpful for ARC-compatibility)

• Component: Media
• Status: RESOLVED FIXED

[Darin Adler]

Simplify and remove some unused video element code (helpful for ARC-compatibility)

[Darin Adler]

Created attachment 341893 [details]
Patch

[Daniel Bates]

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?

[Darin Adler]

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.

[Darin Adler]

Created attachment 341953 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[Darin Adler]

Committed r232510: <https://trac.webkit.org/changeset/232510>

[Radar WebKit Bug Importer]

<rdar://problem/40810185>

[Darin Adler]

Looking at naming conventions in the code, maybe instead of objCAVFoundationAVPlayer I should have named this function something like playerAVFObjC?