Bug 54531 – [GTK] [Windows] Plugins fail to load

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Bakhtiar Hasmanan]

In my test webkitgtk (1.2.7) did queried the registry and some common plugins location but none loaded. Seems like just found the path but not the module? 

Is it supported here?

[Martin Robinson]

I'd be suprised if plugins work on Windows.

[Bakhtiar Hasmanan]

Is it possible to play at least youtube's video (flv) with gstreamer's ffmpeg? So no need to use flash plugin.

[Martin Robinson]

(In reply to comment #2)
> Is it possible to play at least youtube's video (flv) with gstreamer's ffmpeg? So no need to use flash plugin.

Many people are using HTML5 video to watch YouTube. I'm not sure how well this works on Windows though.

[Kalev Lember]

Created attachment 148389 [details]
Patch

Switch to using PluginPackageWin.cpp and PluginViewWin.cpp on Windows platform, and leave plugins/gtk/ only for XP_UNIX platforms. With this we can share a lot of code with other ports and don't have to reimplement all the Windows-specific code in plugins/gtk/.

[WebKit Review Bot]

Attachment 148389 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'GNUmakefile.am', u'Source/WT..." exit_code: 1
Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:1169:  Use the class HWndDC instead of calling GetDC to avoid potential memory leaks.  [runtime/leaky_pattern] [5]
Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:1170:  Use adoptPtr and OwnPtr<HDC> when calling CreateCompatibleDC to avoid potential memory leaks.  [runtime/leaky_pattern] [5]
Total errors found: 2 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Martin Robinson]

Comment on attachment 148389 [details]
Patch

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

Great work! It seems like we can remove the code duplication in GraphicsContext though.

> Source/WebCore/GNUmakefile.am:979
> +if TARGET_WIN32
> +libWebCore_la_LIBADD = -lversion
> +endif
> +

Probably want to drop a little comment explaining why this is necessary.

> Source/WebCore/GNUmakefile.list.am:4823
> +	Source/WebCore/plugins/gtk/PluginPackageGtk.cpp \
> +	Source/WebCore/plugins/gtk/PluginViewGtk.cpp \

This looks like it breaks non-X11, non Win32 builds. For instance, DirectFB. Granted plugins probably do not work there anyway.

> Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:1134
> +#if (PLATFORM(GTK) && OS(WINDOWS))
> +static void setRGBABitmapAlpha(unsigned char* bytes, size_t length, unsigned char level)

Why not simply add the GraphicsContextWin.cpp and GraphicsContextCairoWin.cpp files to the source list instead of duplicating all this code here?

> Source/WebCore/plugins/win/PluginViewWin.cpp:113
> +    if (GdkWindow *window = gtk_widget_get_window(client))

Asterisks go next to the variable name in WebKit.

> Source/WebCore/plugins/win/PluginViewWin.cpp:114
> +        return (HWND)GDK_WINDOW_HWND(window);

Are you sure you have to cast here? If you do, you should use a C++ style cast (static_cast).

[Kalev Lember]

(In reply to comment #6)
> (From update of attachment 148389 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=148389&action=review
> 
> Great work! It seems like we can remove the code duplication in GraphicsContext though.

Thanks!


> > Source/WebCore/GNUmakefile.am:979
> > +if TARGET_WIN32
> > +libWebCore_la_LIBADD = -lversion
> > +endif
> > +
> 
> Probably want to drop a little comment explaining why this is necessary.

Done.


> > Source/WebCore/GNUmakefile.list.am:4823
> > +	Source/WebCore/plugins/gtk/PluginPackageGtk.cpp \
> > +	Source/WebCore/plugins/gtk/PluginViewGtk.cpp \
> 
> This looks like it breaks non-X11, non Win32 builds. For instance, DirectFB. Granted plugins probably do not work there anyway.

Indeed. I've now moved this to TARGET_WIN32 else section, although I'm wondering if we should instead use PluginPackageNone.cpp and PluginViewNone.cpp on such platforms.


> > Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp:1134
> > +#if (PLATFORM(GTK) && OS(WINDOWS))
> > +static void setRGBABitmapAlpha(unsigned char* bytes, size_t length, unsigned char level)
> 
> Why not simply add the GraphicsContextWin.cpp and GraphicsContextCairoWin.cpp files to the source list instead of duplicating all this code here?

I'll give this a try. Is there any EWS bot for checking that the CairoWin port still builds?


> > Source/WebCore/plugins/win/PluginViewWin.cpp:113
> > +    if (GdkWindow *window = gtk_widget_get_window(client))
> 
> Asterisks go next to the variable name in WebKit.

Fixed.


> > Source/WebCore/plugins/win/PluginViewWin.cpp:114
> > +        return (HWND)GDK_WINDOW_HWND(window);
> 
> Are you sure you have to cast here? If you do, you should use a C++ style cast (static_cast).

Yeah, the cast is needed: confusingly enough, GDK_WINDOW_HWND() returns HGDIOBJ instead of a HWND.

[Kalev Lember]

Created attachment 148536 [details]
Patch v2

Thanks for the review, I've updated the patch accordingly.

[WebKit Review Bot]

Comment on attachment 148536 [details]
Patch v2

Clearing flags on attachment: 148536

Committed r120956: <http://trac.webkit.org/changeset/120956>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.