Bug 77255 – Compilation warning building TestNetscapePlugin

• Status: RESOLVED
• Resolution: INVALID
• Component: Tools / Tests

[Martin Robinson]

I see this when building the GTK+ port on my machine:

  CXX    Tools/DumpRenderTree/TestNetscapePlugIn/TestNetscapePlugin_libtestnetscapeplugin_la-main.lo
../../Tools/DumpRenderTree/TestNetscapePlugIn/PluginObject.cpp: In function ‘void pluginLogWithWindowObjectVariableArgs(NPObject*, NPP, const char*, ...)’:

[Martin Robinson]

Created attachment 124392 [details]
Patch

[Martin Robinson]

I missed the warning!


../../Tools/DumpRenderTree/TestNetscapePlugIn/PluginObject.cpp: In function ‘void pluginLog(NPP, const char*, ...)’:
../../Tools/DumpRenderTree/TestNetscapePlugIn/PluginObject.cpp:100:50: warning: function might be possible candidate for ‘gnu_printf’ format attribute [-Wmissing-format-attribute]

[WebKit Review Bot]

Comment on attachment 124392 [details]
Patch

Attachment 124392 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11350906

[Martin Robinson]

Created attachment 124397 [details]
Try to fix the Chromium build

[WebKit Review Bot]

Comment on attachment 124397 [details]
Try to fix the Chromium build

Attachment 124397 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11350917

[Early Warning System Bot]

Comment on attachment 124397 [details]
Try to fix the Chromium build

Attachment 124397 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11349910

[Martin Robinson]

Created attachment 124442 [details]
Try to fix the builds round 2

[Martin Robinson]

Created attachment 124444 [details]
Try to fix the windows build

[Martin Robinson]

Created attachment 124445 [details]
Try once more to fix the Windows build

[Adam Roben (:aroben)]

Comment on attachment 124445 [details]
Try once more to fix the Windows build

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

> Tools/DumpRenderTree/TestNetscapePlugIn/win/TestNetscapePluginCommon.vsprops:9
> +		AdditionalIncludeDirectories=""$(ProjectDir)";"$(ProjectDir)..";"$(ConfigurationBuildDir)\Include";"$(ConfigurationBuildDir)\Include\JavaScriptCore";"$(ConfigurationBuildDir)\Include\WebCore\ForwardingHeaders";"$(ConfigurationBuildDir)\Include\DumpRenderTree\ForwardingHeaders";"$(WebKitLibrariesDir)\include""

I think instead of adding this include directory you need to add $(ConfigurationBuildDir)\Include\private\JavaScriptCore

[Martin Robinson]

Created attachment 125155 [details]
Patch

[Martin Robinson]

Created attachment 125161 [details]
Also include the parent directory this time

[Martin Robinson]

Created attachment 125190 [details]
Remove dependency on FastMalloc

[Martin Robinson]

(In reply to comment #13)
> Created an attachment (id=125190) [details]
> Remove dependency on FastMalloc

It seems I have managed to satisy n+1 build systems via clumsy thrashing around and help from friendly community members, so I will land this now. Thank you!

[Eric Seidel (no email)]

Comment on attachment 125190 [details]
Remove dependency on FastMalloc

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

> Tools/ChangeLog:15
> +        * DumpRenderTree/TestNetscapePlugIn/config.h: Added. Copied from DumpRenderTree.

Why do we need a config.h at all?

> Tools/DumpRenderTree/DumpRenderTree.gyp/DumpRenderTree.gyp:288
> +                '<(source_dir)/JavaScriptCore/wtf', # wtf/text/*.h refers headers in wtf/ without wtf/.

I'm about to fix that. :)

> Tools/DumpRenderTree/TestNetscapePlugIn/config.h:2
> + * Copyright (C) 2008 Nuanti Ltd.

??

> Tools/DumpRenderTree/TestNetscapePlugIn/config.h:21
> +#define Config_H

Given how many config.h's we have, seems makign this TNP_Config_h makes more sense.

[Martin Robinson]

(In reply to comment #15)
> (From update of attachment 125190 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=125190&action=review
> 
> > Tools/ChangeLog:15
> > +        * DumpRenderTree/TestNetscapePlugIn/config.h: Added. Copied from DumpRenderTree.
> 
> Why do we need a config.h at all?

config.h handles the proper includes and defines to use WTF. There seems to be a lot of manual setup before inclusion of the WTF headers.

> > Tools/DumpRenderTree/DumpRenderTree.gyp/DumpRenderTree.gyp:288
> > +                '<(source_dir)/JavaScriptCore/wtf', # wtf/text/*.h refers headers in wtf/ without wtf/.
> 
> I'm about to fix that. :)

Great. :)

> > Tools/DumpRenderTree/TestNetscapePlugIn/config.h:2
> > + * Copyright (C) 2008 Nuanti Ltd.
> 
> ??

Since I copied the config.h from DumpRenderTree, I had to preserve the copyright there. We cannot use the same config.h becaues the one in DumpRenderTree activates FastMalloc. Another approach to this whole mess would be to simply duplicate the WTF_ATTRIBUTE_PRINTF macros in TestNetscapePlugin instead of relying on the versions in WTF. That may make the patch simpler.

> > Tools/DumpRenderTree/TestNetscapePlugIn/config.h:21
> > +#define Config_H
> 
> Given how many config.h's we have, seems makign this TNP_Config_h makes more sense.

Okay. I'll do this based on your opinion on my question above.

[Ryosuke Niwa]

Isn't this patch obsolete now?

[Martin Robinson]

I think I'll try a less intrusive approach that simply adds the macro conditionally when compiling WebKitGTK+. Introducing the WTF headers and config.h into TestNetscapePlugin seems a bit overkill on consideration.

[Martin Robinson]

I don't see these errors any longer. I think they've been fixed.