[Webkit-unassigned] [Bug 150941] [WinCairo][Video][MediaFoundation] Video should be rendered in provided graphics context.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=150941

--- Comment #7 from Alex Christensen <achristensen at apple.com> ---
Comment on attachment 264880
  --> https://bugs.webkit.org/attachment.cgi?id=264880
Patch

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

> Source/WebCore/platform/graphics/win/MediaFoundationUtils.h:9
> +#define CheckPointer(x, hr) if (x == nullptr) { return hr; }

WebKit style: if (!x)
Also, I think this macro isn't good.  Just have lots of early returns.  That'll make it more clear and consistent with the rest of WebKit.

> Source/WebCore/platform/graphics/win/MediaFoundationUtils.h:82
> +        return m_pType != nullptr;

Again, no comparing with nullptr.  !!m_pType

> Source/WebCore/platform/graphics/win/MediaFoundationUtils.h:87
> +        assert(IsValid());

ASSERT here and elsewhere.

> Source/WebCore/platform/graphics/win/MediaFoundationUtils.h:100
> +        : m_pType(nullptr)
> +    {
> +        if (pType) {
> +            m_pType = pType;
> +            m_pType->AddRef();  
> +        }

This is strange.  I'd prefer this:
    : m_pType(pType)
{
    if (m_pType)
        m_pType->AddRef();
}

> Source/WebCore/platform/graphics/win/MediaFoundationUtils.h:104
> +    // Copy ctor
> +    MediaType(const MediaType&  mt)

Comments like this not needed.

> Source/WebCore/platform/graphics/win/MediaFoundationUtils.h:1133
> +template<class T>
> +class AsyncCallback : public IMFAsyncCallback {

This is cool, but it's only used once.  Is it always called on the main thread, or do we need Locks when using this?

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20151106/1474bc93/attachment.html>