[webkit-reviews] review denied: [Bug 131830] media foundation video element skeleton : [Attachment 229599] patch

[bugzilla-daemon at webkit.org]

Brent Fulgham <bfulgham at webkit.org> has denied Alex Christensen
<alex.christensen at flexsim.com>'s request for review:
Bug 131830: media foundation video element skeleton
https://bugs.webkit.org/show_bug.cgi?id=131830

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

------- Additional Comments from Brent Fulgham <bfulgham at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=229599&action=review


This looks like a great start. Please use "notImplemented()" in the various
stubs you have. Also, are there specific build requirements you need to use
MediaFoundation? Is it only available under Windows 8 or something?

We might want to protect the MediaFoundation stuff inside an SDK guard or
something.

> Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.cpp:55

> +{

I'd recommend putting "notImplemented()" in all of these locations. Then we can
remove them as you flesh out support.

> Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.cpp:69

> +void MediaPlayerPrivateMediaFoundation::load(const String&) { }

Ditto for all of these stubs:

load(const String&)
{
    notImplemented();
}

> Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.h:30
> + 

Get rid of this single space character!

> Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.h:34
> +    static void registerMediaEngine(MediaEngineRegistrar registrar);

Remove 'registrar' please.

> Source/WebKit/win/FullscreenVideoController.cpp:28
> +#if ENABLE(VIDEO) && !USE(GSTREAMER) && !USE(MEDIA_FOUNDATION)

Why don't you want to support full screen with media foundation?