Change the way _EXPORT macros look.

Change the way _EXPORT macros look.

With the current setup, if you have a header file my_class.h

class BASE_EXPORT MyClass {
 public:
  void MyInlineMethod() { /* do stuff, inline */ }
};

then every cc file that includes my_class.h will have a public symbol
for MyInlineMethod (because inline methods need to be emitted to
every translation unit, and the linker sorts them out). With the
components build, the linker can't decide to drop these inline methods,
so every .so that uses this header file will have the same public symbol.

With this proposed change, the symbol will only be visible in the target
the header file belongs to, and it will be hidden in all other components.
That's cleaner, and it also prevents accident hidden dependencies (say
target A depends on B, and B depends on C. A accidentally uses an inline
function from a class in C. With this change, that would result in a linker
error, and an explicit dependency from A on C would have to be added).

Also add a missing CHROMEOS_IMPLEMENTATION define which went
unnoticed until now.

BUG=90078
TEST=Things still build.
TBR=ben, tony, viettrungluu, thestig, agl, willchan

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=137130

[Nico, 2012-05-14 20:23:13]

Mostly for evan.

[Evan Martin, 2012-05-15 04:27:44]

LGTM if it doesn't break anything.

On Mon, May 14, 2012 at 1:23 PM,  <thakis@chromium.org> wrote:
> Reviewers: rvargas, Evan Martin,
>
> Message:
> Mostly for evan.
>
> Description:
> Change the way _EXPORT macros look.
>
> With the current setup, if you have a header file my_class.h
>
> class BASE_EXPORT MyClass {
>  public:
>  void MyInlineMethod() { /* do stuff, inline */ }
> };
>
> then every cc file that includes my_class.h will have a public symbol
> for MyInlineMethod (because inline methods need to be emitted to
> every translation unit, and the linker sorts them out). With the
> components build, the linker can't decide to drop these inline methods,
> so every .so that uses this header file will have the same public symbol.
>
> With this proposed change, the symbol will only be visible in the target
> the header file belongs to, and it will be hidden in all other components.
> That's cleaner, and it also prevents accident hidden dependencies (say
> target A depends on B, and B depends on C. A accidentally uses an inline
> function from a class in C. With this change, that would result in a linker
> error, and an explicit dependency from A on C would have to be added).
>
> BUG=90078
> TEST=Things still build.
>
> Please review this at http://codereview.chromium.org/10386108/
>
> SVN Base: svn://svn.chromium.org/chrome/trunk/src
>
> Affected files:
>  M ash/ash_export.h
>  M base/base_export.h
>  M base/i18n/base_i18n_export.h
>  M chromeos/chromeos_export.h
>  M content/common/content_export.h
>  M crypto/crypto_export.h
>  M gpu/command_buffer/client/gles2_c_lib_export.h
>  M gpu/command_buffer/client/gles2_impl_export.h
>  M gpu/command_buffer/common/gles2_utils_export.h
>  M gpu/gpu_export.h
>  M ipc/ipc_export.h
>  M media/base/media_export.h
>  M net/base/net_export.h
>  M ppapi/proxy/ppapi_proxy_export.h
>  M ppapi/shared_impl/ppapi_shared_export.h
>  M ppapi/thunk/ppapi_thunk_export.h
>  M printing/printing_export.h
>  M sql/sql_export.h
>  M ui/aura/aura_export.h
>  M ui/base/ui_export.h
>  M ui/compositor/compositor_export.h
>  M ui/gfx/gl/gl_export.h
>  M ui/gfx/gl/gl_surface_cgl.h
>  M ui/oak/oak_export.h
>  M ui/surface/surface_export.h
>  M ui/views/views_export.h
>  M webkit/appcache/appcache_export.h
>  M webkit/blob/blob_export.h
>  M webkit/forms/webkit_forms_export.h
>  M webkit/glue/webkit_glue_export.h
>  M webkit/plugins/webkit_plugins_export.h
>
>

