Fix Windows build warning LNK4217 for blink_platform.dll.

Fix Windows build warning LNK4217 for blink_platform.dll.

[1] third_party/WebKit/public:offscreen_canvas_mojo_bindings_blink
  ==> [2] cc/ipc:interfaces_blink
    ==> [3] gpu/ipc/common:interfaces_blink
The above three targets are all linked into blink_platform.dll,
but [2] and [3] use gfx.mojom.Size typemap defined in
third_party/WebKit/Source/platform/mojo/GeometryStructTraits.{cpp,h},
which is exported from bink_platform.dll.
Thus, generated C++ bindings of [2] and [3] include GeometryStructTraits.h
while themselves are part of blink_platform.dll, means they are
importing(dllimport) local defined symbols marked as dllexport, caused LNK4217.

This CL moves this struct traits to a separate source_set target,
instead of exporting it from blink_platform.dll.

BUG=654776, 669352
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2574593003
Cr-Commit-Position: refs/heads/master@{#443994}
Committed: https://chromium.googlesource.com/chromium/src/+/4724c1ac34d346dd8b44c4abc33b344a744a2d69

[leonhsl(Using Gerrit), 2016-12-13 08:36:46]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-12-13 08:37:17]

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, 2016-12-13 10:25:17]

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

[commit-bot: I haz the power, 2016-12-13 10:25:18]

Dry run: This issue passed the CQ dry run.

[leonhsl(Using Gerrit), 2016-12-14 08:10:20]

Description was changed from

==========
Fix Windows build warning LNK4217 for blink_platform.dll.

BUG=669352
==========

to

==========
Fix Windows build warning LNK4217 for blink_platform.dll.

BUG=669352
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[leonhsl(Using Gerrit), 2016-12-14 09:44:53]

Description was changed from

==========
Fix Windows build warning LNK4217 for blink_platform.dll.

BUG=669352
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Fix Windows build warning LNK4217 for blink_platform.dll.

[1] third_party/WebKit/public:offscreen_canvas_mojo_bindings_blink
  ==> [2] cc/ipc:interfaces_blink
    ==> [3] gpu/ipc/common:interfaces_blink
The above three targets are all linked into blink_platform.dll,
but [2] and [3] use gfx.mojom.Size typemap defined in
third_party/WebKit/Source/platform/mojo/GeometryStructTraits.{cpp,h},
which is exported from bink_platform.dll.
Thus, generated C++ bindings of [2] and [3] include GeometryStructTraits.h
while themselves are part of blink_platform.dll, means they are
importing(dllimport) local defined symbols, so we must add the same
export macros to them to avoid LNK4217 warnings.

BUG=669352
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[leonhsl(Using Gerrit), 2016-12-14 10:09:47]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-12-14 10:10:00]

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

[leonhsl(Using Gerrit), 2016-12-14 10:11:05]

leon.han@intel.com changed reviewers:
+ jam@chromium.org, yzshen@chromium.org

[leonhsl(Using Gerrit), 2016-12-14 10:11:06]

Hi, would you PTAL at this? Thanks.

[commit-bot: I haz the power, 2016-12-14 12:04:31]

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

[commit-bot: I haz the power, 2016-12-14 12:04:32]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[leonhsl(Using Gerrit), 2016-12-14 13:15:56]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-12-14 13:16:09]

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, 2016-12-14 13:57:01]

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

[commit-bot: I haz the power, 2016-12-14 13:57:02]

Dry run: This issue passed the CQ dry run.

[yzshen1, 2016-12-14 17:19:34]

lgtm

[jam, 2016-12-14 17:53:39]

https://codereview.chromium.org/2574593003/diff/40001/cc/ipc/BUILD.gn
File cc/ipc/BUILD.gn (right):

https://codereview.chromium.org/2574593003/diff/40001/cc/ipc/BUILD.gn#newcode70
cc/ipc/BUILD.gn:70: export_class_attribute_blink = "BLINK_PLATFORM_EXPORT"
I don't understand this change and the next file.

Why isn't coe in cc/ using CC_EXPORT?

