Implementing glTF 1.0 parser

Implementing glTF 1.0 parser

- Documentation at https://github.com/KhronosGroup/glTF/tree/2.0/specification/1.0
- Elements which we will not be using are excluded, such as materials, textures, shaders, skins. We might want to use them later when glTF 2.0 is released. For now, we plan to handle this natively.
- Unittests are included.
- Small refactoring of the rendering classes inheritance to account for non-quad objects, which will be used later for rendering the controller.
- Minor fixes on code I encountered.

BUG=705006
Review-Url: https://codereview.chromium.org/2757213003
Cr-Commit-Position: refs/heads/master@{#459547}
Committed: https://chromium.googlesource.com/chromium/src/+/a76f0b59ca704c56c693b2390a8033f834b08a3f

[acondor_, 2017-03-20 21:55:45]

acondor@google.com changed reviewers:
+ mthiesse@chromium.org

[acondor_, 2017-03-20 21:55:46]

Please perform an initial review.

[mthiesse, 2017-03-20 21:57:08]

Description was changed from

==========
Implementing glTF 1.0 parser

- Documentation at
https://github.com/KhronosGroup/glTF/tree/2.0/specification/1.0
- Elements which we will not be using are excluded, such as materials, textures,
shaders, skins. We might want to use them later when glTF 2.0 is released. For
now, we plan to handle this nativelly.
- Unittests are included.
- Small refactoring of the rendering classes inheritance to account for non-quad
objects, which will be used later for rendering the controller.
- Minor fixes on code I encountered.

BUG=644562
==========

to

==========
Implementing glTF 1.0 parser

- Documentation at
https://github.com/KhronosGroup/glTF/tree/2.0/specification/1.0
- Elements which we will not be using are excluded, such as materials, textures,
shaders, skins. We might want to use them later when glTF 2.0 is released. For
now, we plan to handle this natively.
- Unittests are included.
- Small refactoring of the rendering classes inheritance to account for non-quad
objects, which will be used later for rendering the controller.
- Minor fixes on code I encountered.

BUG=644562
==========

[mthiesse, 2017-03-20 22:32:45]

Heading out, will continue review tomorrow, but left some initial nit-picky
comments :P

https://codereview.chromium.org/2757213003/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/gltf_parser.h (right):

https://codereview.chromium.org/2757213003/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/gltf_parser.h:1: // Copyright 2016 The Chromium
Authors. All rights reserved.
nit: Copyright 2017

https://codereview.chromium.org/2757213003/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/gltf_parser.h:17: namespace gltf {
Move all of the code inside of this namespace to a new header/cc file
gltf_asset.h/cc

https://codereview.chromium.org/2757213003/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/gltf_parser.h:31: struct BufferView {
Can you add a comment above these structs saying they match structures in the
1.0 spec, and link to the spec?

[acondor_, 2017-03-21 15:26:21]

acondor@google.com changed reviewers:
+ bajones@chromium.org

[acondor_, 2017-03-21 15:26:22]

The first suggestions addressed. Also, I tried to stick more to the format,
specially regarding default values and optional properties.
It's still on my mind to move these structures and parser to either components/
or base/, according to what we decide.

https://codereview.chromium.org/2757213003/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/gltf_parser.h (right):

https://codereview.chromium.org/2757213003/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/gltf_parser.h:1: // Copyright 2016 The Chromium
Authors. All rights reserved.
On 2017/03/20 22:32:44, mthiesse wrote:
> nit: Copyright 2017

Done.

https://codereview.chromium.org/2757213003/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/gltf_parser.h:17: namespace gltf {
On 2017/03/20 22:32:44, mthiesse wrote:
> Move all of the code inside of this namespace to a new header/cc file
> gltf_asset.h/cc

Done.

https://codereview.chromium.org/2757213003/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/gltf_parser.h:31: struct BufferView {
On 2017/03/20 22:32:44, mthiesse wrote:
> Can you add a comment above these structs saying they match structures in the
> 1.0 spec, and link to the spec?

Done.

[bajones, 2017-03-21 15:44:24]

https://codereview.chromium.org/2757213003/diff/20001/chrome/browser/android/...
File chrome/browser/android/vr_shell/gltf_asset.h (right):

https://codereview.chromium.org/2757213003/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/gltf_asset.h:93: std::size_t
AddBuffer(std::unique_ptr<Buffer> buffer) {
I know this are all pretty simple functions, but I think that Chrome's style
guide still prefers them to be in the .cc file. Get/SetScene is probably the
exception.

https://codereview.chromium.org/2757213003/diff/20001/chrome/browser/android/...
File chrome/browser/android/vr_shell/gltf_parser.cc (right):

https://codereview.chromium.org/2757213003/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/gltf_parser.cc:60: CHECK(uri.substr(0,
kBase64HeaderSize) == kBase64Header);
This is a smart simplification for a first pass, but given that we're trying to
be sensitive to size forcing assets to be base64 encoded unfortunately works
against us. I don't think it should necessarily block this CL, but we may want
to consider supporting the binary glTF format up front if we're trying to avoid
multiple files. Then at least the buffers don't have to be encoded.

