[webkit-reviews] review denied: [Bug 194899] [WPE] Add support for holepunch using an external video player : [Attachment 362608] Patch

[bugzilla-daemon at webkit.org]

Xabier Rodríguez Calvar <calvaris at igalia.com> has denied Miguel Gomez
<magomez at igalia.com>'s request for review:
Bug 194899: [WPE] Add support for holepunch using an external video player
https://bugs.webkit.org/show_bug.cgi?id=194899

Attachment 362608: Patch

https://bugs.webkit.org/attachment.cgi?id=362608&action=review




--- Comment #3 from Xabier Rodríguez Calvar <calvaris at igalia.com> ---
Comment on attachment 362608
  --> https://bugs.webkit.org/attachment.cgi?id=362608
Patch

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

Anyway, considering that you added a custom mime type for this, I don't think
we need to mess up with controls, at least for now.

> Source/WebCore/ChangeLog:11
> +	   This can be used to allow a player placed on a lower plane than the
broser to become visible.

broser -> browser

> Source/WebCore/platform/graphics/MediaPlayer.h:246
> +    virtual bool mediaPlayerControlsEnabled() const { return false; }

As save value I would return true?

> Source/WebCore/platform/graphics/MediaPlayer.h:585
> +    bool controlsEnabled() const { return
client().mediaPlayerControlsEnabled(); }
> +    void setControlsEnabled(bool enabled) {
client().mediaPlayerSetControlsEnabled(enabled); }

areControlsEnabled and setAreControlsEnabled all the way up and down the onion,
please.

> Source/WebCore/platform/graphics/holepunch/MediaPlayerPrivateHolePunch.cpp:30
> +using namespace std;

Let's remove this and qualify all names just in case we could class with any
WTF member.

> Source/WebCore/platform/graphics/holepunch/MediaPlayerPrivateHolePunch.cpp:69
> +    return const_cast<MediaPlayerPrivateGStreamerBase*>(this);

I think it is wrong. I don't know why you do a const_cast and why you try to
cast a class "this" does not derive from... Does it even build?

> Source/WebCore/platform/graphics/holepunch/MediaPlayerPrivateHolePunch.h:54
> +    void load(const String&) override { };

You can probably make all override final in this file.