mojo leveldb: Remove the created file thread.

mojo leveldb: Remove the created file thread.

This runs what was a background thread for file operations on the same
thread as the mojo application, which is the db thread in chrome, and
the main thread when run as an app.

BUG=585587
Committed: https://crrev.com/b3d5935f0e9ca0d610a2b73c227d2d71dc31cf24
Cr-Commit-Position: refs/heads/master@{#383799}

[Elliot Glaysher, 2016-03-28 23:49:42]

erg@chromium.org changed reviewers:
+ jam@chromium.org

[jam, 2016-03-29 05:58:00]

https://codereview.chromium.org/1839823002/diff/20001/components/leveldb/leve...
File components/leveldb/leveldb_mojo_proxy.cc (right):

https://codereview.chromium.org/1839823002/diff/20001/components/leveldb/leve...
components/leveldb/leveldb_mojo_proxy.cc:43: // reply from mojo do we return
from this.
nit: no point in repeating this comment everywhere. or even writing it at all,
since it's an obvious comment one can see from reading the code, per
https://google.github.io/styleguide/cppguide.html#Implementation_Comments

https://codereview.chromium.org/1839823002/diff/20001/components/leveldb/leve...
components/leveldb/leveldb_mojo_proxy.cc:65: done_event.Wait();
since this code block is duplicated in all methods, and also the Impls need to
run the even if it exists. would it be simpler to instead have a member method
like


void RunInternal(const Closure& task) {
  if (task_runner_->BelongsToCurrentThread()) {
   task.Run();
  } else {
    base::WaitableEvent done_event(false, false);
    task_runner_->PostTaskAndReply(FROM_HERE, task,
base::Bind(&base::WaitableEvent::Signal, base::Unretained(done_event)));
  }
}

and then each of these proxy methods can just be
Runinternal(base::Bind(LevelDBMojoProxy::FooImpl, this, ...)); ?

https://codereview.chromium.org/1839823002/diff/20001/components/leveldb/leve...
File components/leveldb/leveldb_mojo_proxy.h (right):

https://codereview.chromium.org/1839823002/diff/20001/components/leveldb/leve...
components/leveldb/leveldb_mojo_proxy.h:23: // MojoEnv is an object passed to
the leveldb implementation which can be
nit: just to be consistent with how these projects are named: mojo->Mojo,
leveldb->LevelDB

https://codereview.chromium.org/1839823002/diff/20001/components/leveldb/leve...
components/leveldb/leveldb_mojo_proxy.h:56: // Synchronously flushes
|directory_|.
nit: s/flushes/sync

[Elliot Glaysher, 2016-03-29 17:33:29]

https://codereview.chromium.org/1839823002/diff/20001/components/leveldb/leve...
File components/leveldb/leveldb_mojo_proxy.cc (right):

https://codereview.chromium.org/1839823002/diff/20001/components/leveldb/leve...
components/leveldb/leveldb_mojo_proxy.cc:65: done_event.Wait();
On 2016/03/29 05:58:00, jam wrote:
> since this code block is duplicated in all methods, and also the Impls need to
> run the even if it exists. would it be simpler to instead have a member method
> like
> 
> 
> void RunInternal(const Closure& task) {
>   if (task_runner_->BelongsToCurrentThread()) {
>    task.Run();
>   } else {
>     base::WaitableEvent done_event(false, false);
>     task_runner_->PostTaskAndReply(FROM_HERE, task,
> base::Bind(&base::WaitableEvent::Signal, base::Unretained(done_event)));
>   }
> }
> 
> and then each of these proxy methods can just be
> Runinternal(base::Bind(LevelDBMojoProxy::FooImpl, this, ...)); ?

So how does the WaitableEvent block the current message loop then?

This code is being called from leveldb internals and must complete
synchronously. I must wait on the event immediately after the
PostTask(AndReply). If I use PostTaskAndReply, I don't have a MessageLoop to
wait on.

Going with the jist of the idea though, I did rework this to have a
RunInternal(const base::Callback<void(WaitableEvent*)>&) though.

[jam, 2016-03-29 19:16:48]

lgtm

[Elliot Glaysher, 2016-03-29 19:17:49]

The CQ bit was checked by erg@chromium.org

[commit-bot: I haz the power, 2016-03-29 19:18:08]

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

[commit-bot: I haz the power, 2016-03-29 20:09:47]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-03-29 20:12:01]

Description was changed from

==========
mojo leveldb: Remove the created file thread.

This runs what was a background thread for file operations on the same
thread as the mojo application, which is the db thread in chrome, and
the main thread when run as an app.

BUG=585587
==========

to

==========
mojo leveldb: Remove the created file thread.

This runs what was a background thread for file operations on the same
thread as the mojo application, which is the db thread in chrome, and
the main thread when run as an app.

BUG=585587

Committed: https://crrev.com/b3d5935f0e9ca0d610a2b73c227d2d71dc31cf24
Cr-Commit-Position: refs/heads/master@{#383799}
==========

[commit-bot: I haz the power, 2016-03-29 20:12:02]

Patchset 4 (id:??) landed as
https://crrev.com/b3d5935f0e9ca0d610a2b73c227d2d71dc31cf24
Cr-Commit-Position: refs/heads/master@{#383799}