Bug 163308 – REGRESSION (r206025): All YouTube videos play with black bars on all four sides

• Component: New Bugs
• Status: RESOLVED FIXED

[Jer Noble]

REGRESSION (r206025): All YouTube videos play with black bars on all four sides

[Jer Noble]

rdar://problem/28613862

[Jer Noble]

Created attachment 291360 [details]
Patch

[Darin Adler]

Comment on attachment 291360 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=291360&action=review

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaSourceAVFObjC.mm:692
>          RetainPtr<id> observer = weakThis->m_sizeChangeObservers.takeFirst();

What is going on here? How is it valuable to set a local variable that is then never used?

> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:960
> +    m_cachedSize = FloatSize();

I think { } is nicer here than FloatSize().

> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:988
>              m_cachedSize = formatSize;

What guarantees this new size is not zero?

[Jer Noble]

(In reply to comment #3)
> Comment on attachment 291360 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=291360&action=review
> 
> > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaSourceAVFObjC.mm:692
> >          RetainPtr<id> observer = weakThis->m_sizeChangeObservers.takeFirst();
> 
> What is going on here? How is it valuable to set a local variable that is
> then never used?

I looked back through this file's history, hoping that I'd find some case where I'd previously used the results of observer.  Alas, it's never been used. I'll take care of this in a follow-up.

> > Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:960
> > +    m_cachedSize = FloatSize();
> 
> I think { } is nicer here than FloatSize().

Ok!

> > Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:988
> >              m_cachedSize = formatSize;
> 
> What guarantees this new size is not zero?

Nothing, and good point. Maybe I'll make this an Optional<FloatSize> and use !m_cachedSize instead of .isZero().

[Jer Noble]

Created attachment 291509 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 291509 [details]
Patch for landing

Clearing flags on attachment: 291509

Committed r207584: <http://trac.webkit.org/changeset/207584>