Add a dependency for FlatBuffers.

Add a dependency for FlatBuffers.

FlatBuffers is a library for efficient cross platform serialization
of data structures. It allows to directly access serialized data
without unpacking/parsing it first and is intended to be used for
memory mapping data structures into renderer processes.

BUG=TDB

Committed: https://crrev.com/1f0e8e7f0278e1d4a9ad8fade720730d2053197b
Cr-Commit-Position: refs/heads/master@{#392043}

[battre, 2016-04-21 14:52:42]

battre@chromium.org changed reviewers:
+ brettw@chromium.org, engedy@chromium.org, jochen@chromium.org,
palmer@chromium.org

[battre, 2016-04-21 14:52:42]

Brett/Jochen for checking the build scripts and OWNERS approval in third_party/.
Chris for security sign-off.
Balazs as optional reviewer.

Please not that the trybots cannot be triggered, yet, because I need to ask for
setting up a mirror of the FlatBuffers repository.

[jochen (gone - plz use gerrit), 2016-04-21 14:59:00]

did OSS approve?

I didn't review the flatc.{gni,gypi}, Brett, can you look at that?

https://codereview.chromium.org/1907603005/diff/1/third_party/flatbuffers/BUI...
File third_party/flatbuffers/BUILD.gn (right):

https://codereview.chromium.org/1907603005/diff/1/third_party/flatbuffers/BUI...
third_party/flatbuffers/BUILD.gn:21: compiler_files = [
should be a source_set

https://codereview.chromium.org/1907603005/diff/1/third_party/flatbuffers/BUI...
third_party/flatbuffers/BUILD.gn:49: sources = [
can you mark this library as test only

https://codereview.chromium.org/1907603005/diff/1/third_party/flatbuffers/BUI...
third_party/flatbuffers/BUILD.gn:59: sources = compiler_files + [
same here

https://codereview.chromium.org/1907603005/diff/1/third_party/flatbuffers/fla...
File third_party/flatbuffers/flatbuffers.gyp (right):

https://codereview.chromium.org/1907603005/diff/1/third_party/flatbuffers/fla...
third_party/flatbuffers/flatbuffers.gyp:7: 'compiler_files': [
should be a static_library

https://codereview.chromium.org/1907603005/diff/1/third_party/flatbuffers/fla...
third_party/flatbuffers/flatbuffers.gyp:81: # The following files are not
included in :flatbuffers
odd indenting?

[battre, 2016-04-21 15:31:34]

Thank you. I have requested OSS approval.

https://codereview.chromium.org/1907603005/diff/1/third_party/flatbuffers/BUI...
File third_party/flatbuffers/BUILD.gn (right):

https://codereview.chromium.org/1907603005/diff/1/third_party/flatbuffers/BUI...
third_party/flatbuffers/BUILD.gn:21: compiler_files = [
On 2016/04/21 14:59:00, jochen wrote:
> should be a source_set

Done.

https://codereview.chromium.org/1907603005/diff/1/third_party/flatbuffers/BUI...
third_party/flatbuffers/BUILD.gn:49: sources = [
On 2016/04/21 14:59:00, jochen wrote:
> can you mark this library as test only

Done.

https://codereview.chromium.org/1907603005/diff/1/third_party/flatbuffers/BUI...
third_party/flatbuffers/BUILD.gn:59: sources = compiler_files + [
On 2016/04/21 14:59:00, jochen wrote:
> same here

Done.

https://codereview.chromium.org/1907603005/diff/1/third_party/flatbuffers/fla...
File third_party/flatbuffers/flatbuffers.gyp (right):

https://codereview.chromium.org/1907603005/diff/1/third_party/flatbuffers/fla...
third_party/flatbuffers/flatbuffers.gyp:7: 'compiler_files': [
On 2016/04/21 14:59:00, jochen wrote:
> should be a static_library

Done.

https://codereview.chromium.org/1907603005/diff/1/third_party/flatbuffers/fla...
third_party/flatbuffers/flatbuffers.gyp:81: # The following files are not
included in :flatbuffers
On 2016/04/21 14:59:00, jochen wrote:
> odd indenting?

Done.

[brettw, 2016-04-21 20:00:03]

https://codereview.chromium.org/1907603005/diff/40001/third_party/flatbuffers...
File third_party/flatbuffers/BUILD.gn (right):

https://codereview.chromium.org/1907603005/diff/40001/third_party/flatbuffers...
third_party/flatbuffers/BUILD.gn:9: static_library("flatbuffers") {
This should be a source_set.

https://codereview.chromium.org/1907603005/diff/40001/third_party/flatbuffers...
third_party/flatbuffers/BUILD.gn:22: sources = [
Can you set
  visibility = [ ":*" ]
here so targets outside of here don't depend on this (just good hygiene).

https://codereview.chromium.org/1907603005/diff/40001/third_party/flatbuffers...
third_party/flatbuffers/BUILD.gn:67: executable("flatbuffers_unittest") {
This should use the test() template in //testing/test.gni (it will produce an
executable on desktop so shouldn't change what you did here).

https://codereview.chromium.org/1907603005/diff/40001/third_party/flatbuffers...
File third_party/flatbuffers/flatc.gni (right):

https://codereview.chromium.org/1907603005/diff/40001/third_party/flatbuffers...
third_party/flatbuffers/flatc.gni:41: template("flatbuffers_library") {
I prefer to name the gni files the same as the template contained in them. maybe
just "flatbuffer" would be good for both?

https://codereview.chromium.org/1907603005/diff/40001/third_party/flatbuffers...
third_party/flatbuffers/flatc.gni:52: action_foreach(action_name) {
This is wrong for cross-compile cases. You can solve this and also delete your
wrapper script by using //build/compiled_action.gni (compiled_action_foreach
template). This template will handle all of the cases that need fixing here.

https://codereview.chromium.org/1907603005/diff/40001/third_party/flatbuffers...
third_party/flatbuffers/flatc.gni:109: "//third_party/flatbuffers:flatbuffers",
Delete the redundant ":flatbuffers"

[battre, 2016-04-25 14:31:48]

Thank you for the review.

https://codereview.chromium.org/1907603005/diff/40001/third_party/flatbuffers...
File third_party/flatbuffers/BUILD.gn (right):

https://codereview.chromium.org/1907603005/diff/40001/third_party/flatbuffers...
third_party/flatbuffers/BUILD.gn:9: static_library("flatbuffers") {
On 2016/04/21 20:00:03, brettw wrote:
> This should be a source_set.

Done. I have also deleted flathash.cpp, which should not be there.

https://codereview.chromium.org/1907603005/diff/40001/third_party/flatbuffers...
third_party/flatbuffers/BUILD.gn:22: sources = [
On 2016/04/21 20:00:03, brettw wrote:
> Can you set
>   visibility = [ ":*" ]
> here so targets outside of here don't depend on this (just good hygiene).

Done.

https://codereview.chromium.org/1907603005/diff/40001/third_party/flatbuffers...
third_party/flatbuffers/BUILD.gn:67: executable("flatbuffers_unittest") {
On 2016/04/21 20:00:02, brettw wrote:
> This should use the test() template in //testing/test.gni (it will produce an
> executable on desktop so shouldn't change what you did here).

Done.

https://codereview.chromium.org/1907603005/diff/40001/third_party/flatbuffers...
File third_party/flatbuffers/flatc.gni (right):

https://codereview.chromium.org/1907603005/diff/40001/third_party/flatbuffers...
third_party/flatbuffers/flatc.gni:41: template("flatbuffers_library") {
On 2016/04/21 20:00:03, brettw wrote:
> I prefer to name the gni files the same as the template contained in them.
maybe
> just "flatbuffer" would be good for both?

Done. I am uncertain what to do with the .gypi file. Should I rename that as
well? In that case, we would have a flatbuffers.gyp for the compiler and headers
and a flatbuffer.gypi to compile the IDLs. That could be confusing.

https://codereview.chromium.org/1907603005/diff/40001/third_party/flatbuffers...
third_party/flatbuffers/flatc.gni:52: action_foreach(action_name) {
On 2016/04/21 20:00:03, brettw wrote:
> This is wrong for cross-compile cases. You can solve this and also delete your
> wrapper script by using //build/compiled_action.gni (compiled_action_foreach
> template). This template will handle all of the cases that need fixing here.

Done.

https://codereview.chromium.org/1907603005/diff/40001/third_party/flatbuffers...
third_party/flatbuffers/flatc.gni:109: "//third_party/flatbuffers:flatbuffers",
On 2016/04/21 20:00:03, brettw wrote:
> Delete the redundant ":flatbuffers"

Done.

[wvo, 2016-04-25 16:33:22]

Note, the library is called "FlatBuffers" (or "flatbuffers"), a "flatbuffer" is
an individual buffer, and should probably not be used to refer to the project.

[palmer, 2016-04-26 23:27:06]

I think the main thing to do at this point is get a fuzzer running for this.
Once that's up and running, we can land this.

[battre, 2016-04-27 12:16:26]

On 2016/04/25 16:33:22, wvo wrote:
> Note, the library is called "FlatBuffers" (or "flatbuffers"), a "flatbuffer"
is
> an individual buffer, and should probably not be used to refer to the project.

I think that's already the case. The project is referred to by flatbuffers (e.g.
in BUILD.gn) but the script to compile a single IDL file is called "flatbuffer".

Best regards,
Dominic

[palmer, 2016-04-28 21:07:28]

OK, so, given that the way we intend to use FB in the immediate term is
low-risk, and that it will soon be fuzzed, and that the code is clean and fixes
will be relatively easy if fuzzing raises any bugs, this LGTM from a security
perspective. Thanks to all for discussion and feedback!

[battre, 2016-04-29 16:45:35]

Thank you.

Brett, could you now take another look please?

Thanks,
Dominic

[brettw, 2016-05-02 17:13:25]

lgtm

https://codereview.chromium.org/1907603005/diff/60001/third_party/flatbuffers...
File third_party/flatbuffers/BUILD.gn (right):

https://codereview.chromium.org/1907603005/diff/60001/third_party/flatbuffers...
third_party/flatbuffers/BUILD.gn:54:
import("//third_party/flatbuffers/flatbuffer.gni")
Can you put all the imports at the top, sorted like header files??

https://codereview.chromium.org/1907603005/diff/60001/third_party/flatbuffers...
third_party/flatbuffers/BUILD.gn:67: import("//testing/test.gni")
Ditto

[battre, 2016-05-03 07:11:08]

Thanks a lot.

I will commit as soon as the repository is being mirrored.

[battre, 2016-05-06 06:48:59]

battre@chromium.org changed reviewers:
+ phajdan.jr@chromium.org

[battre, 2016-05-06 06:48:59]

+phajdan.jr for OWNER's approval of tools/checklicenses/checklicenses.py

[Paweł Hajdan Jr., 2016-05-06 10:22:02]

https://codereview.chromium.org/1907603005/diff/100001/tools/checklicenses/ch...
File tools/checklicenses/checklicenses.py (right):

https://codereview.chromium.org/1907603005/diff/100001/tools/checklicenses/ch...
tools/checklicenses/checklicenses.py:349: # Flatbuffers is known to be licenced
under an Apache (v2.0) license.
Do the files have license headers or do we just fail to recognize them?

In the former case, please file an upstream bug to get them added and link to it
here.

[Paweł Hajdan Jr., 2016-05-06 11:11:47]

LGTM

[battre, 2016-05-06 12:06:20]

The CQ bit was checked by battre@chromium.org

[battre, 2016-05-06 12:06:21]

The patchset sent to the CQ was uploaded after l-g-t-m from palmer@chromium.org,
brettw@chromium.org
Link to the patchset: https://codereview.chromium.org/1907603005/#ps120001
(title: "Addressed comments on license")

[commit-bot: I haz the power, 2016-05-06 12:06:30]

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

[commit-bot: I haz the power, 2016-05-06 12:59:13]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-05-06 13:00:26]

Description was changed from

==========
Add a dependency for FlatBuffers.

FlatBuffers is a library for efficient cross platform serialization
of data structures. It allows to directly access serialized data
without unpacking/parsing it first and is intended to be used for
memory mapping data structures into renderer processes.

BUG=TDB
==========

to

==========
Add a dependency for FlatBuffers.

FlatBuffers is a library for efficient cross platform serialization
of data structures. It allows to directly access serialized data
without unpacking/parsing it first and is intended to be used for
memory mapping data structures into renderer processes.

BUG=TDB

Committed: https://crrev.com/1f0e8e7f0278e1d4a9ad8fade720730d2053197b
Cr-Commit-Position: refs/heads/master@{#392043}
==========

[commit-bot: I haz the power, 2016-05-06 13:00:27]

Patchset 7 (id:??) landed as
https://crrev.com/1f0e8e7f0278e1d4a9ad8fade720730d2053197b
Cr-Commit-Position: refs/heads/master@{#392043}