[webkit-reviews] review denied: [Bug 39474] [GStreamer] GTK XOverlay support in GStreamerGWorld : [Attachment 61777] fullscreen video support in WebKit/gtk

[bugzilla-daemon at webkit.org]

Gustavo Noronha (kov) <gns at gnome.org> has denied Philippe Normand
<pnormand at igalia.com>'s request for review:
Bug 39474: [GStreamer] GTK XOverlay support in GStreamerGWorld
https://bugs.webkit.org/show_bug.cgi?id=39474

Attachment 61777: fullscreen video support in WebKit/gtk
https://bugs.webkit.org/attachment.cgi?id=61777&action=review

------- Additional Comments from Gustavo Noronha (kov) <gns at gnome.org>
 286	 gtk_widget_show_all(m_window);
 287	 gtk_widget_realize(m_window);

No point in showing, then realizing. Showing triggers realizing. You seem to
only need realize at this point, but it seems you want it shown anyway, so just
show it.

 290	 GdkCursor* cursor = gdk_cursor_new(GDK_BLANK_CURSOR);
 291	 m_cursor = gdk_window_get_cursor(window);
 292	 gdk_window_set_cursor(window, cursor);

I think you need to ref this cursor. Since the cursor is owned by the window,
the pointer you got will not necessarily (and more likely not) exist after you
set the blank cursor on it.

 298	 gtk_window_fullscreen(GTK_WINDOW(m_window));
 299	 gtk_widget_show_all(m_hudWindow);
 300	 showHud(true);

You are already showing the m_hudWindow inside showHud, why do you need this
call to gtk_widget_show_all() here?

 319	 // Scale the hud to the full width of the screen.
 320	 gtk_window_resize(GTK_WINDOW(m_hudWindow), fullscreenRectangle.width,
hudHeight);

Scale -> resize. Scale gives the wrong idea.

Some general comments: we have a number of callbacks taking the video
controller object as the data. This has been, I think, one of the top three
causes of crashes in webkitgtk code as far as I remember. mrobinson figured out
a way to make this kind of code more robust, using GClosures. Check this change
out, I'd recommend doing something similar for these:
http://trac.webkit.org/changeset/57901. Also, you don't seem to have many
checks in place for the case where enterFullScreen fails, and returns 0.

There are two things I really don't like: the code uses the media element
directly for many things, and seems to duplicate some code, I think that might
be something we'll have to accept, though, I haven't been able to assess if you
could use the player instead of messing with the element. I really dislike how
enterFullscreen returns a gulong, though. That feels really dirty heh. I would
prefer having another method, platformWindow(), say, that would return a
typedef'ed type, having each implementation return its window type, with no
need for ugly reinterpret_cast'ing, and stuff. enterFullScreen could return a
boolean to let the caller know things went through or not. So you'd have,
instead of:

 266	 m_window =
reinterpret_cast<GtkWidget*>(m_mediaElement->platformMedia().media.gstreamerGWo
rld->enterFullscreen());

this:

    if
(!m_mediaElement->platformMedia().media.gstreamerGWorld->enterFullscreen())
	return;

    m_window =
m_mediaElement->platformMedia().media.gstreamerGWorld->platformWindow();

I don't really like these huge chains, too, btw, can we just hold a pointer to
gstreamerGWorld? Finally, I think this design is a good enough one for a first
iteration, but I think we should consider factoring out the "interfacing with
the media element/player" into a MediaPlayerClient so that we could provide API
for browsers to know about/control the media elements in a way that is higher
level than using the DOM bindings. I'll say r- for now so we can get these
comments worked on, thanks for working on this, again =).