[Webkit-unassigned] [Bug 140337] [WinCairo][Video] Windows Media Foundation implementation is not completed.

[bugzilla-daemon at webkit.org]

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

Alex Christensen <alex.christensen at flexsim.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #244624|review?                     |review-
              Flags|                            |

--- Comment #11 from Alex Christensen <alex.christensen at flexsim.com> ---
Comment on attachment 244624
  --> https://bugs.webkit.org/attachment.cgi?id=244624
Patch

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

This is great!  Let's do another iteration of refining before putting it in, though.

> Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.cpp:49
> +    , m_hwndVideo(0)

This should probably be nullptr.

> Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.cpp:133
> +    HRESULT hr = m_mediaSession->Pause();

This could just be put into the next line.  We don't really need an explicit local variable name if we only use it once outside of an assert.

> Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.cpp:239
> +    AsyncCallback* callback = new AsyncCallback(this, true);

Where is this deleted?

> Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.cpp:240
> +    m_mediaSession->BeginGetEvent(callback, nullptr);

Should we check the return value of this to see if it succeeded?

> Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.cpp:261
> +    HRESULT hr = MFCreateSourceResolver(&m_sourceResolver);

Again, this could just be put inside the next line.

> Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.cpp:262
> +    if (!SUCCEEDED(hr))

That's what the FAILED macro is for.

> Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.cpp:272
> +    return SUCCEEDED(hr);

This should always be true, right?  Otherwise it would have returned early.

> Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.cpp:327
> +        AsyncCallback* callback = new AsyncCallback(this, true);

Again, should this be deleted?

> Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.cpp:353
> +    for (int i = 0; i < sourceStreams; i++) {

i should be a DWORD to keep type comparisons as consistent as possible.

> Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.cpp:420
> +    wcex.hInstance = 0;

nullptr

> Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.cpp:421
> +    wcex.hIcon = 0;

nullptr? Is HICON a pointer type?

> Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.cpp:423
> +    wcex.hbrBackground = 0;

probably nullptr.

> Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.cpp:426
> +    wcex.hIconSm = 0;

probably nullptr.

> Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.cpp:436
> +    HWND hWndParent = 0;

nullptr

> Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.cpp:440
> +    if (view && view->hostWindow())

else return early, I think.  Otherwise CreateWindowEx could be given nullptr as its parent.

> Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.cpp:543
> +    m_videoDisplay->SetVideoPosition(nullptr, &rc);

It would be good to assert m_videoDisplay and m_player before using them here.

> Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.cpp:583
> +    int refCount = m_refCount;

ULONG.

> Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.h:118
> +        virtual HRESULT STDMETHODCALLTYPE QueryInterface(REFIID riid, __RPC__deref_out void __RPC_FAR *__RPC_FAR *ppvObject);

These should probably have override after them.

> Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.h:126
> +        int m_refCount;

This should be a ULONG.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20150117/9626f4f3/attachment-0002.html>