[webkit-reviews] review denied: [Bug 58346] Convert media controls hooks to a client interface : [Attachment 89917] Patch

[bugzilla-daemon at webkit.org]

Dimitri Glazkov (Google) <dglazkov at chromium.org> has denied Steve Lacey
<sjl at chromium.org>'s request for review:
Bug 58346: Convert media controls hooks to a client interface
https://bugs.webkit.org/show_bug.cgi?id=58346

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

------- Additional Comments from Dimitri Glazkov (Google)
<dglazkov at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=89917&action=review

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:22965
> +				1F0D6E0E13590DE700236592 /* MediaControlBase.h
in Headers */,

The XCode changes look like they haven't been sorted. Can you run
sort-XCode-project-files on this?

> Source/WebCore/html/shadow/MediaControlBase.h:34
> +#include <HTMLDivElement.h>
> +#include <HTMLMediaElement.h>
> +#include <HTMLNames.h>

"", not <>

> Source/WebCore/html/shadow/MediaControlBase.h:38
> +class MediaControlBase : public HTMLDivElement {

I am sorry about flip-flopping here, but looking at callsites, it's clear that
they are working with an interface, not a base class. So we should call this
new thing MediaControls, not MediaControlBase.

> Source/WebCore/html/shadow/MediaControlBase.h:42
> +    // This function is to be implemented in your platform-specific media

port-specific.

> Source/WebCore/html/shadow/MediaControlBase.h:73
> +    MediaControlBase(HTMLMediaElement* mediaElement)
> +	   : HTMLDivElement(HTMLNames::divTag, mediaElement->document()) { }

I know you want to keep this change to only have a header file, but you could
save a couple of header includes if you just have a source file with it.

> Source/WebCore/html/shadow/MediaControlBase.h:81
> +#endif

extra break after first endif would be helpful.

> Source/WebCore/html/shadow/MediaControlRootElement.h:28
> +#ifndef MediaControlRootElement_h
> +#define MediaControlRootElement_h

Ha! Thanks for noticing this one.