Teach mojo_shell how to load http URLs

Teach mojo_shell how to load http URLs

After this CL, we can load content over HTTP. In order to make
that work, I needed to introduce a number of basic concepts
into the shell, including an IO thread and a directory to
store data (such as the HTTP cache).

I've tried to keep the net dependencies separated from the
bulk of the mojo_shell code because I expect we'll want to
move the net dependencies to an app that runs on top of the
Mojo platform rather than linked into the mojo_shell itself.
To that end, the mojo::loader::Loader class doesn't leak any
net headers to its clients.

The version of the loader in this CL is extremely basic. We'll
undoubtedly make it fancier as time goes on. The next step
after this CL is to store the result in a file and then load
that file as a shared object.

R=aa@chromium.org

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

[abarth-chromium, 2013-10-18 01:20:45]

@aa: This CL builds on https://codereview.chromium.org/27536014/ and adds
support for http URLs.  The main open question with this CL is where to put the
HTTP cache.  We might need a CL before this one that creates a temporary
directory for the shell to store data.

https://codereview.chromium.org/27943002/diff/1/mojo/loader/url_request_conte...
File mojo/loader/url_request_context_getter.cc (right):

https://codereview.chromium.org/27943002/diff/1/mojo/loader/url_request_conte...
mojo/loader/url_request_context_getter.cc:64:
base::FilePath().Append(FILE_PATH_LITERAL("tmp/mojo/Cache"));
Where do we want to store the HTTP cache?

[abarth-chromium, 2013-10-18 01:21:38]

Another thing to consider about this CL is that it creates two new threads: a
networking thread and a cache thread.  I'm not sure how crazy we want to go with
threads...

[darin (slow to review), 2013-10-18 04:03:18]

Note, we are going to need an I/O thread in the shell process to support
Mojo IPC. it seems reasonable to share that I/O thread with the network
stack.

-Darin


On Thu, Oct 17, 2013 at 6:21 PM, <abarth@chromium.org> wrote:

> Another thing to consider about this CL is that it creates two new
> threads: a
> networking thread and a cache thread.  I'm not sure how crazy we want to
> go with
> threads...
>
>
https://codereview.chromium.**org/27943002/<https://codereview.chromium.org/2...
>

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

[abarth-chromium, 2013-10-18 04:54:49]

On 2013/10/18 04:03:18, darin wrote:
> Note, we are going to need an I/O thread in the shell process to support
> Mojo IPC. it seems reasonable to share that I/O thread with the network
> stack.

Ok.  I'll re-work this CL so that the shell provides the IO thread to the
loader.  Thanks!

[abarth-chromium, 2013-10-18 06:17:02]

I've re-worked this so that the shell provides the IO thread.  I still need to
sort out where to put the HTTP cache on disk.

[abarth-chromium, 2013-10-18 06:55:54]

Ok, this is ready for review now.  It seems to work in local testing I did with
an extra patch that actually kicks off a load and prints the fetched data. 
PTAL.  Thanks!

[darin (slow to review), 2013-10-18 07:11:29]

https://codereview.chromium.org/27943002/diff/123001/mojo/loader/loader.cc
File mojo/loader/loader.cc (right):

https://codereview.chromium.org/27943002/diff/123001/mojo/loader/loader.cc#ne...
mojo/loader/loader.cc:42: }
nit: "}  // namespace"

https://codereview.chromium.org/27943002/diff/123001/mojo/loader/loader.cc#ne...
mojo/loader/loader.cc:58: Loader::~Loader() {
what if Loader is destroyed before OnURLFetchComplete gets called? it seems you
could end up in a situation where FetchDelegate holds a junk pointer to a
Loader::Delegate.

Also, I think you need to put the fetcher in a scoped_ptr<net::URLFetcher>
member variable.  normally, this would be on the Loader class, and the Loader
class would implement URLFetcherDelegate. If you want to create an inner class
to hide those details, that's fine. Just make the Loader own the inner class
instance.

https://codereview.chromium.org/27943002/diff/123001/mojo/loader/url_request_...
File mojo/loader/url_request_context_getter.cc (right):

https://codereview.chromium.org/27943002/diff/123001/mojo/loader/url_request_...
mojo/loader/url_request_context_getter.cc:5: #include
"mojo/loader/url_request_context_getter.h"
nit: new line below here.

https://codereview.chromium.org/27943002/diff/123001/mojo/loader/url_request_...
mojo/loader/url_request_context_getter.cc:42: new
net::StaticHttpUserAgentSettings("en-us,en", "Mojo/0.1"));
oh, look at you!

https://codereview.chromium.org/27943002/diff/123001/mojo/loader/url_request_...
File mojo/loader/url_request_context_getter.h (right):

https://codereview.chromium.org/27943002/diff/123001/mojo/loader/url_request_...
mojo/loader/url_request_context_getter.h:23: ~URLRequestContextGetter();
a presubmit check is likely to complain about this not being marked virtual.

[abarth-chromium, 2013-10-18 07:27:16]

https://codereview.chromium.org/27943002/diff/123001/mojo/loader/loader.cc
File mojo/loader/loader.cc (right):

https://codereview.chromium.org/27943002/diff/123001/mojo/loader/loader.cc#ne...
mojo/loader/loader.cc:42: }
On 2013/10/18 07:11:29, darin wrote:
> nit: "}  // namespace"

