Add Protobuf support in cc for gfx objects

Add Protobuf support in cc for gfx objects

Add protobuf serialization to the following:
- gfx::Point
- gfx::PointF
- gfx::Rect
- gfx::RectF
- gfx::Size
- gfx::SizeF
- gfx::Transform

Add a helper class to handle serialization and deserialization.  Add
unit tests as well.

BUG=541321
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/ddb0b49f3f9fa19f64cc3bacd1bdd3070571b383
Cr-Commit-Position: refs/heads/master@{#354846}

[David Trainor- moved to gerrit, 2015-10-08 20:51:46]

dtrainor@chromium.org changed reviewers:
+ danakj@chromium.org

[David Trainor- moved to gerrit, 2015-10-08 20:51:46]

Running some try jobs to make sure the gyp build works (been using GN locally
lately and I messed up my gyp workflow a bit).  Will fix.

Can you take a look at the overall idea/location of these?  I know there are a
bunch of options for how to do this and I'm not sure if this is the best.

Thanks!

[David Trainor- moved to gerrit, 2015-10-12 17:05:53]

Had to move this to cc/proto :(.  It looks like Chromium has no support for
including .proto files from other directories.  Currently the protoc compiler
builds the .proto files at the local directory, which means the compiled C++
code doesn't work with cross-directory imports.

I spent some time trying to change the protoc_wrapper.py to get this to work.  I
was able to get the protobufs importing properly, but the compiled C++ code
still built some statics from the wrong base directory.  Even if we get this to
work, it might require changing every proto import in chromium, so I want to
work on that in parallel.

[danakj, 2015-10-14 20:08:38]

danakj@chromium.org changed reviewers:
+ vmpstr@chromium.org

[danakj, 2015-10-14 20:10:00]

On 2015/10/12 17:05:53, David Trainor wrote:
> Had to move this to cc/proto :(.  It looks like Chromium has no support for
> including .proto files from other directories.  Currently the protoc compiler
> builds the .proto files at the local directory, which means the compiled C++
> code doesn't work with cross-directory imports.
> 
> I spent some time trying to change the protoc_wrapper.py to get this to work. 
I
> was able to get the protobufs importing properly, but the compiled C++ code
> still built some statics from the wrong base directory.  Even if we get this
to
> work, it might require changing every proto import in chromium, so I want to
> work on that in parallel.

Hm this isn't great. But I never have seen protos in chromium so consider me
completely clueless here.

I don't understand the problem can you explain a bit more?

From what I can see in this patch it would be gfx_conversions.cc including the
proto files. In your earlier patch set that was in ui/gfx/geometry, so it was in
the same directory as the protos.

[David Trainor- moved to gerrit, 2015-10-14 21:49:11]

On 2015/10/14 20:10:00, danakj wrote:
> On 2015/10/12 17:05:53, David Trainor wrote:
> > Had to move this to cc/proto :(.  It looks like Chromium has no support for
> > including .proto files from other directories.  Currently the protoc
compiler
> > builds the .proto files at the local directory, which means the compiled C++
> > code doesn't work with cross-directory imports.
> > 
> > I spent some time trying to change the protoc_wrapper.py to get this to
work. 
> I
> > was able to get the protobufs importing properly, but the compiled C++ code
> > still built some statics from the wrong base directory.  Even if we get this
> to
> > work, it might require changing every proto import in chromium, so I want to
> > work on that in parallel.
> 
> Hm this isn't great. But I never have seen protos in chromium so consider me
> completely clueless here.
> 
> I don't understand the problem can you explain a bit more?
> 
> From what I can see in this patch it would be gfx_conversions.cc including the
> proto files. In your earlier patch set that was in ui/gfx/geometry, so it was
in
> the same directory as the protos.

Sure!

Basically, protoc_wrapper.py and the corresponding GN/gyp drivers of that script
don't support .proto files importing other .proto files from different
directories.  They end up compiling the .proto file from the actual folder where
it lives instead of the src/ root.  That means the generated C++ code's special
static methods and includes are all local, so it won't compile.  Importing with
"../../ui/gfx/proto/..." didn't work either.

The proper solution would be to compile all protos relative to the src/ root
directory, so it'd be possible to do imports like import
"ui/gfx/proto/point.proto".  I tried changing the build scripts to do this and
still had issues with the generated C++ static helper methods that protoc
creates for each message (proto messages call these functions on all
sub-messages, so if the name is wrong stuff doesn't link).

Overall I ended burning a few days on this and ended up filing a bug at
crbug.com/542423 to track fixing it.  I plan on trying to fix it in parallel
with Khushal's and Tommy's work.  Once it's fixed I'll move the protos to better
locations they probably should be in.  I'm a little worried that fixing that bug
will require changing imports for *all* protos in Chromium to be src/ relative,
so it might be a longer run CL that could be a pain to land.  So for now I put
the protos in cc/proto, since all of the cc proto communication is
self-contained.

[danakj, 2015-10-14 22:41:13]

On 2015/10/14 21:49:11, David Trainor wrote:
> On 2015/10/14 20:10:00, danakj wrote:
> > On 2015/10/12 17:05:53, David Trainor wrote:
> > > Had to move this to cc/proto :(.  It looks like Chromium has no support
for
> > > including .proto files from other directories.  Currently the protoc
> compiler
> > > builds the .proto files at the local directory, which means the compiled
C++
> > > code doesn't work with cross-directory imports.
> > > 
> > > I spent some time trying to change the protoc_wrapper.py to get this to
> work. 
> > I
> > > was able to get the protobufs importing properly, but the compiled C++
code
> > > still built some statics from the wrong base directory.  Even if we get
this
> > to
> > > work, it might require changing every proto import in chromium, so I want
to
> > > work on that in parallel.
> > 
> > Hm this isn't great. But I never have seen protos in chromium so consider me
> > completely clueless here.
> > 
> > I don't understand the problem can you explain a bit more?
> > 
> > From what I can see in this patch it would be gfx_conversions.cc including
the
> > proto files. In your earlier patch set that was in ui/gfx/geometry, so it
was
> in
> > the same directory as the protos.
> 
> Sure!
> 
> Basically, protoc_wrapper.py and the corresponding GN/gyp drivers of that
script
> don't support .proto files importing other .proto files from different
> directories.  They end up compiling the .proto file from the actual folder
where
> it lives instead of the src/ root.  That means the generated C++ code's
special
> static methods and includes are all local, so it won't compile.  Importing
with
> "../../ui/gfx/proto/..." didn't work either.
> 
> The proper solution would be to compile all protos relative to the src/ root
> directory, so it'd be possible to do imports like import
> "ui/gfx/proto/point.proto".  I tried changing the build scripts to do this and
> still had issues with the generated C++ static helper methods that protoc
> creates for each message (proto messages call these functions on all
> sub-messages, so if the name is wrong stuff doesn't link).
> 
> Overall I ended burning a few days on this and ended up filing a bug at
> crbug.com/542423 to track fixing it.  I plan on trying to fix it in parallel
> with Khushal's and Tommy's work.  Once it's fixed I'll move the protos to
better
> locations they probably should be in.  I'm a little worried that fixing that
bug
> will require changing imports for *all* protos in Chromium to be src/
relative,
> so it might be a longer run CL that could be a pain to land.  So for now I put
> the protos in cc/proto, since all of the cc proto communication is
> self-contained.

So it is because there will be cc protos that reference the gfx protos that
makes they have to live by the cc ones?

[danakj, 2015-10-14 22:42:38]

https://codereview.chromium.org/1394353002/diff/60001/cc/proto/point.proto
File cc/proto/point.proto (right):

https://codereview.chromium.org/1394353002/diff/60001/cc/proto/point.proto#ne...
cc/proto/point.proto:12: optional int32 x = 1;
Is it not problematic that int inside Point can be bigger than 32 bits?

[David Trainor- moved to gerrit, 2015-10-14 22:57:51]

https://codereview.chromium.org/1394353002/diff/60001/cc/proto/point.proto
File cc/proto/point.proto (right):

https://codereview.chromium.org/1394353002/diff/60001/cc/proto/point.proto#ne...
cc/proto/point.proto:12: optional int32 x = 1;
On 2015/10/14 22:42:37, danakj wrote:
> Is it not problematic that int inside Point can be bigger than 32 bits?

Oops good point :(.  Thanks!  Changed size too and added a unit test against
std::numeric_limits<int> (I think that should test those cases).

[danakj, 2015-10-14 23:10:11]

Repeating question to be sure I got this right:

So it is because there will be cc protos that reference the gfx protos that
makes they have to live by the cc ones?

https://codereview.chromium.org/1394353002/diff/60001/cc/BUILD.gn
File cc/BUILD.gn (right):

https://codereview.chromium.org/1394353002/diff/60001/cc/BUILD.gn#newcode526
cc/BUILD.gn:526: "//third_party/protobuf:protobuf_lite",
how come this isn't a dep of //cc/proto instead/only?

https://codereview.chromium.org/1394353002/diff/60001/cc/cc.gyp
File cc/cc.gyp (right):

https://codereview.chromium.org/1394353002/diff/60001/cc/cc.gyp#newcode595
cc/cc.gyp:595: 'enable_wexit_time_destructors': 1,
what is it? can you explain in comment

https://codereview.chromium.org/1394353002/diff/60001/cc/proto/BUILD.gn
File cc/proto/BUILD.gn (right):

https://codereview.chromium.org/1394353002/diff/60001/cc/proto/BUILD.gn#newco...
cc/proto/BUILD.gn:18: # possible to include protos from other
directories/targets.
can you file a bug and point to it?

https://codereview.chromium.org/1394353002/diff/60001/cc/proto/BUILD.gn#newco...
cc/proto/BUILD.gn:33: extra_configs = [
"//build/config/compiler:wexit_time_destructors" ]
ditto

https://codereview.chromium.org/1394353002/diff/60001/cc/proto/gfx_conversions.h
File cc/proto/gfx_conversions.h (right):

https://codereview.chromium.org/1394353002/diff/60001/cc/proto/gfx_conversion...
cc/proto/gfx_conversions.h:33: CC_EXPORT void ProtoToPoint(const proto::Point&
proto, gfx::Point* point);
do we need this

https://codereview.chromium.org/1394353002/diff/60001/cc/proto/gfx_conversion...
cc/proto/gfx_conversions.h:34: CC_EXPORT gfx::Point ProtoToPoint(const
proto::Point& proto);
and this?

https://codereview.chromium.org/1394353002/diff/60001/cc/proto/gfx_conversion...
File cc/proto/gfx_conversions_unittest.cc (right):

https://codereview.chromium.org/1394353002/diff/60001/cc/proto/gfx_conversion...
cc/proto/gfx_conversions_unittest.cc:114: const gfx::RectF rect(1.f, 2.f, 3.f,
4.f);
can you use numbers not representable as ints for all these float tests?

https://codereview.chromium.org/1394353002/diff/60001/cc/proto/point.proto
File cc/proto/point.proto (right):

https://codereview.chromium.org/1394353002/diff/60001/cc/proto/point.proto#ne...
cc/proto/point.proto:13: optional int32 y = 2;
why are these all optional? (i'm a noob here)

https://codereview.chromium.org/1394353002/diff/60001/ui/gfx/gfx.gyp
File ui/gfx/gfx.gyp (right):

https://codereview.chromium.org/1394353002/diff/60001/ui/gfx/gfx.gyp#newcode183
ui/gfx/gfx.gyp:183: 
drop this diff :)

[David Trainor- moved to gerrit, 2015-10-15 22:18:02]

https://codereview.chromium.org/1394353002/diff/60001/cc/BUILD.gn
File cc/BUILD.gn (right):

https://codereview.chromium.org/1394353002/diff/60001/cc/BUILD.gn#newcode526
cc/BUILD.gn:526: "//third_party/protobuf:protobuf_lite",
On 2015/10/14 23:10:10, danakj wrote:
> how come this isn't a dep of //cc/proto instead/only?

I can do this in GN, but gyp with component=shared_library seems to fail when I
build cc and cc_unittests without this and the one in cc_tests.gyp :(.

https://codereview.chromium.org/1394353002/diff/60001/cc/cc.gyp
File cc/cc.gyp (right):

https://codereview.chromium.org/1394353002/diff/60001/cc/cc.gyp#newcode595
cc/cc.gyp:595: 'enable_wexit_time_destructors': 1,
On 2015/10/14 23:10:10, danakj wrote:
> what is it? can you explain in comment

I wasn't sure either, but sync did it for their proto targets.  So I looked into
it and it seems to be used by a lot of places in Chromium to prevent exit time
destructors.  Added a comment.

https://codereview.chromium.org/1394353002/diff/60001/cc/proto/BUILD.gn
File cc/proto/BUILD.gn (right):

https://codereview.chromium.org/1394353002/diff/60001/cc/proto/BUILD.gn#newco...
cc/proto/BUILD.gn:18: # possible to include protos from other
directories/targets.
On 2015/10/14 23:10:10, danakj wrote:
> can you file a bug and point to it?

Done.

https://codereview.chromium.org/1394353002/diff/60001/cc/proto/BUILD.gn#newco...
cc/proto/BUILD.gn:33: extra_configs = [
"//build/config/compiler:wexit_time_destructors" ]
On 2015/10/14 23:10:10, danakj wrote:
> ditto

Done.

https://codereview.chromium.org/1394353002/diff/60001/cc/proto/gfx_conversions.h
File cc/proto/gfx_conversions.h (right):

https://codereview.chromium.org/1394353002/diff/60001/cc/proto/gfx_conversion...
cc/proto/gfx_conversions.h:33: CC_EXPORT void ProtoToPoint(const proto::Point&
proto, gfx::Point* point);
On 2015/10/14 23:10:10, danakj wrote:
> do we need this

Yeah we could probably get away with only one.  The second includes the copy
constructor but seems to be cleaner from a code perspective.  I'll probably
remove this one for now.

https://codereview.chromium.org/1394353002/diff/60001/cc/proto/gfx_conversion...
File cc/proto/gfx_conversions_unittest.cc (right):

https://codereview.chromium.org/1394353002/diff/60001/cc/proto/gfx_conversion...
cc/proto/gfx_conversions_unittest.cc:114: const gfx::RectF rect(1.f, 2.f, 3.f,
4.f);
On 2015/10/14 23:10:10, danakj wrote:
> can you use numbers not representable as ints for all these float tests?

Done.

https://codereview.chromium.org/1394353002/diff/60001/cc/proto/point.proto
File cc/proto/point.proto (right):

https://codereview.chromium.org/1394353002/diff/60001/cc/proto/point.proto#ne...
cc/proto/point.proto:13: optional int32 y = 2;
On 2015/10/14 23:10:10, danakj wrote:
> why are these all optional? (i'm a noob here)

It's a general rule to never make something required with protobufs.  If you do,
you can't ever do versioning control and omit the field in the future.

https://codereview.chromium.org/1394353002/diff/60001/ui/gfx/gfx.gyp
File ui/gfx/gfx.gyp (right):

https://codereview.chromium.org/1394353002/diff/60001/ui/gfx/gfx.gyp#newcode183
ui/gfx/gfx.gyp:183: 
On 2015/10/14 23:10:10, danakj wrote:
> drop this diff :)

Done.

[David Trainor- moved to gerrit, 2015-10-15 22:18:59]

On 2015/10/15 22:18:02, David Trainor wrote:
> https://codereview.chromium.org/1394353002/diff/60001/cc/BUILD.gn
> File cc/BUILD.gn (right):
> 
> https://codereview.chromium.org/1394353002/diff/60001/cc/BUILD.gn#newcode526
> cc/BUILD.gn:526: "//third_party/protobuf:protobuf_lite",
> On 2015/10/14 23:10:10, danakj wrote:
> > how come this isn't a dep of //cc/proto instead/only?
> 
> I can do this in GN, but gyp with component=shared_library seems to fail when
I
> build cc and cc_unittests without this and the one in cc_tests.gyp :(.
> 
> https://codereview.chromium.org/1394353002/diff/60001/cc/cc.gyp
> File cc/cc.gyp (right):
> 
> https://codereview.chromium.org/1394353002/diff/60001/cc/cc.gyp#newcode595
> cc/cc.gyp:595: 'enable_wexit_time_destructors': 1,
> On 2015/10/14 23:10:10, danakj wrote:
> > what is it? can you explain in comment
> 
> I wasn't sure either, but sync did it for their proto targets.  So I looked
into
> it and it seems to be used by a lot of places in Chromium to prevent exit time
> destructors.  Added a comment.
> 
> https://codereview.chromium.org/1394353002/diff/60001/cc/proto/BUILD.gn
> File cc/proto/BUILD.gn (right):
> 
>
https://codereview.chromium.org/1394353002/diff/60001/cc/proto/BUILD.gn#newco...
> cc/proto/BUILD.gn:18: # possible to include protos from other
> directories/targets.
> On 2015/10/14 23:10:10, danakj wrote:
> > can you file a bug and point to it?
> 
> Done.
> 
>
https://codereview.chromium.org/1394353002/diff/60001/cc/proto/BUILD.gn#newco...
> cc/proto/BUILD.gn:33: extra_configs = [
> "//build/config/compiler:wexit_time_destructors" ]
> On 2015/10/14 23:10:10, danakj wrote:
> > ditto
> 
> Done.
> 
>
https://codereview.chromium.org/1394353002/diff/60001/cc/proto/gfx_conversions.h
> File cc/proto/gfx_conversions.h (right):
> 
>
https://codereview.chromium.org/1394353002/diff/60001/cc/proto/gfx_conversion...
> cc/proto/gfx_conversions.h:33: CC_EXPORT void ProtoToPoint(const proto::Point&
> proto, gfx::Point* point);
> On 2015/10/14 23:10:10, danakj wrote:
> > do we need this
> 
> Yeah we could probably get away with only one.  The second includes the copy
> constructor but seems to be cleaner from a code perspective.  I'll probably
> remove this one for now.
> 
>
https://codereview.chromium.org/1394353002/diff/60001/cc/proto/gfx_conversion...
> File cc/proto/gfx_conversions_unittest.cc (right):
> 
>
https://codereview.chromium.org/1394353002/diff/60001/cc/proto/gfx_conversion...
> cc/proto/gfx_conversions_unittest.cc:114: const gfx::RectF rect(1.f, 2.f, 3.f,
> 4.f);
> On 2015/10/14 23:10:10, danakj wrote:
> > can you use numbers not representable as ints for all these float tests?
> 
> Done.
> 
> https://codereview.chromium.org/1394353002/diff/60001/cc/proto/point.proto
> File cc/proto/point.proto (right):
> 
>
https://codereview.chromium.org/1394353002/diff/60001/cc/proto/point.proto#ne...
> cc/proto/point.proto:13: optional int32 y = 2;
> On 2015/10/14 23:10:10, danakj wrote:
> > why are these all optional? (i'm a noob here)
> 
> It's a general rule to never make something required with protobufs.  If you
do,
> you can't ever do versioning control and omit the field in the future.
> 
> https://codereview.chromium.org/1394353002/diff/60001/ui/gfx/gfx.gyp
> File ui/gfx/gfx.gyp (right):
> 
>
https://codereview.chromium.org/1394353002/diff/60001/ui/gfx/gfx.gyp#newcode183
> ui/gfx/gfx.gyp:183: 
> On 2015/10/14 23:10:10, danakj wrote:
> > drop this diff :)
> 
> Done.

Oh and yes you have it right.  cc protos use ui/gfx protos, which currently
doesn't work in Chromium unless they live together.

[danakj, 2015-10-15 22:22:33]

https://codereview.chromium.org/1394353002/diff/60001/cc/proto/gfx_conversions.h
File cc/proto/gfx_conversions.h (right):

https://codereview.chromium.org/1394353002/diff/60001/cc/proto/gfx_conversion...
cc/proto/gfx_conversions.h:33: CC_EXPORT void ProtoToPoint(const proto::Point&
proto, gfx::Point* point);
On 2015/10/15 22:18:02, David Trainor wrote:
> On 2015/10/14 23:10:10, danakj wrote:
> > do we need this
> 
> Yeah we could probably get away with only one.  The second includes the copy
> constructor but seems to be cleaner from a code perspective.  I'll probably
> remove this one for now.

It should use RVO and not actually copy too. :)

[David Trainor- moved to gerrit, 2015-10-16 01:19:04]

On 2015/10/15 22:22:33, danakj wrote:
>
https://codereview.chromium.org/1394353002/diff/60001/cc/proto/gfx_conversions.h
> File cc/proto/gfx_conversions.h (right):
> 
>
https://codereview.chromium.org/1394353002/diff/60001/cc/proto/gfx_conversion...
> cc/proto/gfx_conversions.h:33: CC_EXPORT void ProtoToPoint(const proto::Point&
> proto, gfx::Point* point);
> On 2015/10/15 22:18:02, David Trainor wrote:
> > On 2015/10/14 23:10:10, danakj wrote:
> > > do we need this
> > 
> > Yeah we could probably get away with only one.  The second includes the copy
> > constructor but seems to be cleaner from a code perspective.  I'll probably
> > remove this one for now.
> 
> It should use RVO and not actually copy too. :)

Gah good point! :)

[danakj, 2015-10-16 22:04:24]

OK LG overall. Thanks. Some sticky nitty gritty questions left. :)

https://chromiumcodereview.appspot.com/1394353002/diff/60001/cc/cc.gyp
File cc/cc.gyp (right):

https://chromiumcodereview.appspot.com/1394353002/diff/60001/cc/cc.gyp#newcod...
cc/cc.gyp:595: 'enable_wexit_time_destructors': 1,
On 2015/10/15 22:18:02, David Trainor wrote:
> On 2015/10/14 23:10:10, danakj wrote:
> > what is it? can you explain in comment
> 
> I wasn't sure either, but sync did it for their proto targets.  So I looked
into
> it and it seems to be used by a lot of places in Chromium to prevent exit time
> destructors.  Added a comment.

But why is it important for the proto code? Is it? Why isn't it on everywhere in
cc? I'd like to understand..

https://chromiumcodereview.appspot.com/1394353002/diff/100001/cc/BUILD.gn
File cc/BUILD.gn (right):

https://chromiumcodereview.appspot.com/1394353002/diff/100001/cc/BUILD.gn#new...
cc/BUILD.gn:521: "//cc/proto:cc_proto",
how come you need :cc_proto here, and no just //cc/proto?

https://chromiumcodereview.appspot.com/1394353002/diff/100001/cc/BUILD.gn#new...
cc/BUILD.gn:870: "//cc/proto:cc_proto",
ditto

[David Trainor- moved to gerrit, 2015-10-19 16:02:08]

On 2015/10/16 22:04:24, danakj wrote:
> OK LG overall. Thanks. Some sticky nitty gritty questions left. :)
> 
> https://chromiumcodereview.appspot.com/1394353002/diff/60001/cc/cc.gyp
> File cc/cc.gyp (right):
> 
>
https://chromiumcodereview.appspot.com/1394353002/diff/60001/cc/cc.gyp#newcod...
> cc/cc.gyp:595: 'enable_wexit_time_destructors': 1,
> On 2015/10/15 22:18:02, David Trainor wrote:
> > On 2015/10/14 23:10:10, danakj wrote:
> > > what is it? can you explain in comment
> > 
> > I wasn't sure either, but sync did it for their proto targets.  So I looked
> into
> > it and it seems to be used by a lot of places in Chromium to prevent exit
time
> > destructors.  Added a comment.
> 
> But why is it important for the proto code? Is it? Why isn't it on everywhere
in
> cc? I'd like to understand..

Yeah I was equally confused.  I did some investigation and the initial goal from
thakis@ was to add it globally, but some people added/relied on exit time
destructors so he just added it to any target that didn't currently use them
back in 2011.  I traced this back to crbug.com/101600 and a few specific patches
in chrome.gyp and content.gyp:

https://codereview.chromium.org/8547005
https://codereview.chromium.org/8495043

It seems like if we could add it to the cc/ target that would be beneficial as
well (assuming cc doesn't currently use them).

> 
> https://chromiumcodereview.appspot.com/1394353002/diff/100001/cc/BUILD.gn
> File cc/BUILD.gn (right):
> 
>
https://chromiumcodereview.appspot.com/1394353002/diff/100001/cc/BUILD.gn#new...
> cc/BUILD.gn:521: "//cc/proto:cc_proto",
> how come you need :cc_proto here, and no just //cc/proto?
> 
>
https://chromiumcodereview.appspot.com/1394353002/diff/100001/cc/BUILD.gn#new...
> cc/BUILD.gn:870: "//cc/proto:cc_proto",
> ditto

[David Trainor- moved to gerrit, 2015-10-19 17:04:38]

https://chromiumcodereview.appspot.com/1394353002/diff/100001/cc/BUILD.gn
File cc/BUILD.gn (right):

https://chromiumcodereview.appspot.com/1394353002/diff/100001/cc/BUILD.gn#new...
cc/BUILD.gn:521: "//cc/proto:cc_proto",
On 2015/10/16 22:04:24, danakj wrote:
> how come you need :cc_proto here, and no just //cc/proto?

Ok I found out how to do this.  In component builds you can't use the same name
for a built lib as other targets, and something else was also named "proto." 
And to do //cc/proto you need a //cc/proto:proto target.  I ended up using a
group that lets me make this cleaner.

https://chromiumcodereview.appspot.com/1394353002/diff/100001/cc/BUILD.gn#new...
cc/BUILD.gn:870: "//cc/proto:cc_proto",
On 2015/10/16 22:04:24, danakj wrote:
> ditto

see above

[danakj, 2015-10-19 17:24:30]

Thanks! LGTM.

Let's add that exit destructors thing to cc also in another CL?

https://chromiumcodereview.appspot.com/1394353002/diff/120001/cc/proto/BUILD.gn
File cc/proto/BUILD.gn (right):

https://chromiumcodereview.appspot.com/1394353002/diff/120001/cc/proto/BUILD....
cc/proto/BUILD.gn:7: group("proto") {
Can you leave a comment explaining to future-us why you used a group here? (Also
yay)

https://chromiumcodereview.appspot.com/1394353002/diff/120001/cc/proto/BUILD....
cc/proto/BUILD.gn:14: visibility = [ ":proto" ]
I did a google search for gn visibility and got nothing. Lacking docs, could you
leave a comment about why this is here also?

[danakj, 2015-10-19 17:25:17]

Description was changed from

==========
Add Protobuf support to gfx objects

Add protobuf serialization to the following:
- gfx::Point
- gfx::PointF
- gfx::Rect
- gfx::RectF
- gfx::Size
- gfx::SizeF
- gfx::Transform

Add a helper class to handle serialization and deserialization.  Add
unit tests as well.

BUG=541321
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Add Protobuf support in cc for gfx objects

Add protobuf serialization to the following:
- gfx::Point
- gfx::PointF
- gfx::Rect
- gfx::RectF
- gfx::Size
- gfx::SizeF
- gfx::Transform

Add a helper class to handle serialization and deserialization.  Add
unit tests as well.

BUG=541321
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[David Trainor- moved to gerrit, 2015-10-19 18:50:38]

sounds good.  Will do!

https://chromiumcodereview.appspot.com/1394353002/diff/120001/cc/proto/BUILD.gn
File cc/proto/BUILD.gn (right):

https://chromiumcodereview.appspot.com/1394353002/diff/120001/cc/proto/BUILD....
cc/proto/BUILD.gn:7: group("proto") {
On 2015/10/19 17:24:30, danakj wrote:
> Can you leave a comment explaining to future-us why you used a group here?
(Also
> yay)

Done.

https://chromiumcodereview.appspot.com/1394353002/diff/120001/cc/proto/BUILD....
cc/proto/BUILD.gn:14: visibility = [ ":proto" ]
On 2015/10/19 17:24:30, danakj wrote:
> I did a google search for gn visibility and got nothing. Lacking docs, could
you
> leave a comment about why this is here also?

Done.  Basically only exposes this target to the targets specified in visibility
(so only "proto" can see/depend on this).

[David Trainor- moved to gerrit, 2015-10-19 19:41:42]

The CQ bit was checked by dtrainor@chromium.org

[David Trainor- moved to gerrit, 2015-10-19 19:41:42]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org
Link to the patchset:
https://chromiumcodereview.appspot.com/1394353002/#ps180001 (title: "Move deps
to public_deps")

[commit-bot: I haz the power, 2015-10-19 19:43:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1394353002/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1394353002/180001

[commit-bot: I haz the power, 2015-10-19 20:14:52]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2015-10-19 20:15:37]

Patchset 10 (id:??) landed as
https://crrev.com/ddb0b49f3f9fa19f64cc3bacd1bdd3070571b383
Cr-Commit-Position: refs/heads/master@{#354846}