[fsp] Refactor handling operations.

[fsp] Refactor handling operations.

This CL reorganizes code before adding the read directory implementation.

First of all, operations are now extracted to separate classes, to avoid huge
file_system_provider.cc. Such operation classes are now arguments for the
RequestManager which replaces the former pair of [success, error] callbacks.

Such operation classes are now easy to test, because of ability to cut out
the EventRouter by using Operation::SetDispatchEventImplForTests.

Finally, a RequestValue class has been introduced, which replaces former
base::DictionaryValue. The former solution was not good, since we already
parse the base::Value in api function implementations (using generated from
IDL parsers). So, passing a raw base::Value and later parsing is again is an
overkill.

The RequestValue class is a trivial wrapper for all kinds of IDL values, which
can be passed through the RequestManager to Operation classes.

TEST=unit_tests, browser_tests: *FileSystemProvider*
BUG=248427
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=266516

[mtomasz, 2014-04-24 08:34:51]

@kinaba: PTAL. Thanks!

[kinaba, 2014-04-25 06:35:06]

https://codereview.chromium.org/257493004/diff/40001/chrome/browser/chromeos/...
File
chrome/browser/chromeos/extensions/file_system_provider/file_system_provider_api.cc
(right):

https://codereview.chromium.org/257493004/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/extensions/file_system_provider/file_system_provider_api.cc:195:
scoped_ptr<Params> params(Params::Create(*args_));
nit: The changes for this function looks unneccessary.

https://codereview.chromium.org/257493004/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/file_system_provider/operations/operation.h
(right):

https://codereview.chromium.org/257493004/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/operations/operation.h:18: class
Value;
nit: not used.

https://codereview.chromium.org/257493004/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/operations/operation.h:48: void
SetDispatchEventImplForTests(const DispatchEventImplCallback& callback);
ForTest, or ForTesting. (src/PRESUBMIT.py has a check for either of these two
patterns.)

https://codereview.chromium.org/257493004/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/operations/operation.h:58:
extensions::EventRouter* event_router_;
Does this field need to be protected? Can it go to private, or even, be removed?
At least in the current implementation dispatch_event_impl_ is perfectly
wrapping it.

https://codereview.chromium.org/257493004/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/file_system_provider/provided_file_system.h
(right):

https://codereview.chromium.org/257493004/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/provided_file_system.h:25: //
Provided file system implemenrtation. Forwards requests between providers and
nit: implementation

https://codereview.chromium.org/257493004/diff/40001/chrome/browser/chromeos/...
File
chrome/browser/chromeos/file_system_provider/provided_file_system_interface.h
(right):

https://codereview.chromium.org/257493004/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/provided_file_system_interface.h:42:
virtual base::WeakPtr<ProvidedFileSystemInterface> GetWeakPtr() = 0;
Do you have specific reason for this method to be public?

https://codereview.chromium.org/257493004/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/file_system_provider/request_manager_unittest.cc
(right):

https://codereview.chromium.org/257493004/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/request_manager_unittest.cc:111:
explicit FakeHandler(base::WeakPtr<EventLogger> logger, bool execute_reply)
nit: no need for explicit since it takes more than one parameter.

https://codereview.chromium.org/257493004/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/file_system_provider/request_value.h (right):

