rlz: Add an implementation of PingServer() that uses chrome's net stack.

rlz: Add an implementation of PingServer() that uses chrome's net stack.

One can select an implementation via a gyp variable that defaults to wininet
on windows.

PingServer() currently has blocking semantics (it's already called on a thread
in chrome), so the chrome net implementation is that way too. However, when
chrome's net stack is used, an URLRequestContextGetter needs to be passed
to rlz explicitly. That's a bit fishy from an API perspective, but having a 2nd
DNS resolver, proxy cache, etc in chrome just for RLZ felt wrong.

Unfortunately, URLFetcher is currently in chrome's 'content' module, so this
adds a dependency on 'content'. akalin is moving it to 'net', and once that's
done only a dependency on 'net' is required. In the test code, I was careful
to not use BrowserThread, so that the test doesn't depend on 'content' either.

BUG=chromium:117741
TEST=run rlz_unittests on mac with a local server, observe request

[thakis, 2012-03-21 21:12:57]

I think this design is the best tradeoff between rlz API stability and reusing
chrome infrastructure in rlz.

https://codereview.appspot.com/5864055/diff/1/lib/financial_ping.cc
File lib/financial_ping.cc (right):

https://codereview.appspot.com/5864055/diff/1/lib/financial_ping.cc#newcode279
lib/financial_ping.cc:279: net::LOAD_DO_NOT_PROMPT_FOR_LOGIN |  // XXX?
willchan: Do I want these?

[thakis, 2012-03-21 21:14:14]

Forgot to say: I want to make chrome/windows use the chrome/net implementation
too, the old codepath is for other projects that use rlz. The reasoning is that
chrome can stop depending on wininet then, which is very desirable according to
local chrome/win hackers.

[Roger Tawa (Google), 2012-03-22 00:31:03]

Ni Nico,

Some comments below.

https://codereview.appspot.com/5864055/diff/5001/lib/financial_ping.cc
File lib/financial_ping.cc (right):

https://codereview.appspot.com/5864055/diff/5001/lib/financial_ping.cc#newcod...
lib/financial_ping.cc:273: request);
nit: indents not correct

https://codereview.appspot.com/5864055/diff/5001/lib/financial_ping.cc#newcod...
lib/financial_ping.cc:293: MessageLoop::QuitClosure(), kTimeout);
nit: indenting above not correct.

https://codereview.appspot.com/5864055/diff/5001/lib/financial_ping.cc#newcod...
lib/financial_ping.cc:300: return fetcher->GetResponseAsString(response);
delete the fetcher ?

https://codereview.appspot.com/5864055/diff/5001/lib/rlz_lib.h
File lib/rlz_lib.h (right):

https://codereview.appspot.com/5864055/diff/5001/lib/rlz_lib.h#newcode68
lib/rlz_lib.h:68: bool RLZ_LIB_API
SetURLRequestContext(net::URLRequestContextGetter* context);
how is the meant to be called?  once at application start up?  does it depend on
particular profile or anything like that?

https://codereview.appspot.com/5864055/diff/5001/win/lib/rlz_lib_test.cc
File win/lib/rlz_lib_test.cc (right):

https://codereview.appspot.com/5864055/diff/5001/win/lib/rlz_lib_test.cc#newc...
win/lib/rlz_lib_test.cc:461: rlz_lib::SetURLRequestContext(NULL);
i don't like this api.  would be better to create a helper class like was
defined for setting supplementary branding, where the ctor sets he context and
the dtor clears it.  minimizes chance of error on client side.

[thakis, 2012-03-22 00:37:57]

Thanks for the comments! I'll upload a patchset that addressed the bits I didn't
reply to later today.

https://codereview.appspot.com/5864055/diff/5001/lib/financial_ping.cc
File lib/financial_ping.cc (right):

https://codereview.appspot.com/5864055/diff/5001/lib/financial_ping.cc#newcod...
lib/financial_ping.cc:300: return fetcher->GetResponseAsString(response);
On 2012/03/22 00:31:03, rogerta wrote:
> delete the fetcher ?

Err, good catch. I'll make it a scoped_ptr.

https://codereview.appspot.com/5864055/diff/5001/lib/rlz_lib.h
File lib/rlz_lib.h (right):

https://codereview.appspot.com/5864055/diff/5001/lib/rlz_lib.h#newcode68
lib/rlz_lib.h:68: bool RLZ_LIB_API
SetURLRequestContext(net::URLRequestContextGetter* context);
On 2012/03/22 00:31:03, rogerta wrote:
> how is the meant to be called?  once at application start up?  does it depend
on
> particular profile or anything like that?

In chrome, it'll be called once at application startup.

https://codereview.appspot.com/5864055/diff/5001/win/lib/rlz_lib_test.cc
File win/lib/rlz_lib_test.cc (right):

https://codereview.appspot.com/5864055/diff/5001/win/lib/rlz_lib_test.cc#newc...
win/lib/rlz_lib_test.cc:461: rlz_lib::SetURLRequestContext(NULL);
On 2012/03/22 00:31:03, rogerta wrote:
> i don't like this api.  would be better to create a helper class like was
> defined for setting supplementary branding, where the ctor sets he context and
> the dtor clears it.  minimizes chance of error on client side.

