Closed Bug 849207 Opened 7 years ago Closed 7 years ago

Use __declspec(dllexport) for exporting skia C++ symbols.


(Core :: Graphics, defect)

Windows 7
Not set





(Reporter: jacek, Assigned: jacek)




(1 file, 1 obsolete file)

Attached patch fix (obsolete) — Splinter Review
It's never a good idea to export C++ by .def file, because it requires hardcoding mangles names (it already broke win64 after landing). GCC (g++) mangles names differently, so this also breaks mingw builds. Using __declspec(dllexport/dllimport) seems like a nicer solution in this case. The attached patch implements that. I decided to use SKIA_IMPLEMENTATION/GR_IMPLEMENTATION for that purpose, since that's what skia uses in normal (as in not m-c import) builds. Also I didn't include a patch file for gfx/skia/patches, I'd prefer to add that after review.
Attachment #722769 - Flags: review?(gwright)
Does this actually work? When I tried doing pretty much the exact same thing, I got failures on try...
My first attempt failed on try, here is new one, with a minor change (it still runs):
As an aside, I'm not sure I'm happy with putting changes into Skia that aren't upstreamable. What would be the downside of just building with SKIA_DLL everywhere, and SKIA_IMPLEMENTATION when building gfx/skia, like in bug 787225?
(ie - just export all public APIs as upstream intended)
I think that should work. The downside would be exporting much more symbols from gkmedias.dll than we need. I'm not sure how much we should care (I assumed we do, that's why I did the patch the way it is). If you prefer the other solution, I will change the patch.
My policy for the Skia tree is to try to minimise the number of patches applied against upstream that aren't upstreamable. I would prefer exporting all symbols in that respect, but my mind can be swayed if there's a significant impact in performance.
I'm OK with this.
Comment on attachment 722769 [details] [diff] [review]

Let's go with exposing all public API functions, as per bug 787225.
Attachment #722769 - Flags: review?(gwright) → review-
Got a test patch working its way through try right now.
Thanks for the patch. It failed, but it just needs a few more changes. Here is my attempt to address them:

It changes two thing.

- It adds SKIA_IMPLEMENTATION and GR_IMPLEMENTATION to gfx/2d. It ends up in the same dll as skia and as such it should use the same declarations (AFAIR mingw is more picky about that than MSVC).

- It adds SK_API to SkString so that the class is exported. It seems strange that upstream doesn't do that. I will try to see if there is a reason later. If there is not, that part should be upstreamable.
Attached patch fix v2Splinter Review
Previous try commit succeeded. I took another look at SkString and it came out that the only use case is a helper in GLContextSkia.cpp (as in no API call strictly requires SkString). This is easy to replace by plain C string operations, avoiding the need to patch upstream skia code.
Attachment #722769 - Attachment is obsolete: true
Attachment #723471 - Flags: review?(gwright)
Comment on attachment 723471 [details] [diff] [review]
fix v2

SkString is actually SK_API declared upstream now, so the changes to GLContextSkia are unnecessary. However, I'm also fine with this change.
Attachment #723471 - Flags: review?(gwright) → review+
Thanks for the review. This orange is also already present on m-c, so it's unrelated.
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
You need to log in before you can comment on or make changes to this bug.