Support for parsing external file references in a glTF resource.

Support for parsing external file references in glTF resource.

The URI parsing was delegated to GURL and net::DataURL::Parse.

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

[acondor_, 2017-03-23 14:42:28]

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

[acondor_, 2017-03-23 14:42:29]

I got the external resource files on glTF working. Please have a look.

[mthiesse, 2017-03-23 15:00:11]

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

https://codereview.chromium.org/2774653002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/gltf_parser.cc:26: Clear();
You probably want to clear on the success case as well, so you're not holding
onto unnecessary memory.

https://codereview.chromium.org/2774653002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/gltf_parser.cc:33: // Use this method if the
glTF file references external files.
Comments should go in the header.

https://codereview.chromium.org/2774653002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/gltf_parser.cc:33: // Use this method if the
glTF file references external files.
It shouldn't matter whether your input is a dictionary or a file. Users should
be able to use either interface regardless of whether or not their glTF file
references external files. The comment should state that if their glTF
references external files, or if their source is a file, that parsing should not
be run on the UI thread.

[acondor_, 2017-03-23 15:29:22]

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

https://codereview.chromium.org/2774653002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/gltf_parser.cc:33: // Use this method if the
glTF file references external files.
On 2017/03/23 15:00:10, mthiesse wrote:
> It shouldn't matter whether your input is a dictionary or a file. Users should
> be able to use either interface regardless of whether or not their glTF file
> references external files. The comment should state that if their glTF
> references external files, or if their source is a file, that parsing should
not
> be run on the UI thread.

Just to clarify, there would be no way to process a dictionary that references
other files, because their path would be relative to the glTF directory.

[acondor_, 2017-03-23 16:28:12]

Hopefully these comments are more clear.

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

https://codereview.chromium.org/2774653002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/gltf_parser.cc:26: Clear();
On 2017/03/23 15:00:10, mthiesse wrote:
> You probably want to clear on the success case as well, so you're not holding
> onto unnecessary memory.

Done.

https://codereview.chromium.org/2774653002/diff/20001/chrome/browser/android/...
chrome/browser/android/vr_shell/gltf_parser.cc:33: // Use this method if the
glTF file references external files.
On 2017/03/23 15:29:21, acondor wrote:
> On 2017/03/23 15:00:10, mthiesse wrote:
> > It shouldn't matter whether your input is a dictionary or a file. Users
should
> > be able to use either interface regardless of whether or not their glTF file
> > references external files. The comment should state that if their glTF
> > references external files, or if their source is a file, that parsing should
> not
> > be run on the UI thread.
> 
> Just to clarify, there would be no way to process a dictionary that references
> other files, because their path would be relative to the glTF directory.

Done.

[mthiesse, 2017-03-24 14:14:49]

lgtm

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

https://codereview.chromium.org/2774653002/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/gltf_parser.h:33: // If so, run it off of the UI
thread.
Actually, instead of telling users what thread to (not) use, we should just warn
them that this function will perform IO.

"// Note: If your gltf references external files this function will perform IO,
and a base path must be specified."

https://codereview.chromium.org/2774653002/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/gltf_parser.h:37: // Call this method out off
the UI thread.
// Note: This function will perform IO.

[acondor_, 2017-03-24 14:52:00]

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

https://codereview.chromium.org/2774653002/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/gltf_parser.h:33: // If so, run it off of the UI
thread.
On 2017/03/24 14:14:49, mthiesse wrote:
> Actually, instead of telling users what thread to (not) use, we should just
warn
> them that this function will perform IO.
> 
> "// Note: If your gltf references external files this function will perform
IO,
> and a base path must be specified."

Done.

https://codereview.chromium.org/2774653002/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/gltf_parser.h:37: // Call this method out off
the UI thread.
On 2017/03/24 14:14:49, mthiesse wrote:
> // Note: This function will perform IO.

Done.

[cjgrant, 2017-03-24 15:11:54]

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

[cjgrant, 2017-03-24 15:11:55]

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

https://codereview.chromium.org/2774653002/diff/60001/chrome/browser/android/...
chrome/browser/android/vr_shell/gltf_parser.cc:28: Clear();
I don't think its warranted here, but if this code were much more complex, we'd
want to look at either:
- making a helper object to contain the parsing state, that's scoped to the
Parse() function and deletes itself when parsing is done, or
- something like base/scoped_generic.h

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

