Manage FreeType symbol visibility using platform specific FT_EXPORT

Manage FreeType symbol visibility using platform specific FT_EXPORT

MSVC does not support the -fvisibility-* option, hence providing
platform specific macros for library export visibility. Also, 
suppress unused function warnings on non-Windows platforms only.

BUG=700830
Review-Url: https://codereview.chromium.org/2738383002
Cr-Commit-Position: refs/heads/master@{#456390}
Committed: https://chromium.googlesource.com/chromium/src/+/567ff79ca421beccab0b5babdeef3d6d9b89b9ea

[drott, 2017-03-10 08:56:44]

The CQ bit was checked by drott@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-10 08:56:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-10 10:25:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-10 10:25:56]

Dry run: This issue passed the CQ dry run.

[drott, 2017-03-10 12:10:16]

The CQ bit was checked by drott@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-10 12:10:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-10 12:14:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-10 12:14:13]

Dry run: This issue passed the CQ dry run.

[drott, 2017-03-10 14:27:05]

Adjust visibility approach for windows

[drott, 2017-03-10 14:28:53]

Description was changed from

==========
Manage FreeType symbol visibility with ftconfig.h again

Go back to the old approach of exporting FreeType symbols by overriding
the FT_Export and FT_Export_Def macros in a local ftconfig.h copy. MSVC
does not support the -fvisibility-* options and I did not succeed
overriding the FT_Export* macros using BUILD.gn defines.

BUG=69876
==========

to

==========
Manage FreeType symbol visibility using platform specific FT_EXPORT

MSVC does not support the -fvisibility-* option, hence providing
platform specific macros for library export visibility.

BUG=69876
==========

[drott, 2017-03-10 14:29:18]

The CQ bit was checked by drott@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-10 14:29:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[drott, 2017-03-10 14:31:22]

Description was changed from

==========
Manage FreeType symbol visibility using platform specific FT_EXPORT

MSVC does not support the -fvisibility-* option, hence providing
platform specific macros for library export visibility.

BUG=69876
==========

to

==========
Manage FreeType symbol visibility using platform specific FT_EXPORT

MSVC does not support the -fvisibility-* option, hence providing
platform specific macros for library export visibility. Also, 
suppress warnings on non-Windows platforms only.

BUG=69876
==========

[drott, 2017-03-10 14:31:45]

Description was changed from

==========
Manage FreeType symbol visibility using platform specific FT_EXPORT

MSVC does not support the -fvisibility-* option, hence providing
platform specific macros for library export visibility. Also, 
suppress warnings on non-Windows platforms only.

BUG=69876
==========

to

==========
Manage FreeType symbol visibility using platform specific FT_EXPORT

MSVC does not support the -fvisibility-* option, hence providing
platform specific macros for library export visibility. Also, 
suppress unused function warnings on non-Windows platforms only.

BUG=69876
==========

[drott, 2017-03-10 14:40:27]

drott@chromium.org changed reviewers:
+ bungeman@chromium.org, dpranke@chromium.org

[commit-bot: I haz the power, 2017-03-10 15:27:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-10 15:27:46]

Dry run: This issue passed the CQ dry run.

[drott, 2017-03-10 15:37:14]

-Wno-unused-function for clang only

[drott, 2017-03-10 15:37:15]

The CQ bit was checked by drott@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-10 15:40:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-10 16:33:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-10 16:33:35]

Dry run: This issue passed the CQ dry run.

[bungeman-skia, 2017-03-10 16:50:44]

Description was changed from

==========
Manage FreeType symbol visibility using platform specific FT_EXPORT

MSVC does not support the -fvisibility-* option, hence providing
platform specific macros for library export visibility. Also, 
suppress unused function warnings on non-Windows platforms only.

BUG=69876
==========

to

==========
Manage FreeType symbol visibility using platform specific FT_EXPORT

MSVC does not support the -fvisibility-* option, hence providing
platform specific macros for library export visibility. Also, 
suppress unused function warnings on non-Windows platforms only.