https://codereview.chromium.org/2757213003/diff/20001/chrome/browser/android/...
File chrome/browser/android/vr_shell/vr_math.cc (right):

https://codereview.chromium.org/2757213003/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_math.cc:111: DCHECK(x_left < x_right &&
y_bottom < y_top && z_near < z_far &&
What's the reasoning for making this a DCHECK? Worth noting that it was (perhaps
unintentionally) catching divide by zero conditions.

https://codereview.chromium.org/2757213003/diff/20001/chrome/browser/android/...
File chrome/browser/android/vr_shell/vr_shell_gl.cc (right):

https://codereview.chromium.org/2757213003/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell_gl.cc:7: #include <chrono>
Not clear what this is needed for.

[mthiesse, 2017-03-21 16:06:55]

https://codereview.chromium.org/2757213003/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/gltf_parser.cc (right):

https://codereview.chromium.org/2757213003/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/gltf_parser.cc:92: // Only inline data is
supported
nit: add period.

https://codereview.chromium.org/2757213003/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/gltf_parser.cc:99:
CHECK(buffer_dict->GetInteger("byteLength", &byte_length));
byteLength isn't required by the spec, add a comment that because we only
support inline data we need a length?

https://codereview.chromium.org/2757213003/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/gltf_parser.cc:100: CHECK(int{buffer->length()}
== byte_length);
static_cast<int>(buffer->length())

https://codereview.chromium.org/2757213003/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/gltf_parser.cc:118:
buffer_view_dict->GetInteger("byteLength", &buffer_view->byte_length));
byteLength and target are not required per the spec, comment on why we need them
to be there?

https://codereview.chromium.org/2757213003/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/gltf_parser.cc:140:
CHECK(accessor_dict->GetInteger("byteStride", &accessor->byte_stride));
Also not required per spec, add comment?

