Remove UrlFetcher from remoting and use the one in net instead.

Remove UrlFetcher from remoting and use the one in net instead.

BUG=133790
TEST=Stop and restart the Me2Me host. It should still work.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=143798

[Jamie, 2012-06-22 19:44:41]

ptal

[Wez, 2012-06-22 20:14:38]

https://chromiumcodereview.appspot.com/10637008/diff/7001/remoting/host/gaia_...
File remoting/host/gaia_oauth_client.cc (right):

https://chromiumcodereview.appspot.com/10637008/diff/7001/remoting/host/gaia_...
remoting/host/gaia_oauth_client.cc:50: public net::URLFetcherDelegate {
Do we actually need this implementation to be split into a caller-owned object
and ref-counted core? It doesn't look like the Core is ref-counted any more, and
even in the previous implementation, it looks like Unretained would have been
valid, since the object was being Bind()ed for callback, not task posting.

https://chromiumcodereview.appspot.com/10637008/diff/7001/remoting/host/gaia_...
remoting/host/gaia_oauth_client.cc:63: virtual void OnURLFetchComplete(const
net::URLFetcher* source) OVERRIDE;
nit: Prefix this with a blank line and comment "net::URLFetcherDelegate
interface"

https://chromiumcodereview.appspot.com/10637008/diff/7001/remoting/host/gaia_...
remoting/host/gaia_oauth_client.cc:82: bool request_is_oauth_refresh_;
Looking at the rest of the class, I think it'd be cleaner to keep distinct
request pointers for the two request types.

https://chromiumcodereview.appspot.com/10637008/diff/7001/remoting/host/gaia_...
remoting/host/gaia_oauth_client.cc:173:
request_->AddExtraRequestHeader("Authorization: Bearer " + access_token_);
Do we need to sanitize access_token_, or will AddExtraRequestHeader() verify
that we're not adding a malformed header if it contains bad data?

https://chromiumcodereview.appspot.com/10637008/diff/7001/remoting/host/host_...
File remoting/host/host_port_allocator.cc (right):

https://chromiumcodereview.appspot.com/10637008/diff/7001/remoting/host/host_...
remoting/host/host_port_allocator.cc:24: net::URLFetcherDelegate {
Style guide requires inheritance to be public.

https://chromiumcodereview.appspot.com/10637008/diff/7001/remoting/host/host_...
remoting/host/host_port_allocator.cc:42: void OnURLFetchComplete(const
net::URLFetcher* url_fetcher);
nit: "net::URLFetcherDelegate interface"

https://chromiumcodereview.appspot.com/10637008/diff/7001/remoting/host/host_...
remoting/host/host_port_allocator.cc:101: const net::URLFetcher* source) {
nit: source -> url_fetcher?

https://chromiumcodereview.appspot.com/10637008/diff/7001/remoting/host/host_...
remoting/host/host_port_allocator.cc:101: const net::URLFetcher* source) {
Why does OnURLFetchComplete get passed a const net::URLFetcher? In what sense is
it const?

[Jamie, 2012-06-22 22:42:02]

https://chromiumcodereview.appspot.com/10637008/diff/7001/remoting/host/gaia_...
File remoting/host/gaia_oauth_client.cc (right):

https://chromiumcodereview.appspot.com/10637008/diff/7001/remoting/host/gaia_...
remoting/host/gaia_oauth_client.cc:50: public net::URLFetcherDelegate {
On 2012/06/22 20:14:39, Wez wrote:
> Do we actually need this implementation to be split into a caller-owned object
> and ref-counted core? It doesn't look like the Core is ref-counted any more,
and
> even in the previous implementation, it looks like Unretained would have been
> valid, since the object was being Bind()ed for callback, not task posting.

This implementation is based on one used by cloud print. I think you're right
that it doesn't need the separation any more, but I think that should be a
separate CL. I want this to be just about switching over from the old to the new
URLFetcher class so that it's easier to check that it's been done correctly.

https://chromiumcodereview.appspot.com/10637008/diff/7001/remoting/host/gaia_...
remoting/host/gaia_oauth_client.cc:63: virtual void OnURLFetchComplete(const
net::URLFetcher* source) OVERRIDE;
On 2012/06/22 20:14:39, Wez wrote:
> nit: Prefix this with a blank line and comment "net::URLFetcherDelegate
> interface"

Done.

https://chromiumcodereview.appspot.com/10637008/diff/7001/remoting/host/gaia_...
remoting/host/gaia_oauth_client.cc:82: bool request_is_oauth_refresh_;
On 2012/06/22 20:14:39, Wez wrote:
> Looking at the rest of the class, I think it'd be cleaner to keep distinct
> request pointers for the two request types.

That's what I had at first, but we check that the request is not already set
when refreshing a token. We could change that to check both fetchers, but I
don't think that's any clearer, and this design makes explicit the fact that you
can only have one request in flight at a time.

https://chromiumcodereview.appspot.com/10637008/diff/7001/remoting/host/gaia_...
remoting/host/gaia_oauth_client.cc:173:
request_->AddExtraRequestHeader("Authorization: Bearer " + access_token_);
On 2012/06/22 20:14:39, Wez wrote:
> Do we need to sanitize access_token_, or will AddExtraRequestHeader() verify
> that we're not adding a malformed header if it contains bad data?

It DCHECKs that it doesn't contain CRLF. Something stronger would be nice, so
I've added code that causes the token refresh operation to fail if the returned
token contains CRLF.

https://chromiumcodereview.appspot.com/10637008/diff/7001/remoting/host/host_...
File remoting/host/host_port_allocator.cc (right):

https://chromiumcodereview.appspot.com/10637008/diff/7001/remoting/host/host_...
remoting/host/host_port_allocator.cc:24: net::URLFetcherDelegate {
On 2012/06/22 20:14:39, Wez wrote:
> Style guide requires inheritance to be public.

Done.

https://chromiumcodereview.appspot.com/10637008/diff/7001/remoting/host/host_...
remoting/host/host_port_allocator.cc:42: void OnURLFetchComplete(const
net::URLFetcher* url_fetcher);
On 2012/06/22 20:14:39, Wez wrote:
> nit: "net::URLFetcherDelegate interface"

Done.

https://chromiumcodereview.appspot.com/10637008/diff/7001/remoting/host/host_...
remoting/host/host_port_allocator.cc:101: const net::URLFetcher* source) {
On 2012/06/22 20:14:39, Wez wrote:
> nit: source -> url_fetcher?

It's called source elsewhere, including in the URLFetcherDelegate interface.

https://chromiumcodereview.appspot.com/10637008/diff/7001/remoting/host/host_...
remoting/host/host_port_allocator.cc:101: const net::URLFetcher* source) {
On 2012/06/22 20:14:39, Wez wrote:
> Why does OnURLFetchComplete get passed a const net::URLFetcher? In what sense
is
> it const?

There's a comment on the Start() method that says you must not change any
settings after calling Start. I think this is to enforce that.

[Wez, 2012-06-22 23:58:34]

lgtm

https://chromiumcodereview.appspot.com/10637008/diff/7001/remoting/host/gaia_...
File remoting/host/gaia_oauth_client.cc (right):

https://chromiumcodereview.appspot.com/10637008/diff/7001/remoting/host/gaia_...
remoting/host/gaia_oauth_client.cc:50: public net::URLFetcherDelegate {
On 2012/06/22 22:42:02, Jamie wrote:
> On 2012/06/22 20:14:39, Wez wrote:
> > Do we actually need this implementation to be split into a caller-owned
object
> > and ref-counted core? It doesn't look like the Core is ref-counted any more,
> and
> > even in the previous implementation, it looks like Unretained would have
been
> > valid, since the object was being Bind()ed for callback, not task posting.
> 
> This implementation is based on one used by cloud print. I think you're right
> that it doesn't need the separation any more, but I think that should be a
> separate CL. I want this to be just about switching over from the old to the
new
> URLFetcher class so that it's easier to check that it's been done correctly.

SGTM

https://chromiumcodereview.appspot.com/10637008/diff/7001/remoting/host/gaia_...
remoting/host/gaia_oauth_client.cc:82: bool request_is_oauth_refresh_;
On 2012/06/22 22:42:02, Jamie wrote:
> On 2012/06/22 20:14:39, Wez wrote:
> > Looking at the rest of the class, I think it'd be cleaner to keep distinct
> > request pointers for the two request types.
> 
> That's what I had at first, but we check that the request is not already set
> when refreshing a token. We could change that to check both fetchers, but I
> don't think that's any clearer, and this design makes explicit the fact that
you
> can only have one request in flight at a time.

I take your point, but you should really use a "state" enum here, rather than a
Boolean to express that.

https://chromiumcodereview.appspot.com/10637008/diff/16001/remoting/host/gaia...
File remoting/host/gaia_oauth_client.cc (right):

https://chromiumcodereview.appspot.com/10637008/diff/16001/remoting/host/gaia...
remoting/host/gaia_oauth_client.cc:157: if (access_token.find("\r\n") !=
std::string::npos) {
Is there any more stringent check we can make, e.g. that the access_token only
contain a ASCII characters? There are other weird things besides newlines.

[Jamie, 2012-06-23 00:20:23]

fyi

https://chromiumcodereview.appspot.com/10637008/diff/7001/remoting/host/gaia_...
File remoting/host/gaia_oauth_client.cc (right):

https://chromiumcodereview.appspot.com/10637008/diff/7001/remoting/host/gaia_...
remoting/host/gaia_oauth_client.cc:82: bool request_is_oauth_refresh_;
On 2012/06/22 23:58:35, Wez wrote:
> On 2012/06/22 22:42:02, Jamie wrote:
> > On 2012/06/22 20:14:39, Wez wrote:
> > > Looking at the rest of the class, I think it'd be cleaner to keep distinct
> > > request pointers for the two request types.
> > 
> > That's what I had at first, but we check that the request is not already set
> > when refreshing a token. We could change that to check both fetchers, but I
> > don't think that's any clearer, and this design makes explicit the fact that
> you
> > can only have one request in flight at a time.
> 
> I take your point, but you should really use a "state" enum here, rather than
a
> Boolean to express that.

Done.

https://chromiumcodereview.appspot.com/10637008/diff/16001/remoting/host/gaia...
File remoting/host/gaia_oauth_client.cc (right):

https://chromiumcodereview.appspot.com/10637008/diff/16001/remoting/host/gaia...
remoting/host/gaia_oauth_client.cc:157: if (access_token.find("\r\n") !=
std::string::npos) {
On 2012/06/22 23:58:35, Wez wrote:
> Is there any more stringent check we can make, e.g. that the access_token only
> contain a ASCII characters? There are other weird things besides newlines.

I think an embedded CRLF is the only thing that would make it invalid
HTTP--anything else is the server's problem :) Regarding restricting to ASCII, I
don't know if OAuth makes any guarantees as to the format of access tokens, so
I'd prefer not to enforce that here.

[commit-bot: I haz the power, 2012-06-23 04:19:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jamiewalch@chromium.org/10637008/18002

[commit-bot: I haz the power, 2012-06-23 08:35:28]

Change committed as 143798