Use a proper URLLoaderAssociatedRequest in MojoAsyncResourceHandler tests

Use a proper URLLoaderAssociatedRequest in MojoAsyncResourceHandler tests

We used to pass null URLLoaderAssociatedRequest to MojoAsyncResourceHandler. As
we are planning to set an error_handler to the associated binding, we need to
pass a proper URLLoaderAssociatedRequest.

BUG=603396
Committed: https://crrev.com/ffcc0b4318abf391735e2eb2f46381a3264bd2c6
Cr-Commit-Position: refs/heads/master@{#431764}

[yhirano, 2016-11-09 01:48:00]

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-09 01:48:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yhirano, 2016-11-09 01:50:33]

Description was changed from

==========
Use a proper URLLoaderAssociatedRequest in MojoAsyncResourceHandler tests.

We used to pass null URLLoaderAssociatedRequest to MojoAsyncResourceHandler. As
we are planning to set a error_handler to the associated binding, we need to
pass a proper URLLoaderAssociatedRequest.

BUG=603396
==========

to

==========
Use a proper URLLoaderAssociatedRequest in MojoAsyncResourceHandler tests.

We used to pass null URLLoaderAssociatedRequest to MojoAsyncResourceHandler. As
we are planning to set an error_handler to the associated binding, we need to
pass a proper URLLoaderAssociatedRequest.

BUG=603396
==========

[yhirano, 2016-11-09 01:54:31]

Description was changed from

==========
Use a proper URLLoaderAssociatedRequest in MojoAsyncResourceHandler tests.

We used to pass null URLLoaderAssociatedRequest to MojoAsyncResourceHandler. As
we are planning to set an error_handler to the associated binding, we need to
pass a proper URLLoaderAssociatedRequest.

BUG=603396
==========

to

==========
Use a proper URLLoaderAssociatedRequest in MojoAsyncResourceHandler tests

We used to pass null URLLoaderAssociatedRequest to MojoAsyncResourceHandler. As
we are planning to set an error_handler to the associated binding, we need to
pass a proper URLLoaderAssociatedRequest.

BUG=603396
==========

[commit-bot: I haz the power, 2016-11-09 02:13:35]

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

[commit-bot: I haz the power, 2016-11-09 02:13:36]

Dry run: This issue passed the CQ dry run.

[yhirano, 2016-11-09 03:39:28]

yhirano@chromium.org changed reviewers:
+ yzshen@chromium.org

[yhirano, 2016-11-09 03:39:29]

Can you take a look?

See also: https://codereview.chromium.org/2466843002/

[yhirano, 2016-11-09 05:46:02]

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-09 05:46:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-09 06:07:49]

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

[commit-bot: I haz the power, 2016-11-09 06:07:51]

Dry run: This issue passed the CQ dry run.

[yzshen1, 2016-11-09 17:40:48]

https://codereview.chromium.org/2489793002/diff/20001/content/browser/loader/...
File content/browser/loader/mojo_async_resource_handler_unittest.cc (right):

https://codereview.chromium.org/2489793002/diff/20001/content/browser/loader/...
content/browser/loader/mojo_async_resource_handler_unittest.cc:355:
scoped_refptr<TestURLLoaderFactory::Waiter> waiter(
I think |waiter| may not be necessary, we could:
- keep a pointer to the TestURLLoaderFactory instance, say, factory_impl. You
could use the returned value of MakeStrongBinding(), or make a non-strong
binding and manage the instance lifetime yourself.
- after you call url_loader_factory_->CreateLoaderAndStart(), call
url_loader_factory_.FlushForTesting(), that will make sure the previous call
arrives at the other side.
- let TestURLLoaderFactory::CreateLoaderAndStart() store the incoming request
and ptr_info.
- get the request and ptr from factory_impl.

[yhirano, 2016-11-10 05:46:05]

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-10 05:46:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yhirano, 2016-11-10 05:47:22]

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-10 05:47:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yhirano, 2016-11-10 05:49:16]

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-10 05:49:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yhirano, 2016-11-10 07:33:53]

https://codereview.chromium.org/2489793002/diff/20001/content/browser/loader/...
File content/browser/loader/mojo_async_resource_handler_unittest.cc (right):