https://codereview.chromium.org/2757213003/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/gltf_parser.cc:159: for (const auto&
primitive_value : *list) {
nit: Move parsing of mesh.primitive into separate function for readability.

https://codereview.chromium.org/2757213003/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/gltf_parser.cc:160: const base::DictionaryValue*
primitive_dict;
Add comment that we ignore the required material attribute as we don't support
meshes.

https://codereview.chromium.org/2757213003/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/gltf_parser.cc:166:
CHECK(primitive_dict->GetString("indices", &indices_key));
Add comment, not required per spec.

https://codereview.chromium.org/2757213003/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/gltf_parser.cc:170:
CHECK(primitive_dict->GetInteger("mode", &primitive->mode));
Add comment, not required per spec.

https://codereview.chromium.org/2757213003/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/gltf_parser.cc:171:
CHECK(primitive_dict->GetDictionary("attributes", &attributes));
Attributes isn't required per spec. Do we need it, or can we handle empty
attributes?

https://codereview.chromium.org/2757213003/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/gltf_parser.cc:237:
CHECK(scene_dict->GetList("nodes", &list));
Not required per spec

https://codereview.chromium.org/2757213003/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/gltf_parser.h (right):

https://codereview.chromium.org/2757213003/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/gltf_parser.h:82: auto id = buffers_.size();
nit: s/id/index throughout? id makes this look like a map, when it's just a
vector.

https://codereview.chromium.org/2757213003/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/gltf_parser.h:141: const Scene* GetScene() const
{ return scene_; }
Can we change this to GetCurrentScene, and SetCurrentScene? Pretty confusing
as-is.

https://codereview.chromium.org/2757213003/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/gltf_parser.h:143: void SetScene(const Scene*
scene) { scene_ = scene; }
Is there ever a reason to call SetScene multiple times? Why not add add Scene*
to the constructor?

[cjgrant, 2017-03-21 16:16:46]

cjgrant@chromium.org changed reviewers:
+ cjgrant@chromium.org

[cjgrant, 2017-03-21 16:16:47]

https://codereview.chromium.org/2757213003/diff/20001/chrome/browser/android/...
File chrome/browser/android/vr_shell/gltf_parser.cc (right):

https://codereview.chromium.org/2757213003/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/gltf_parser.cc:24: CHECK(gltf_version == "1.0");
General comment for here and many other spots:  Feels like a "failed to parse"
result should be returned instead of blowing up on CHECK.  Deferring to Brandon
on this though.

https://codereview.chromium.org/2757213003/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/gltf_parser.cc:45: CHECK(scene_it !=
scene_ids_.end());
Same question as above - anything that could fail due to bad input probably
shouldn't crash the app.

https://codereview.chromium.org/2757213003/diff/20001/chrome/browser/android/...
File chrome/browser/android/vr_shell/vr_shell_renderer.cc (right):

https://codereview.chromium.org/2757213003/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell_renderer.cc:247: vertex_buffer_ = 0;
This could now be done in the header file, FWIW.

[acondor_, 2017-03-21 19:31:30]

Addressing suggestions.

    - Removing CHECKs in favor of returning nullptr when parsing fails.
    - Moved glTF objects into their own header and source files.
    - Addressing nit suggestions.

https://codereview.chromium.org/2757213003/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/gltf_parser.cc (right):

https://codereview.chromium.org/2757213003/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/gltf_parser.cc:99:
CHECK(buffer_dict->GetInteger("byteLength", &byte_length));
On 2017/03/21 16:06:54, mthiesse wrote:
> byteLength isn't required by the spec, add a comment that because we only
> support inline data we need a length?

Actually we don't need it.

https://codereview.chromium.org/2757213003/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/gltf_parser.cc:100: CHECK(int{buffer->length()}
== byte_length);
On 2017/03/21 16:06:54, mthiesse wrote:
> static_cast<int>(buffer->length())

Done.

https://codereview.chromium.org/2757213003/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/gltf_parser.cc:118:
buffer_view_dict->GetInteger("byteLength", &buffer_view->byte_length));
On 2017/03/21 16:06:54, mthiesse wrote:
> byteLength and target are not required per the spec, comment on why we need
them
> to be there?

Acknowledged.

https://codereview.chromium.org/2757213003/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/gltf_parser.cc:140:
CHECK(accessor_dict->GetInteger("byteStride", &accessor->byte_stride));
On 2017/03/21 16:06:54, mthiesse wrote:
> Also not required per spec, add comment?

Acknowledged.

https://codereview.chromium.org/2757213003/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/gltf_parser.cc:160: const base::DictionaryValue*
primitive_dict;
On 2017/03/21 16:06:54, mthiesse wrote:
> Add comment that we ignore the required material attribute as we don't support
> meshes.

Done.

