Implement generic CreateCopyResultCallback and RunAndQuit.

Implement generic CreateCopyResultCallback and RunAndQuit.

Many drive methods returns result via callbacks, but there are variety of
types. Also, in test code, we copy the result value in the callback, and check
the expectation after the invocation. As a result, many
CopyResultFromXxxCallback(AndQuit) methods are defined.
The methods introduced by this CL are testing utilities for the code.
We can eliminate boilerplate testing utilities by this (roughly, 550-600 LoC). The actual elimination
will be done in following CLs.

BUG=180569
TEST=Ran unit_tests


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

[hidehiko, 2013-03-12 16:25:31]

Thank you for your review in advance,
- hidehiko

[hashimoto, 2013-03-13 03:34:00]

https://codereview.chromium.org/12785003/diff/23001/chrome/browser/google_api...
File chrome/browser/google_apis/test_util.h (right):

https://codereview.chromium.org/12785003/diff/23001/chrome/browser/google_api...
chrome/browser/google_apis/test_util.h:195: // architecture, so we defined lots
of callback to receive results of
"so we defined lots of callback" and the "callback" in the next sentence are
confusing.
Maybe we can remove "so we defined..." from this sentence?

https://codereview.chromium.org/12785003/diff/23001/chrome/browser/google_api...
chrome/browser/google_apis/test_util.h:200: //   class Operation;  // Testee
Operation class.
This comment seems a bit verbose.
Can't we simplify the example like this?

ResultType1 result1;
ResultType2 result2;

PerformSomeAsynchronousTask(param1, param2, CreateCopyResultCallback(&result1,
&result2));
test_util::RunBlockingPoolTask()  // Run message loop to complete the
asynchronous task.

