Bug 54531

Summary: [GTK] [Windows] Plugins fail to load
Product: WebKit Reporter: Bakhtiar Hasmanan <mr.tiar>
Component: WebKitGTKAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: joone.hur, kalevlember, mrobinson, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Windows XP   
Bug Depends on:    
Bug Blocks: 89499, 89501    
Attachments:
Description Flags
Patch
mrobinson: review-
Patch v2 none

Description Bakhtiar Hasmanan 2011-02-15 22:26:08 PST
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?
Comment 1 Martin Robinson 2011-02-16 00:48:30 PST
I'd be suprised if plugins work on Windows.
Comment 2 Bakhtiar Hasmanan 2011-05-18 08:33:29 PDT
Is it possible to play at least youtube's video (flv) with gstreamer's ffmpeg? So no need to use flash plugin.
Comment 3 Martin Robinson 2011-05-18 09:40:47 PDT
(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.
Comment 4 Kalev Lember 2012-06-19 12:17:42 PDT
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/.
Comment 5 WebKit Review Bot 2012-06-19 12:20:39 PDT
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.
Comment 6 Martin Robinson 2012-06-19 13:38:38 PDT
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).
Comment 7 Kalev Lember 2012-06-19 14:39:04 PDT
(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.
Comment 8 Kalev Lember 2012-06-20 04:29:13 PDT
Created attachment 148536 [details]
Patch v2

Thanks for the review, I've updated the patch accordingly.
Comment 9 WebKit Review Bot 2012-06-21 12:20:15 PDT
Comment on attachment 148536 [details]
Patch v2

Clearing flags on attachment: 148536

Committed r120956: <http://trac.webkit.org/changeset/120956>
Comment 10 WebKit Review Bot 2012-06-21 12:20:21 PDT
All reviewed patches have been landed.  Closing bug.