BUG=skia:69876
==========

[bungeman-skia, 2017-03-10 16:52:00]

Description was changed from

==========
Manage FreeType symbol visibility using platform specific FT_EXPORT

MSVC does not support the -fvisibility-* option, hence providing
platform specific macros for library export visibility. Also, 
suppress unused function warnings on non-Windows platforms only.

BUG=skia:69876
==========

to

==========
Manage FreeType symbol visibility using platform specific FT_EXPORT

MSVC does not support the -fvisibility-* option, hence providing
platform specific macros for library export visibility. Also, 
suppress unused function warnings on non-Windows platforms only.

BUG=69876
==========

[bungeman-skia, 2017-03-10 17:02:55]

bungeman@google.com changed reviewers:
+ bungeman@google.com

[bungeman-skia, 2017-03-10 17:02:56]

I'm; not sure what the BUG in the current description is, it seems to go
somewhere random?

https://codereview.chromium.org/2738383002/diff/40001/third_party/freetype/in...
File third_party/freetype/include/freetype-custom-config/ftoption.h (right):

https://codereview.chromium.org/2738383002/diff/40001/third_party/freetype/in...
third_party/freetype/include/freetype-custom-config/ftoption.h:314: #define
FT_EXPORT_DEF(x) __declspec(dllexport) x
How are you getting away with using only dllexport and not dllimport? Are all
the trybots non-component builds? Usually on Windows one sees something more
like https://lists.nongnu.org/archive/html/freetype/2009-05/msg00004.html which
also takes into account that in a non-component build you don't want to export
anything (Chromium doesn't want to expose the FreeType symbols in a
non-component build if it can be avoided).

[bungeman-skia, 2017-03-10 17:50:08]

Maybe something like

#if ?windows? && ?component_build?
#if FT2_BUILD_LIBRARY
#define FT_EXPORT(x) extern __declspec(dllexport) x
#else
#define FT_EXPORT(x) extern __declspec(dllimport) x
#endif
#endif

[drott, 2017-03-13 10:12:18]

Improve windows visibility matching

[drott, 2017-03-13 10:12:22]

The CQ bit was checked by drott@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-13 10:12:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[drott, 2017-03-13 10:17:17]

On 2017/03/10 at 17:50:08, bungeman wrote:
> Maybe something like
> 
> #if ?windows? && ?component_build?
> #if FT2_BUILD_LIBRARY
> #define FT_EXPORT(x) extern __declspec(dllexport) x
> #else
> #define FT_EXPORT(x) extern __declspec(dllimport) x
> #endif
> #endif

Yes, slightly different now: 
if ?windows?

// do MSVC style markup
if ?component?
if ?inside freetype?
// dllexport
else
// dllimport
endif
endif // if component

else
// set gcc attribute
endif

[drott, 2017-03-13 10:17:37]

Thanks for the review, hope this works now.

https://codereview.chromium.org/2738383002/diff/40001/third_party/freetype/in...
File third_party/freetype/include/freetype-custom-config/ftoption.h (right):

https://codereview.chromium.org/2738383002/diff/40001/third_party/freetype/in...
third_party/freetype/include/freetype-custom-config/ftoption.h:314: #define
FT_EXPORT_DEF(x) __declspec(dllexport) x
On 2017/03/10 at 17:02:56, bungeman-skia wrote:
> How are you getting away with using only dllexport and not dllimport? Are all
the trybots non-component builds? Usually on Windows one sees something more
like https://lists.nongnu.org/archive/html/freetype/2009-05/msg00004.html which
also takes into account that in a non-component build you don't want to export
anything (Chromium doesn't want to expose the FreeType symbols in a
non-component build if it can be avoided).

Thanks for pointing this out. I am not very familiar with Windows linking, but
according to https://msdn.microsoft.com/en-us/library/aa271769(v=vs.60).aspx I
can get away without dllimport at the expense of an extra redirect thunk, if I
understood correctly. In the updated patch I try to match dllexport/dllimport if
it's a component build and don't add any extra qualifications when it's static
linkage. On non Windows, I add the gcc-style visibility attribute.

