Bug 24688

Summary: Build fix Symbian; clean Up WebKit/Qt if ENABLE_NETSCAPE_PLUGIN_API = 0
Product: WebKit Reporter: Laszlo Gombos <laszlo.gombos>
Component: WebKit QtAssignee: Simon Hausmann <hausmann>
Status: RESOLVED FIXED    
Severity: Normal CC: hausmann, norbert.leser, yael, zecke
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: All   
Attachments:
Description Flags
Proposed fix
none
Revised patch eric: review+

Description Laszlo Gombos 2009-03-18 17:08:32 PDT
ChangeSet r38116 sets ENABLE_NETSCAPE_PLUGIN_API to 0 for Symbian builds; however the ChangeSet fails to turn on the plugin stubs for Symbian (in qt/TemporaryLinkStubs.cpp), so QtLauncher for Symbian fails to link with missing symbols.

In addition ChangeSet r41528 introduced the ability to set ENABLE_NETSCAPE_PLUGIN_API to 0 for all the platforms (including Mac, Win and Linux), but those platforms that enable NETSCAPE_PLUGIN_API support by default will have the same linking problem as the Symbian build.

A patch will follow.
Comment 1 Laszlo Gombos 2009-03-18 17:12:50 PDT
Created attachment 28740 [details]
Proposed fix

Clean ups in addition to fixing the bugs mentioned in the report)
 - All WebKit ports should now share Netscape plugin stubs (introduced by ChangeSet r41786)
 - The decision about Netscape plugin support for Qt is now in one place only (WebCore.pro) - it use to be in the source code as well, and the two were out-of-sync
Comment 2 Simon Hausmann 2009-03-27 09:41:28 PDT
Lazlo, I just tried to build with the patch applied (before landing) and I noticed that it doesn't work entirely:

tmp/PluginViewQt.o: In function `WebCore::PluginView::userAgentStatic()':
PluginViewQt.cpp:(.text+0x60): multiple definition of `WebCore::PluginView::userAgentStatic()'
tmp/TemporaryLinkStubs.o:TemporaryLinkStubs.cpp:(.text+0x0): first defined here
tmp/PluginViewQt.o: In function `WebCore::PluginView::getValueStatic(NPNVariable, void*)':
PluginViewQt.cpp:(.text+0x70): multiple definition of `WebCore::PluginView::getValueStatic(NPNVariable, void*)'
tmp/TemporaryLinkStubs.o:TemporaryLinkStubs.cpp:(.text+0x10): first defined here

I think we may still need the old defines around the two remaining functions in TemporaryLinkStubs.cpp. What do you think?
Comment 3 Laszlo Gombos 2009-03-27 10:58:22 PDT
Created attachment 29014 [details]
Revised patch

Apologies, I do not know how could I miss that.

If I'm not mistaken we can just remove those 2 functions from TemporaryLinkStubs and make the code even simpler.
Comment 4 Eric Seidel (no email) 2009-04-29 14:46:29 PDT
Comment on attachment 28740 [details]
Proposed fix

bugzilla doesn't seem smart enough to remove things from the commit queue when obsolete.  or at least our commit queue query isn't smart enough.
Comment 5 Laszlo Gombos 2009-05-04 12:19:25 PDT
Simon, Can you please review this or assign it to someone else to review ? Thanks, Laszlo.
Comment 6 Eric Seidel (no email) 2009-05-11 06:30:39 PDT
Comment on attachment 29014 [details]
Revised patch

Looks sane to me.
Comment 7 Holger Freyther 2009-05-11 09:22:42 PDT
Landed in r43493.