Bug 44508

Summary: [EFL] Add PlatformVideoWindowEfl.cpp for WebKit EFL
Product: WebKit Reporter: Gyuyoung Kim <gyuyoung.kim>
Component: WebKit EFLAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: antognolli+webkit, commit-queue, lucas.de.marchi, mrobinson, pnormand, tonikitoo
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
Bug Depends on: 44590    
Bug Blocks: 44098    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
mrobinson: review-
Patch none

Description Gyuyoung Kim 2010-08-24 02:48:20 PDT
It seems to me that WebKit EFL also needs to have PlatformVideoWindowEfl.cpp in order to use gstreamer as it's video framework.
Comment 1 Gyuyoung Kim 2010-08-24 03:05:55 PDT
Created attachment 65250 [details]
Patch

I modify PlatformVideoWindow.h and create PlatformVideoWindowEfl.cpp.
Comment 2 Philippe Normand 2010-08-24 03:14:24 PDT
This is needed only for fullscreen video display. You should already be able to use the gst webkitvideo sink. For painting Cairo is used, see ImageGStreamerCairo.cpp ... For EFL you might need a new ImageGStreamer implementation, dunno.
Comment 3 Philippe Normand 2010-08-24 04:28:33 PDT
Comment on attachment 65250 [details]
Patch


>+#if PLATFORM(EFL)
>+#if ENABLE(GLIB_SUPPORT)
>+#include <glib.h>
>+#endif
>+#endif
>+

I think the guards can be removed because glib.h is required for the use of gulong below anyway.
The only reason why it works currently is that gtk.h includes glib.h