[yzshen1, 2016-12-14 18:09:53]

On 2016/12/14 17:53:39, jam wrote:
> https://codereview.chromium.org/2574593003/diff/40001/cc/ipc/BUILD.gn
> File cc/ipc/BUILD.gn (right):
> 
>
https://codereview.chromium.org/2574593003/diff/40001/cc/ipc/BUILD.gn#newcode70
> cc/ipc/BUILD.gn:70: export_class_attribute_blink = "BLINK_PLATFORM_EXPORT"
> I don't understand this change and the next file.
> 
> Why isn't coe in cc/ using CC_EXPORT?

I think the reason is these generated files are compiled into the blink platform
target.
If we want to make these files a separate target that blink platform depends on,
we will need to move some typemapping related code out of blink platform as
well. That seems possible  in this case.

[leonhsl(Using Gerrit), 2016-12-15 03:06:49]

On 2016/12/14 18:09:53, yzshen1 wrote:
> On 2016/12/14 17:53:39, jam wrote:
> > https://codereview.chromium.org/2574593003/diff/40001/cc/ipc/BUILD.gn
> > File cc/ipc/BUILD.gn (right):
> > 
> >
>
https://codereview.chromium.org/2574593003/diff/40001/cc/ipc/BUILD.gn#newcode70
> > cc/ipc/BUILD.gn:70: export_class_attribute_blink = "BLINK_PLATFORM_EXPORT"
> > I don't understand this change and the next file.
> > 
> > Why isn't coe in cc/ using CC_EXPORT?
> 
> I think the reason is these generated files are compiled into the blink
platform
> target.
> If we want to make these files a separate target that blink platform depends
on,
> we will need to move some typemapping related code out of blink platform as
> well. That seems possible  in this case.

Hi John,
These 2 mojom targets(cc/ipc:interfaces_blink and
gpu/ipc/common:interfaces_blink):
   - are source_set,
   - are linked into blink_platform.dll,
   - call functions marked as 'blink export', that is gfx.mojom.size struct
traits,
   - have no other parent target except blink_platform.dll.
so we should also mark them as 'blink export'.

Hi Yuzhu,
Sorry I can't quite understand the suggestion about 'make these files a separate
target', would you please help to elaborate more? Thanks:)

[yzshen1, 2016-12-15 18:34:58]

