mandoline filesystem: add a sqlite3 vfs to proxy filesystem usage.

mandoline filesystem: add a sqlite3 vfs to proxy filesystem usage.

This adds a //sql/mojo library which can be linked into preexisting
sqlite3 code which adds a new VFS which transparently proxies filesystem
usage to the mojo:filesystem application.

We create a new sql_apptests.mojo target, which currently has all the
sql connection_unittests.cc (minus 2 hard ones), all statement and
transaction unit tests and refactor the sql testing stuff so that we
have two implementations of an SQLTestBase class: one that uses files
raw and one that proxies to the filesystem process.

Notably, this patch does not implement file locking, which will have to
be implemented before we can safely use this, but will be a large enough
patch in and of itself that I'm punting on it for this patch.

BUG=493311
Committed: https://crrev.com/102ceb4133685db70ac15cb5bcb134a85736fe86
Cr-Commit-Position: refs/heads/master@{#335415}

[Elliot Glaysher, 2015-06-10 20:35:15]

erg@chromium.org changed reviewers:
+ shess@chromium.org, sky@chromium.org

[Elliot Glaysher, 2015-06-10 20:35:16]

sky: mojo-y stuff
shess: sql-y stuff

https://codereview.chromium.org/1176653002/diff/150001/mojo/util/capture_util.h
File mojo/util/capture_util.h (right):

https://codereview.chromium.org/1176653002/diff/150001/mojo/util/capture_util...
mojo/util/capture_util.h:11: 
This was moved from files_test_base.h since it's more general code. If you know
of a better place for me to put this, I'm all ears.

https://codereview.chromium.org/1176653002/diff/150001/sql/connection_unittes...
File sql/connection_unittest.cc (right):

https://codereview.chromium.org/1176653002/diff/150001/sql/connection_unittes...
sql/connection_unittest.cc:528: TEST_F(SQLConnectionTest, RazeLocked) {
This test will be reenabled on mojo once I get locking working, which will be
the next patch.

[sky, 2015-06-10 21:26:27]

I just looked at components/filesystem and mojo/ . I briefly looked at sql/mojo,
but shess is a better reviewer for the bulk of it.
LGTM

https://codereview.chromium.org/1176653002/diff/150001/sql/mojo/enable_mojo_f...
File sql/mojo/enable_mojo_fs.cc (right):

https://codereview.chromium.org/1176653002/diff/150001/sql/mojo/enable_mojo_f...
sql/mojo/enable_mojo_fs.cc:20: // Implementation of the sqlite3 Mojo proxying
vfs.
I'm assuming shess is reviewing all this.

https://codereview.chromium.org/1176653002/diff/150001/sql/mojo/enable_mojo_fs.h
File sql/mojo/enable_mojo_fs.h (right):

https://codereview.chromium.org/1176653002/diff/150001/sql/mojo/enable_mojo_f...
sql/mojo/enable_mojo_fs.h:19: ScopedMojoFilesystemVFS(filesystem::DirectoryPtr
directory);
explicit?

[Scott Hess - ex-Googler, 2015-06-10 22:35:46]

First pass to stir the pot on sql/ stuff.

https://codereview.chromium.org/1176653002/diff/150001/sql/connection.cc
File sql/connection.cc (right):

https://codereview.chromium.org/1176653002/diff/150001/sql/connection.cc#newc...
sql/connection.cc:639: sqlite3_vfs* vfs = sqlite3_vfs_find(NULL);
I don't think it's appropriate to change this code from depending on our code to
depending on SQLite depending on our code.  It should depend more directly.

[I see _why_ you did it, so I'll try to circle back with other thoughts as I
process the review.]

[[I didn't think of anything.  I still dislike it.]]

https://codereview.chromium.org/1176653002/diff/150001/sql/connection_unittes...
File sql/connection_unittest.cc (right):

https://codereview.chromium.org/1176653002/diff/150001/sql/connection_unittes...
sql/connection_unittest.cc:528: TEST_F(SQLConnectionTest, RazeLocked) {
On 2015/06/10 20:35:16, Elliot Glaysher wrote:
> This test will be reenabled on mojo once I get locking working, which will be
> the next patch.

Note that the current sql/ code depends on SQLite's test framework via the
import instructions in third_party/sqlite/README.chromium, and there are really
not dependable "SQLite works like SQLite" tests here.  Obviously some of that
will fall out as a side effect, but it may be that you should land some tests
proving that your VFS layer works like the default VFS on various fronts.

I'm not thinking so much "SELECT works, INSERT works, etc", more I'm thinking
"BEGIN" versus "BEGIN IMMEDIATE" versus "BEGIN EXCLUSIVE" work differently in
the expected way, or two transactions which start with BEGIN and try to upgrade
their lock get the expected result for one of them, or that PRAGMA locking_mode
(and any others we use in connection.cc) work right, and the different journal
modes work, etc.  The meta-level stuff which won't be flushed out directly
("SELECT works" should be flushed out pretty directly).

https://codereview.chromium.org/1176653002/diff/150001/sql/connection_unittes...
sql/connection_unittest.cc:584: WriteJunkToDatabase(true);
Would prefer a little bit more readable sense of what this does without having
to consult some header to see what the true and the false mean.

I realize the prior "wb" vs "rb+" may not be readable to all people :-).

https://codereview.chromium.org/1176653002/diff/150001/sql/mojo/enable_mojo_f...
File sql/mojo/enable_mojo_fs.cc (right):

https://codereview.chromium.org/1176653002/diff/150001/sql/mojo/enable_mojo_f...
sql/mojo/enable_mojo_fs.cc:47: filesystem::DirectoryPtr root_directory;
To make sure I'm clear on this - all databases for all connections used by all
clients of this will use this single root directory, right?

https://codereview.chromium.org/1176653002/diff/150001/sql/mojo/enable_mojo_f...
sql/mojo/enable_mojo_fs.cc:78:
GetFSFile(file).~InterfacePtr<filesystem::File>();
Symmetry-wise, why not:
   delete (&GetFSFile(file)) filesystem::~FilePtr();
?  I probably have the syntax wrong, basically just wondering why the placement
new is entirely different from what probably is the placement delete but maybe
isn't.

In the long run it might make sense to just construct/destruct the MojoVFSFile
instance itself, and let the contents unwind naturally.

https://codereview.chromium.org/1176653002/diff/150001/sql/mojo/enable_mojo_f...
sql/mojo/enable_mojo_fs.cc:111: return SQLITE_IOERR_SHORT_READ;
Good to know about!

https://codereview.chromium.org/1176653002/diff/150001/sql/mojo/enable_mojo_f...
sql/mojo/enable_mojo_fs.cc:207: // TODO(erg): Unsure if we need to manually
handle SQLITE_FCNTL_FILE_POINTER.
SQLITE_FCNTL_FILE_POINTER is magic, and is handled up in sqlite3_file_control()
in main.c.

https://codereview.chromium.org/1176653002/diff/150001/sql/mojo/enable_mojo_f...
sql/mojo/enable_mojo_fs.cc:215: return SQLITE_OK;
Returns a value, SQLITE_OK being 0 will just happen to work.  Instead return 0
and SQLite will use SQLITE_DEFAULT_SECTOR_SIZE.

https://codereview.chromium.org/1176653002/diff/150001/sql/mojo/enable_mojo_f...
sql/mojo/enable_mojo_fs.cc:221: return SQLITE_OK;
This returns a bitmask.  Again, SQLITE_OK happens to be 0 which will happen to
work, but probably better to just return 0.

https://codereview.chromium.org/1176653002/diff/150001/sql/mojo/enable_mojo_f...
sql/mojo/enable_mojo_fs.cc:238: 0,                            /* xShmMap */
You don't need to provide these for version 1.  If you're providing them for
future reference WRT things we discussed about memory-mapped files, I think you
need to go all the way to version 3.

I believe that the version 2 stuff is required to enable WAL mode (which I plan
to do across the board at some point), and version 3 is required to do
memory-mapped I/O (which I'd also like to enable by default at some point).

https://codereview.chromium.org/1176653002/diff/150001/sql/mojo/enable_mojo_f...
sql/mojo/enable_mojo_fs.cc:244: // who allocates sqlite3_file?
The caller provides szOsFile bytes as part of a greater allocation.  It's funky.

https://codereview.chromium.org/1176653002/diff/150001/sql/mojo/enable_mojo_f...
sql/mojo/enable_mojo_fs.cc:295: // TODO(erg): sync_dir?
This may have per-platform warts, unfortunately.  I haven't dug in to see what
they might be.  The default for sql::Connection is to use TRUNCATE mode, so the
transaction state is signaled by zero-sized log rather than missing log, so if
this is wrong we might never notice.  [I don't know what to make of that, but
*shrug*.]

https://codereview.chromium.org/1176653002/diff/150001/sql/mojo/enable_mojo_f...
sql/mojo/enable_mojo_fs.cc:308: bool is_wriable = false;
is_writable.

https://codereview.chromium.org/1176653002/diff/150001/sql/mojo/enable_mojo_f...
sql/mojo/enable_mojo_fs.cc:324: return SQLITE_IOERR;
So ... does someone guarantee that any file which exists is readable by the
current user?  I realize that the intention would be that things are managed,
etc, but in practice when someone on the Internet mucks up their filesystem,
will the error happen in a sensible place like here, or will it be come later
IOERR on read or write?

https://codereview.chromium.org/1176653002/diff/150001/sql/mojo/enable_mojo_f...
sql/mojo/enable_mojo_fs.cc:338: // folly.
IMHO you should upgrade this to "Don't let SQLite dynamically load things".  So
far it has never come up.

[Hmm, I see that we halfway-define SQLITE_OMIT_LOAD_EXTENSION , which seems
likely to be a bug.  With that this should never even be called, but I think it
would be best to code it to error rather than CHECK.]

https://codereview.chromium.org/1176653002/diff/150001/sql/mojo/enable_mojo_f...
sql/mojo/enable_mojo_fs.cc:358: }
This one in particular might make more sense to implement in terms of our
randomness generator.  The way SQLite's randomness pool is handled has some
funky edges, and we probably already have APIs to implement it in a few lines.

The other functions below might also make sense to implement that way, too. 
xGetLastError() was an experiment that didn't pan out, it could just return 0.

https://codereview.chromium.org/1176653002/diff/150001/sql/mojo/enable_mojo_f...
sql/mojo/enable_mojo_fs.cc:405: vfs_set_ = true;
This could be implemented in terms of a check on the current default vfs, rather
than a parallel check.

https://codereview.chromium.org/1176653002/diff/150001/sql/mojo/enable_mojo_f...
sql/mojo/enable_mojo_fs.cc:408: data->parent = sqlite3_vfs_find(kParentVFS);
Use NULL to say "The default VFS for this system", and restore the default when
you unregister.

https://codereview.chromium.org/1176653002/diff/150001/sql/mojo/enable_mojo_f...
sql/mojo/enable_mojo_fs.cc:417:
ScopedMojoFilesystemVFS::~ScopedMojoFilesystemVFS() {
Before clearing the pAppData, this needs to reset the default vfs to the
original, and then call sqlite3_vfs_unregister().  I don't know if
sqlite3_vfs_unregister() does the right thing WRT the new default if the
existing default is unregistered, but IMHO it would be  better to restore the
default from point of registration than to rely on that.

https://codereview.chromium.org/1176653002/diff/150001/sql/mojo/sql_test_base.cc
File sql/mojo/sql_test_base.cc (right):

https://codereview.chromium.org/1176653002/diff/150001/sql/mojo/sql_test_base...
sql/mojo/sql_test_base.cc:45: bool SQLTestBase::CorruptSizeInHeaderOfPath(const
base::FilePath& path) {
This duplication bugs me.  WDYT about a helper to read the header, another to
re-write the header, with a function to corrupt the header that composes the
two?  Alternately a function corrupt the header passed to a function to rewrite
the header?

https://codereview.chromium.org/1176653002/diff/150001/sql/mojo/sql_test_base.h
File sql/mojo/sql_test_base.h (right):

https://codereview.chromium.org/1176653002/diff/150001/sql/mojo/sql_test_base...
sql/mojo/sql_test_base.h:57: // Writes "Now is the winter of our discontent." to
the start of the file.
I don't think the string itself needs to be described.

[Elliot Glaysher, 2015-06-12 22:36:44]

https://codereview.chromium.org/1176653002/diff/150001/sql/connection.cc
File sql/connection.cc (right):

https://codereview.chromium.org/1176653002/diff/150001/sql/connection.cc#newc...
sql/connection.cc:639: sqlite3_vfs* vfs = sqlite3_vfs_find(NULL);
On 2015/06/10 22:35:45, Scott Hess wrote:
> I don't think it's appropriate to change this code from depending on our code
to
> depending on SQLite depending on our code.  It should depend more directly.
> 
> [I see _why_ you did it, so I'll try to circle back with other thoughts as I
> process the review.]
> 
> [[I didn't think of anything.  I still dislike it.]]

Per our conversation yesterday, I added a check on the name of the vfs to make
sure its one of the win32, unix or mojo vfses.

https://codereview.chromium.org/1176653002/diff/150001/sql/connection_unittes...
File sql/connection_unittest.cc (right):

https://codereview.chromium.org/1176653002/diff/150001/sql/connection_unittes...
sql/connection_unittest.cc:528: TEST_F(SQLConnectionTest, RazeLocked) {
On 2015/06/10 22:35:45, Scott Hess wrote:
> On 2015/06/10 20:35:16, Elliot Glaysher wrote:
> > This test will be reenabled on mojo once I get locking working, which will
be
> > the next patch.
> 
> Note that the current sql/ code depends on SQLite's test framework via the
> import instructions in third_party/sqlite/README.chromium, and there are
really
> not dependable "SQLite works like SQLite" tests here.  Obviously some of that
> will fall out as a side effect, but it may be that you should land some tests
> proving that your VFS layer works like the default VFS on various fronts.
> 
> I'm not thinking so much "SELECT works, INSERT works, etc", more I'm thinking
> "BEGIN" versus "BEGIN IMMEDIATE" versus "BEGIN EXCLUSIVE" work differently in
> the expected way, or two transactions which start with BEGIN and try to
upgrade
> their lock get the expected result for one of them, or that PRAGMA
locking_mode
> (and any others we use in connection.cc) work right, and the different journal
> modes work, etc.  The meta-level stuff which won't be flushed out directly
> ("SELECT works" should be flushed out pretty directly).

So, the sqlite test suite does have VFS tests...but they don't test any of that.
(In fact, I can't find the strings "BEGIN IMMEDIATE" or "BEGIN EXCLUSIVE" in any
chrome code.)

I've added a vfs_unittest.cc file with some basic corner cases. I'm not sure if
this is what you wanted.

(I'm also confused why this comment is here; I continue to intend to punt on
locking to get the basic implementation in. Locking will be done in the next
patch though.)

https://codereview.chromium.org/1176653002/diff/150001/sql/connection_unittes...
sql/connection_unittest.cc:584: WriteJunkToDatabase(true);
On 2015/06/10 22:35:45, Scott Hess wrote:
> Would prefer a little bit more readable sense of what this does without having
> to consult some header to see what the true and the false mean.
> 
> I realize the prior "wb" vs "rb+" may not be readable to all people :-).

Done.

https://codereview.chromium.org/1176653002/diff/150001/sql/mojo/enable_mojo_f...
File sql/mojo/enable_mojo_fs.cc (right):

https://codereview.chromium.org/1176653002/diff/150001/sql/mojo/enable_mojo_f...
sql/mojo/enable_mojo_fs.cc:47: filesystem::DirectoryPtr root_directory;
On 2015/06/10 22:35:46, Scott Hess wrote:
> To make sure I'm clear on this - all databases for all connections used by all
> clients of this will use this single root directory, right?

...within a service/origin combination.

The current model that I'm targeting is having a top level profile dir, and then
each subdir is passed to a service origin. (ie, the network service gets
sandboxed to the //profile/mojo:network_service/ directory.) Later, we're going
to have to deal with combinations of service/origin pairs (ie,
//profile/mojo:html_viewer/http___google.com/), but that's a bit out.

https://codereview.chromium.org/1176653002/diff/150001/sql/mojo/enable_mojo_f...
sql/mojo/enable_mojo_fs.cc:78:
GetFSFile(file).~InterfacePtr<filesystem::File>();
On 2015/06/10 22:35:45, Scott Hess wrote:
> Symmetry-wise, why not:
>    delete (&GetFSFile(file)) filesystem::~FilePtr();
> ?  I probably have the syntax wrong, basically just wondering why the
placement
> new is entirely different from what probably is the placement delete but maybe
> isn't.
> 
> In the long run it might make sense to just construct/destruct the MojoVFSFile
> instance itself, and let the contents unwind naturally.

Because there's no such thing as placement delete.

http://www.stroustrup.com/bs_faq2.html#placement-delete

https://codereview.chromium.org/1176653002/diff/150001/sql/mojo/enable_mojo_f...
sql/mojo/enable_mojo_fs.cc:207: // TODO(erg): Unsure if we need to manually
handle SQLITE_FCNTL_FILE_POINTER.
On 2015/06/10 22:35:45, Scott Hess wrote:
> SQLITE_FCNTL_FILE_POINTER is magic, and is handled up in
sqlite3_file_control()
> in main.c.

Done.

https://codereview.chromium.org/1176653002/diff/150001/sql/mojo/enable_mojo_f...
sql/mojo/enable_mojo_fs.cc:215: return SQLITE_OK;
On 2015/06/10 22:35:45, Scott Hess wrote:
> Returns a value, SQLITE_OK being 0 will just happen to work.  Instead return 0
> and SQLite will use SQLITE_DEFAULT_SECTOR_SIZE.

Done.

https://codereview.chromium.org/1176653002/diff/150001/sql/mojo/enable_mojo_f...
sql/mojo/enable_mojo_fs.cc:221: return SQLITE_OK;
On 2015/06/10 22:35:45, Scott Hess wrote:
> This returns a bitmask.  Again, SQLITE_OK happens to be 0 which will happen to
> work, but probably better to just return 0.

Done.

https://codereview.chromium.org/1176653002/diff/150001/sql/mojo/enable_mojo_f...
sql/mojo/enable_mojo_fs.cc:238: 0,                            /* xShmMap */
On 2015/06/10 22:35:45, Scott Hess wrote:
> You don't need to provide these for version 1.  If you're providing them for
> future reference WRT things we discussed about memory-mapped files, I think
you
> need to go all the way to version 3.
> 
> I believe that the version 2 stuff is required to enable WAL mode (which I
plan
> to do across the board at some point), and version 3 is required to do
> memory-mapped I/O (which I'd also like to enable by default at some point).

Deleted. (I had put it for future reference, but we'll cross that bridge later.)

https://codereview.chromium.org/1176653002/diff/150001/sql/mojo/enable_mojo_f...
sql/mojo/enable_mojo_fs.cc:244: // who allocates sqlite3_file?
On 2015/06/10 22:35:45, Scott Hess wrote:
> The caller provides szOsFile bytes as part of a greater allocation.  It's
funky.

(Comment was left over from a previous time when I didn't get this. should have
deleted it; the whole placement new thing depends on this and I have the
adjustment of szOsFile otherwise correct below. Deleted.)

https://codereview.chromium.org/1176653002/diff/150001/sql/mojo/enable_mojo_f...
sql/mojo/enable_mojo_fs.cc:295: // TODO(erg): sync_dir?
On 2015/06/10 22:35:45, Scott Hess wrote:
> This may have per-platform warts, unfortunately.  I haven't dug in to see what
> they might be.  The default for sql::Connection is to use TRUNCATE mode, so
the
> transaction state is signaled by zero-sized log rather than missing log, so if
> this is wrong we might never notice.  [I don't know what to make of that, but
> *shrug*.]

I have added a Flush() on sync_dir, which is what the unix vfs does. (syncDir is
unused on Windows.)

(I have also added a comment here about how windows has crazy retry behaviour
and that we might need to support that in the future.)

https://codereview.chromium.org/1176653002/diff/150001/sql/mojo/enable_mojo_f...
sql/mojo/enable_mojo_fs.cc:308: bool is_wriable = false;
On 2015/06/10 22:35:45, Scott Hess wrote:
> is_writable.

Done.

https://codereview.chromium.org/1176653002/diff/150001/sql/mojo/enable_mojo_f...
sql/mojo/enable_mojo_fs.cc:338: // folly.
On 2015/06/10 22:35:45, Scott Hess wrote:
> IMHO you should upgrade this to "Don't let SQLite dynamically load things". 
So
> far it has never come up.
> 
> [Hmm, I see that we halfway-define SQLITE_OMIT_LOAD_EXTENSION , which seems
> likely to be a bug.  With that this should never even be called, but I think
it
> would be best to code it to error rather than CHECK.]

Done.

https://codereview.chromium.org/1176653002/diff/150001/sql/mojo/enable_mojo_f...
sql/mojo/enable_mojo_fs.cc:358: }
On 2015/06/10 22:35:45, Scott Hess wrote:
> This one in particular might make more sense to implement in terms of our
> randomness generator.  The way SQLite's randomness pool is handled has some
> funky edges, and we probably already have APIs to implement it in a few lines.
> 
> The other functions below might also make sense to implement that way, too. 
> xGetLastError() was an experiment that didn't pan out, it could just return 0.

The timing stuff looks complicated enough that I'm not going to mess with it,
but I did the randomness and the last error suggestion.

https://codereview.chromium.org/1176653002/diff/150001/sql/mojo/enable_mojo_f...
sql/mojo/enable_mojo_fs.cc:417:
ScopedMojoFilesystemVFS::~ScopedMojoFilesystemVFS() {
On 2015/06/10 22:35:45, Scott Hess wrote:
> Before clearing the pAppData, this needs to reset the default vfs to the
> original, and then call sqlite3_vfs_unregister().  I don't know if
> sqlite3_vfs_unregister() does the right thing WRT the new default if the
> existing default is unregistered, but IMHO it would be  better to restore the
> default from point of registration than to rely on that.

These three comments about how we deal with the default fs are Done.

https://codereview.chromium.org/1176653002/diff/150001/sql/mojo/enable_mojo_fs.h
File sql/mojo/enable_mojo_fs.h (right):

https://codereview.chromium.org/1176653002/diff/150001/sql/mojo/enable_mojo_f...
sql/mojo/enable_mojo_fs.h:19: ScopedMojoFilesystemVFS(filesystem::DirectoryPtr
directory);
On 2015/06/10 21:26:27, sky wrote:
> explicit?

Done.

https://codereview.chromium.org/1176653002/diff/150001/sql/mojo/sql_test_base.cc
File sql/mojo/sql_test_base.cc (right):

https://codereview.chromium.org/1176653002/diff/150001/sql/mojo/sql_test_base...
sql/mojo/sql_test_base.cc:45: bool SQLTestBase::CorruptSizeInHeaderOfPath(const
base::FilePath& path) {
On 2015/06/10 22:35:46, Scott Hess wrote:
> This duplication bugs me.  WDYT about a helper to read the header, another to
> re-write the header, with a function to corrupt the header that composes the
> two?  Alternately a function corrupt the header passed to a function to
rewrite
> the header?

Created a corrupt the header function shared with the other impl, leaving this
function that just does the specific IO-y stuff.

https://codereview.chromium.org/1176653002/diff/150001/sql/mojo/sql_test_base.h
File sql/mojo/sql_test_base.h (right):

https://codereview.chromium.org/1176653002/diff/150001/sql/mojo/sql_test_base...
sql/mojo/sql_test_base.h:57: // Writes "Now is the winter of our discontent." to
the start of the file.
On 2015/06/10 22:35:46, Scott Hess wrote:
> I don't think the string itself needs to be described.

Done.

[Elliot Glaysher, 2015-06-16 20:46:40]

shess: friendly ping

[Scott Hess - ex-Googler, 2015-06-17 19:32:59]

https://codereview.chromium.org/1176653002/diff/150001/sql/connection.cc
File sql/connection.cc (right):

https://codereview.chromium.org/1176653002/diff/150001/sql/connection.cc#newc...
sql/connection.cc:639: sqlite3_vfs* vfs = sqlite3_vfs_find(NULL);
On 2015/06/12 22:36:43, Elliot Glaysher wrote:
> On 2015/06/10 22:35:45, Scott Hess wrote:
> > I don't think it's appropriate to change this code from depending on our
code
> to
> > depending on SQLite depending on our code.  It should depend more directly.
> > 
> > [I see _why_ you did it, so I'll try to circle back with other thoughts as I
> > process the review.]
> > 
> > [[I didn't think of anything.  I still dislike it.]]
> 
> Per our conversation yesterday, I added a check on the name of the vfs to make
> sure its one of the win32, unix or mojo vfses.

I'm happier with it, though not ecstatic.  At least we're saving some poor
client from having to know about it :-).

https://codereview.chromium.org/1176653002/diff/150001/sql/connection_unittes...
File sql/connection_unittest.cc (right):

https://codereview.chromium.org/1176653002/diff/150001/sql/connection_unittes...
sql/connection_unittest.cc:528: TEST_F(SQLConnectionTest, RazeLocked) {
On 2015/06/12 22:36:43, Elliot Glaysher wrote:
> On 2015/06/10 22:35:45, Scott Hess wrote:
> > On 2015/06/10 20:35:16, Elliot Glaysher wrote:
> > > This test will be reenabled on mojo once I get locking working, which will
> be
> > > the next patch.
> > 
> > Note that the current sql/ code depends on SQLite's test framework via the
> > import instructions in third_party/sqlite/README.chromium, and there are
> really
> > not dependable "SQLite works like SQLite" tests here.  Obviously some of
that
> > will fall out as a side effect, but it may be that you should land some
tests
> > proving that your VFS layer works like the default VFS on various fronts.
> > 
> > I'm not thinking so much "SELECT works, INSERT works, etc", more I'm
thinking
> > "BEGIN" versus "BEGIN IMMEDIATE" versus "BEGIN EXCLUSIVE" work differently
in
> > the expected way, or two transactions which start with BEGIN and try to
> upgrade
> > their lock get the expected result for one of them, or that PRAGMA
> locking_mode
> > (and any others we use in connection.cc) work right, and the different
journal
> > modes work, etc.  The meta-level stuff which won't be flushed out directly
> > ("SELECT works" should be flushed out pretty directly).
> 
> So, the sqlite test suite does have VFS tests...but they don't test any of
that.
> (In fact, I can't find the strings "BEGIN IMMEDIATE" or "BEGIN EXCLUSIVE" in
any
> chrome code.)
> 
> I've added a vfs_unittest.cc file with some basic corner cases. I'm not sure
if
> this is what you wanted.
> 
> (I'm also confused why this comment is here; I continue to intend to punt on
> locking to get the basic implementation in. Locking will be done in the next
> patch though.)

To be clear, I mean that as a general rule, Chromium doesn't really test that
SQLite works.  Not just locking, anything.  The Chromium tests will naturally
provide _some_ amount of coverage.  Mostly I'm just suggesting that while you're
implementing a specific vfs "method", be thinking of whether there are any
higher-level tests which might be in order to exercise that method to make sure
your implementation is hitting the special cases you're writing.

https://codereview.chromium.org/1176653002/diff/150001/sql/mojo/enable_mojo_f...
File sql/mojo/enable_mojo_fs.cc (right):

https://codereview.chromium.org/1176653002/diff/150001/sql/mojo/enable_mojo_f...
sql/mojo/enable_mojo_fs.cc:47: filesystem::DirectoryPtr root_directory;
On 2015/06/12 22:36:43, Elliot Glaysher wrote:
> On 2015/06/10 22:35:46, Scott Hess wrote:
> > To make sure I'm clear on this - all databases for all connections used by
all
> > clients of this will use this single root directory, right?
> 
> ...within a service/origin combination.
> 
> The current model that I'm targeting is having a top level profile dir, and
then
> each subdir is passed to a service origin. (ie, the network service gets
> sandboxed to the //profile/mojo:network_service/ directory.) Later, we're
going
> to have to deal with combinations of service/origin pairs (ie,
> //profile/mojo:html_viewer/http___google.com/), but that's a bit out.

So all connections used by all clients in a single process, since each process
has a global default vfs?

https://codereview.chromium.org/1176653002/diff/150001/sql/mojo/enable_mojo_f...
sql/mojo/enable_mojo_fs.cc:78:
GetFSFile(file).~InterfacePtr<filesystem::File>();
On 2015/06/12 22:36:44, Elliot Glaysher wrote:
> On 2015/06/10 22:35:45, Scott Hess wrote:
> > Symmetry-wise, why not:
> >    delete (&GetFSFile(file)) filesystem::~FilePtr();
> > ?  I probably have the syntax wrong, basically just wondering why the
> placement
> > new is entirely different from what probably is the placement delete but
maybe
> > isn't.
> > 
> > In the long run it might make sense to just construct/destruct the
MojoVFSFile
> > instance itself, and let the contents unwind naturally.
> 
> Because there's no such thing as placement delete.
> 
> http://www.stroustrup.com/bs_faq2.html#placement-delete

But you need to call the destructor, and as a casual reader, I have no idea
where InterfacePtr<>() is coming from, since this is the only point in the file
where it is referenced.  I can guess that InterfacePtr<filesystem::File> might
somehow relate to filesystem::FilePtr, but it seems like it would be more
straightforward to just call ~filesystem::FilePtr() directly.

https://codereview.chromium.org/1176653002/diff/210001/sql/connection.cc
File sql/connection.cc (right):

https://codereview.chromium.org/1176653002/diff/210001/sql/connection.cc#newc...
sql/connection.cc:610: std::string AsUTF8ForSQL(const base::FilePath& path) {
Please add this to Open()'s implementation while you're in here.

https://codereview.chromium.org/1176653002/diff/210001/sql/connection_unittes...
File sql/connection_unittest.cc (right):

https://codereview.chromium.org/1176653002/diff/210001/sql/connection_unittes...
sql/connection_unittest.cc:619:
WriteJunkToDatabase(SQLTestBase::TYPE_OPEN_AND_APPEND);
"wb" was truncate, but the "rb+" case is read/write overwriting from the
beginning of the file.  So in this case the file would still have many aspects
of "valid SQLite file".  Practically, there are multiple ways SQLite detects
badness, when originally written these two approaches threw different errors,
but now they're the same, so maybe the implementation changed to make the tests
redundant.

Beyond all that, I _think_ that "valid SQLite file with junk at the end" may be
acceptable under some circumstances.  I haven't personally understood those
circumstances, I've just seen comments that make me think that sometimes
truncation is expected not to work.

https://codereview.chromium.org/1176653002/diff/210001/sql/mojo/enable_mojo_f...
File sql/mojo/enable_mojo_fs.cc (right):

https://codereview.chromium.org/1176653002/diff/210001/sql/mojo/enable_mojo_f...
sql/mojo/enable_mojo_fs.cc:33: // Additional data that we store in the
sqlite3_vfs's |pAppData| member.
A weird thought just occurred to me.  Could pAppData just be a pointer to the
scoper?  Then the scoper could store these bits of info, and you could ignore
the management of the associated data for the vfs.

https://codereview.chromium.org/1176653002/diff/210001/sql/mojo/enable_mojo_f...
sql/mojo/enable_mojo_fs.cc:51: }
Why is this reinterpret_cast<> and GetDirectory() at the bottom of the file is
static_cast<>?  Seems like one or the other is wrong (and if this one should be
static_cast<>, then other reinterpret_cast<> calls might be questioned).

https://codereview.chromium.org/1176653002/diff/210001/sql/mojo/enable_mojo_f...
sql/mojo/enable_mojo_fs.cc:102: memcpy(buffer, &mojo_data.front(),
mojo_data.size());
Pull this out for sharing before the full-read test, I think.

https://codereview.chromium.org/1176653002/diff/210001/sql/mojo/enable_mojo_f...
sql/mojo/enable_mojo_fs.cc:289: // like that in the far future if we ever
support Windows.
Yeah, this sucks.  It would probably make more sense for the retry code to be on
the far side of the IPC interface, though.

https://codereview.chromium.org/1176653002/diff/210001/sql/mojo/enable_mojo_f...
sql/mojo/enable_mojo_fs.cc:426: CHECK(sqlite3_vfs_register(previous_fs, 1) ==
SQLITE_OK);
Suggest you restore the registration setup before invalidating the vfs.  It
won't hurt anything, and it would protect against unexpected occurrences like
SQLite making vfs calls as part of the implementation of unregister.

https://codereview.chromium.org/1176653002/diff/210001/sql/mojo/enable_mojo_f...
sql/mojo/enable_mojo_fs.cc:430: return
static_cast<MojoVFSData*>(mojo_vfs.pAppData)->root_directory;
Maybe this should be:
  return GetRootDirectory(&mojo_vfs);

https://codereview.chromium.org/1176653002/diff/210001/sql/mojo/vfs_unittest.cc
File sql/mojo/vfs_unittest.cc (right):

https://codereview.chromium.org/1176653002/diff/210001/sql/mojo/vfs_unittest....
sql/mojo/vfs_unittest.cc:99: // Opening a non-existent file exclusively should
work.
s/exclusively//;

https://codereview.chromium.org/1176653002/diff/210001/sql/mojo/vfs_unittest....
sql/mojo/vfs_unittest.cc:103: SQLITE_OPEN_CREATE  | SQLITE_OPEN_READWRITE,
Seems to be an excess space left of the |.  Also below.

https://codereview.chromium.org/1176653002/diff/210001/sql/mojo/vfs_unittest....
sql/mojo/vfs_unittest.cc:107: // Opening it a second time exclusively shouldn't.
"second time should work."

https://codereview.chromium.org/1176653002/diff/210001/sql/mojo/vfs_unittest....
sql/mojo/vfs_unittest.cc:269: char data_buffer[kBufferSize + 5];
Suggest pre-filling the buffer with something that you know won't match the
trailing nul characters.

https://codereview.chromium.org/1176653002/diff/210001/sql/mojo/vfs_unittest....
sql/mojo/vfs_unittest.cc:275: const int kBufferWithFiveNullsSize =
arraysize(kBuffer);
kBufferWithFiveNullsSize might make a good size for data_buffer.

https://codereview.chromium.org/1176653002/diff/210001/sql/test/sql_test_base.h
File sql/test/sql_test_base.h (right):

https://codereview.chromium.org/1176653002/diff/210001/sql/test/sql_test_base...
sql/test/sql_test_base.h:25: // version derives from something other than
mojo::test::ApplicationTestBase
s/something other than//;

https://codereview.chromium.org/1176653002/diff/210001/sql/test/sql_test_base...
sql/test/sql_test_base.h:57: bool CorruptSizeInHeaderOfPath(const
base::FilePath& path);
Should this assume db_path()?

[I was going to say the same about GetPathExists(), but that is used to test
auxiliary files, too.]

https://codereview.chromium.org/1176653002/diff/210001/sql/test/test_helpers.cc
File sql/test/test_helpers.cc (right):

https://codereview.chromium.org/1176653002/diff/210001/sql/test/test_helpers....
sql/test/test_helpers.cc:98: void CorruptSizeInHeaderMemory(unsigned char*
header, int64_t db_size) {
Makes sense to me!

[Elliot Glaysher, 2015-06-17 21:59:35]

https://codereview.chromium.org/1176653002/diff/150001/sql/mojo/enable_mojo_f...
File sql/mojo/enable_mojo_fs.cc (right):

https://codereview.chromium.org/1176653002/diff/150001/sql/mojo/enable_mojo_f...
sql/mojo/enable_mojo_fs.cc:47: filesystem::DirectoryPtr root_directory;
On 2015/06/17 19:32:58, Scott Hess wrote:
> On 2015/06/12 22:36:43, Elliot Glaysher wrote:
> > On 2015/06/10 22:35:46, Scott Hess wrote:
> > > To make sure I'm clear on this - all databases for all connections used by
> all
> > > clients of this will use this single root directory, right?
> > 
> > ...within a service/origin combination.
> > 
> > The current model that I'm targeting is having a top level profile dir, and
> then
> > each subdir is passed to a service origin. (ie, the network service gets
> > sandboxed to the //profile/mojo:network_service/ directory.) Later, we're
> going
> > to have to deal with combinations of service/origin pairs (ie,
> > //profile/mojo:html_viewer/http___google.com/), but that's a bit out.
> 
> So all connections used by all clients in a single process, since each process
> has a global default vfs?

All sql::Connections in a process will use the same vfs. That vfs in that one
process connects to the filesystem service. The filesystem service will vend a
directory which is origin bound to the origin of the calling program (currently
mojo://sql_apptests and soon mojo://network_service).

https://codereview.chromium.org/1176653002/diff/150001/sql/mojo/enable_mojo_f...
sql/mojo/enable_mojo_fs.cc:78:
GetFSFile(file).~InterfacePtr<filesystem::File>();
On 2015/06/17 19:32:58, Scott Hess wrote:
> On 2015/06/12 22:36:44, Elliot Glaysher wrote:
> > On 2015/06/10 22:35:45, Scott Hess wrote:
> > > Symmetry-wise, why not:
> > >    delete (&GetFSFile(file)) filesystem::~FilePtr();
> > > ?  I probably have the syntax wrong, basically just wondering why the
> > placement
> > > new is entirely different from what probably is the placement delete but
> maybe
> > > isn't.
> > > 
> > > In the long run it might make sense to just construct/destruct the
> MojoVFSFile
> > > instance itself, and let the contents unwind naturally.
> > 
> > Because there's no such thing as placement delete.
> > 
> > http://www.stroustrup.com/bs_faq2.html#placement-delete
> 
> But you need to call the destructor, and as a casual reader, I have no idea
> where InterfacePtr<>() is coming from, since this is the only point in the
file
> where it is referenced.  I can guess that InterfacePtr<filesystem::File> might
> somehow relate to filesystem::FilePtr, but it seems like it would be more
> straightforward to just call ~filesystem::FilePtr() directly.

Ah. I see what you're saying. Calling ~FilePtr(). (FilePtr is a typedef of
InterfacePtr<File>.)

(Have to use the using because you can't call ~filesystem::FilePtr() because you
can't use full qualifiers when manualy calling destructors.)

https://codereview.chromium.org/1176653002/diff/210001/sql/connection.cc
File sql/connection.cc (right):

https://codereview.chromium.org/1176653002/diff/210001/sql/connection.cc#newc...
sql/connection.cc:610: std::string AsUTF8ForSQL(const base::FilePath& path) {
On 2015/06/17 19:32:58, Scott Hess wrote:
> Please add this to Open()'s implementation while you're in here.

Done.

https://codereview.chromium.org/1176653002/diff/210001/sql/connection_unittes...
File sql/connection_unittest.cc (right):

https://codereview.chromium.org/1176653002/diff/210001/sql/connection_unittes...
sql/connection_unittest.cc:619:
WriteJunkToDatabase(SQLTestBase::TYPE_OPEN_AND_APPEND);
On 2015/06/17 19:32:58, Scott Hess wrote:
> "wb" was truncate, but the "rb+" case is read/write overwriting from the
> beginning of the file.  So in this case the file would still have many aspects
> of "valid SQLite file".  Practically, there are multiple ways SQLite detects
> badness, when originally written these two approaches threw different errors,
> but now they're the same, so maybe the implementation changed to make the
tests
> redundant.
> 
> Beyond all that, I _think_ that "valid SQLite file with junk at the end" may
be
> acceptable under some circumstances.  I haven't personally understood those
> circumstances, I've just seen comments that make me think that sometimes
> truncation is expected not to work.

Changed name.

("TYPE_OPEN_AND_APPEND" actually opened and wrote to the beginning of the file.
The name was wrong.)

https://codereview.chromium.org/1176653002/diff/210001/sql/mojo/enable_mojo_f...
File sql/mojo/enable_mojo_fs.cc (right):

https://codereview.chromium.org/1176653002/diff/210001/sql/mojo/enable_mojo_f...
sql/mojo/enable_mojo_fs.cc:33: // Additional data that we store in the
sqlite3_vfs's |pAppData| member.
On 2015/06/17 19:32:58, Scott Hess wrote:
> A weird thought just occurred to me.  Could pAppData just be a pointer to the
> scoper?  Then the scoper could store these bits of info, and you could ignore
> the management of the associated data for the vfs.

Done.

https://codereview.chromium.org/1176653002/diff/210001/sql/mojo/enable_mojo_f...
sql/mojo/enable_mojo_fs.cc:51: }
On 2015/06/17 19:32:58, Scott Hess wrote:
> Why is this reinterpret_cast<> and GetDirectory() at the bottom of the file is
> static_cast<>?  Seems like one or the other is wrong (and if this one should
be
> static_cast<>, then other reinterpret_cast<> calls might be questioned).

You can static_cast a void pointer to anything. You can only reinterpret_cast<>
a pointer of one type to another incompatible type.

You are correct that this one is wrong and can be static_cast. The usage in
GetFSFile is right (it must be a reinterpret_cast<>). GetDirectory was correct,
but is now gone due to stuffing MojoVFSData into the scoper.

https://codereview.chromium.org/1176653002/diff/210001/sql/mojo/enable_mojo_f...
sql/mojo/enable_mojo_fs.cc:102: memcpy(buffer, &mojo_data.front(),
mojo_data.size());
On 2015/06/17 19:32:58, Scott Hess wrote:
> Pull this out for sharing before the full-read test, I think.

Done, but we still have to check mojo_data.size() above since sqlite issues 0
reads.

https://codereview.chromium.org/1176653002/diff/210001/sql/mojo/enable_mojo_f...
sql/mojo/enable_mojo_fs.cc:426: CHECK(sqlite3_vfs_register(previous_fs, 1) ==
SQLITE_OK);
On 2015/06/17 19:32:58, Scott Hess wrote:
> Suggest you restore the registration setup before invalidating the vfs.  It
> won't hurt anything, and it would protect against unexpected occurrences like
> SQLite making vfs calls as part of the implementation of unregister.

Done.

https://codereview.chromium.org/1176653002/diff/210001/sql/mojo/vfs_unittest.cc
File sql/mojo/vfs_unittest.cc (right):

https://codereview.chromium.org/1176653002/diff/210001/sql/mojo/vfs_unittest....
sql/mojo/vfs_unittest.cc:103: SQLITE_OPEN_CREATE  | SQLITE_OPEN_READWRITE,
On 2015/06/17 19:32:58, Scott Hess wrote:
> Seems to be an excess space left of the |.  Also below.

Done.

https://codereview.chromium.org/1176653002/diff/210001/sql/mojo/vfs_unittest....
sql/mojo/vfs_unittest.cc:107: // Opening it a second time exclusively shouldn't.
On 2015/06/17 19:32:58, Scott Hess wrote:
> "second time should work."

Done.

https://codereview.chromium.org/1176653002/diff/210001/sql/mojo/vfs_unittest....
sql/mojo/vfs_unittest.cc:269: char data_buffer[kBufferSize + 5];
On 2015/06/17 19:32:58, Scott Hess wrote:
> Suggest pre-filling the buffer with something that you know won't match the
> trailing nul characters.

Done.

https://codereview.chromium.org/1176653002/diff/210001/sql/mojo/vfs_unittest....
sql/mojo/vfs_unittest.cc:275: const int kBufferWithFiveNullsSize =
arraysize(kBuffer);
On 2015/06/17 19:32:58, Scott Hess wrote:
> kBufferWithFiveNullsSize might make a good size for data_buffer.

Done.

https://codereview.chromium.org/1176653002/diff/210001/sql/test/sql_test_base.h
File sql/test/sql_test_base.h (right):

https://codereview.chromium.org/1176653002/diff/210001/sql/test/sql_test_base...
sql/test/sql_test_base.h:25: // version derives from something other than
mojo::test::ApplicationTestBase
On 2015/06/17 19:32:59, Scott Hess wrote:
> s/something other than//;

Done, and fixed in the mojo version too.

https://codereview.chromium.org/1176653002/diff/210001/sql/test/sql_test_base...
sql/test/sql_test_base.h:57: bool CorruptSizeInHeaderOfPath(const
base::FilePath& path);
On 2015/06/17 19:32:58, Scott Hess wrote:
> Should this assume db_path()?
> 
> [I was going to say the same about GetPathExists(), but that is used to test
> auxiliary files, too.]

Done.

[Scott Hess - ex-Googler, 2015-06-19 20:48:02]

LGTM, author's choice on the nits.

https://codereview.chromium.org/1176653002/diff/150001/sql/mojo/enable_mojo_f...
File sql/mojo/enable_mojo_fs.cc (right):

https://codereview.chromium.org/1176653002/diff/150001/sql/mojo/enable_mojo_f...
sql/mojo/enable_mojo_fs.cc:47: filesystem::DirectoryPtr root_directory;
On 2015/06/17 21:59:34, Elliot Glaysher wrote:
> On 2015/06/17 19:32:58, Scott Hess wrote:
> > On 2015/06/12 22:36:43, Elliot Glaysher wrote:
> > > On 2015/06/10 22:35:46, Scott Hess wrote:
> > > > To make sure I'm clear on this - all databases for all connections used
by
> > all
> > > > clients of this will use this single root directory, right?
> > > 
> > > ...within a service/origin combination.
> > > 
> > > The current model that I'm targeting is having a top level profile dir,
and
> > then
> > > each subdir is passed to a service origin. (ie, the network service gets
> > > sandboxed to the //profile/mojo:network_service/ directory.) Later, we're
> > going
> > > to have to deal with combinations of service/origin pairs (ie,
> > > //profile/mojo:html_viewer/http___google.com/), but that's a bit out.
> > 
> > So all connections used by all clients in a single process, since each
process
> > has a global default vfs?
> 
> All sql::Connections in a process will use the same vfs. That vfs in that one
> process connects to the filesystem service. The filesystem service will vend a
> directory which is origin bound to the origin of the calling program
(currently
> mojo://sql_apptests and soon mojo://network_service).

Acknowledged.

https://codereview.chromium.org/1176653002/diff/150001/sql/mojo/enable_mojo_f...
sql/mojo/enable_mojo_fs.cc:78:
GetFSFile(file).~InterfacePtr<filesystem::File>();
On 2015/06/17 21:59:34, Elliot Glaysher wrote:
> On 2015/06/17 19:32:58, Scott Hess wrote:
> > On 2015/06/12 22:36:44, Elliot Glaysher wrote:
> > > On 2015/06/10 22:35:45, Scott Hess wrote:
> > > > Symmetry-wise, why not:
> > > >    delete (&GetFSFile(file)) filesystem::~FilePtr();
> > > > ?  I probably have the syntax wrong, basically just wondering why the
> > > placement
> > > > new is entirely different from what probably is the placement delete but
> > maybe
> > > > isn't.
> > > > 
> > > > In the long run it might make sense to just construct/destruct the
> > MojoVFSFile
> > > > instance itself, and let the contents unwind naturally.
> > > 
> > > Because there's no such thing as placement delete.
> > > 
> > > http://www.stroustrup.com/bs_faq2.html#placement-delete
> > 
> > But you need to call the destructor, and as a casual reader, I have no idea
> > where InterfacePtr<>() is coming from, since this is the only point in the
> file
> > where it is referenced.  I can guess that InterfacePtr<filesystem::File>
might
> > somehow relate to filesystem::FilePtr, but it seems like it would be more
> > straightforward to just call ~filesystem::FilePtr() directly.
> 
> Ah. I see what you're saying. Calling ~FilePtr(). (FilePtr is a typedef of
> InterfacePtr<File>.)
> 
> (Have to use the using because you can't call ~filesystem::FilePtr() because
you
> can't use full qualifiers when manualy calling destructors.)

Ewwww.

https://codereview.chromium.org/1176653002/diff/210001/sql/mojo/enable_mojo_f...
File sql/mojo/enable_mojo_fs.cc (right):

https://codereview.chromium.org/1176653002/diff/210001/sql/mojo/enable_mojo_f...
sql/mojo/enable_mojo_fs.cc:102: memcpy(buffer, &mojo_data.front(),
mojo_data.size());
On 2015/06/17 21:59:34, Elliot Glaysher wrote:
> On 2015/06/17 19:32:58, Scott Hess wrote:
> > Pull this out for sharing before the full-read test, I think.
> 
> Done, but we still have to check mojo_data.size() above since sqlite issues 0
> reads.

Acknowledged.

https://codereview.chromium.org/1176653002/diff/230001/sql/connection.cc
File sql/connection.cc (right):

https://codereview.chromium.org/1176653002/diff/230001/sql/connection.cc#newc...
sql/connection.cc:323: return OpenInternal(AsUTF8ForSQL(path), RETRY_ON_POISON);
+1!  The placement of this #if has annoyed me for so long.

https://codereview.chromium.org/1176653002/diff/230001/sql/mojo/enable_mojo_f...
File sql/mojo/enable_mojo_fs.cc (right):

https://codereview.chromium.org/1176653002/diff/230001/sql/mojo/enable_mojo_f...
sql/mojo/enable_mojo_fs.cc:5: #include "sql/mojo/enable_mojo_fs.h"
Since this is in a mojo/ subdir, willing to defer to any naming pre-existing
naming scheme this is importing, but would this make more sense as "mojo_vfs"
"mojo_fs_vfs" or something like that describing what it is rather than the
reason it exists?

https://codereview.chromium.org/1176653002/diff/230001/sql/mojo/sql_test_base.h
File sql/mojo/sql_test_base.h (right):

https://codereview.chromium.org/1176653002/diff/230001/sql/mojo/sql_test_base...
sql/mojo/sql_test_base.h:36: TYPE_OPEN_NORMALLY
How about TYPE_OVERWRITE and TYPE_OVERWRITE_AND_TRUNCATE?  I believe the file
already exists in both cases, and the operation is more similar than the current
naming maybe indicates.

Another alternative would be WriteJunkDatabase vs WriteJunkToDatabase (or Into)
as function names and not bother with the additional type.

[Elliot Glaysher, 2015-06-19 21:07:18]

The CQ bit was checked by erg@chromium.org

[Elliot Glaysher, 2015-06-19 21:07:19]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org,
shess@chromium.org
Link to the patchset: https://codereview.chromium.org/1176653002/#ps250001
(title: "fix shess nits")

[commit-bot: I haz the power, 2015-06-19 21:07:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1176653002/250001

[commit-bot: I haz the power, 2015-06-19 22:38:17]

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

[commit-bot: I haz the power, 2015-06-19 22:38:18]

Try jobs failed on following builders:
  win8_chromium_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)

[Elliot Glaysher, 2015-06-19 23:12:45]

The CQ bit was checked by erg@chromium.org

[Elliot Glaysher, 2015-06-19 23:12:46]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org,
shess@chromium.org
Link to the patchset: https://codereview.chromium.org/1176653002/#ps270001
(title: "fix win 8")

[commit-bot: I haz the power, 2015-06-19 23:14:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1176653002/270001

[commit-bot: I haz the power, 2015-06-20 01:38:38]

Committed patchset #15 (id:270001)

[commit-bot: I haz the power, 2015-06-20 01:39:26]

Patchset 15 (id:??) landed as
https://crrev.com/102ceb4133685db70ac15cb5bcb134a85736fe86
Cr-Commit-Position: refs/heads/master@{#335415}