https://codereview.chromium.org/2757213003/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/gltf_parser.cc:166:
CHECK(primitive_dict->GetString("indices", &indices_key));
On 2017/03/21 16:06:54, mthiesse wrote:
> Add comment, not required per spec.

Acknowledged.

https://codereview.chromium.org/2757213003/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/gltf_parser.cc:170:
CHECK(primitive_dict->GetInteger("mode", &primitive->mode));
On 2017/03/21 16:06:54, mthiesse wrote:
> Add comment, not required per spec.

Acknowledged.

https://codereview.chromium.org/2757213003/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/gltf_parser.cc:171:
CHECK(primitive_dict->GetDictionary("attributes", &attributes));
On 2017/03/21 16:06:54, mthiesse wrote:
> Attributes isn't required per spec. Do we need it, or can we handle empty
> attributes?

We can handle them. Recall that the only model we will use at first is
controlled by us.

https://codereview.chromium.org/2757213003/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/gltf_parser.cc:237:
CHECK(scene_dict->GetList("nodes", &list));
On 2017/03/21 16:06:54, mthiesse wrote:
> Not required per spec

Acknowledged.

https://codereview.chromium.org/2757213003/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/gltf_parser.h (right):

https://codereview.chromium.org/2757213003/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/gltf_parser.h:141: const Scene* GetScene() const
{ return scene_; }
On 2017/03/21 16:06:54, mthiesse wrote:
> Can we change this to GetCurrentScene, and SetCurrentScene? Pretty confusing
> as-is.

Doing Main instead of Current.

https://codereview.chromium.org/2757213003/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/gltf_parser.h:143: void SetScene(const Scene*
scene) { scene_ = scene; }
On 2017/03/21 16:06:55, mthiesse wrote:
> Is there ever a reason to call SetScene multiple times? Why not add add Scene*
> to the constructor?

Because the parser needs to set this at the end, when all the scenes have
already being constructed.

https://codereview.chromium.org/2757213003/diff/20001/chrome/browser/android/...
File chrome/browser/android/vr_shell/gltf_parser.cc (right):

https://codereview.chromium.org/2757213003/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/gltf_parser.cc:60: CHECK(uri.substr(0,
kBase64HeaderSize) == kBase64Header);
On 2017/03/21 15:44:24, bajones wrote:
> This is a smart simplification for a first pass, but given that we're trying
to
> be sensitive to size forcing assets to be base64 encoded unfortunately works
> against us. I don't think it should necessarily block this CL, but we may want
> to consider supporting the binary glTF format up front if we're trying to
avoid
> multiple files. Then at least the buffers don't have to be encoded.

For reference, our current model is < 20k and we are going to deliver it through
component updater.
But first I want to get the model rendered and then focus on issues about
delivering. At that point, I can implement the binary reader, which doesn't seem
like a great amount of work on top of this.

https://codereview.chromium.org/2757213003/diff/20001/chrome/browser/android/...
File chrome/browser/android/vr_shell/vr_math.cc (right):

https://codereview.chromium.org/2757213003/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_math.cc:111: DCHECK(x_left < x_right &&
y_bottom < y_top && z_near < z_far &&
On 2017/03/21 15:44:24, bajones wrote:
> What's the reasoning for making this a DCHECK? Worth noting that it was
(perhaps
> unintentionally) catching divide by zero conditions.

I'm not changing functionality here, only making this line compliant with style.
Should we file a bug on this? But I'm pretty sure that if this check fails, then
the developer using this function has a bug.

https://codereview.chromium.org/2757213003/diff/20001/chrome/browser/android/...
File chrome/browser/android/vr_shell/vr_shell_gl.cc (right):

https://codereview.chromium.org/2757213003/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell_gl.cc:7: #include <chrono>
On 2017/03/21 15:44:24, bajones wrote:
> Not clear what this is needed for.

Function 'TimeInMicroseconds' uses chrono. It compiles even if it's excluded,
but I think it's better to be explicit.