Thanks.  My Chromium style is a bit rusty.  :)

https://codereview.chromium.org/27943002/diff/123001/mojo/loader/loader.cc#ne...
mojo/loader/loader.cc:58: Loader::~Loader() {
On 2013/10/18 07:11:29, darin wrote:
> what if Loader is destroyed before OnURLFetchComplete gets called? it seems
you
> could end up in a situation where FetchDelegate holds a junk pointer to a
> Loader::Delegate.

That's a good point.

> Also, I think you need to put the fetcher in a scoped_ptr<net::URLFetcher>
> member variable.  normally, this would be on the Loader class, and the Loader
> class would implement URLFetcherDelegate. If you want to create an inner class
> to hide those details, that's fine. Just make the Loader own the inner class
> instance.

Makes sense.  Will fix.

https://codereview.chromium.org/27943002/diff/123001/mojo/loader/url_request_...
File mojo/loader/url_request_context_getter.cc (right):

https://codereview.chromium.org/27943002/diff/123001/mojo/loader/url_request_...
mojo/loader/url_request_context_getter.cc:5: #include
"mojo/loader/url_request_context_getter.h"
On 2013/10/18 07:11:29, darin wrote:
> nit: new line below here.

Does Chromium style put the primary header at the top of the file?  I didn't see
that done in app_container, which is how I got confused.

https://codereview.chromium.org/27943002/diff/123001/mojo/loader/url_request_...
mojo/loader/url_request_context_getter.cc:42: new
net::StaticHttpUserAgentSettings("en-us,en", "Mojo/0.1"));
On 2013/10/18 07:11:29, darin wrote:
> oh, look at you!

A man can dream!

https://codereview.chromium.org/27943002/diff/123001/mojo/loader/url_request_...
File mojo/loader/url_request_context_getter.h (right):

https://codereview.chromium.org/27943002/diff/123001/mojo/loader/url_request_...
mojo/loader/url_request_context_getter.h:23: ~URLRequestContextGetter();
On 2013/10/18 07:11:29, darin wrote:
> a presubmit check is likely to complain about this not being marked virtual.

Ok.  It's virtual from the base class, but I'll mark it virtual.

[abarth-chromium, 2013-10-18 08:02:13]

Ok, I think I've fixed the lifetime issue.  Now the Loader class acts more like
a factory for Job objects.  The delegate owns the Job and the Job owns the
URLFetcher.  PTAL.  Thanks!

[Aaron Boodman, 2013-10-18 17:59:41]

I don't know enough about this area to comment intelligently, but LGTM to
unblock.

https://codereview.chromium.org/27943002/diff/173001/mojo/loader/job.h
File mojo/loader/job.h (right):

https://codereview.chromium.org/27943002/diff/173001/mojo/loader/job.h#newcode13
mojo/loader/job.h:13: class Job {
I think a class-header comment is warranted here explaining what a Job is
conceptually.

https://codereview.chromium.org/27943002/diff/173001/mojo/loader/loader.cc
File mojo/loader/loader.cc (right):

https://codereview.chromium.org/27943002/diff/173001/mojo/loader/loader.cc#ne...
mojo/loader/loader.cc:18: class JobImpl : public net::URLFetcherDelegate, public
Job {
Out of curiosity, I don't understand why the distinction here between Job and
JobImpl.

https://codereview.chromium.org/27943002/diff/173001/mojo/shell/app_container.h
File mojo/shell/app_container.h (right):

https://codereview.chromium.org/27943002/diff/173001/mojo/shell/app_container...
mojo/shell/app_container.h:9: #include "base/files/file_path.h"
Can forward-declare FilePath here.

[abarth-chromium, 2013-10-18 18:05:28]

https://codereview.chromium.org/27943002/diff/173001/mojo/loader/loader.cc
File mojo/loader/loader.cc (right):

https://codereview.chromium.org/27943002/diff/173001/mojo/loader/loader.cc#ne...
mojo/loader/loader.cc:18: class JobImpl : public net::URLFetcherDelegate, public
Job {
On 2013/10/18 17:59:42, Aaron Boodman wrote:
> Out of curiosity, I don't understand why the distinction here between Job and
> JobImpl.

The idea is to avoid leaking the net headers to clients of mojo::loader.  I'd
like to avoid leaking dependencies on net so that we can later move the loader
out of mojo_shell and into a service, which will keep mojo_shell lean and mean
(since it's going to be loaded into every process).

[abarth-chromium, 2013-10-18 18:33:51]

Other comments address.  Thanks for the review.

@Darin: I'm happy to address any additional comments you have when you get back
from your vacation.  Thanks for the comments last night.

[commit-bot: I haz the power, 2013-10-18 19:02:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/abarth@chromium.org/27943002/233001

[commit-bot: I haz the power, 2013-10-18 21:56:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/abarth@chromium.org/27943002/233001

[abarth-chromium, 2013-10-18 22:09:05]

Committed patchset #6 manually as r229481.

[commit-bot: I haz the power, 2013-10-18 23:54:06]

Failed to trigger a try job on chromium_presubmit
HTTP Error 400: Bad Request

[commit-bot: I haz the power, 2013-10-18 23:58:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/abarth@chromium.org/27943002/453001

[abarth-chromium, 2013-10-19 00:17:15]

Committed patchset #7 manually as r229496.