On 2016/12/15 03:06:49, leonhsl wrote:
> On 2016/12/14 18:09:53, yzshen1 wrote:
> > On 2016/12/14 17:53:39, jam wrote:
> > > https://codereview.chromium.org/2574593003/diff/40001/cc/ipc/BUILD.gn
> > > File cc/ipc/BUILD.gn (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2574593003/diff/40001/cc/ipc/BUILD.gn#newcode70
> > > cc/ipc/BUILD.gn:70: export_class_attribute_blink = "BLINK_PLATFORM_EXPORT"
> > > I don't understand this change and the next file.
> > > 
> > > Why isn't coe in cc/ using CC_EXPORT?
> > 
> > I think the reason is these generated files are compiled into the blink
> platform
> > target.
> > If we want to make these files a separate target that blink platform depends
> on,
> > we will need to move some typemapping related code out of blink platform as
> > well. That seems possible  in this case.
> 
> Hi John,
> These 2 mojom targets(cc/ipc:interfaces_blink and
> gpu/ipc/common:interfaces_blink):
>    - are source_set,
>    - are linked into blink_platform.dll,
>    - call functions marked as 'blink export', that is gfx.mojom.size struct
> traits,
>    - have no other parent target except blink_platform.dll.
> so we should also mark them as 'blink export'.
> 
> Hi Yuzhu,
> Sorry I can't quite understand the suggestion about 'make these files a
separate
> target', would you please help to elaborate more? Thanks:)


I am fine with your current approach, that idea is a possible alternative. Take
cc/ipc:interfaces_blink as example, we could:
- create a separate component which links in the generated files of
cc/ipc:interfaces_blink, for example, cc/ipc:interface_blink_component. (And we
export those mojom definitions properly.)
- and then let blink_platform depend on cc/ipc:interface_blink_component.
- however, cc/ipc:interface_blink actually needs
WebKit/Source/platform/mojo/GeometryStructTraits.{h, cc} because of typemapping,
so we need to move those files out of blink_platform as well, either into a
separate component or in cc/ipc:interface_blink_component.
- usually those struct traits files depend on pretty complex types that are hard
to pull out of the original targets. But in this case GeometryStructTraits.{h,
cc} only depends on WebSize.h, and therefore shouldn't be a problem.

[leonhsl(Using Gerrit), 2016-12-16 03:15:14]

On 2016/12/15 18:34:58, yzshen1 wrote:
> On 2016/12/15 03:06:49, leonhsl wrote:
> > On 2016/12/14 18:09:53, yzshen1 wrote:
> > > On 2016/12/14 17:53:39, jam wrote:
> > > > https://codereview.chromium.org/2574593003/diff/40001/cc/ipc/BUILD.gn
> > > > File cc/ipc/BUILD.gn (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2574593003/diff/40001/cc/ipc/BUILD.gn#newcode70
> > > > cc/ipc/BUILD.gn:70: export_class_attribute_blink =
"BLINK_PLATFORM_EXPORT"
> > > > I don't understand this change and the next file.
> > > > 
> > > > Why isn't coe in cc/ using CC_EXPORT?
> > > 
> > > I think the reason is these generated files are compiled into the blink
> > platform
> > > target.
> > > If we want to make these files a separate target that blink platform
depends
> > on,
> > > we will need to move some typemapping related code out of blink platform
as
> > > well. That seems possible  in this case.
> > 
> > Hi John,
> > These 2 mojom targets(cc/ipc:interfaces_blink and
> > gpu/ipc/common:interfaces_blink):
> >    - are source_set,
> >    - are linked into blink_platform.dll,
> >    - call functions marked as 'blink export', that is gfx.mojom.size struct
> > traits,
> >    - have no other parent target except blink_platform.dll.
> > so we should also mark them as 'blink export'.
> > 
> > Hi Yuzhu,
> > Sorry I can't quite understand the suggestion about 'make these files a
> separate
> > target', would you please help to elaborate more? Thanks:)
> 
> 
> I am fine with your current approach, that idea is a possible alternative.
Take
> cc/ipc:interfaces_blink as example, we could:
> - create a separate component which links in the generated files of
> cc/ipc:interfaces_blink, for example, cc/ipc:interface_blink_component. (And
we
> export those mojom definitions properly.)
> - and then let blink_platform depend on cc/ipc:interface_blink_component.
> - however, cc/ipc:interface_blink actually needs
> WebKit/Source/platform/mojo/GeometryStructTraits.{h, cc} because of
typemapping,
> so we need to move those files out of blink_platform as well, either into a
> separate component or in cc/ipc:interface_blink_component.
> - usually those struct traits files depend on pretty complex types that are
hard
> to pull out of the original targets. But in this case GeometryStructTraits.{h,
> cc} only depends on WebSize.h, and therefore shouldn't be a problem.

Understood and thanks!

Friendly ping John, WDYT? Thanks.

[jam, 2016-12-16 20:31:33]

On 2016/12/16 03:15:14, leonhsl wrote:
> On 2016/12/15 18:34:58, yzshen1 wrote:
> > On 2016/12/15 03:06:49, leonhsl wrote:
> > > On 2016/12/14 18:09:53, yzshen1 wrote:
> > > > On 2016/12/14 17:53:39, jam wrote:
> > > > > https://codereview.chromium.org/2574593003/diff/40001/cc/ipc/BUILD.gn
> > > > > File cc/ipc/BUILD.gn (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/2574593003/diff/40001/cc/ipc/BUILD.gn#newcode70
> > > > > cc/ipc/BUILD.gn:70: export_class_attribute_blink =
> "BLINK_PLATFORM_EXPORT"
> > > > > I don't understand this change and the next file.
> > > > > 
> > > > > Why isn't coe in cc/ using CC_EXPORT?
> > > > 
> > > > I think the reason is these generated files are compiled into the blink
> > > platform
> > > > target.
> > > > If we want to make these files a separate target that blink platform
> depends
> > > on,
> > > > we will need to move some typemapping related code out of blink platform
> as
> > > > well. That seems possible  in this case.
> > > 
> > > Hi John,
> > > These 2 mojom targets(cc/ipc:interfaces_blink and
> > > gpu/ipc/common:interfaces_blink):
> > >    - are source_set,
> > >    - are linked into blink_platform.dll,
> > >    - call functions marked as 'blink export', that is gfx.mojom.size
struct
> > > traits,
> > >    - have no other parent target except blink_platform.dll.
> > > so we should also mark them as 'blink export'.
> > > 
> > > Hi Yuzhu,
> > > Sorry I can't quite understand the suggestion about 'make these files a
> > separate
> > > target', would you please help to elaborate more? Thanks:)
> > 
> > 
> > I am fine with your current approach, that idea is a possible alternative.
> Take
> > cc/ipc:interfaces_blink as example, we could:
> > - create a separate component which links in the generated files of
> > cc/ipc:interfaces_blink, for example, cc/ipc:interface_blink_component. (And
> we
> > export those mojom definitions properly.)
> > - and then let blink_platform depend on cc/ipc:interface_blink_component.
> > - however, cc/ipc:interface_blink actually needs
> > WebKit/Source/platform/mojo/GeometryStructTraits.{h, cc} because of
> typemapping,
> > so we need to move those files out of blink_platform as well, either into a
> > separate component or in cc/ipc:interface_blink_component.
> > - usually those struct traits files depend on pretty complex types that are
> hard
> > to pull out of the original targets. But in this case
GeometryStructTraits.{h,
> > cc} only depends on WebSize.h, and therefore shouldn't be a problem.
> 
> Understood and thanks!
> 
> Friendly ping John, WDYT? Thanks.

Yes that sounds like a better approach, it's odd to see src/cc targets using
BLINK_PLATFORM_EXPORT=1 and that could also make things fragile

[leonhsl(Using Gerrit), 2016-12-18 08:50:04]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-12-18 08:50:12]

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, 2016-12-18 08:54:21]

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

[commit-bot: I haz the power, 2016-12-18 08:54:22]

Dry run: Try jobs failed on following builders:
  linux_trusty_blink_rel on master.tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_trusty_blink_rel/b...)
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[leonhsl(Using Gerrit), 2016-12-18 08:58:55]

