Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1770693002:
export blink::Platform symbols in shared_library builds (Closed)
|
Created:
4 years, 9 months ago by Mostyn Bramley-Moore Modified:
4 years, 9 months ago CC:
blink-reviews, blink-reviews-api_chromium.org, chromium-reviews, dglazkov+blink Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionexport blink::Platform symbols in shared_library builds
This unbreaks GCC component=shared_library builds, which fail to link
libblink_platform.so due to missing vtable, since CL 1660383002 landed.
BUG=548254
Committed: https://crrev.com/e9f54102f094afa87dd22bdcbc26a5aec911e882
Cr-Commit-Position: refs/heads/master@{#379577}
Patch Set 1 : export all blink::Platform symbols #Patch Set 2 : don't export previously hidden blink::Platform symbols #Patch Set 3 : avoid visual studio error C2487 #
Total comments: 9
Patch Set 4 : just export all symbols from Platform #Messages
Total messages: 27 (11 generated)
The CQ bit was checked by mostynb@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1770693002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1770693002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by mostynb@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1770693002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1770693002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
mostynb@opera.com changed reviewers: + dcheng@chromium.org, hajimehoshi@chromium.org
@dcheng: can you please take a look at this GCC fix? Since this only affects component=shared_library builds, perhaps we could just export the class and remove the attributes from all the members instead?
https://codereview.chromium.org/1770693002/diff/40001/third_party/WebKit/publ... File third_party/WebKit/public/platform/Platform.h (right): https://codereview.chromium.org/1770693002/diff/40001/third_party/WebKit/publ... third_party/WebKit/public/platform/Platform.h:126: class BLINK_PLATFORM_EXPORT_CLASS Platform { Just use BLINK_PLATFORM_EXPORT https://codereview.chromium.org/1770693002/diff/40001/third_party/WebKit/publ... third_party/WebKit/public/platform/Platform.h:141: BLINK_PLATFORM_HIDDEN virtual WebCookieJar* cookieJar() { return nullptr; } Why do we need to mark these as hidden? Why not just leave them the way they are? https://codereview.chromium.org/1770693002/diff/40001/third_party/WebKit/publ... File third_party/WebKit/public/platform/WebCommon.h (right): https://codereview.chromium.org/1770693002/diff/40001/third_party/WebKit/publ... third_party/WebKit/public/platform/WebCommon.h:63: #define BLINK_PLATFORM_HIDDEN I wouldn't change these macros.
https://codereview.chromium.org/1770693002/diff/40001/third_party/WebKit/publ... File third_party/WebKit/public/platform/Platform.h (right): https://codereview.chromium.org/1770693002/diff/40001/third_party/WebKit/publ... third_party/WebKit/public/platform/Platform.h:126: class BLINK_PLATFORM_EXPORT_CLASS Platform { On 2016/03/07 04:42:08, dcheng wrote: > Just use BLINK_PLATFORM_EXPORT This was a workaround to keep visual studio happy, alternatively I think we can remote BLINK_PLATFORM_EXPORT from the members. https://codereview.chromium.org/1770693002/diff/40001/third_party/WebKit/publ... third_party/WebKit/public/platform/Platform.h:141: BLINK_PLATFORM_HIDDEN virtual WebCookieJar* cookieJar() { return nullptr; } On 2016/03/07 04:42:08, dcheng wrote: > Why do we need to mark these as hidden? Why not just leave them the way they > are? It's possible to do that, the only difference is that we might expose more global symbols from the shared library. https://codereview.chromium.org/1770693002/diff/40001/third_party/WebKit/publ... File third_party/WebKit/public/platform/WebCommon.h (right): https://codereview.chromium.org/1770693002/diff/40001/third_party/WebKit/publ... third_party/WebKit/public/platform/WebCommon.h:63: #define BLINK_PLATFORM_HIDDEN On 2016/03/07 04:42:08, dcheng wrote: > I wouldn't change these macros. OK. BTW, the presubmit hooks complain that macros should not be indented- should I fix that here, or will that be taken care of in a clang-format run later?
The CQ bit was checked by mostynb@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1770693002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1770693002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== export blink::Platform symbols This unbreaks GCC component=shared_library builds, which fail to link libblink_platform.so due to missing vtable, since CL 1660383002 landed. BUG=548254 ========== to ========== export blink::Platform symbols in shared_library builds This unbreaks GCC component=shared_library builds, which fail to link libblink_platform.so due to missing vtable, since CL 1660383002 landed. BUG=548254 ==========
lgtm https://codereview.chromium.org/1770693002/diff/40001/third_party/WebKit/publ... File third_party/WebKit/public/platform/Platform.h (right): https://codereview.chromium.org/1770693002/diff/40001/third_party/WebKit/publ... third_party/WebKit/public/platform/Platform.h:126: class BLINK_PLATFORM_EXPORT_CLASS Platform { On 2016/03/07 at 06:38:31, Mostyn Bramley-Moore wrote: > On 2016/03/07 04:42:08, dcheng wrote: > > Just use BLINK_PLATFORM_EXPORT > > This was a workaround to keep visual studio happy, alternatively I think we can remote BLINK_PLATFORM_EXPORT from the members. That's OK, the export macros were never really meant to hide symbols in a shared library build anyway.
Thanks. https://codereview.chromium.org/1770693002/diff/40001/third_party/WebKit/publ... File third_party/WebKit/public/platform/Platform.h (right): https://codereview.chromium.org/1770693002/diff/40001/third_party/WebKit/publ... third_party/WebKit/public/platform/Platform.h:126: class BLINK_PLATFORM_EXPORT_CLASS Platform { > That's OK, the export macros were never really meant to hide symbols in a shared > library build anyway. I'm curious- what else do GCC visibility attributes do?
The CQ bit was checked by mostynb@opera.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1770693002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1770693002/60001
Message was sent while issue was closed.
Description was changed from ========== export blink::Platform symbols in shared_library builds This unbreaks GCC component=shared_library builds, which fail to link libblink_platform.so due to missing vtable, since CL 1660383002 landed. BUG=548254 ========== to ========== export blink::Platform symbols in shared_library builds This unbreaks GCC component=shared_library builds, which fail to link libblink_platform.so due to missing vtable, since CL 1660383002 landed. BUG=548254 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== export blink::Platform symbols in shared_library builds This unbreaks GCC component=shared_library builds, which fail to link libblink_platform.so due to missing vtable, since CL 1660383002 landed. BUG=548254 ========== to ========== export blink::Platform symbols in shared_library builds This unbreaks GCC component=shared_library builds, which fail to link libblink_platform.so due to missing vtable, since CL 1660383002 landed. BUG=548254 Committed: https://crrev.com/e9f54102f094afa87dd22bdcbc26a5aec911e882 Cr-Commit-Position: refs/heads/master@{#379577} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/e9f54102f094afa87dd22bdcbc26a5aec911e882 Cr-Commit-Position: refs/heads/master@{#379577}
Message was sent while issue was closed.
https://codereview.chromium.org/1770693002/diff/40001/third_party/WebKit/publ... File third_party/WebKit/public/platform/Platform.h (right): https://codereview.chromium.org/1770693002/diff/40001/third_party/WebKit/publ... third_party/WebKit/public/platform/Platform.h:126: class BLINK_PLATFORM_EXPORT_CLASS Platform { On 2016/03/07 at 17:32:08, Mostyn Bramley-Moore wrote: > > That's OK, the export macros were never really meant to hide symbols in a shared > > library build anyway. > > I'm curious- what else do GCC visibility attributes do? I think this is the main thing (controlling symbol visibility). But in Chromium, we just use it so that the shared library build works.
Message was sent while issue was closed.
lgtm, thanks |
