Update to newer network service implementation and mojoms from monet

Update to newer network service implementation and mojoms from monet

This updates to using the network service implementation and mojoms from
https://github.com/domokit/monet/commit/050294719f6890ceea4b27540d66295fe6e710a3

R=blundell@chromium.org, qsr@chromium.org, viettrungluu@chromium.org

Committed: https://chromium.googlesource.com/external/mojo/+/d4709749733bb7833b338f9ffaa42a51b516f2d5

[jamesr, 2015-05-22 23:53:52]

jamesr@chromium.org changed reviewers:
+ qsr@chromium.org, viettrungluu@chromium.org

[viettrungluu, 2015-05-23 00:07:28]

https://codereview.chromium.org/1157783002/diff/1/examples/pdf_viewer/pdf_vie...
File examples/pdf_viewer/pdf_viewer.cc (right):

https://codereview.chromium.org/1157783002/diff/1/examples/pdf_viewer/pdf_vie...
examples/pdf_viewer/pdf_viewer.cc:189: if (headers[i]->name == "Content-Length")
{
Header field names are in fact case-insensitive
(http://www.w3.org/Protocols/rfc2616/rfc2616-sec4.html#sec4.2).

https://codereview.chromium.org/1157783002/diff/1/examples/pdf_viewer/pdf_vie...
examples/pdf_viewer/pdf_viewer.cc:191:
base::StringToInt(headers[i]->value.To<std::string>(), &length);
I wonder if Content-Length has been validated, or if it's possible for negative
values to sneak through to here....

[qsr, 2015-05-26 11:34:03]

LGTM with nits.

https://codereview.chromium.org/1157783002/diff/1/services/url_response_disk_...
File services/url_response_disk_cache/url_response_disk_cache_impl.cc (right):

https://codereview.chromium.org/1157783002/diff/1/services/url_response_disk_...
services/url_response_disk_cache/url_response_disk_cache_impl.cc:29: const
uint32_t kCurrentVersion = 1;
Please increase this, as you changed the definition of entry.

https://codereview.chromium.org/1157783002/diff/1/services/url_response_disk_...
services/url_response_disk_cache/url_response_disk_cache_impl.cc:294: }
You should be able to do:
entry->headers = response->headers.Clone()

Or even, as we just discard response there, entry->headers =
response->headers.Pass();

[jamesr, 2015-05-26 21:02:05]

https://codereview.chromium.org/1157783002/diff/1/examples/pdf_viewer/pdf_vie...
File examples/pdf_viewer/pdf_viewer.cc (right):

https://codereview.chromium.org/1157783002/diff/1/examples/pdf_viewer/pdf_vie...
examples/pdf_viewer/pdf_viewer.cc:191:
base::StringToInt(headers[i]->value.To<std::string>(), &length);
On 2015/05/23 00:07:28, viettrungluu wrote:
> I wonder if Content-Length has been validated, or if it's possible for
negative
> values to sneak through to here....

Parsing the content-length here at all is really dodgy, what we want to do is
receive all bytes from the network.  Updated this (and png-viewer) to use
mojo::common::BlockingCopyToString instead

[jamesr, 2015-05-26 21:04:07]

On 2015/05/26 11:34:03, qsr wrote:
> LGTM with nits.
> 
>
https://codereview.chromium.org/1157783002/diff/1/services/url_response_disk_...
> File services/url_response_disk_cache/url_response_disk_cache_impl.cc (right):
> 
>
https://codereview.chromium.org/1157783002/diff/1/services/url_response_disk_...
> services/url_response_disk_cache/url_response_disk_cache_impl.cc:29: const
> uint32_t kCurrentVersion = 1;
> Please increase this, as you changed the definition of entry.
> 
>
https://codereview.chromium.org/1157783002/diff/1/services/url_response_disk_...
> services/url_response_disk_cache/url_response_disk_cache_impl.cc:294: }
> You should be able to do:
> entry->headers = response->headers.Clone()
> 
> Or even, as we just discard response there, entry->headers =
> response->headers.Pass();

Unfortunately I can't do that.  |response| is a URLResponsePtr so
response->headers is an Array<HttpHeaderPtr>.  entry is a CacheEntryPtr so
entry->headers is a Array<CacheHeaderPtr>.  I made the mojo::CacheEntry type
independent of the network service mojoms but maybe that was a mistake

[jamesr, 2015-05-26 23:14:24]

Actually, I think having the cache data types be isolated is nice since it
represents a serialized value that we probably want to control separately.

[viettrungluu, 2015-05-26 23:21:54]

LGTM w/optional nits.

https://codereview.chromium.org/1157783002/diff/20001/examples/pdf_viewer/pdf...
File examples/pdf_viewer/pdf_viewer.cc (right):

https://codereview.chromium.org/1157783002/diff/20001/examples/pdf_viewer/pdf...
examples/pdf_viewer/pdf_viewer.cc:166:
mojo::common::BlockingCopyToString(response->body.Pass(), &data_);
nit: I don't think "mojo::" is necessary.... (Of course, this code shouldn't be
in the mojo namespace at all.)

https://codereview.chromium.org/1157783002/diff/20001/examples/png_viewer/png...
File examples/png_viewer/png_viewer.cc (right):

https://codereview.chromium.org/1157783002/diff/20001/examples/png_viewer/png...
examples/png_viewer/png_viewer.cc:149:
mojo::common::BlockingCopyToString(response->body.Pass(), &data);
...

[jamesr, 2015-05-26 23:26:31]

https://codereview.chromium.org/1157783002/diff/20001/examples/pdf_viewer/pdf...
File examples/pdf_viewer/pdf_viewer.cc (right):

https://codereview.chromium.org/1157783002/diff/20001/examples/pdf_viewer/pdf...
examples/pdf_viewer/pdf_viewer.cc:166:
mojo::common::BlockingCopyToString(response->body.Pass(), &data_);
On 2015/05/26 23:21:53, viettrungluu wrote:
> nit: I don't think "mojo::" is necessary.... (Of course, this code shouldn't
be
> in the mojo namespace at all.)

it's not, since this is in mojo::examples::, but i don't really like having
naming lookups be relative to a parent of the current. "common::Blocking.."
could mean "::common::Blocking.." or "::mojo::common::Blocking.." or
"::mojo:examples::common::Blocking...". saying "::mojo::common::..." would be
even less ambiguous but this felt like a good balance to me. i'm inclined to
leave this as is (and moving this out of the mojo:: namespace would be ideal, of
course)

[blundell, 2015-05-27 08:33:17]

blundell@chromium.org changed reviewers:
+ blundell@chromium.org

[blundell, 2015-05-27 08:33:18]

AuthenticatingURLLoader changes lgtm

[jamesr, 2015-05-27 20:25:02]

Committed patchset #4 (id:60001) manually as
d4709749733bb7833b338f9ffaa42a51b516f2d5 (presubmit successful).