https://codereview.chromium.org/2489793002/diff/20001/content/browser/loader/...
content/browser/loader/mojo_async_resource_handler_unittest.cc:355:
scoped_refptr<TestURLLoaderFactory::Waiter> waiter(
On 2016/11/09 17:40:48, yzshen1 wrote:
> I think |waiter| may not be necessary, we could:
> - keep a pointer to the TestURLLoaderFactory instance, say, factory_impl. You
> could use the returned value of MakeStrongBinding(), or make a non-strong
> binding and manage the instance lifetime yourself.
> - after you call url_loader_factory_->CreateLoaderAndStart(), call
> url_loader_factory_.FlushForTesting(), that will make sure the previous call
> arrives at the other side.
> - let TestURLLoaderFactory::CreateLoaderAndStart() store the incoming request
> and ptr_info.
> - get the request and ptr from factory_impl.

Thanks, done.

[commit-bot: I haz the power, 2016-11-10 07:47:17]

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

[commit-bot: I haz the power, 2016-11-10 07:47:18]

Dry run: This issue passed the CQ dry run.

[yzshen1, 2016-11-10 17:28:36]

LGTM with two more nits.

https://codereview.chromium.org/2489793002/diff/80001/content/browser/loader/...
File content/browser/loader/mojo_async_resource_handler_unittest.cc (right):

https://codereview.chromium.org/2489793002/diff/80001/content/browser/loader/...
content/browser/loader/mojo_async_resource_handler_unittest.cc:289:
mojom::URLLoaderAssociatedRequest LoaderRequest() {
style nit: Please consider either naming it PassLoaderRequest() or making it a
getter:
  mojom::URLLoaderAssociatedRequest& loader_request()

https://codereview.chromium.org/2489793002/diff/80001/content/browser/loader/...
content/browser/loader/mojo_async_resource_handler_unittest.cc:308:
base::WeakPtrFactory<TestURLLoaderFactory> weak_factory_;
I think this is not needed. mojo::MakeStrongBinding() returns a
base::WeakPtr<StrongBinding<Interface>> for you. You could use that instead.

[yhirano, 2016-11-11 02:23:03]

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-11 02:23:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-11 02:45:13]

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

[commit-bot: I haz the power, 2016-11-11 02:45:14]

Dry run: This issue passed the CQ dry run.

[yhirano, 2016-11-11 03:13:37]

https://codereview.chromium.org/2489793002/diff/80001/content/browser/loader/...
File content/browser/loader/mojo_async_resource_handler_unittest.cc (right):

https://codereview.chromium.org/2489793002/diff/80001/content/browser/loader/...
content/browser/loader/mojo_async_resource_handler_unittest.cc:289:
mojom::URLLoaderAssociatedRequest LoaderRequest() {
On 2016/11/10 17:28:36, yzshen1 wrote:
> style nit: Please consider either naming it PassLoaderRequest() or making it a
> getter:
>   mojom::URLLoaderAssociatedRequest& loader_request()

Done.

https://codereview.chromium.org/2489793002/diff/80001/content/browser/loader/...
content/browser/loader/mojo_async_resource_handler_unittest.cc:308:
base::WeakPtrFactory<TestURLLoaderFactory> weak_factory_;
On 2016/11/10 17:28:36, yzshen1 wrote:
> I think this is not needed. mojo::MakeStrongBinding() returns a
> base::WeakPtr<StrongBinding<Interface>> for you. You could use that instead.

Done.

[yhirano, 2016-11-11 03:14:22]

yhirano@chromium.org changed reviewers:
+ csharrison@chromium.org

[yhirano, 2016-11-11 03:14:23]

+csharrison@ for OWNER reivew.

[Charlie Harrison, 2016-11-11 22:09:15]

generally lgtm

[yhirano, 2016-11-12 03:36:26]

The CQ bit was checked by yhirano@chromium.org

[yhirano, 2016-11-12 03:36:26]

The patchset sent to the CQ was uploaded after l-g-t-m from yzshen@chromium.org
Link to the patchset: https://codereview.chromium.org/2489793002/#ps100001
(title: "fix")

[commit-bot: I haz the power, 2016-11-12 03:36:45]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-12 04:02:11]

Description was changed from

==========
Use a proper URLLoaderAssociatedRequest in MojoAsyncResourceHandler tests

We used to pass null URLLoaderAssociatedRequest to MojoAsyncResourceHandler. As
we are planning to set an error_handler to the associated binding, we need to
pass a proper URLLoaderAssociatedRequest.

BUG=603396
==========

to

==========
Use a proper URLLoaderAssociatedRequest in MojoAsyncResourceHandler tests

We used to pass null URLLoaderAssociatedRequest to MojoAsyncResourceHandler. As
we are planning to set an error_handler to the associated binding, we need to
pass a proper URLLoaderAssociatedRequest.

BUG=603396
==========

[commit-bot: I haz the power, 2016-11-12 04:02:12]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-11-12 04:05:17]

Description was changed from

==========
Use a proper URLLoaderAssociatedRequest in MojoAsyncResourceHandler tests

We used to pass null URLLoaderAssociatedRequest to MojoAsyncResourceHandler. As
we are planning to set an error_handler to the associated binding, we need to
pass a proper URLLoaderAssociatedRequest.

BUG=603396
==========

to

==========
Use a proper URLLoaderAssociatedRequest in MojoAsyncResourceHandler tests

We used to pass null URLLoaderAssociatedRequest to MojoAsyncResourceHandler. As
we are planning to set an error_handler to the associated binding, we need to
pass a proper URLLoaderAssociatedRequest.

BUG=603396

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

[commit-bot: I haz the power, 2016-11-12 04:05:18]

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