Patchset #4 (id:60001) has been deleted

[leonhsl(Using Gerrit), 2016-12-18 08:59:09]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-12-18 08:59:11]

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

[leonhsl(Using Gerrit), 2016-12-18 09:10:35]

Description was changed from

==========
Fix Windows build warning LNK4217 for blink_platform.dll.

[1] third_party/WebKit/public:offscreen_canvas_mojo_bindings_blink
  ==> [2] cc/ipc:interfaces_blink
    ==> [3] gpu/ipc/common:interfaces_blink
The above three targets are all linked into blink_platform.dll,
but [2] and [3] use gfx.mojom.Size typemap defined in
third_party/WebKit/Source/platform/mojo/GeometryStructTraits.{cpp,h},
which is exported from bink_platform.dll.
Thus, generated C++ bindings of [2] and [3] include GeometryStructTraits.h
while themselves are part of blink_platform.dll, means they are
importing(dllimport) local defined symbols, so we must add the same
export macros to them to avoid LNK4217 warnings.

BUG=669352
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Fix Windows build warning LNK4217 for blink_platform.dll.

[1] third_party/WebKit/public:offscreen_canvas_mojo_bindings_blink
  ==> [2] cc/ipc:interfaces_blink
    ==> [3] gpu/ipc/common:interfaces_blink