https://codereview.chromium.org/2757213003/diff/20001/chrome/browser/android/...
File chrome/browser/android/vr_shell/vr_shell_renderer.cc (right):

https://codereview.chromium.org/2757213003/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_shell_renderer.cc:247: vertex_buffer_ = 0;
On 2017/03/21 16:16:47, cjgrant wrote:
> This could now be done in the header file, FWIW.

Done.

[mthiesse, 2017-03-21 20:24:38]

https://codereview.chromium.org/2757213003/diff/40001/chrome/browser/android/...
File chrome/browser/android/vr_shell/gltf_parser.cc (right):

https://codereview.chromium.org/2757213003/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/gltf_parser.cc:30: asset_.reset();
Rather than making asset_ a member variable and having to reset it in all
failure cases, why not make it a local unique_ptr here, and pass the
gltf::Asset* into each of the Set* functions?

Alternatively, make a private ParseInternal() that does the actual parsing and
returns a nullptr without worrying about creating or cleaning up asset_, and let
Parse() worry about that.

https://codereview.chromium.org/2757213003/diff/40001/chrome/browser/android/...
File chrome/browser/android/vr_shell/gltf_parser.h (right):

https://codereview.chromium.org/2757213003/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/gltf_parser.h:17: // Parser for glTF 1.0
specification
Add a strongly worded comment here saying that this definitely should not be
used for untrusted glTF models (should only be used by models we create, that
are thoroughly tested), and mention what parts of the spec are and are not
supported in this parser.

https://codereview.chromium.org/2757213003/diff/40001/chrome/browser/android/...
File chrome/browser/android/vr_shell/gltf_parser_unittest.cc (right):

https://codereview.chromium.org/2757213003/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/gltf_parser_unittest.cc:16:
base::DictionaryValue asset;
I think rather than constructing a dictionary that matches what you would json
parse out of a gltf file, we should be using an actual gltf file and parsing it
here in the test. It would also be easier to update or replace the test data
that way.

cjgrant@, thoughts?

[cjgrant, 2017-03-21 20:38:03]

https://codereview.chromium.org/2757213003/diff/40001/chrome/browser/android/...
File chrome/browser/android/vr_shell/gltf_parser_unittest.cc (right):

https://codereview.chromium.org/2757213003/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/gltf_parser_unittest.cc:16:
base::DictionaryValue asset;
On 2017/03/21 20:24:38, mthiesse wrote:
> I think rather than constructing a dictionary that matches what you would json
> parse out of a gltf file, we should be using an actual gltf file and parsing
it
> here in the test. It would also be easier to update or replace the test data
> that way.
> 
> cjgrant@, thoughts?

I agree.  The JSON would be easier to read, and even though you have to touch
two files, I think building the dictionary manually is cumbersome.  We do that
in UI unit tests, and it's nasty.

However, if we want to stress-test the parser, we could use the base::Value
dictionary methods to break/modify that original dictionary, and ensure the
parser reports failures.

[acondor_, 2017-03-22 16:25:29]

Addressed the small suggestions and moved the test data for the glTF parser to a
file.

https://codereview.chromium.org/2757213003/diff/20001/chrome/browser/android/...
File chrome/browser/android/vr_shell/gltf_asset.h (right):

https://codereview.chromium.org/2757213003/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/gltf_asset.h:93: std::size_t
AddBuffer(std::unique_ptr<Buffer> buffer) {
On 2017/03/21 15:44:23, bajones wrote:
> I know this are all pretty simple functions, but I think that Chrome's style
> guide still prefers them to be in the .cc file. Get/SetScene is probably the
> exception.

Done.

https://codereview.chromium.org/2757213003/diff/20001/chrome/browser/android/...
File chrome/browser/android/vr_shell/gltf_parser.cc (right):

https://codereview.chromium.org/2757213003/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/gltf_parser.cc:24: CHECK(gltf_version == "1.0");
On 2017/03/21 16:16:47, cjgrant wrote:
> General comment for here and many other spots:  Feels like a "failed to parse"
> result should be returned instead of blowing up on CHECK.  Deferring to
Brandon
> on this though.

Done.

https://codereview.chromium.org/2757213003/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/gltf_parser.cc:45: CHECK(scene_it !=
scene_ids_.end());
On 2017/03/21 16:16:47, cjgrant wrote:
> Same question as above - anything that could fail due to bad input probably
> shouldn't crash the app.

