Mojo: Refactor URLLoader interface.

Mojo: Refactor URLLoader interface.

This change simplifies the URLLoader interface, eliminating the URLLoaderClient interface. The URLResponse now includes the DataPipeConsumerHandle for the response body stream as well as information about a possible redirect response.

One nice thing about this change is that you can now pass around the URLResponsePtr without also having to pass around the DataPipeConsumerHandle for the response body stream.

This didn't enable me to eliminate ResponseDetails as I think that structure should include not only the URLResponse but also the URLLoader used to generate the response. (That enables the LaunchInstance to get destroyed immediately after delegating the response to the handler app as it no longer needs to stick around to keep the URLLoader alive.) The recipient of the URLLoader might be interested in calling the new QueryStatus method to find out more information about the URL load (e.g., Blink wants to know the encoded size of a response body, which may not be available at the time when response headers are received).

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

[Aaron Boodman, 2014-07-10 18:48:08]

are you ready for me to review this?

[darin (slow to review), 2014-07-10 19:16:47]

Yes. Thanks!


On Thu, Jul 10, 2014 at 11:48 AM, <aa@chromium.org> wrote:

> are you ready for me to review this?
>
> https://codereview.chromium.org/373373002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Aaron Boodman, 2014-07-10 20:05:00]

lgtm

https://codereview.chromium.org/373373002/diff/40001/mojo/examples/wget/wget.cc
File mojo/examples/wget/wget.cc (right):

https://codereview.chromium.org/373373002/diff/40001/mojo/examples/wget/wget....
mojo/examples/wget/wget.cc:18: public:
Out of curiosity, why have a separate ResponsePrinter class rather than passing
a mojo::Callback to some method of WGetApp or some static function?

https://codereview.chromium.org/373373002/diff/40001/mojo/services/launcher/l...
File mojo/services/launcher/launcher.cc (right):

https://codereview.chromium.org/373373002/diff/40001/mojo/services/launcher/l...
mojo/services/launcher/launcher.cc:162: if (!handler_url.empty()) {
Maybe print out an error or something for now?

https://codereview.chromium.org/373373002/diff/40001/mojo/services/network/ur...
File mojo/services/network/url_loader_impl.cc (right):

https://codereview.chromium.org/373373002/diff/40001/mojo/services/network/ur...
mojo/services/network/url_loader_impl.cc:197: if ((*defer_redirect =
!auto_follow_redirects_))
Do you need the extra parens?

https://codereview.chromium.org/373373002/diff/40001/mojo/services/network/ur...
mojo/services/network/url_loader_impl.cc:214: callback_ =
Callback<void(URLResponsePtr)>();
It looks like if an empty mojo::Callback gets Run() its a silent no-op. Perhaps
it should be a DCHECK instead. Or you should add a DHCECK that it's not empty at
the place that callback_ is Run() here.

https://codereview.chromium.org/373373002/diff/40001/mojo/services/network/ur...
mojo/services/network/url_loader_impl.cc:309: DCHECK(bytes_read == 0 ||
!url_request_->status().is_success());
Nit: Recommend separate DCHECKs so that you can see which one failed more
easily.

[darin (slow to review), 2014-07-10 20:34:40]

On 2014/07/10 20:05:00, Aaron Boodman wrote:
> lgtm
> 
>
https://codereview.chromium.org/373373002/diff/40001/mojo/examples/wget/wget.cc
> File mojo/examples/wget/wget.cc (right):
> 
>
https://codereview.chromium.org/373373002/diff/40001/mojo/examples/wget/wget....
> mojo/examples/wget/wget.cc:18: public:
> Out of curiosity, why have a separate ResponsePrinter class rather than
passing
> a mojo::Callback to some method of WGetApp or some static function?

It turns out that this is an app built against the standalone mojo environment,
not using src/base/. As a result, I don't have base::Bind. We may need to
provide
a variant of Bind to work with mojo::Callback, or figure out some other story
for
callbacks in the standalone mojo environment.


> 
>
https://codereview.chromium.org/373373002/diff/40001/mojo/services/launcher/l...
> File mojo/services/launcher/launcher.cc (right):
> 
>
https://codereview.chromium.org/373373002/diff/40001/mojo/services/launcher/l...
> mojo/services/launcher/launcher.cc:162: if (!handler_url.empty()) {
> Maybe print out an error or something for now?

OK


>
https://codereview.chromium.org/373373002/diff/40001/mojo/services/network/ur...
> File mojo/services/network/url_loader_impl.cc (right):
> 
>
https://codereview.chromium.org/373373002/diff/40001/mojo/services/network/ur...
> mojo/services/network/url_loader_impl.cc:197: if ((*defer_redirect =
> !auto_follow_redirects_))
> Do you need the extra parens?

Nope, thanks. Actually, I meant to put a "!" in front of this expression!
Instead, I'll simplify the code to just return early if auto_follow_redirects_
is true. There is no need to set *defer_redirect in that case, as its default
value is false.


>
https://codereview.chromium.org/373373002/diff/40001/mojo/services/network/ur...
> mojo/services/network/url_loader_impl.cc:214: callback_ =
> Callback<void(URLResponsePtr)>();
> It looks like if an empty mojo::Callback gets Run() its a silent no-op.
Perhaps
> it should be a DCHECK instead. Or you should add a DHCECK that it's not empty
at
> the place that callback_ is Run() here.

Note that callbacks can be weakly bound. base::Bind lets you bind to a WeakPtr.
So, even if you had a non-null callback, its plausible that the callback might
do nothing when run. In the case of the URLLoaderImpl, I don't think it really
cares if someone is there to observe the callback.


>
https://codereview.chromium.org/373373002/diff/40001/mojo/services/network/ur...
> mojo/services/network/url_loader_impl.cc:309: DCHECK(bytes_read == 0 ||
> !url_request_->status().is_success());
> Nit: Recommend separate DCHECKs so that you can see which one failed more
> easily.

Good idea. Actually, I can just delete this line entirely. It doesn't add
anything.

[darin (slow to review), 2014-07-10 22:14:37]

The CQ bit was checked by darin@chromium.org

[commit-bot: I haz the power, 2014-07-10 22:15:51]

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

[commit-bot: I haz the power, 2014-07-11 03:02:33]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[commit-bot: I haz the power, 2014-07-11 04:14:58]

Change committed as 282535