The above three targets are all linked into blink_platform.dll,
but [2] and [3] use gfx.mojom.Size typemap defined in
third_party/WebKit/Source/platform/mojo/GeometryStructTraits.{cpp,h},
which is exported from bink_platform.dll.
Thus, generated C++ bindings of [2] and [3] include GeometryStructTraits.h
while themselves are part of blink_platform.dll, means they are
importing(dllimport) local defined symbols marked as dllexport, caused LNK4217.

This CL moves this struct traits to a separate source_set target,
instead of exporting it from blink_platform.dll.

BUG=669352
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[commit-bot: I haz the power, 2016-12-18 11:07:09]

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

[commit-bot: I haz the power, 2016-12-18 11:07:10]

Dry run: This issue passed the CQ dry run.

[leonhsl(Using Gerrit), 2016-12-19 09:49:07]

Hi, Yuzhu, John, I considered another option maybe we can take, uploaded ps#4,
which has passed all trybots and I've confirmed the LNK4217 warnings for
blink_platform have also disappeared, would you PTAnL? Thanks.

[leonhsl(Using Gerrit), 2016-12-19 09:53:29]

leon.han@intel.com changed reviewers:
+ tsepez@chromium.org

[leonhsl(Using Gerrit), 2016-12-19 09:53:29]

+Tom, Thanks.

[yzshen1, 2016-12-19 17:34:56]

https://codereview.chromium.org/2574593003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/mojo/BUILD.gn (right):

https://codereview.chromium.org/2574593003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/mojo/BUILD.gn:13:
"//ui/gfx/geometry/mojo:mojo_blink__generator",
Can it depend on ui/gfx/geometry/mojo:mojo_shared_cpp_sources? __generator is
not meant to be a public-facing target.

You need to change the include in GeometryStructTraits.h to
geometry.mojom-shared.h. And I don't think you need
"//mojo/public/cpp/bindings:wtf_support" either.

[Tom Sepez, 2016-12-19 17:41:55]

On 2016/12/19 17:34:56, yzshen1 wrote:
>
https://codereview.chromium.org/2574593003/diff/80001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/platform/mojo/BUILD.gn (right):
> 
>
https://codereview.chromium.org/2574593003/diff/80001/third_party/WebKit/Sour...
> third_party/WebKit/Source/platform/mojo/BUILD.gn:13:
> "//ui/gfx/geometry/mojo:mojo_blink__generator",
> Can it depend on ui/gfx/geometry/mojo:mojo_shared_cpp_sources? __generator is
> not meant to be a public-facing target.
> 
> You need to change the include in GeometryStructTraits.h to
> geometry.mojom-shared.h. And I don't think you need
> "//mojo/public/cpp/bindings:wtf_support" either.

Code itself seems OK, but get sign-off from jam/yzshen on the layering.

[leonhsl(Using Gerrit), 2016-12-20 01:11:35]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-12-20 01:12:13]

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, 2016-12-20 02:50:36]

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

[commit-bot: I haz the power, 2016-12-20 02:50:36]

Dry run: This issue passed the CQ dry run.

[leonhsl(Using Gerrit), 2016-12-20 03:00:20]

Uploaded ps#5, PTAnL, Thanks.
  #Also confirmed that LNK4217 warnings for blink_platform have disappeared.

https://codereview.chromium.org/2574593003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/mojo/BUILD.gn (right):

https://codereview.chromium.org/2574593003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/mojo/BUILD.gn:13:
"//ui/gfx/geometry/mojo:mojo_blink__generator",
On 2016/12/19 17:34:56, yzshen1 wrote:
> Can it depend on ui/gfx/geometry/mojo:mojo_shared_cpp_sources? __generator is
> not meant to be a public-facing target.
> 
> You need to change the include in GeometryStructTraits.h to
> geometry.mojom-shared.h. And I don't think you need
> "//mojo/public/cpp/bindings:wtf_support" either.

Done and Thanks. Understood more about traits now!

[leonhsl(Using Gerrit), 2016-12-22 02:47:15]

Description was changed from

==========
Fix Windows build warning LNK4217 for blink_platform.dll.