[commit-bot: I haz the power, 2012-05-15 04:45:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/thakis@chromium.org/10386108/1

[commit-bot: I haz the power, 2012-05-15 04:46:13]

Can't apply patch for file ui/gfx/gl/gl_export.h.
While running patch -p1 --forward --force;
A         ui/gfx/gl
patching file ui/gfx/gl/gl_export.h
Hunk #1 FAILED at 16.
1 out of 1 hunk FAILED -- saving rejects to file ui/gfx/gl/gl_export.h.rej

[Nico, 2012-05-15 04:51:43]

rebase

[commit-bot: I haz the power, 2012-05-15 04:52:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/thakis@chromium.org/10386108/4002

[commit-bot: I haz the power, 2012-05-15 04:53:03]

Presubmit check for 10386108-4002 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    ui/compositor
    base/i18n
    ash
    webkit/appcache
    ui/gl
    ppapi/proxy
    printing
    ui/surface
    crypto
    webkit/blob
    ui
    ppapi
    base
    webkit/plugins
    webkit
    ui/views
    net
    content/common
    webkit/forms
    ui/aura

Presubmit checks took 3.2s to calculate.

[Nico, 2012-05-15 05:00:18]

ben, tony, viettrungluu, thestig, agl, willchan: OWNERS tbr

[commit-bot: I haz the power, 2012-05-15 05:02:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/thakis@chromium.org/10386108/4002

[commit-bot: I haz the power, 2012-05-15 06:06:32]

The commit queue went berserk retrying too often for a
seemingly flaky test. Builder is android, revision is 137076, job name
was 10386108-4002 (previous was lost) (previous was lost) (previous was lost)
(previous was lost).

[commit-bot: I haz the power, 2012-05-15 06:08:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/thakis@chromium.org/10386108/4002

[commit-bot: I haz the power, 2012-05-15 07:11:56]

The commit queue went berserk retrying too often for a
seemingly flaky test. Builder is android, revision is 137081, job name
was 10386108-4002 (previous was lost) (previous was lost) (previous was lost)
(previous was lost).

[commit-bot: I haz the power, 2012-05-15 13:01:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/thakis@chromium.org/10386108/4002

[commit-bot: I haz the power, 2012-05-15 13:39:50]

Try job failure for 10386108-4002 (retry) on linux_chromeos for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[Nico, 2012-05-15 14:01:23]

cros

[commit-bot: I haz the power, 2012-05-15 14:02:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/thakis@chromium.org/10386108/1034

[Ben Goodger (Google), 2012-05-15 16:09:40]

lgtm

[tfarina, 2012-05-15 18:07:23]

https://chromiumcodereview.appspot.com/10386108/diff/1034/ui/aura/aura_export.h
File ui/aura/aura_export.h (right):

https://chromiumcodereview.appspot.com/10386108/diff/1034/ui/aura/aura_export...
ui/aura/aura_export.h:22: #if defined(AURA_IMPLEMENTATION)
nit: should we do the same to ui/app_list/app_list_export.h?

[Nico, 2012-05-15 18:15:42]

https://chromiumcodereview.appspot.com/10386108/diff/1034/ui/aura/aura_export.h
File ui/aura/aura_export.h (right):

https://chromiumcodereview.appspot.com/10386108/diff/1034/ui/aura/aura_export...
ui/aura/aura_export.h:22: #if defined(AURA_IMPLEMENTATION)
On 2012/05/15 18:07:24, tfarina wrote:
> nit: should we do the same to ui/app_list/app_list_export.h?

Yes. I tried to add it to all _export.h files. Maybe that file landed while this
CL was in review?

[tfarina, 2012-05-15 18:19:20]

On 2012/05/15 18:15:42, Nico wrote:
>
https://chromiumcodereview.appspot.com/10386108/diff/1034/ui/aura/aura_export.h
> File ui/aura/aura_export.h (right):
> 
>
https://chromiumcodereview.appspot.com/10386108/diff/1034/ui/aura/aura_export...
> ui/aura/aura_export.h:22: #if defined(AURA_IMPLEMENTATION)
> On 2012/05/15 18:07:24, tfarina wrote:
> > nit: should we do the same to ui/app_list/app_list_export.h?
> 
> Yes. I tried to add it to all _export.h files.
Should I send a patch for review?

> Maybe that file landed while this CL was in review?
It's a bit old already, it's there since May 9.

[tfarina, 2012-05-15 18:20:36]

On Tue, May 15, 2012 at 3:19 PM,  <tfarina@chromium.org> wrote:
>> Yes. I tried to add it to all _export.h files.
>
> Should I send a patch for review?
>
Ah, I see that you are already fixing it.


-- 
Thiago