This call isn't strictly necessary, it's just for cleanliness. The actual client
code won't do this.

If you want, I can write a RAII class in this test file and use it here.

[thakis, 2012-03-22 00:38:13]

Thanks for the comments! I'll upload a patchset that addressed the bits I didn't
reply to later today.

[thakis, 2012-03-22 05:12:13]

https://codereview.appspot.com/5864055/diff/5001/lib/financial_ping.cc
File lib/financial_ping.cc (right):

https://codereview.appspot.com/5864055/diff/5001/lib/financial_ping.cc#newcod...
lib/financial_ping.cc:273: request);
On 2012/03/22 00:31:03, rogerta wrote:
> nit: indents not correct

Done.

https://codereview.appspot.com/5864055/diff/5001/lib/financial_ping.cc#newcod...
lib/financial_ping.cc:293: MessageLoop::QuitClosure(), kTimeout);
On 2012/03/22 00:31:03, rogerta wrote:
> nit: indenting above not correct.

Done.

https://codereview.appspot.com/5864055/diff/5001/lib/financial_ping.cc#newcod...
lib/financial_ping.cc:300: return fetcher->GetResponseAsString(response);
On 2012/03/22 00:37:57, thakis wrote:
> On 2012/03/22 00:31:03, rogerta wrote:
> > delete the fetcher ?
> 
> Err, good catch. I'll make it a scoped_ptr.

Done.

https://codereview.appspot.com/5864055/diff/5001/win/lib/rlz_lib_test.cc
File win/lib/rlz_lib_test.cc (right):

https://codereview.appspot.com/5864055/diff/5001/win/lib/rlz_lib_test.cc#newc...
win/lib/rlz_lib_test.cc:461: rlz_lib::SetURLRequestContext(NULL);
On 2012/03/22 00:37:57, thakis wrote:
> On 2012/03/22 00:31:03, rogerta wrote:
> > i don't like this api.  would be better to create a helper class like was
> > defined for setting supplementary branding, where the ctor sets he context
and
> > the dtor clears it.  minimizes chance of error on client side.
> 
> This call isn't strictly necessary, it's just for cleanliness. The actual
client
> code won't do this.
> 
> If you want, I can write a RAII class in this test file and use it here.

Done.

[Roger Tawa (Google), 2012-03-22 12:56:00]

lgtm, one nit below

https://codereview.appspot.com/5864055/diff/1013/lib/rlz_lib_test.cc
File lib/rlz_lib_test.cc (right):

https://codereview.appspot.com/5864055/diff/1013/lib/rlz_lib_test.cc#newcode442
lib/rlz_lib_test.cc:442: rlz_lib::SetURLRequestContext(NULL);
indent

[willchan, 2012-03-22 19:32:14]

Mostly nits. The main thing I'm concerned about is your threading model now.

https://codereview.appspot.com/5864055/diff/10001/lib/financial_ping.cc
File lib/financial_ping.cc (right):

https://codereview.appspot.com/5864055/diff/10001/lib/financial_ping.cc#newco...
lib/financial_ping.cc:279: net::LOAD_DO_NOT_SEND_AUTH_DATA |  // XXX?
Sure, won't hurt.

https://codereview.appspot.com/5864055/diff/10001/lib/financial_ping.cc#newco...
lib/financial_ping.cc:280: net::LOAD_DO_NOT_PROMPT_FOR_LOGIN |  // XXX?
Yeah, you should use this one.

https://codereview.appspot.com/5864055/diff/10001/lib/financial_ping.h
File lib/financial_ping.h (right):

https://codereview.appspot.com/5864055/diff/10001/lib/financial_ping.h#newcode53
lib/financial_ping.h:53: static bool
SetURLRequestContext(net::URLRequestContextGetter* context);
Is the threading model documented anywhere?

https://codereview.appspot.com/5864055/diff/10001/rlz.gyp
File rlz.gyp (right):

https://codereview.appspot.com/5864055/diff/10001/rlz.gyp#newcode82
rlz.gyp:82: '../content/content.gyp:content_common',
I would think you would add the net dependency, rather than relying on
transitive dependencies.

[thakis, 2012-03-23 01:14:23]

Thanks, Will!

I'm addressing the nits in https://codereview.appspot.com/5882054

https://codereview.appspot.com/5864055/diff/10001/lib/financial_ping.h
File lib/financial_ping.h (right):

https://codereview.appspot.com/5864055/diff/10001/lib/financial_ping.h#newcode53
lib/financial_ping.h:53: static bool
SetURLRequestContext(net::URLRequestContextGetter* context);
On 2012/03/22 19:32:14, willchan wrote:
> Is the threading model documented anywhere?

I don't think it's explicitly documented. Basically, every function that can be
accessed from several threads and isn't pure takes the rlz store lock.

As discussed over IRC, rlz used to schedule a new thread of every ping. I moved
it over to the file thread (here http://codereview.chromium.org/9404021 if
you're curious), which means the chrome net implementation now blocks the file
thread, which isn't so hot.

I'll check if brett's thread pool is now read to use and move the calling code
either to that or to creating a new thread using base/thread.h