[1] third_party/WebKit/public:offscreen_canvas_mojo_bindings_blink
  ==> [2] cc/ipc:interfaces_blink
    ==> [3] gpu/ipc/common:interfaces_blink
The above three targets are all linked into blink_platform.dll,
but [2] and [3] use gfx.mojom.Size typemap defined in
third_party/WebKit/Source/platform/mojo/GeometryStructTraits.{cpp,h},
which is exported from bink_platform.dll.
Thus, generated C++ bindings of [2] and [3] include GeometryStructTraits.h
while themselves are part of blink_platform.dll, means they are
importing(dllimport) local defined symbols marked as dllexport, caused LNK4217.

This CL moves this struct traits to a separate source_set target,
instead of exporting it from blink_platform.dll.

BUG=669352
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Fix Windows build warning LNK4217 for blink_platform.dll.

[1] third_party/WebKit/public:offscreen_canvas_mojo_bindings_blink
  ==> [2] cc/ipc:interfaces_blink
    ==> [3] gpu/ipc/common:interfaces_blink
The above three targets are all linked into blink_platform.dll,
but [2] and [3] use gfx.mojom.Size typemap defined in
third_party/WebKit/Source/platform/mojo/GeometryStructTraits.{cpp,h},
which is exported from bink_platform.dll.
Thus, generated C++ bindings of [2] and [3] include GeometryStructTraits.h
while themselves are part of blink_platform.dll, means they are
importing(dllimport) local defined symbols marked as dllexport, caused LNK4217.

This CL moves this struct traits to a separate source_set target,
instead of exporting it from blink_platform.dll.

BUG=654776,669352
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[leonhsl(Using Gerrit), 2016-12-23 00:38:57]

Soft ping~

[leonhsl(Using Gerrit), 2016-12-23 08:39:00]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-12-23 08:39:07]

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

[leonhsl(Using Gerrit), 2016-12-23 08:49:55]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-12-23 08:50:04]

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, 2016-12-23 11:03:26]

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

[commit-bot: I haz the power, 2016-12-23 11:03:28]

Dry run: This issue passed the CQ dry run.

[leonhsl(Using Gerrit), 2017-01-03 03:30:55]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-03 03:31:02]

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-01-03 05:15:49]

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

[commit-bot: I haz the power, 2017-01-03 05:15:50]

Dry run: This issue passed the CQ dry run.

[Tom Sepez, 2017-01-03 18:39:30]

On 2016/12/23 00:38:57, leonhsl wrote:
> Soft ping~

RS LGTM, but be sure about the layering.

[leonhsl(Using Gerrit), 2017-01-06 09:53:56]

Ping John and Yuzhu, thanks.

[yzshen1, 2017-01-06 18:31:48]

On 2017/01/06 09:53:56, leonhsl wrote:
> Ping John and Yuzhu, thanks.

LGTM

Sorry I thought I have LGed.

[leonhsl(Using Gerrit), 2017-01-11 03:26:23]

Ping John~

[leonhsl(Using Gerrit), 2017-01-16 09:38:32]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-16 09:38:41]

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-01-16 11:04:44]

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

[commit-bot: I haz the power, 2017-01-16 11:04:45]

Dry run: This issue passed the CQ dry run.

[leonhsl(Using Gerrit), 2017-01-16 12:25:39]

leon.han@intel.com changed reviewers:
+ haraken@chromium.org

[leonhsl(Using Gerrit), 2017-01-16 12:25:40]

+haraken@, would you mind to take an OWNER review for this? Thanks.

[haraken, 2017-01-16 12:31:45]

haraken@chromium.org changed reviewers:
+ esprehn@chromium.org

[haraken, 2017-01-16 12:31:46]

https://codereview.chromium.org/2574593003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/mojo/BUILD.gn (right):

https://codereview.chromium.org/2574593003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/mojo/BUILD.gn:1: # Copyright 2016 The
Chromium Authors. All rights reserved.

2017

https://codereview.chromium.org/2574593003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/mojo/GeometryStructTraits.h (right):

https://codereview.chromium.org/2574593003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/mojo/GeometryStructTraits.h:9: #include
"ui/gfx/geometry/mojo/geometry.mojom-shared.h"

