Bug 43204 – [GTK] Add symbol export macros to C++ DOM bindings

• Component: WebCore Misc.
• Status: RESOLVED INVALID

[Kevin Ollivier]

The C++ DOM bindings currently don't have macros to control their export, so on Windows or when using visibility hidden with GCC, these symbols won't be exposed by the WebKit shared library.

[Kevin Ollivier]

Created attachment 62976 [details]
Adds export macro to C++ DOM bindings.

[Nikolas Zimmermann]

Comment on attachment 62976 [details]
Adds export macro to C++ DOM bindings.

In general this looks fine, but I think you should better align the macros with the versions in JavaScriptCore/API/JSBase.h.
See JS_EXPORT as example, maybe add a comment to both files that the logic should stay in sync. The declspec(dllimport) vs. dllexport logic looks reversed compared to JSBase.h, is this intentional?

You should also grep if there are any other variations of these or similar macros throughout WebKit (WebCore/JavaScriptCore/WebKit).

[Kevin Ollivier]

(In reply to comment #2)
> (From update of attachment 62976 [details])
> In general this looks fine, but I think you should better align the macros with the versions in JavaScriptCore/API/JSBase.h.
> See JS_EXPORT as example, maybe add a comment to both files that the logic should stay in sync. The declspec(dllimport) vs. dllexport logic looks reversed compared to JSBase.h, is this intentional?

I think I actually followed the macros in JavaScriptCore/config.h as a base since we use default symbol visibility on GCC and thus only needed Windows support. I added the GCC code later since I figured we should handle the case where people have hidden symbols by default on GCC. Changing the order, of course, is no problem (and avoids a extra check on defined(GCC)), and I'll go ahead and add the note too.

> You should also grep if there are any other variations of these or similar macros throughout WebKit (WebCore/JavaScriptCore/WebKit).

Yeah, actually, I probably know about every place in the code that such defines exist at this point, as I've had to delve into this quite a bit while working on #27551. :)

After getting #27551 taken care of, what I'd actually like to do is come up with something like SYMBOL_EXPORT, defined using the JSBase.h logic, and have projects define WHATEVER_EXPORT to SYMBOL_EXPORT based whenever a symbol should be exported. The big question is where to define SYMBOL_EXPORT, though, in a way that it's pretty much universally available.

[Kevin Ollivier]

Created attachment 62999 [details]
Updated patch implementing suggestions by Nikolas

[Nikolas Zimmermann]

Comment on attachment 62999 [details]
Updated patch implementing suggestions by Nikolas

Patch looks fine: Though I'd suggest to add a new wtf/VisibilityMacros.h file or (wtf/ExportControl.h whatever you can come up with), that tries to unifies the existing macros.
I'd feel more comfortable reviewing such a patch...

What do you think? Can you try it?

[Kevin Ollivier]

(In reply to comment #5)
> (From update of attachment 62999 [details])
> Patch looks fine: Though I'd suggest to add a new wtf/VisibilityMacros.h file or (wtf/ExportControl.h whatever you can come up with), that tries to unifies the existing macros.
> I'd feel more comfortable reviewing such a patch...
> 
> What do you think? Can you try it?

I would like to, at least at some point, but this issue is thornier than it looks on the surface. (Something I learned in #27551.) To use these macros pretty consistently throughout the codebase, we need to know for each ports which headers they allow a particular project to include, which headers they may (or may not) copy over from JSCore, etc., and we may also need to modify at least a couple of the ports' build systems, particularly for things like JSCore's testAPI, which is supposed to only be able to access JSCore public API (JavaScriptCore/API) headers. As I was somewhat surprised to discover, several ports treat wtf as an internal implementation detail of JSCore, so in certain areas including headers from it will break some intentionally designed public / private separation. This is part of the reason we've ended up with several duplications of these macros in the first place, and part of the reason that I'm not 100% clear on how I'd even go about implementing the solution.

Given the issues involved, I think the appropriate thing to do here is to file a new bug on the consistency issue and perhaps get some feedback on the idea and suggestions on how to handle it best. I think we should consider this patch by itself for now.

[Adam Barth]

Comment on attachment 62999 [details]
Updated patch implementing suggestions by Nikolas

Please unify the preprocessor defines with JSC using WTF.  Code everywhere can depend on WTF.  We use it all over the place.  You just have to play some tricks with forwarding headers to get things to work.

[Martin Robinson]

Pretty sure the C++ DOM bindings are gone now.