Bug 163308

Summary:

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

Product:

WebKit

Reporter:

Jer Noble <jer.noble>

Component:

New Bugs

Assignee:

Jer Noble <jer.noble>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

commit-queue

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	darin: review+
Patch for landing	none

Description Jer Noble 2016-10-11 17:09:31 PDT

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

Comment 1 Jer Noble 2016-10-11 17:14:11 PDT

rdar://problem/28613862

Comment 2 Jer Noble 2016-10-12 09:16:08 PDT

Created attachment 291360 [details]
Patch

Comment 3 Darin Adler 2016-10-13 12:37:14 PDT

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?

Comment 4 Jer Noble 2016-10-13 12:58:00 PDT

(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().

Comment 5 Jer Noble 2016-10-13 13:15:09 PDT

Created attachment 291509 [details]
Patch for landing

Comment 6 WebKit Commit Bot 2016-10-19 20:12:57 PDT

Comment on attachment 291509 [details]
Patch for landing

Clearing flags on attachment: 291509

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