Would you help me understand what *-shared.h means? How is it different from
*-blink.h?

In general, our convention has been that we only include *-blink.h in Blink.

+esprehn

[leonhsl(Using Gerrit), 2017-01-17 03:40:07]

Thank you for review! And sorry for my late reply..

https://codereview.chromium.org/2574593003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/mojo/BUILD.gn (right):

https://codereview.chromium.org/2574593003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/mojo/BUILD.gn:1: # Copyright 2016 The
Chromium Authors. All rights reserved.
On 2017/01/16 12:31:46, haraken wrote:
> 
> 2017

Acknowledged.

https://codereview.chromium.org/2574593003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/mojo/GeometryStructTraits.h (right):

https://codereview.chromium.org/2574593003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/mojo/GeometryStructTraits.h:9: #include
"ui/gfx/geometry/mojo/geometry.mojom-shared.h"
On 2017/01/16 12:31:46, haraken wrote:
> 
> Would you help me understand what *-shared.h means? How is it different from
> *-blink.h?
> 
> In general, our convention has been that we only include *-blink.h in Blink.
> 
> +esprehn

xxx.mojom-shared.h is included by both xxx.mojom.h and xxx.mojom-blink.h, it
defines xxx::mojom::FooDataView class used by mojo internal
serialization/deserialization process for both xxx::mojom::Foo and
xxx::mojom::blink::Foo, so actually just providing struct traits impl for
xxx::mojom::FooDataView(defined in *-shared.h) is enough to do typemapping for
both Chromium variant and Blink variant. Including xxx.mojom-blink.h instead
would possibly bring in some unnecessary headers/deps. 
I switched to use *-shared.h at ps#5 to address comments from Yuzhu at
https://codereview.chromium.org/2574593003/diff/80001/third_party/WebKit/Sour....

Also, I found some other places in Blink are using *-shared.h in struct traits:
https://cs.chromium.org/search/?q=mojom-shared.h+file:%5Esrc/third_party/WebK...
And actually this CL is doing the similar work with
third_party/WebKit/public/web/window_features.typemap, putting struct traits
impl into a separate source_set target which is declared as a dep by the
xxxx.typemap file.

[haraken, 2017-01-17 03:49:00]

On 2017/01/17 03:40:07, leonhsl wrote:
> Thank you for review! And sorry for my late reply..
> 
>
https://codereview.chromium.org/2574593003/diff/160001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/platform/mojo/BUILD.gn (right):
> 
>
https://codereview.chromium.org/2574593003/diff/160001/third_party/WebKit/Sou...
> third_party/WebKit/Source/platform/mojo/BUILD.gn:1: # Copyright 2016 The
> Chromium Authors. All rights reserved.
> On 2017/01/16 12:31:46, haraken wrote:
> > 
> > 2017
> 
> Acknowledged.
> 
>
https://codereview.chromium.org/2574593003/diff/160001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/platform/mojo/GeometryStructTraits.h (right):
> 
>
https://codereview.chromium.org/2574593003/diff/160001/third_party/WebKit/Sou...
> third_party/WebKit/Source/platform/mojo/GeometryStructTraits.h:9: #include
> "ui/gfx/geometry/mojo/geometry.mojom-shared.h"
> On 2017/01/16 12:31:46, haraken wrote:
> > 
> > Would you help me understand what *-shared.h means? How is it different from
> > *-blink.h?
> > 
> > In general, our convention has been that we only include *-blink.h in Blink.
> > 
> > +esprehn
> 
> xxx.mojom-shared.h is included by both xxx.mojom.h and xxx.mojom-blink.h, it
> defines xxx::mojom::FooDataView class used by mojo internal
> serialization/deserialization process for both xxx::mojom::Foo and
> xxx::mojom::blink::Foo, so actually just providing struct traits impl for
> xxx::mojom::FooDataView(defined in *-shared.h) is enough to do typemapping for
> both Chromium variant and Blink variant. Including xxx.mojom-blink.h instead
> would possibly bring in some unnecessary headers/deps. 
> I switched to use *-shared.h at ps#5 to address comments from Yuzhu at
>
https://codereview.chromium.org/2574593003/diff/80001/third_party/WebKit/Sour....
> 
> Also, I found some other places in Blink are using *-shared.h in struct
traits:
>
https://cs.chromium.org/search/?q=mojom-shared.h+file:%5Esrc/third_party/WebK...
> And actually this CL is doing the similar work with
> third_party/WebKit/public/web/window_features.typemap, putting struct traits
> impl into a separate source_set target which is declared as a dep by the
> xxxx.typemap file.