> namespace WebCore {
> 
> class PlatformVideoWindow : public RefCounted<PlatformVideoWindow> {
>@@ -39,12 +45,16 @@ class PlatformVideoWindow : public RefCounted<PlatformVideoWindow> {
>         PlatformVideoWindow();
>         ~PlatformVideoWindow();
> 
>+#if PLATFORM(GTK)
>         PlatformWindowType* window() const { return m_window; }
>+#endif

>         gulong videoWindowId() const { return m_videoWindowId; }
> 
>     private:
>         gulong m_videoWindowId;
>+#if PLATFORM(GTK)
>         PlatformWindowType* m_videoWindow;
>+#endif

This is wrong. You need to add a typedef for your platform so PlatformWindowType is valid.
Comment 4 Gyuyoung Kim 2010-08-24 05:15:58 PDT
(In reply to comment #3)
> (From update of attachment 65250 [details])
> 
> >+#if PLATFORM(EFL)
> >+#if ENABLE(GLIB_SUPPORT)
> >+#include <glib.h>
> >+#endif
> >+#endif
> >+
> 
> I think the guards can be removed because glib.h is required for the use of gulong below anyway.
> The only reason why it works currently is that gtk.h includes glib.h
> 
> > namespace WebCore {
> > 
> > class PlatformVideoWindow : public RefCounted<PlatformVideoWindow> {
> >@@ -39,12 +45,16 @@ class PlatformVideoWindow : public RefCounted<PlatformVideoWindow> {
> >         PlatformVideoWindow();
> >         ~PlatformVideoWindow();
> > 
> >+#if PLATFORM(GTK)
> >         PlatformWindowType* window() const { return m_window; }
> >+#endif
> 
> >         gulong videoWindowId() const { return m_videoWindowId; }
> > 
> >     private:
> >         gulong m_videoWindowId;
> >+#if PLATFORM(GTK)
> >         PlatformWindowType* m_videoWindow;
> >+#endif
> 
> This is wrong. You need to add a typedef for your platform so PlatformWindowType is valid.

I agree with your opinion. I am going to consider your guidance.

Thank you.
Comment 5 Philippe Normand 2010-08-24 08:40:44 PDT
BTW fullscreen video display shouldn't be blocker for an initial HTML5 video support in WebKit EFL.

You just need to avoid painting the fullscreen button in your media controls and make the webkitSupportsFullscreen method of the gst player return false in your case.
Comment 6 Gyuyoung Kim 2010-08-24 22:00:55 PDT
(In reply to comment #3)
> (From update of attachment 65250 [details])
> 
> >+#if PLATFORM(EFL)
> >+#if ENABLE(GLIB_SUPPORT)
> >+#include <glib.h>
> >+#endif
> >+#endif
> >+
> 
> I think the guards can be removed because glib.h is required for the use of gulong below anyway.
> The only reason why it works currently is that gtk.h includes glib.h

WebKit EFL doesn't use gtk. So, I include glib.h for EFL. Is this wrong ?
Comment 7 Philippe Normand 2010-08-24 23:13:07 PDT
(In reply to comment #6)
> (In reply to comment #3)
> > (From update of attachment 65250 [details] [details])
> > 
> > >+#if PLATFORM(EFL)
> > >+#if ENABLE(GLIB_SUPPORT)
> > >+#include <glib.h>
> > >+#endif
> > >+#endif
> > >+
> > 
> > I think the guards can be removed because glib.h is required for the use of gulong below anyway.
> > The only reason why it works currently is that gtk.h includes glib.h
> 
> WebKit EFL doesn't use gtk. So, I include glib.h for EFL. Is this wrong ?

It is totally right, I was just asking to remove the #ifs
Comment 8 Gyuyoung Kim 2010-08-25 00:45:09 PDT
Created attachment 65381 [details]
Patch

I define PlatformWindowType for EFL.

Thank you.
Comment 9 Philippe Normand 2010-08-25 00:55:49 PDT
Comment on attachment 65381 [details]
Patch

>--- a/WebCore/platform/graphics/gstreamer/PlatformVideoWindow.h
>+++ b/WebCore/platform/graphics/gstreamer/PlatformVideoWindow.h
>@@ -28,6 +28,15 @@
> #if PLATFORM(GTK)
> #include <gtk/gtk.h>
> typedef GtkWidget PlatformWindowType;
>+#elif PLATFORM(EFL)
>+#include "Widget.h"
>+typedef PlatformWidget PlatformWindowType;
>+#endif
>+

Oh nice I didn't know about Widget.h
So we duplicated efforts here. I think it'd make sense to remove PlatformWindowType and use PlatformWidget directly.

>+#if PLATFORM(EFL)
>+#if ENABLE(GLIB_SUPPORT)
>+#include <glib.h>
>+#endif
> #endif
> 

I said in previous comment that the #ifs are not necessary.

The rest of the patch looks ok to me!
Comment 10 Gyuyoung Kim 2010-08-25 01:03:31 PDT
(In reply to comment #9)
> (From update of attachment 65381 [details])
> >--- a/WebCore/platform/graphics/gstreamer/PlatformVideoWindow.h
> >+++ b/WebCore/platform/graphics/gstreamer/PlatformVideoWindow.h
> >@@ -28,6 +28,15 @@
> > #if PLATFORM(GTK)
> > #include <gtk/gtk.h>
> > typedef GtkWidget PlatformWindowType;
> >+#elif PLATFORM(EFL)
> >+#include "Widget.h"
> >+typedef PlatformWidget PlatformWindowType;
> >+#endif
> >+
> 
> Oh nice I didn't know about Widget.h
> So we duplicated efforts here. I think it'd make sense to remove PlatformWindowType and use PlatformWidget directly.
> 

I wonder how to use PlatformWidget directly. In order to use the PlatformformWidget directly, I think we need to modify code as below,

 48 #if PLATFORM(GTK)
 49         PlatformWindowType* window() const { return m_window; }
 50 #elif PLATFORM(EFL)
 51         PlatformWidget* window() const { return m_window; }
 52 #endif


> >+#if PLATFORM(EFL)
> >+#if ENABLE(GLIB_SUPPORT)
> >+#include <glib.h>
> >+#endif
> > #endif
> > 
> 
> I said in previous comment that the #ifs are not necessary.
> 
> The rest of the patch looks ok to me!

I modify above code as below,

 31 #elif PLATFORM(EFL)
 32 #if ENABLE(GLIB_SUPPORT)
 33 #include <glib.h>
 34 #endif
 35 #include "Widget.h"
 36 typedef PlatformWidget PlatformWindowType;
 37 #endif

Is this ok ?
Comment 11 Philippe Normand 2010-08-25 01:13:27 PDT
(In reply to comment #10)

> I wonder how to use PlatformWidget directly. In order to use the PlatformformWidget directly, I think we need to modify code as below,
> 
>  48 #if PLATFORM(GTK)
>  49         PlatformWindowType* window() const { return m_window; }
>  50 #elif PLATFORM(EFL)
>  51         PlatformWidget* window() const { return m_window; }
>  52 #endif
> 
> 

No, it would be best to make the GTK port use PlatformWidget as well, it will all be more coherent and readable.

> I modify above code as below,
> 
>  31 #elif PLATFORM(EFL)
>  32 #if ENABLE(GLIB_SUPPORT)
>  33 #include <glib.h>
>  34 #endif
>  35 #include "Widget.h"
>  36 typedef PlatformWidget PlatformWindowType;
>  37 #endif
> 
> Is this ok ?

No. I thought I was clear, but it seems not :)
glib.h can safely be included *outside* any #ifs like this:

#include <glib.h>

If you make use of PlatformWidget no #if at all should be needed in this file.
Comment 12 Philippe Normand 2010-08-25 01:27:24 PDT
(In reply to comment #11)
> 
> > I modify above code as below,
> > 
> >  31 #elif PLATFORM(EFL)
> >  32 #if ENABLE(GLIB_SUPPORT)
> >  33 #include <glib.h>
> >  34 #endif
> >  35 #include "Widget.h"
> >  36 typedef PlatformWidget PlatformWindowType;
> >  37 #endif
> > 
> > Is this ok ?
> 

Well I understand your port doesn't have a hard dependency on GLib, so yeah you need guards, maybe like:

#if PLATFORM(GTK) || ENABLE(GLIB_SUPPORT)
#include <glib.h>
#endif

?
Comment 13 Philippe Normand 2010-08-25 01:43:52 PDT
Or even:

#if !PLATFORM(EFL) || ENABLE(GLIB_SUPPORT)
#include <glib.h>
#endif
Comment 14 Gyuyoung Kim 2010-08-25 02:01:32 PDT
Yes, WebKit EFL doesn't have hard dependency with glib. WebKit EFL should enable ENABLE_GLIB_SUPPORT macro to use glib. If you are ok, I will upload new patch again.

+#include "Widget.h"
+
 #if PLATFORM(GTK)
 #include <gtk/gtk.h>
 typedef GtkWidget PlatformWindowType;
+#elif PLATFORM(EFL)
+#include "Widget.h"
+typedef PlatformWidget PlatformWindowType;
+#endif
+
+#if !PLATFORM(EFL) || ENABLE(GLIB_SUPPORT)
+#include <glib.h>
 #endif
Comment 15 Gyuyoung Kim 2010-08-25 02:02:43 PDT
(In reply to comment #14)
> Yes, WebKit EFL doesn't have hard dependency with glib. WebKit EFL should enable ENABLE_GLIB_SUPPORT macro to use glib. If you are ok, I will upload new patch again.
> 
> +#include "Widget.h"
> +
>  #if PLATFORM(GTK)
>  #include <gtk/gtk.h>
>  typedef GtkWidget PlatformWindowType;
> +#elif PLATFORM(EFL)
> +#include "Widget.h"
> +typedef PlatformWidget PlatformWindowType;
> +#endif
> +
> +#if !PLATFORM(EFL) || ENABLE(GLIB_SUPPORT)
> +#include <glib.h>
>  #endif

Sorry, please remove "#include "Widget.h"

> +#include "Widget.h"
> +
Comment 16 Philippe Normand 2010-08-25 02:17:48 PDT
(In reply to comment #15)
> >  #if PLATFORM(GTK)
> >  #include <gtk/gtk.h>
> >  typedef GtkWidget PlatformWindowType;
> > +#elif PLATFORM(EFL)
> > +#include "Widget.h"
> > +typedef PlatformWidget PlatformWindowType;
> > +#endif
> > +
> > +#if !PLATFORM(EFL) || ENABLE(GLIB_SUPPORT)
> > +#include <glib.h>
> >  #endif
> 

Ok much better, what about PlatformWidget use for the GTK port too? I would be willing to test such patch :)

Also PlatformWidget is a pointer already. So you need to update the declarations of ::window(), m_videoWindow and m_window accordingly.
Comment 17 Gyuyoung Kim 2010-08-25 02:43:54 PDT
Created attachment 65388 [details]
Patch

I modify this patch according to your guidance. 

I built WebKit GTK based on this patch as well. There was no build breaks.
Comment 18 Philippe Normand 2010-08-25 02:58:32 PDT
Comment on attachment 65388 [details]
Patch

Nice, thanks :)
One last thing (from my side, at least):

> #if PLATFORM(GTK)
> #include <gtk/gtk.h>
>-typedef GtkWidget PlatformWindowType;
>+#endif

That #if can be removed all together. We no longer require to include gtk.h there but then we should move it to PlatformVideoWindowGtk.cpp I think. But as this patch is EFL-specific I can make that change myself in a separate patch.
Comment 19 Gyuyoung Kim 2010-08-25 03:04:19 PDT
(In reply to comment #18)
> (From update of attachment 65388 [details])
> Nice, thanks :)
> One last thing (from my side, at least):
> 
> > #if PLATFORM(GTK)
> > #include <gtk/gtk.h>
> >-typedef GtkWidget PlatformWindowType;
> >+#endif
> 
> That #if can be removed all together. We no longer require to include gtk.h there but then we should move it to PlatformVideoWindowGtk.cpp I think. But as this patch is EFL-specific I can make that change myself in a separate patch.

I couldn't remove "#include <gtk/gtk.h>" because of build error in PlatformVideoWindowGtk.cpp. If you want to move "#include <gtk/gtk.h>" to PlatformVideoWindowGtk.cpp, I can do it new patch. Do you want ?
Comment 20 Philippe Normand 2010-08-25 03:13:42 PDT
(In reply to comment #19)
> (In reply to comment #18)
> > (From update of attachment 65388 [details] [details])
> > Nice, thanks :)
> > One last thing (from my side, at least):
> > 
> > > #if PLATFORM(GTK)
> > > #include <gtk/gtk.h>
> > >-typedef GtkWidget PlatformWindowType;
> > >+#endif
> > 
> > That #if can be removed all together. We no longer require to include gtk.h there but then we should move it to PlatformVideoWindowGtk.cpp I think. But as this patch is EFL-specific I can make that change myself in a separate patch.
> 
> I couldn't remove "#include <gtk/gtk.h>" because of build error in PlatformVideoWindowGtk.cpp. If you want to move "#include <gtk/gtk.h>" to PlatformVideoWindowGtk.cpp, I can do it new patch. Do you want ?

In comment 19 I was thinking about yes. Sure, if you want file a new bug and patch after this one lands :)
Comment 21 Gyuyoung Kim 2010-08-25 03:15:39 PDT
(In reply to comment #20)
> (In reply to comment #19)
> > (In reply to comment #18)
> > > (From update of attachment 65388 [details] [details] [details])
> > > Nice, thanks :)
> > > One last thing (from my side, at least):
> > > 
> > > > #if PLATFORM(GTK)
> > > > #include <gtk/gtk.h>
> > > >-typedef GtkWidget PlatformWindowType;
> > > >+#endif
> > > 
> > > That #if can be removed all together. We no longer require to include gtk.h there but then we should move it to PlatformVideoWindowGtk.cpp I think. But as this patch is EFL-specific I can make that change myself in a separate patch.
> > 
> > I couldn't remove "#include <gtk/gtk.h>" because of build error in PlatformVideoWindowGtk.cpp. If you want to move "#include <gtk/gtk.h>" to PlatformVideoWindowGtk.cpp, I can do it new patch. Do you want ?
> 
> In comment 19 I was thinking about yes. Sure, if you want file a new bug and patch after this one lands :)

Sure, I will do that after landing this patch. :) 

Thanks.
Comment 22 Rafael Antognolli 2010-08-25 07:24:22 PDT
(In reply to comment #12)
> (In reply to comment #11)
> > 
> > > I modify above code as below,
> > > 
> > >  31 #elif PLATFORM(EFL)
> > >  32 #if ENABLE(GLIB_SUPPORT)
> > >  33 #include <glib.h>
> > >  34 #endif
> > >  35 #include "Widget.h"
> > >  36 typedef PlatformWidget PlatformWindowType;
> > >  37 #endif
> > > 
> > > Is this ok ?
> > 
> 
> Well I understand your port doesn't have a hard dependency on GLib, so yeah you need guards, maybe like:
> 
> #if PLATFORM(GTK) || ENABLE(GLIB_SUPPORT)
> #include <glib.h>
> #endif
> 
> ?

Actually, no guards are needed since Gyuyoung's patch is for gstreamer video backend, and gstreamer needs glib anyway. We even force ENABLE_GLIB_SUPPORT if ENABLE_VIDEO_SUPPORT is true.

Maybe if later another video implementation that doesn't use gstreamer is available, the yes, we could make this optional. And I think the following line (unless changed to something else with ifdefs too) wouldn't compile without #include <glib.h>:
         gulong videoWindowId() const { return m_videoWindowId; }
Comment 23 Gyuyoung Kim 2010-08-26 18:07:02 PDT
Hello  Philippe Normand,

Who can review this patch ?
Comment 24 Philippe Normand 2010-08-26 22:58:26 PDT
(In reply to comment #23)
> Hello  Philippe Normand,
> 
> Who can review this patch ?

Well Rafael is right about including glib.h without guards.

Then either Xan, Martin or Gustavo could review it maybe
Comment 25 Martin Robinson 2010-08-26 23:14:05 PDT
Comment on attachment 65388 [details]
Patch


> +#include "Widget.h"
> +
>  #include <wtf/PassRefPtr.h>

Please remove this extra line. There is no need to separate the include styles.

>  #if PLATFORM(GTK)
>  #include <gtk/gtk.h>
> -typedef GtkWidget PlatformWindowType;
> +#endif
> +
> +#if !PLATFORM(EFL) || ENABLE(GLIB_SUPPORT)
> +#include <glib.h>
>  #endif

Is the only reason for these #ifdefs and #includes gulong? Having
GLib and/or GTK+ includes in a header files can increase the compilation
time unecessarily. If possible, please just use unsigned long instead of
gulong and remove all the #ifdefs and #includes. Thanks!
  
> diff --git a/WebCore/platform/graphics/gstreamer/PlatformVideoWindowEfl.cpp b/WebCore/platform/graphics/gstreamer/PlatformVideoWindowEfl.cpp

Should this also be added to the CMake build scripts?
Comment 26 Gyuyoung Kim 2010-08-26 23:58:49 PDT
Created attachment 65682 [details]
Patch

Thanks, I fix this patch according to your comment.
I move the "#include <gtk/gtk.h>" to PlatformVideoWindowGtk.cpp.

BTW, I add the PlatformVideoWindowEfl.cpp to CMakeListEfl.txt in Bug 44098.
(Bug 44098 is blocked by this bug.)
Because, I want to add this file together with other files regarding gstreamer. So, the PlatformVideoWindowEfl.cpp will be added after landing this patch.
Comment 27 Gyuyoung Kim 2010-08-29 19:27:31 PDT
Hello Martin Robinson,

Please review again. :)
Comment 28 Gyuyoung Kim 2010-08-31 19:30:56 PDT
Hello Martin Robinson,

I am looking forward to land this patch in mainline.
I'd like to get your opinion regarding your patch.
Comment 29 Gyuyoung Kim 2010-08-31 22:01:40 PDT
wrong expression : "your patch" => "my patch"
Comment 30 Martin Robinson 2010-08-31 22:23:42 PDT
Comment on attachment 65682 [details]
Patch

LGTM.
Comment 31 WebKit Commit Bot 2010-08-31 23:05:14 PDT
Comment on attachment 65682 [details]
Patch

Clearing flags on attachment: 65682

Committed r66578: <http://trac.webkit.org/changeset/66578>
Comment 32 WebKit Commit Bot 2010-08-31 23:05:20 PDT
All reviewed patches have been landed.  Closing bug.