Support synchronous mode for FileSystem/FileWriter API

Support synchronous mode for FileSystem/FileWriter API

BUG=257349
TEST=no behavioral changes (until we switch blink code)
R=michaeln@chromium.org

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

[kinuko, 2013-07-31 07:04:11]

Making the bridge code work synchronously.. can you take a look?

[michaeln, 2013-07-31 23:34:03]

i think this lgtm, but its gotten very dense in there as to what actually gets
run when

https://codereview.chromium.org/21091007/diff/1/content/child/fileapi/webfile...
File content/child/fileapi/webfilesystem_impl.cc (right):

https://codereview.chromium.org/21091007/diff/1/content/child/fileapi/webfile...
content/child/fileapi/webfilesystem_impl.cc:102: if
(callbacks->shouldBlockUntilCompletion())
Looks like this all assumes that a call that originates on the main thread will
never block until completion (there is no logic to delete 'this' in that case).
Is that right?

Maybe add a DCHECK here for that?

https://codereview.chromium.org/21091007/diff/1/content/child/fileapi/webfile...
content/child/fileapi/webfilesystem_impl.cc:120: WaitableCallbackResults() :
event_(new base::WaitableEvent(false, false)) {}
would manual reset make more sense since this is a one time event?

https://codereview.chromium.org/21091007/diff/1/content/child/fileapi/webfile...
content/child/fileapi/webfilesystem_impl.cc:146: waitable_results));
maybe bind NULL here for waitable_results since it's not used on the other code
path immediately below

[michaeln, 2013-07-31 23:51:27]

https://codereview.chromium.org/21091007/diff/1/content/child/fileapi/webfile...
File content/child/fileapi/webfilesystem_impl.cc (right):

https://codereview.chromium.org/21091007/diff/1/content/child/fileapi/webfile...
content/child/fileapi/webfilesystem_impl.cc:329: CurrentWorkerId(),
callbacks_id, waitable_results)),
Using base::Unretained(waitable_results) for the callback closure here, and
using scoped_ptr.Pass() for the last param to CallDispatcherOnMainThread (just
below)... might make ownership more clear.

[kinuko, 2013-08-01 06:12:26]

https://codereview.chromium.org/21091007/diff/1/content/child/fileapi/webfile...
File content/child/fileapi/webfilesystem_impl.cc (right):

https://codereview.chromium.org/21091007/diff/1/content/child/fileapi/webfile...
content/child/fileapi/webfilesystem_impl.cc:102: if
(callbacks->shouldBlockUntilCompletion())
On 2013/07/31 23:34:03, michaeln wrote:
> Looks like this all assumes that a call that originates on the main thread
will
> never block until completion (there is no logic to delete 'this' in that
case).
> Is that right?
> 
> Maybe add a DCHECK here for that?

Done.

https://codereview.chromium.org/21091007/diff/1/content/child/fileapi/webfile...
content/child/fileapi/webfilesystem_impl.cc:120: WaitableCallbackResults() :
event_(new base::WaitableEvent(false, false)) {}
On 2013/07/31 23:34:03, michaeln wrote:
> would manual reset make more sense since this is a one time event?

Done.

https://codereview.chromium.org/21091007/diff/1/content/child/fileapi/webfile...
content/child/fileapi/webfilesystem_impl.cc:146: waitable_results));
On 2013/07/31 23:34:03, michaeln wrote:
> maybe bind NULL here for waitable_results since it's not used on the other
code
> path immediately below

Done. Also added DCHECK(!waitable_results) in the main-thread code path.

https://codereview.chromium.org/21091007/diff/1/content/child/fileapi/webfile...
content/child/fileapi/webfilesystem_impl.cc:329: CurrentWorkerId(),
callbacks_id, waitable_results)),
On 2013/07/31 23:51:27, michaeln wrote:
> Using base::Unretained(waitable_results) for the callback closure here, and
> using scoped_ptr.Pass() for the last param to CallDispatcherOnMainThread (just
> below)... might make ownership more clear.

Partially done.

Pass() clears up before we pass the value to bind, so we need another ptr
variable... instead changed code to pass waitable_results with make_scoped_ptr
to CallDispatcherOnMainThread.  Also added base::Unretained to make it clearer.

[commit-bot: I haz the power, 2013-08-01 10:31:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kinuko@chromium.org/21091007/31001

[kinuko, 2013-08-01 13:51:33]

Committed patchset #2 manually as r215024 (presubmit successful).