https://codereview.chromium.org/12785003/diff/23001/chrome/browser/google_api...
chrome/browser/google_apis/test_util.h:240: template<bool IsNonMovableClass,
typename T> struct InTypeHelper {
All other parts are dealing with IsMovable, only this part is dealing with the
negation, IsNonMovable.
How about something like "UseConstRef"?

https://codereview.chromium.org/12785003/diff/23001/chrome/browser/google_api...
chrome/browser/google_apis/test_util.h:247: // Simulate the move operation. To
avoid value copy in argument, the method
s/Simulate/Simulates/?

"To avoid value copy" does not apply to scoped_ptr, the argument must be pointer
because scoped_ptr::Pass has side effects.
Can we remove this part from the comment?

https://codereview.chromium.org/12785003/diff/23001/chrome/browser/google_api...
chrome/browser/google_apis/test_util.h:268: void CopyResultCallback1(
Naming functions in Name1, Name2, ... looks worse than sharing one name.
Could you refine these function names?

https://codereview.chromium.org/12785003/diff/23001/chrome/browser/google_api...
chrome/browser/google_apis/test_util.h:271: *out =
CopyResultCallbackHelper<T1>::MoveHelperType::Move(&in);
How about adding a static method Move to CopyResultCallbackHelper?

[kinaba, 2013-03-13 04:07:44]

https://codereview.chromium.org/12785003/diff/23001/chrome/browser/google_api...
File chrome/browser/google_apis/test_util.h (right):

https://codereview.chromium.org/12785003/diff/23001/chrome/browser/google_api...
chrome/browser/google_apis/test_util.h:260: base::is_class<T>::value &&
!IsMovable<T>::value, T>::InType InType;
I don't feel it worth caring whether is_class<T> or not. Why don't we check just
IsMovable<T> and merge InTypeHelper into MoveHelper.

Then perhaps MoveHelper will be mergable to CopyResultCallbackHelper, like:
template<typename T, bool Movable=IsMovable<T>::value>
strcut CopyResultCallbackHelper { ...movable-case... };
template<typename T>
strcut CopyResultCallbackHelper<T, false> { ...non-movable-case... };

[kinaba, 2013-03-13 04:18:13]

https://codereview.chromium.org/12785003/diff/23001/chrome/browser/google_api...
File chrome/browser/google_apis/test_util.h (right):

https://codereview.chromium.org/12785003/diff/23001/chrome/browser/google_api...
chrome/browser/google_apis/test_util.h:260: base::is_class<T>::value &&
!IsMovable<T>::value, T>::InType InType;
Or even,

template<typename T>
struct CopyResultCallbackHelper { non-movable cases };
template<typename T>
struct CopyResultCallbackHelperForMovableTypes { movable cases };

struct CopyResultCallbackHelper<scoped_ptr>
 : CopyResultCallbackHelperForMovableTypes<scoped_ptr> {};

no need for true_type/false_type.

[hidehiko, 2013-03-13 05:42:10]

Thank you for your review. PTAL?

https://codereview.chromium.org/12785003/diff/23001/chrome/browser/google_api...
File chrome/browser/google_apis/test_util.h (right):

https://codereview.chromium.org/12785003/diff/23001/chrome/browser/google_api...
chrome/browser/google_apis/test_util.h:195: // architecture, so we defined lots
of callback to receive results of
On 2013/03/13 03:34:00, hashimoto wrote:
> "so we defined lots of callback" and the "callback" in the next sentence are
> confusing.
> Maybe we can remove "so we defined..." from this sentence?

Refined. How about this?

https://codereview.chromium.org/12785003/diff/23001/chrome/browser/google_api...
chrome/browser/google_apis/test_util.h:200: //   class Operation;  // Testee
Operation class.
On 2013/03/13 03:34:00, hashimoto wrote:
> This comment seems a bit verbose.
> Can't we simplify the example like this?
> 
> ResultType1 result1;
> ResultType2 result2;
> 
> PerformSomeAsynchronousTask(param1, param2, CreateCopyResultCallback(&result1,
> &result2));
> test_util::RunBlockingPoolTask()  // Run message loop to complete the
> asynchronous task.

Done.

https://codereview.chromium.org/12785003/diff/23001/chrome/browser/google_api...
chrome/browser/google_apis/test_util.h:240: template<bool IsNonMovableClass,
typename T> struct InTypeHelper {
On 2013/03/13 03:34:00, hashimoto wrote:
> All other parts are dealing with IsMovable, only this part is dealing with the
> negation, IsNonMovable.
> How about something like "UseConstRef"?

Done.

https://codereview.chromium.org/12785003/diff/23001/chrome/browser/google_api...
chrome/browser/google_apis/test_util.h:247: // Simulate the move operation. To
avoid value copy in argument, the method
On 2013/03/13 03:34:00, hashimoto wrote:
> s/Simulate/Simulates/?
> 
> "To avoid value copy" does not apply to scoped_ptr, the argument must be
pointer
> because scoped_ptr::Pass has side effects.
> Can we remove this part from the comment?

OK. Refined the comment, but please let me keep mentioning about the pointer.

https://codereview.chromium.org/12785003/diff/23001/chrome/browser/google_api...
chrome/browser/google_apis/test_util.h:260: base::is_class<T>::value &&
!IsMovable<T>::value, T>::InType InType;
On 2013/03/13 04:18:13, kinaba wrote:
> Or even,
> 
> template<typename T>
> struct CopyResultCallbackHelper { non-movable cases };
> template<typename T>
> struct CopyResultCallbackHelperForMovableTypes { movable cases };
> 
> struct CopyResultCallbackHelper<scoped_ptr>
>  : CopyResultCallbackHelperForMovableTypes<scoped_ptr> {};
> 
> no need for true_type/false_type.

There are three kinds of types here.
- non-class type (e.g. primitive types, enums).
  The input type is |T|. It is not "move" supported,
  so copy the value.
- class type without chrome's move support (e.g. base::FilePath)
  The input type is |const T&|. Copy the value.
- class type with chrome's move support (e.g. scoped_ptr<U>).
  The input type is |T|. Move the value via Pass() method.

Thus it is necessary to check is_class here.
Changed the implementation to use inheritance.

https://codereview.chromium.org/12785003/diff/23001/chrome/browser/google_api...
chrome/browser/google_apis/test_util.h:268: void CopyResultCallback1(
On 2013/03/13 03:34:00, hashimoto wrote:
> Naming functions in Name1, Name2, ... looks worse than sharing one name.
> Could you refine these function names?

Hmm, I thought we need to static_cast to resolve function pointer of the
overloaded methods, but for templates (at least) g++ correctly resolves the
overloading by the number of template parameters. I'm trying to send this to
try-bot, but it seems stuck now, unfortunately. I'll send it later, and if it
turned out one of our compilers failed let me revert to names with the arity
suffix (instead of casting).

https://codereview.chromium.org/12785003/diff/23001/chrome/browser/google_api...
chrome/browser/google_apis/test_util.h:271: *out =
CopyResultCallbackHelper<T1>::MoveHelperType::Move(&in);
On 2013/03/13 03:34:00, hashimoto wrote:
> How about adding a static method Move to CopyResultCallbackHelper?

Inherited by MoveHelper.

[hashimoto, 2013-03-13 06:27:52]

lgtm

https://codereview.chromium.org/12785003/diff/32001/chrome/browser/google_api...
File chrome/browser/google_apis/test_util.h (right):

https://codereview.chromium.org/12785003/diff/32001/chrome/browser/google_api...
chrome/browser/google_apis/test_util.h:257: :
InTypeHelper<base::is_class<T>::value && !IsMovable<T>::value, T>,
nit: Please add a comment about that this boolean expression corresponds to
UseConstRef.

[kinaba, 2013-03-13 06:34:06]

lgtm

https://codereview.chromium.org/12785003/diff/23001/chrome/browser/google_api...
File chrome/browser/google_apis/test_util.h (right):

https://codereview.chromium.org/12785003/diff/23001/chrome/browser/google_api...
chrome/browser/google_apis/test_util.h:237: // 1) If argument type |T| is
non-class type or supporting Chrome's Move
In offline chat I was convinced why it cannot be const T& for non-class types.

What the template has to compute is the _exact_ signature of callbacks
we want to create, not just types suitable for move semantics.
In our case, because we're using non-ref parameters for primitive and movable
types for callbacks declared in drive_service_interface.h, we have to respect it
here.

Could you clarify about it in the comment?

[hidehiko, 2013-03-13 08:49:19]

Thank you for your review.
Enqueueing to the CQ to run try server.

https://codereview.chromium.org/12785003/diff/23001/chrome/browser/google_api...
File chrome/browser/google_apis/test_util.h (right):

https://codereview.chromium.org/12785003/diff/23001/chrome/browser/google_api...
chrome/browser/google_apis/test_util.h:237: // 1) If argument type |T| is
non-class type or supporting Chrome's Move
On 2013/03/13 06:34:06, kinaba wrote:
> In offline chat I was convinced why it cannot be const T& for non-class types.
> 
> What the template has to compute is the _exact_ signature of callbacks
> we want to create, not just types suitable for move semantics.
> In our case, because we're using non-ref parameters for primitive and movable
> types for callbacks declared in drive_service_interface.h, we have to respect
it
> here.
> 
> Could you clarify about it in the comment?

Done.

https://codereview.chromium.org/12785003/diff/32001/chrome/browser/google_api...
File chrome/browser/google_apis/test_util.h (right):

https://codereview.chromium.org/12785003/diff/32001/chrome/browser/google_api...
chrome/browser/google_apis/test_util.h:257: :
InTypeHelper<base::is_class<T>::value && !IsMovable<T>::value, T>,
On 2013/03/13 06:27:52, hashimoto wrote:
> nit: Please add a comment about that this boolean expression corresponds to
> UseConstRef.

Done. Also, swapped the condition in the comment to order the condition, true
then false.

[commit-bot: I haz the power, 2013-03-13 08:49:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hidehiko@chromium.org/12785003/37001

[commit-bot: I haz the power, 2013-03-13 19:05:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hidehiko@chromium.org/12785003/37001

[commit-bot: I haz the power, 2013-03-13 20:42:58]

Change committed as 187941