Make classes for union types exported.

Make classes for union types exported.

BUG=358074
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=193016

[tasak, 2015-04-01 05:58:04]

tasak@google.com changed reviewers:
+ bashi@chromium.org, haraken@chromium.org

[tasak, 2015-04-01 05:58:05]

PTAL?

[haraken, 2015-04-01 07:03:13]

LGTM

https://codereview.chromium.org/1054443002/diff/1/Source/bindings/templates/u...
File Source/bindings/templates/union.h (right):

https://codereview.chromium.org/1054443002/diff/1/Source/bindings/templates/u...
Source/bindings/templates/union.h:34: #if COMPILER(MSVC) &&
defined(COMPONENT_BUILD) && LINK_CORE_MODULES_SEPARATELY

Add a comment on this.

[bashi, 2015-04-01 07:16:51]

LGTM

https://codereview.chromium.org/1054443002/diff/1/Source/bindings/scripts/v8_...
File Source/bindings/scripts/v8_union.py (right):

https://codereview.chromium.org/1054443002/diff/1/Source/bindings/scripts/v8_...
Source/bindings/scripts/v8_union.py:30: def union_context(union_types,
interfaces_info, component):
nit: we generally put component-specific logic in code_generator_v8.py. For
consistency I slightly prefer to follow the current convention for now.

That said, I think that we eventually want to integrate such component-specific
logic into a specific place (e.g. ComponentInfo) so this is just a nit.

https://codereview.chromium.org/1054443002/diff/1/Source/bindings/scripts/v8_...
Source/bindings/scripts/v8_union.py:54: header_includes.add('%s/%sExport.h' %
(component, component.capitalize()))
nit: how about adding ComonentInfo.include_path_for_export like
include_path_for_union_types?

https://codereview.chromium.org/1054443002/diff/1/Source/bindings/scripts/v8_...
Source/bindings/scripts/v8_union.py:63: 'exported': '%s_EXPORT ' %
component.upper(),
nit: alphabetical order.

https://codereview.chromium.org/1054443002/diff/1/Source/bindings/scripts/v8_...
Source/bindings/scripts/v8_union.py:63: 'exported': '%s_EXPORT ' %
component.upper(),
nit: I'd prefer dropping the trailing space and adding spaces in templates.

[tasak, 2015-04-02 05:26:29]

Patchset #2 (id:20001) has been deleted

[tasak, 2015-04-02 05:30:16]

Patchset #2 (id:40001) has been deleted

[tasak, 2015-04-02 05:31:37]

Thank you for review.

https://codereview.chromium.org/1054443002/diff/1/Source/bindings/scripts/v8_...
File Source/bindings/scripts/v8_union.py (right):

https://codereview.chromium.org/1054443002/diff/1/Source/bindings/scripts/v8_...
Source/bindings/scripts/v8_union.py:30: def union_context(union_types,
interfaces_info, component):
On 2015/04/01 07:16:51, bashi1 wrote:
> nit: we generally put component-specific logic in code_generator_v8.py. For
> consistency I slightly prefer to follow the current convention for now.
> 
> That said, I think that we eventually want to integrate such
component-specific
> logic into a specific place (e.g. ComponentInfo) so this is just a nit.

Done.

https://codereview.chromium.org/1054443002/diff/1/Source/bindings/scripts/v8_...
Source/bindings/scripts/v8_union.py:54: header_includes.add('%s/%sExport.h' %
(component, component.capitalize()))
On 2015/04/01 07:16:51, bashi1 wrote:
> nit: how about adding ComonentInfo.include_path_for_export like
> include_path_for_union_types?

Done.

https://codereview.chromium.org/1054443002/diff/1/Source/bindings/scripts/v8_...
Source/bindings/scripts/v8_union.py:63: 'exported': '%s_EXPORT ' %
component.upper(),
On 2015/04/01 07:16:51, bashi1 wrote:
> nit: I'd prefer dropping the trailing space and adding spaces in templates.

Temporarily I would like to use the trailing space.

https://codereview.chromium.org/1054443002/diff/1/Source/bindings/scripts/v8_...
Source/bindings/scripts/v8_union.py:63: 'exported': '%s_EXPORT ' %
component.upper(),
On 2015/04/01 07:16:51, bashi1 wrote:
> nit: alphabetical order.

Moved 'exported' in the almost same way as ComponentInfo.

https://codereview.chromium.org/1054443002/diff/1/Source/bindings/templates/u...
File Source/bindings/templates/union.h (right):

https://codereview.chromium.org/1054443002/diff/1/Source/bindings/templates/u...
Source/bindings/templates/union.h:34: #if COMPILER(MSVC) &&
defined(COMPONENT_BUILD) && LINK_CORE_MODULES_SEPARATELY
On 2015/04/01 07:03:13, haraken wrote:
> 
> Add a comment on this.

Done.

[haraken, 2015-04-02 09:13:09]

LGTM

https://codereview.chromium.org/1054443002/diff/60001/Source/bindings/scripts...
File Source/bindings/scripts/utilities.py (right):

https://codereview.chromium.org/1054443002/diff/60001/Source/bindings/scripts...
Source/bindings/scripts/utilities.py:167: return 'modules/ModulesExport.h'

I'd propose introducing wtf/Export.h (and remove CoreExport.h, ModulesExport.h
and PlatformExport.h) so that caller sites don't worry about what Export.h they
need to include. A follow-up CL is fine.

https://codereview.chromium.org/1054443002/diff/60001/Source/bindings/templat...
File Source/bindings/templates/union.h (right):

https://codereview.chromium.org/1054443002/diff/60001/Source/bindings/templat...
Source/bindings/templates/union.h:34: #if COMPILER(MSVC) &&
defined(COMPONENT_BUILD) && LINK_CORE_MODULES_SEPARATELY

It would be better to change LINK_CORE_MODULES_SEPARATELY to a gyp flag and
remove defined(COMPONENT_BUILD) from here. A follow-up CL is fine.

[tasak, 2015-04-02 09:29:55]

The CQ bit was checked by tasak@google.com

[tasak, 2015-04-02 09:29:55]

The patchset sent to the CQ was uploaded after l-g-t-m from bashi@chromium.org
Link to the patchset: https://codereview.chromium.org/1054443002/#ps60001
(title: " ")

[commit-bot: I haz the power, 2015-04-02 09:30:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1054443002/60001

[commit-bot: I haz the power, 2015-04-02 09:33:48]

Committed patchset #2 (id:60001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=193016