Thanks for the clarification. LGTM.

[leonhsl(Using Gerrit), 2017-01-17 05:06:00]

https://codereview.chromium.org/2574593003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/mojo/BUILD.gn (right):

https://codereview.chromium.org/2574593003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/mojo/BUILD.gn:1: # Copyright 2016 The
Chromium Authors. All rights reserved.
On 2017/01/17 03:40:07, leonhsl wrote:
> On 2017/01/16 12:31:46, haraken wrote:
> > 
> > 2017
> 
> Acknowledged.

Done.

[leonhsl(Using Gerrit), 2017-01-17 05:06:13]

The CQ bit was checked by leon.han@intel.com

[leonhsl(Using Gerrit), 2017-01-17 05:06:14]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
haraken@chromium.org, yzshen@chromium.org
Link to the patchset: https://codereview.chromium.org/2574593003/#ps180001
(title: "Rebase and change new code file's license 2016-->2017")

[commit-bot: I haz the power, 2017-01-17 05:06:27]

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-01-17 06:52:51]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1484629573895000,
"parent_rev": "acfaa8216d01efc4da558881290577e79b4c950b", "commit_rev":
"4724c1ac34d346dd8b44c4abc33b344a744a2d69"}

[commit-bot: I haz the power, 2017-01-17 06:53:47]

Description was changed from

==========
Fix Windows build warning LNK4217 for blink_platform.dll.

[1] third_party/WebKit/public:offscreen_canvas_mojo_bindings_blink
  ==> [2] cc/ipc:interfaces_blink
    ==> [3] gpu/ipc/common:interfaces_blink
The above three targets are all linked into blink_platform.dll,
but [2] and [3] use gfx.mojom.Size typemap defined in
third_party/WebKit/Source/platform/mojo/GeometryStructTraits.{cpp,h},
which is exported from bink_platform.dll.
Thus, generated C++ bindings of [2] and [3] include GeometryStructTraits.h
while themselves are part of blink_platform.dll, means they are
importing(dllimport) local defined symbols marked as dllexport, caused LNK4217.

This CL moves this struct traits to a separate source_set target,
instead of exporting it from blink_platform.dll.

BUG=654776,669352
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Fix Windows build warning LNK4217 for blink_platform.dll.

[1] third_party/WebKit/public:offscreen_canvas_mojo_bindings_blink
  ==> [2] cc/ipc:interfaces_blink
    ==> [3] gpu/ipc/common:interfaces_blink
The above three targets are all linked into blink_platform.dll,
but [2] and [3] use gfx.mojom.Size typemap defined in
third_party/WebKit/Source/platform/mojo/GeometryStructTraits.{cpp,h},
which is exported from bink_platform.dll.
Thus, generated C++ bindings of [2] and [3] include GeometryStructTraits.h
while themselves are part of blink_platform.dll, means they are
importing(dllimport) local defined symbols marked as dllexport, caused LNK4217.

This CL moves this struct traits to a separate source_set target,
instead of exporting it from blink_platform.dll.

BUG=654776,669352
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2574593003
Cr-Commit-Position: refs/heads/master@{#443994}
Committed:
https://chromium.googlesource.com/chromium/src/+/4724c1ac34d346dd8b44c4abc33b...
==========

[commit-bot: I haz the power, 2017-01-17 06:53:49]

Committed patchset #9 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/4724c1ac34d346dd8b44c4abc33b...