https://codereview.chromium.org/257493004/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/request_value.h:27: static
RequestValue* CreateForUnmountSuccess(
Returning scoped_ptr<RequestValue> may be slightly easier and safer.
Callers need to wrap it with scoped_ptr soon anyhow.

https://codereview.chromium.org/257493004/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/request_value.h:31: static
RequestValue* CreateForTesting(const std::string& params);
ditto

[mtomasz, 2014-04-28 00:42:46]

https://codereview.chromium.org/257493004/diff/40001/chrome/browser/chromeos/...
File
chrome/browser/chromeos/extensions/file_system_provider/file_system_provider_api.cc
(right):

https://codereview.chromium.org/257493004/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/extensions/file_system_provider/file_system_provider_api.cc:195:
scoped_ptr<Params> params(Params::Create(*args_));
On 2014/04/25 06:35:06, kinaba wrote:
> nit: The changes for this function looks unneccessary.

Done.

https://codereview.chromium.org/257493004/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/file_system_provider/operations/operation.h
(right):

https://codereview.chromium.org/257493004/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/operations/operation.h:18: class
Value;
On 2014/04/25 06:35:06, kinaba wrote:
> nit: not used.

Done.

https://codereview.chromium.org/257493004/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/operations/operation.h:48: void
SetDispatchEventImplForTests(const DispatchEventImplCallback& callback);
On 2014/04/25 06:35:06, kinaba wrote:
> ForTest, or ForTesting. (src/PRESUBMIT.py has a check for either of these two
> patterns.)

Good point. Done.

https://codereview.chromium.org/257493004/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/operations/operation.h:58:
extensions::EventRouter* event_router_;
On 2014/04/25 06:35:06, kinaba wrote:
> Does this field need to be protected? Can it go to private, or even, be
removed?
> At least in the current implementation dispatch_event_impl_ is perfectly
> wrapping it.

Removed. Done.

https://codereview.chromium.org/257493004/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/file_system_provider/provided_file_system.h
(right):

https://codereview.chromium.org/257493004/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/provided_file_system.h:25: //
Provided file system implemenrtation. Forwards requests between providers and
On 2014/04/25 06:35:06, kinaba wrote:
> nit: implementation

Done.

https://codereview.chromium.org/257493004/diff/40001/chrome/browser/chromeos/...
File
chrome/browser/chromeos/file_system_provider/provided_file_system_interface.h
(right):

https://codereview.chromium.org/257493004/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/provided_file_system_interface.h:42:
virtual base::WeakPtr<ProvidedFileSystemInterface> GetWeakPtr() = 0;
On 2014/04/25 06:35:06, kinaba wrote:
> Do you have specific reason for this method to be public?

This CL is a split of a big CL. I just removed this, since it should be added
later. Done.

https://codereview.chromium.org/257493004/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/file_system_provider/request_manager_unittest.cc
(right):

https://codereview.chromium.org/257493004/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/request_manager_unittest.cc:111:
explicit FakeHandler(base::WeakPtr<EventLogger> logger, bool execute_reply)
On 2014/04/25 06:35:06, kinaba wrote:
> nit: no need for explicit since it takes more than one parameter.

Done.

https://codereview.chromium.org/257493004/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/file_system_provider/request_value.h (right):

https://codereview.chromium.org/257493004/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/request_value.h:27: static
RequestValue* CreateForUnmountSuccess(
On 2014/04/25 06:35:06, kinaba wrote:
> Returning scoped_ptr<RequestValue> may be slightly easier and safer.
> Callers need to wrap it with scoped_ptr soon anyhow.

Done.

https://codereview.chromium.org/257493004/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/request_value.h:31: static
RequestValue* CreateForTesting(const std::string& params);
On 2014/04/25 06:35:06, kinaba wrote:
> ditto

Done.

[kinaba, 2014-04-28 00:58:14]

lgtm

[mtomasz, 2014-04-28 00:58:28]

The CQ bit was checked by mtomasz@chromium.org

[mtomasz, 2014-04-28 00:58:30]

The CQ bit was unchecked by mtomasz@chromium.org

[mtomasz, 2014-04-28 00:58:34]

On 2014/04/28 00:58:14, kinaba wrote:
> lgtm

thanks!

[mtomasz, 2014-04-28 00:58:38]

The CQ bit was checked by mtomasz@chromium.org

[commit-bot: I haz the power, 2014-04-28 00:59:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mtomasz@chromium.org/257493004/60001

[commit-bot: I haz the power, 2014-04-28 01:41:23]

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

[commit-bot: I haz the power, 2014-04-28 01:41:23]

Try jobs failed on following builders:
  tryserver.chromium on chromium_presubmit

[mtomasz, 2014-04-28 02:34:36]

The CQ bit was checked by mtomasz@chromium.org

[commit-bot: I haz the power, 2014-04-28 02:35:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mtomasz@chromium.org/257493004/80001

[commit-bot: I haz the power, 2014-04-28 02:44:08]

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

[commit-bot: I haz the power, 2014-04-28 02:44:08]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_rel

[mtomasz, 2014-04-28 04:05:08]

The CQ bit was checked by mtomasz@chromium.org

[commit-bot: I haz the power, 2014-04-28 04:05:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mtomasz@chromium.org/257493004/80001

[commit-bot: I haz the power, 2014-04-28 04:59:36]

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

[commit-bot: I haz the power, 2014-04-28 04:59:37]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_chromeos_rel

[mtomasz, 2014-04-28 05:23:53]

The CQ bit was checked by mtomasz@chromium.org

[commit-bot: I haz the power, 2014-04-28 05:24:35]

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

[commit-bot: I haz the power, 2014-04-28 06:07:34]

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

[commit-bot: I haz the power, 2014-04-28 06:07:34]

Try jobs failed on following builders:
  tryserver.chromium on chromium_presubmit

[mtomasz, 2014-04-28 06:10:45]

The CQ bit was checked by mtomasz@chromium.org

[commit-bot: I haz the power, 2014-04-28 06:12:07]

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

[commit-bot: I haz the power, 2014-04-28 12:12:34]

Change committed as 266516