Done.

https://codereview.chromium.org/2757213003/diff/40001/chrome/browser/android/...
File chrome/browser/android/vr_shell/gltf_parser.cc (right):

https://codereview.chromium.org/2757213003/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/gltf_parser.cc:30: asset_.reset();
On 2017/03/21 20:24:37, mthiesse wrote:
> Rather than making asset_ a member variable and having to reset it in all
> failure cases, why not make it a local unique_ptr here, and pass the
> gltf::Asset* into each of the Set* functions?
> 
> Alternatively, make a private ParseInternal() that does the actual parsing and
> returns a nullptr without worrying about creating or cleaning up asset_, and
let
> Parse() worry about that.

I went for the 2nd option

https://codereview.chromium.org/2757213003/diff/40001/chrome/browser/android/...
File chrome/browser/android/vr_shell/gltf_parser.h (right):

https://codereview.chromium.org/2757213003/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/gltf_parser.h:17: // Parser for glTF 1.0
specification
On 2017/03/21 20:24:37, mthiesse wrote:
> Add a strongly worded comment here saying that this definitely should not be
> used for untrusted glTF models (should only be used by models we create, that
> are thoroughly tested), and mention what parts of the spec are and are not
> supported in this parser.

Done.

https://codereview.chromium.org/2757213003/diff/40001/chrome/browser/android/...
File chrome/browser/android/vr_shell/gltf_parser_unittest.cc (right):

https://codereview.chromium.org/2757213003/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/gltf_parser_unittest.cc:16:
base::DictionaryValue asset;
On 2017/03/21 20:38:02, cjgrant wrote:
> On 2017/03/21 20:24:38, mthiesse wrote:
> > I think rather than constructing a dictionary that matches what you would
json
> > parse out of a gltf file, we should be using an actual gltf file and parsing
> it
> > here in the test. It would also be easier to update or replace the test data
> > that way.
> > 
> > cjgrant@, thoughts?
> 
> I agree.  The JSON would be easier to read, and even though you have to touch
> two files, I think building the dictionary manually is cumbersome.  We do that
> in UI unit tests, and it's nasty.
> 
> However, if we want to stress-test the parser, we could use the base::Value
> dictionary methods to break/modify that original dictionary, and ensure the
> parser reports failures.

Done.

[mthiesse, 2017-03-22 18:53:44]

lgtm

Wait until at least tomorrow to land this, to see if the gpu folks have anything
to say in response to your email.

https://codereview.chromium.org/2757213003/diff/60001/chrome/browser/android/...
File chrome/browser/android/vr_shell/gltf_parser.h (right):

https://codereview.chromium.org/2757213003/diff/60001/chrome/browser/android/...
chrome/browser/android/vr_shell/gltf_parser.h:24: // but only on thoroughly
tested Chromium-defined files.
nit: s/Chromium-defined/Chromium resource/

https://codereview.chromium.org/2757213003/diff/60001/chrome/browser/android/...
File chrome/browser/android/vr_shell/test/data/mock.gltf (right):

