[webkit-reviews] review granted: [Bug 57203] http streams don't always display video with AVFoundation backend : [Attachment 87167] Proposed patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Eric Carlson
<eric.carlson at apple.com>'s request for review:
Bug 57203: http streams don't always display video with AVFoundation backend
https://bugs.webkit.org/show_bug.cgi?id=57203

Attachment 87167: Proposed patch
https://bugs.webkit.org/attachment.cgi?id=87167&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=87167&action=review

>
Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundation.cp
p:444
> -    if (isReadyForVideoSetup() && !hasSetUpVideoRendering())
> +    if (isReadyForVideoSetup() && currentRenderingMode() !=
preferredRenderingMode())

This looks fine, but I have no understanding of the reason for the change. The
change log says what changed, but not why, and similarly the code does not say
why. I’m not asking for a comment, just noting that there’s no way for me to
verify this is a good change.

>
Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundationObj
C.mm:156
> +	   const NSArray *keys = itemKVOProperties();
> +	   for (NSString *keyName in keys)

No need for a local variable here.

>
Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundationObj
C.mm:264
> +	   const NSArray *keys = itemKVOProperties();
> +	   for (NSString *keyName in keys)

No need for a local variable here.

>
Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundationObj
C.mm:676
> +const NSArray* MediaPlayerPrivateAVFoundationObjC::itemKVOProperties()

With an Objective-C type I don’t think the const does any good here other than
requiring you to write const elsewhere. You can still call any method on such a
pointer. Immutability in Objective-C and Foundation is a property of the class,
not the constness of the pointer.

>
Source/WebCore/platform/graphics/avfoundation/MediaPlayerPrivateAVFoundationObj
C.mm:680
> +    static NSArray* keys;
> +    if (!keys) {
> +	   keys = [[NSArray alloc] initWithObjects: @"presentationSize",

This can just be static NSArray * keys = ... because C++ already handles the
one time initialization for us.

On a separate note, I am worried that this code will not work properly under
GC. I seem to recall that when global variables were in C++ functions that we
had to do an explicit CFRetain. Tim Hatcher or Mark Rowe might remember.

Also we normally don’t put spaces after colons like this one.

We should be consistent about where we put the "*" for NSArray.