https://codereview.chromium.org/2774653002/diff/60001/chrome/browser/android/...
chrome/browser/android/vr_shell/gltf_parser_unittest.cc:114: }  // namespace
vr_shell
Could you add at least one test that expects a failure if your model is broken? 
Can be any of the cases that cause a failure.

[acondor_, 2017-03-24 18:00:56]

I scoped the holders and added tests for parsing failures.

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

https://codereview.chromium.org/2774653002/diff/60001/chrome/browser/android/...
chrome/browser/android/vr_shell/gltf_parser.cc:28: Clear();
On 2017/03/24 15:11:54, cjgrant wrote:
> I don't think its warranted here, but if this code were much more complex,
we'd
> want to look at either:
> - making a helper object to contain the parsing state, that's scoped to the
> Parse() function and deletes itself when parsing is done, or
> - something like base/scoped_generic.h

I went for the first suggestion.

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

https://codereview.chromium.org/2774653002/diff/60001/chrome/browser/android/...
chrome/browser/android/vr_shell/gltf_parser_unittest.cc:114: }  // namespace
vr_shell
On 2017/03/24 15:11:54, cjgrant wrote:
> Could you add at least one test that expects a failure if your model is
broken? 
> Can be any of the cases that cause a failure.

Done.

[ddorwin, 2017-03-24 18:06:20]

You might clarify in the description that this is adding support for loading
glTF files (or loading glTF models from files). "External" could be interpreted
as from outside Chrome, as in from websites.

[mthiesse, 2017-03-24 18:11:10]

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

https://codereview.chromium.org/2774653002/diff/80001/chrome/browser/android/...
chrome/browser/android/vr_shell/gltf_parser.cc:30: helper_ = &helper;
Okay, this is worse than what you had before ;)

I'd suggest reverting to the last patchset and using

base::ScopedClosureRunner runner(base::Bind(&GltfParser::Clear,
base::Unretained(this)));

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

Description was changed from

==========
Support for external files in glTF.

The URI parsing was delegated to GURL and net::DataURL::Parse.

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

to

==========
Support for external files in glTF.

The URI parsing was delegated to GURL and net::DataURL::Parse.

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

[acondor_, 2017-03-24 20:36:13]

After a difficult rebasing :(

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

https://codereview.chromium.org/2774653002/diff/80001/chrome/browser/android/...
chrome/browser/android/vr_shell/gltf_parser.cc:30: helper_ = &helper;
On 2017/03/24 18:11:10, mthiesse wrote:
> Okay, this is worse than what you had before ;)
> 
> I'd suggest reverting to the last patchset and using
> 
> base::ScopedClosureRunner runner(base::Bind(&GltfParser::Clear,
> base::Unretained(this)));

Done.

[acondor_, 2017-03-24 20:45:41]

Description was changed from

==========
Support for external files in glTF.

The URI parsing was delegated to GURL and net::DataURL::Parse.

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

to

==========
Support for parsing external file references in glTF resource.

The URI parsing was delegated to GURL and net::DataURL::Parse.

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

[mthiesse, 2017-03-25 15:02:28]

lgtm

[cjgrant, 2017-03-27 14:16:33]

lgtm

[acondor_, 2017-03-27 16:42:00]

The CQ bit was checked by acondor@google.com

[commit-bot: I haz the power, 2017-03-27 16:42:46]

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-27 21:14:56]

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

[commit-bot: I haz the power, 2017-03-27 21:14:57]

Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[acondor_, 2017-03-27 21:21:06]

The CQ bit was checked by acondor@google.com

[commit-bot: I haz the power, 2017-03-27 21:21:48]

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-27 22:53:34]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1490649666704510,
"parent_rev": "5268cf8dc47f95fc4694a71b6c5fca4aab039177", "commit_rev":
"be44edfc498758ae90777f88f80dff56b7fb586c"}

[commit-bot: I haz the power, 2017-03-27 22:54:31]

Description was changed from

==========
Support for parsing external file references in glTF resource.

The URI parsing was delegated to GURL and net::DataURL::Parse.

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

to

==========
Support for parsing external file references in glTF resource.

The URI parsing was delegated to GURL and net::DataURL::Parse.

BUG=705006

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

[commit-bot: I haz the power, 2017-03-27 22:54:32]

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