gin test - add simple file access for JS

The mojo JS message header validation tests load text files that encode binary messages. A simple gin file access module would simplify writing the tests:

file.getSourceRootDirectory() - return the path to the root src directory

file.getFilesInDirectory(path) - return an array of the filenames found in path. Do not return subdirectory names or links.

file.readFileToString(path) - return the contents of the file as a string.

BUG=397554
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=286484

[hansmuller, 2014-07-25 23:29:11]

A little file access to simplify writing the Mojo message validation tests that
read text-encoded binary message files.

[abarth-chromium, 2014-07-27 03:26:47]

LGTM

https://codereview.chromium.org/419673004/diff/60001/gin/test/file.cc
File gin/test/file.cc (right):

https://codereview.chromium.org/419673004/diff/60001/gin/test/file.cc#newcode36
gin/test/file.cc:36: return gin::Converter<std::string>::ToV8(args->isolate(),
contents);
You should be able to make the return type of this function std::string and then
just return contents.

https://codereview.chromium.org/419673004/diff/60001/gin/test/file.cc#newcode43
gin/test/file.cc:43: return gin::Converter<std::string>::ToV8(isolate,
path.AsUTF8Unsafe());
Ditto

https://codereview.chromium.org/419673004/diff/60001/gin/test/file.cc#newcode58
gin/test/file.cc:58: return
gin::Converter<std::vector<std::string>>::ToV8(args->isolate(), names);
Ditto

https://codereview.chromium.org/419673004/diff/60001/gin/test/file.cc#newcode75
gin/test/file.cc:75: base::Bind(GetSourceRootDirectory, isolate))
There's no reason to base::Bind here.  gin::Arguments contains the isolate.

[hansmuller, 2014-07-28 16:44:59]

I'm using JS null return values (see below) so I think I need to use v8::Handle
for the JS method return types. If there's a better way, let me know.

https://codereview.chromium.org/419673004/diff/60001/gin/test/file.cc
File gin/test/file.cc (right):

https://codereview.chromium.org/419673004/diff/60001/gin/test/file.cc#newcode36
gin/test/file.cc:36: return gin::Converter<std::string>::ToV8(args->isolate(),
contents);
On 2014/07/27 03:26:47, abarth wrote:
> You should be able to make the return type of this function std::string and
then just return contents.

I'm returning JS null when no filename is specified or the file doesn't exist. I
don't know of a way to do that if the function just returns std::string?

Likewise for the other two file functions.

Note also: I added some tests to check the null cases.

https://codereview.chromium.org/419673004/diff/60001/gin/test/file.cc#newcode75
gin/test/file.cc:75: base::Bind(GetSourceRootDirectory, isolate))
On 2014/07/27 03:26:47, abarth wrote:
> There's no reason to base::Bind here.  gin::Arguments contains the isolate.

OK.

[abarth-chromium, 2014-07-29 16:47:04]

https://codereview.chromium.org/419673004/diff/100001/gin/test/file.cc
File gin/test/file.cc (right):

https://codereview.chromium.org/419673004/diff/100001/gin/test/file.cc#newcode36
gin/test/file.cc:36: return gin::Converter<std::string>::ToV8(args->isolate(),
contents);
We should use a type like base::NullableString16 instead of manually calling the
converter.  I don't see an 8-bit base::NullableString, but we should add one.

[abarth-chromium, 2014-07-29 16:56:34]

To clarify, this CL still LGTM.  The note about base::NullableString is a
suggestion for future improvements to this code.

[hansmuller, 2014-07-29 17:16:25]

Will commit this CL and then do a follow-on for NullableString8.

https://codereview.chromium.org/419673004/diff/100001/gin/test/file.cc
File gin/test/file.cc (right):

https://codereview.chromium.org/419673004/diff/100001/gin/test/file.cc#newcode36
gin/test/file.cc:36: return gin::Converter<std::string>::ToV8(args->isolate(),
contents);
On 2014/07/29 16:47:04, abarth wrote:
> We should use a type like base::NullableString16 instead of manually calling
the
> converter.  I don't see an 8-bit base::NullableString, but we should add one.

That sounds sensible. I will do that in a separate commit.

[hansmuller, 2014-07-29 17:16:33]

The CQ bit was checked by hansmuller@chromium.org

[commit-bot: I haz the power, 2014-07-29 17:17:56]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hansmuller@chromium.org/419673004/100001

[commit-bot: I haz the power, 2014-07-30 10:39:44]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_chromeos_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[commit-bot: I haz the power, 2014-07-30 11:44:49]

Change committed as 286484