[commit-bot: I haz the power, 2017-03-13 11:07:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-13 11:07:01]

Dry run: This issue passed the CQ dry run.

[bungeman-chromium, 2017-03-13 14:56:46]

I haven't had a chance to actually test drive it yet, but lgtm.

[drott, 2017-03-13 15:49:29]

The CQ bit was checked by drott@chromium.org

[commit-bot: I haz the power, 2017-03-13 15:49:59]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[drott, 2017-03-13 15:50:15]

The CQ bit was unchecked by drott@chromium.org

[drott, 2017-03-13 15:50:50]

Description was changed from

==========
Manage FreeType symbol visibility using platform specific FT_EXPORT

MSVC does not support the -fvisibility-* option, hence providing
platform specific macros for library export visibility. Also, 
suppress unused function warnings on non-Windows platforms only.

BUG=69876
==========

to

==========
Manage FreeType symbol visibility using platform specific FT_EXPORT

MSVC does not support the -fvisibility-* option, hence providing
platform specific macros for library export visibility. Also, 
suppress unused function warnings on non-Windows platforms only.

BUG=700830
==========

[drott, 2017-03-13 15:51:26]

Ah, and I update the bug number now. The previous one was lacking a 3 at the end
from a copy and paste mistake, and was pointing to one of the FreeType
unification bugs.

[drott, 2017-03-13 15:51:31]

The CQ bit was checked by drott@chromium.org

[commit-bot: I haz the power, 2017-03-13 15:52:08]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-13 15:55:50]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1489420291427590,
"parent_rev": "aaa07f46771138c6b4f554ee34dba359ebd68813", "commit_rev":
"567ff79ca421beccab0b5babdeef3d6d9b89b9ea"}

[commit-bot: I haz the power, 2017-03-13 15:56:53]

Description was changed from

==========
Manage FreeType symbol visibility using platform specific FT_EXPORT

MSVC does not support the -fvisibility-* option, hence providing
platform specific macros for library export visibility. Also, 
suppress unused function warnings on non-Windows platforms only.

BUG=700830
==========

to

==========
Manage FreeType symbol visibility using platform specific FT_EXPORT

MSVC does not support the -fvisibility-* option, hence providing
platform specific macros for library export visibility. Also, 
suppress unused function warnings on non-Windows platforms only.

BUG=700830

Review-Url: https://codereview.chromium.org/2738383002
Cr-Commit-Position: refs/heads/master@{#456390}
Committed:
https://chromium.googlesource.com/chromium/src/+/567ff79ca421beccab0b5babdeef...
==========

[commit-bot: I haz the power, 2017-03-13 15:56:55]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/567ff79ca421beccab0b5babdeef...

[bungeman-chromium, 2017-03-13 16:09:23]

On the other hand, we're not even DEPSing in FreeType when on Windows, and if we
were we're not currently building it. So the trybots aren't really trying
anything here, which explains why all the builds work (nothing really changed).

I updated issue 2682803002 to add the DEPS and also build, and it looks like it
should work, but I haven't had time to test it yet.

[drott, 2017-03-13 16:12:59]

On 2017/03/13 at 16:09:23, bungeman wrote:
> On the other hand, we're not even DEPSing in FreeType when on Windows, and if
we were we're not currently building it. So the trybots aren't really trying
anything here, which explains why all the builds work (nothing really changed).
> 
> I updated issue 2682803002 to add the DEPS and also build, and it looks like
it should work, but I haven't had time to test it yet.

I've tried (built & tested) it locally, plus some additional changes. That's why
I was coming to the conclusion that we need a change in how we manage the
visibility. The results are listed in
https://bugs.chromium.org/p/chromium/issues/detail?id=700926 . I plan to
update/incorporate 2682803002 as soon as possible, but it's blocked on PDFium
rolling FreeType atm. Thanks for the review and your help in testing this CL if
you plan to try it locally.