https://codereview.chromium.org/2757213003/diff/60001/chrome/browser/android/...
chrome/browser/android/vr_shell/test/data/mock.gltf:1: {
nit: call this file something like sample.gltf (mock is more of a technical term
in testing, and this isn't a mock)

https://codereview.chromium.org/2757213003/diff/60001/chrome/browser/android/...
File chrome/browser/android/vr_shell/test/paths.h (right):

https://codereview.chromium.org/2757213003/diff/60001/chrome/browser/android/...
chrome/browser/android/vr_shell/test/paths.h:12: PATH_START = 10000,
It looks like the intent is to make this a unique number or something? 10000 is
already used, maybe use 12000? I don't know the reasoning behind this.

[bajones, 2017-03-22 19:04:13]

LGTM with mthiesse@'s nits addressed. And I agree that we should wait to see if
the mailing list has any suggestions, especially in regards to code location.

[acondor_, 2017-03-22 19:34:25]

On hold then.

https://codereview.chromium.org/2757213003/diff/60001/chrome/browser/android/...
File chrome/browser/android/vr_shell/gltf_parser.h (right):

https://codereview.chromium.org/2757213003/diff/60001/chrome/browser/android/...
chrome/browser/android/vr_shell/gltf_parser.h:24: // but only on thoroughly
tested Chromium-defined files.
On 2017/03/22 18:53:43, mthiesse wrote:
> nit: s/Chromium-defined/Chromium resource/

Done.

https://codereview.chromium.org/2757213003/diff/60001/chrome/browser/android/...
File chrome/browser/android/vr_shell/test/data/mock.gltf (right):

https://codereview.chromium.org/2757213003/diff/60001/chrome/browser/android/...
chrome/browser/android/vr_shell/test/data/mock.gltf:1: {
On 2017/03/22 18:53:43, mthiesse wrote:
> nit: call this file something like sample.gltf (mock is more of a technical
term
> in testing, and this isn't a mock)

Done.

https://codereview.chromium.org/2757213003/diff/60001/chrome/browser/android/...
File chrome/browser/android/vr_shell/test/paths.h (right):

https://codereview.chromium.org/2757213003/diff/60001/chrome/browser/android/...
chrome/browser/android/vr_shell/test/paths.h:12: PATH_START = 10000,
On 2017/03/22 18:53:43, mthiesse wrote:
> It looks like the intent is to make this a unique number or something? 10000
is
> already used, maybe use 12000? I don't know the reasoning behind this.

Done.

[acondor_, 2017-03-24 15:12:11]

The CQ bit was checked by acondor@google.com

[acondor_, 2017-03-24 15:12:12]

The patchset sent to the CQ was uploaded after l-g-t-m from
mthiesse@chromium.org, bajones@chromium.org
Link to the patchset: https://codereview.chromium.org/2757213003/#ps80001
(title: "Addressing nit requests.")

[commit-bot: I haz the power, 2017-03-24 15:12:58]

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-03-24 18:07:51]

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

[commit-bot: I haz the power, 2017-03-24 18:07:52]

Failed to apply patch for chrome/browser/android/vr_shell/BUILD.gn:
While running git apply --index -p1;
  error: patch failed: chrome/browser/android/vr_shell/BUILD.gn:102
  error: chrome/browser/android/vr_shell/BUILD.gn: patch does not apply

Patch:       chrome/browser/android/vr_shell/BUILD.gn
Index: chrome/browser/android/vr_shell/BUILD.gn
diff --git a/chrome/browser/android/vr_shell/BUILD.gn
b/chrome/browser/android/vr_shell/BUILD.gn
index
03de7eef578d9a0a647cb82d64d67273c29bba55..c5af37cd32e71d9b311c3da00aaf5b74176aca82
100644
--- a/chrome/browser/android/vr_shell/BUILD.gn
+++ b/chrome/browser/android/vr_shell/BUILD.gn
@@ -20,6 +20,10 @@ if (current_cpu == "arm" || current_cpu == "arm64") {
       "animation.h",
       "easing.cc",
       "easing.h",
+      "gltf_asset.cc",
+      "gltf_asset.h",
+      "gltf_parser.cc",
+      "gltf_parser.h",
       "mailbox_to_surface_bridge.cc",
       "mailbox_to_surface_bridge.h",
       "non_presenting_gvr_delegate.cc",
@@ -102,6 +106,9 @@ if (current_cpu == "arm" || current_cpu == "arm64") {
 if (enable_webvr) {
   test("vr_shell_unittests") {
     sources = [
+      "gltf_parser_unittest.cc",
+      "test/paths.cc",
+      "test/paths.h",
       "ui_elements_unittest.cc",
       "ui_scene_unittest.cc",
     ]
@@ -115,5 +122,9 @@ if (enable_webvr) {
       "//third_party/WebKit/public:blink",
       "//ui/gfx/geometry",
     ]
+
+    data = [
+      "test/data/sample_inline.gltf",
+    ]
   }
 }

[acondor_, 2017-03-24 18:22:26]

Description was changed from

==========
Implementing glTF 1.0 parser

- Documentation at
https://github.com/KhronosGroup/glTF/tree/2.0/specification/1.0
- Elements which we will not be using are excluded, such as materials, textures,
shaders, skins. We might want to use them later when glTF 2.0 is released. For
now, we plan to handle this natively.
- Unittests are included.
- Small refactoring of the rendering classes inheritance to account for non-quad
objects, which will be used later for rendering the controller.
- Minor fixes on code I encountered.

BUG=644562
==========

to

==========
Implementing glTF 1.0 parser

- Documentation at
https://github.com/KhronosGroup/glTF/tree/2.0/specification/1.0
- Elements which we will not be using are excluded, such as materials, textures,
shaders, skins. We might want to use them later when glTF 2.0 is released. For
now, we plan to handle this natively.
- Unittests are included.
- Small refactoring of the rendering classes inheritance to account for non-quad
objects, which will be used later for rendering the controller.
- Minor fixes on code I encountered.

BUG=705006
==========

[acondor_, 2017-03-24 19:06:52]

The CQ bit was checked by acondor@google.com

[acondor_, 2017-03-24 19:06:53]

The patchset sent to the CQ was uploaded after l-g-t-m from
mthiesse@chromium.org, bajones@chromium.org
Link to the patchset: https://codereview.chromium.org/2757213003/#ps100001
(title: "Resolving merge.")

[commit-bot: I haz the power, 2017-03-24 19:07:42]

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-03-24 20:57:24]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1490382412910080,
"parent_rev": "d5514f08a923485a8ef2a58cab355893d0e55fcb", "commit_rev":
"a76f0b59ca704c56c693b2390a8033f834b08a3f"}

[commit-bot: I haz the power, 2017-03-24 20:58:14]

Description was changed from

==========
Implementing glTF 1.0 parser

- Documentation at
https://github.com/KhronosGroup/glTF/tree/2.0/specification/1.0
- Elements which we will not be using are excluded, such as materials, textures,
shaders, skins. We might want to use them later when glTF 2.0 is released. For
now, we plan to handle this natively.
- Unittests are included.
- Small refactoring of the rendering classes inheritance to account for non-quad
objects, which will be used later for rendering the controller.
- Minor fixes on code I encountered.

BUG=705006
==========

to

==========
Implementing glTF 1.0 parser

- Documentation at
https://github.com/KhronosGroup/glTF/tree/2.0/specification/1.0
- Elements which we will not be using are excluded, such as materials, textures,
shaders, skins. We might want to use them later when glTF 2.0 is released. For
now, we plan to handle this natively.
- Unittests are included.
- Small refactoring of the rendering classes inheritance to account for non-quad
objects, which will be used later for rendering the controller.
- Minor fixes on code I encountered.

BUG=705006

Review-Url: https://codereview.chromium.org/2757213003
Cr-Commit-Position: refs/heads/master@{#459547}
Committed:
https://chromium.googlesource.com/chromium/src/+/a76f0b59ca704c56c693b2390a80...
==========

[commit-bot: I haz the power, 2017-03-24 20:58:15]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/a76f0b59ca704c56c693b2390a80...