Created a new process type called the service process to host background task...

Created a new process type called the service process to host background tasks such as the Cloud Print Proxy.
BUG=None.
TEST=None.

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

[sanjeevr, 2010-05-11 17:36:50]

Ping.

[darin (slow to review), 2010-05-11 21:32:09]

http://codereview.chromium.org/2001009/diff/26001/1018
File chrome/app/dummy_main_functions.cc (right):

http://codereview.chromium.org/2001009/diff/26001/1018#newcode49
chrome/app/dummy_main_functions.cc:49: 
nit: no extra new line here

http://codereview.chromium.org/2001009/diff/26001/1020
File chrome/common/chrome_switches.cc (right):

http://codereview.chromium.org/2001009/diff/26001/1020#newcode70
chrome/common/chrome_switches.cc:70: const char kCloudPrintProxyId[]      =
"cloud-print-proxy-id";
nit: there is an effort made to align the '=' characters in a column

http://codereview.chromium.org/2001009/diff/26001/1020#newcode678
chrome/common/chrome_switches.cc:678: const char kServiceProcess[]              
  = "service";
nit: align '=' signs

http://codereview.chromium.org/2001009/diff/26001/1020#newcode950
chrome/common/chrome_switches.cc:950: 
nit: extra new line?

http://codereview.chromium.org/2001009/diff/26001/1021
File chrome/common/chrome_switches.h (right):

http://codereview.chromium.org/2001009/diff/26001/1021#newcode294
chrome/common/chrome_switches.h:294: 
nit: no extra new line

http://codereview.chromium.org/2001009/diff/26001/1024
File chrome/service/cloud_print/cloud_print_proxy.h (right):

http://codereview.chromium.org/2001009/diff/26001/1024#newcode30
chrome/service/cloud_print/cloud_print_proxy.h:30: virtual void
DisableForUser();
nit: a new line after this function might be nice for readability

http://codereview.chromium.org/2001009/diff/26001/1025
File chrome/service/gaia/service_gaia_authenticator.cc (right):

http://codereview.chromium.org/2001009/diff/26001/1025#newcode15
chrome/service/gaia/service_gaia_authenticator.cc:15:
gaia::GaiaAuthenticator(user_agent, service_id, gaia_url),
nit: initializer list is not formatted correctly.  should have the colon on the
next line indented by 4 spaces, as shown in the style guide:

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Constr...

http://codereview.chromium.org/2001009/diff/26001/1025#newcode37
chrome/service/gaia/service_gaia_authenticator.cc:37: GURL post_url =
temp.ReplaceComponents(replacements);
maybe this would all be simpler if written like so:

  GURL post_url("https://" + url.host() + ":" + IntToString(443) + url.path());

http://codereview.chromium.org/2001009/diff/26001/1025#newcode56
chrome/service/gaia/service_gaia_authenticator.cc:56: const int
kMaxBackoffDelaySeconds = 60 * 60;  // (1 hour)
nit: indentation

http://codereview.chromium.org/2001009/diff/26001/1025#newcode95
chrome/service/gaia/service_gaia_authenticator.cc:95:
http_post_completed_.Signal();
this is not a safe usage of Signal.  you need to make the lifetime of the
WaitableEvent reference counted so that you are sure that Signal has returned
before you attempt to destroy the WaitableEvent.  we've had crashes before
caused by not doing this.

you can use the fact that ServiceGaiaAuthenticator is reference counted to help
here.

http://codereview.chromium.org/2001009/diff/26001/1026
File chrome/service/gaia/service_gaia_authenticator.h (right):

http://codereview.chromium.org/2001009/diff/26001/1026#newcode35
chrome/service/gaia/service_gaia_authenticator.h:35: void DoPost(const GURL&
post_url, const std::string& post_body);
nit: new line after DoPost?  should some of these methods be private?

http://codereview.chromium.org/2001009/diff/26001/1027
File chrome/service/net/service_url_request_context.cc (right):

http://codereview.chromium.org/2001009/diff/26001/1027#newcode5
chrome/service/net/service_url_request_context.cc:5: #include
"chrome/service/net/service_url_request_context.h"
i stopped reviewing here.

http://codereview.chromium.org/2001009/diff/26001/1028
File chrome/service/net/service_url_request_context.h (right):

http://codereview.chromium.org/2001009/diff/26001/1028#newcode62
chrome/service/net/service_url_request_context.h:62: return
g_service_process->io_thread()->message_loop_proxy();
would it be good to save a pointer to the MessageLoopProxy on construction of
the ServiceURLRequestContextGetter?  are you sure it is safe to call methods on
g_service_process here?

[darin (slow to review), 2010-05-11 22:16:25]

LGTM

http://codereview.chromium.org/2001009/diff/26001/1027
File chrome/service/net/service_url_request_context.cc (right):

http://codereview.chromium.org/2001009/diff/26001/1027#newcode33
chrome/service/net/service_url_request_context.cc:33:
net::HttpNetworkLayer::CreateFactory(NULL, host_resolver_,
nit: indent by 4 or move 'new net::HttpCache(' up to the previous line

[darin (slow to review), 2010-05-11 22:17:09]

On 2010/05/11 22:16:25, darin wrote:
> LGTM

^^^ whoops, i meant: no further comments.  i'd like to take a look at the next
patch to review the WaitableEvent lifetime issue at least.

[sanjeevr, 2010-05-11 23:25:55]

Made all the changes, please take another look.

http://codereview.chromium.org/2001009/diff/26001/1018
File chrome/app/dummy_main_functions.cc (right):

http://codereview.chromium.org/2001009/diff/26001/1018#newcode49
chrome/app/dummy_main_functions.cc:49: 
On 2010/05/11 21:32:09, darin wrote:
> nit: no extra new line here

Done.

http://codereview.chromium.org/2001009/diff/26001/1020
File chrome/common/chrome_switches.cc (right):

http://codereview.chromium.org/2001009/diff/26001/1020#newcode70
chrome/common/chrome_switches.cc:70: const char kCloudPrintProxyId[]      =
"cloud-print-proxy-id";
On 2010/05/11 21:32:09, darin wrote:
> nit: there is an effort made to align the '=' characters in a column

Done.

http://codereview.chromium.org/2001009/diff/26001/1020#newcode678
chrome/common/chrome_switches.cc:678: const char kServiceProcess[]              
  = "service";
On 2010/05/11 21:32:09, darin wrote:
> nit: align '=' signs

Done.

http://codereview.chromium.org/2001009/diff/26001/1020#newcode950
chrome/common/chrome_switches.cc:950: 
On 2010/05/11 21:32:09, darin wrote:
> nit: extra new line?

Done.

http://codereview.chromium.org/2001009/diff/26001/1021
File chrome/common/chrome_switches.h (right):

http://codereview.chromium.org/2001009/diff/26001/1021#newcode294
chrome/common/chrome_switches.h:294: 
On 2010/05/11 21:32:09, darin wrote:
> nit: no extra new line

Done.

http://codereview.chromium.org/2001009/diff/26001/1024
File chrome/service/cloud_print/cloud_print_proxy.h (right):

http://codereview.chromium.org/2001009/diff/26001/1024#newcode30
chrome/service/cloud_print/cloud_print_proxy.h:30: virtual void
DisableForUser();
On 2010/05/11 21:32:09, darin wrote:
> nit: a new line after this function might be nice for readability

Done.

http://codereview.chromium.org/2001009/diff/26001/1025
File chrome/service/gaia/service_gaia_authenticator.cc (right):

http://codereview.chromium.org/2001009/diff/26001/1025#newcode15
chrome/service/gaia/service_gaia_authenticator.cc:15:
gaia::GaiaAuthenticator(user_agent, service_id, gaia_url),
On 2010/05/11 21:32:09, darin wrote:
> nit: initializer list is not formatted correctly.  should have the colon on
the
> next line indented by 4 spaces, as shown in the style guide:
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Constr...

Done.

http://codereview.chromium.org/2001009/diff/26001/1025#newcode37
chrome/service/gaia/service_gaia_authenticator.cc:37: GURL post_url =
temp.ReplaceComponents(replacements);
On 2010/05/11 21:32:09, darin wrote:
> maybe this would all be simpler if written like so:
> 
>   GURL post_url("https://" + url.host() + ":" + IntToString(443) +
url.path());

Done.

This was code copied verbatim from elsewhere.

http://codereview.chromium.org/2001009/diff/26001/1025#newcode56
chrome/service/gaia/service_gaia_authenticator.cc:56: const int
kMaxBackoffDelaySeconds = 60 * 60;  // (1 hour)
On 2010/05/11 21:32:09, darin wrote:
> nit: indentation

Done.

http://codereview.chromium.org/2001009/diff/26001/1025#newcode95
chrome/service/gaia/service_gaia_authenticator.cc:95:
http_post_completed_.Signal();
On 2010/05/11 21:32:09, darin wrote:
> this is not a safe usage of Signal.  you need to make the lifetime of the
> WaitableEvent reference counted so that you are sure that Signal has returned
> before you attempt to destroy the WaitableEvent.  we've had crashes before
> caused by not doing this.
> 
> you can use the fact that ServiceGaiaAuthenticator is reference counted to
help
> here.

Done.

http://codereview.chromium.org/2001009/diff/26001/1026
File chrome/service/gaia/service_gaia_authenticator.h (right):

http://codereview.chromium.org/2001009/diff/26001/1026#newcode35
chrome/service/gaia/service_gaia_authenticator.h:35: void DoPost(const GURL&
post_url, const std::string& post_body);
On 2010/05/11 21:32:09, darin wrote:
> nit: new line after DoPost?  should some of these methods be private?

Done.

http://codereview.chromium.org/2001009/diff/26001/1027
File chrome/service/net/service_url_request_context.cc (right):

http://codereview.chromium.org/2001009/diff/26001/1027#newcode33
chrome/service/net/service_url_request_context.cc:33:
net::HttpNetworkLayer::CreateFactory(NULL, host_resolver_,
On 2010/05/11 22:16:26, darin wrote:
> nit: indent by 4 or move 'new net::HttpCache(' up to the previous line

Done.

http://codereview.chromium.org/2001009/diff/26001/1028
File chrome/service/net/service_url_request_context.h (right):

http://codereview.chromium.org/2001009/diff/26001/1028#newcode62
chrome/service/net/service_url_request_context.h:62: return
g_service_process->io_thread()->message_loop_proxy();
On 2010/05/11 21:32:09, darin wrote:
> would it be good to save a pointer to the MessageLoopProxy on construction of
> the ServiceURLRequestContextGetter?  are you sure it is safe to call methods
on
> g_service_process here?

Done.

[darin (slow to review), 2010-05-12 05:03:47]

http://codereview.chromium.org/2001009/diff/26001/1025
File chrome/service/gaia/service_gaia_authenticator.cc (right):

http://codereview.chromium.org/2001009/diff/26001/1025#newcode37
chrome/service/gaia/service_gaia_authenticator.cc:37: GURL post_url =
temp.ReplaceComponents(replacements);
> >   GURL post_url("https://" + url.host() + ":" + IntToString(443) +
> url.path());

Actually the default port for HTTPS is 443.  It shouldn't be necessary to
explicitly set the port like this.  It'll just get stripped by GURL anyways.


> This was code copied verbatim from elsewhere.

That should be fixed too, or just file a bug ;-)

[darin (slow to review), 2010-05-12 05:10:51]

http://codereview.chromium.org/2001009/diff/36001/37009
File chrome/service/gaia/service_gaia_authenticator.cc (right):

http://codereview.chromium.org/2001009/diff/36001/37009#newcode84
chrome/service/gaia/service_gaia_authenticator.cc:84:
MessageLoop::current()->DeleteSoon(FROM_HERE, source);
are you sure this is necessary?   i think URLFetcher is designed to support
being deleted within OnURLFetchComplete as the guts of it are kept as a
reference counted Core object.  if this were not necessary, then you could just
make the fetcher be a member variable of ServiceGaiaAuthenticator such that the
URLFetcher instance gets destroyed in ~ServiceGaiaAuthenticator.

http://codereview.chromium.org/2001009/diff/36001/37009#newcode87
chrome/service/gaia/service_gaia_authenticator.cc:87: AddRef();
normally, we just use scoped_refptr for this:

  scoped_refptr<ServiceGaiaAuthenticator> keep_alive(this);

[brettw, 2010-05-12 05:11:39]

http://codereview.chromium.org/2001009/diff/26001/1025
File chrome/service/gaia/service_gaia_authenticator.cc (right):

http://codereview.chromium.org/2001009/diff/26001/1025#newcode37
chrome/service/gaia/service_gaia_authenticator.cc:37: GURL post_url =
temp.ReplaceComponents(replacements);
I try to discourage building URLs by hand. The old code managed to get the worse
parts of the "replacement" approach, while also building the initial URL by
hand.

I'm not really sure what the goal of this is. Are you trying to strip the query
and ref? Are you trying to build an https URL from an http one? Both?

If this is the first:

GURL::Replacements replacements;
replacements.ClearQuery();
replacements.ClearRef();
GURL post_url = url.ReplaceComponents(replacements);

If this is the second:

GURL::Replacements replacements;
replacements.SetScheme("https");
replacements.ClearPort();
GURL post_url = url.ReplaceComponents(replacements);

If the URL is from some random place, you may also want to clear the username
and the password. If the URL is from some reasonable known source, you don't
need to worry about those.

[sanjeevr, 2010-05-12 17:03:59]

This was code copied from BridgedGaiaAuthenticator in syncapi.cc, I did not
pay too much attention to this because it was working code ;). I will look
into this again carefully and fix, sorry about that. I think the original
idea was forcibly add :443 as the port to the GAIA url (the url is passed in
as an argument to the class). Perhaps I can just use the url as is.

On Tue, May 11, 2010 at 10:11 PM, <brettw@chromium.org> wrote:

>
> http://codereview.chromium.org/2001009/diff/26001/1025
>
> File chrome/service/gaia/service_gaia_authenticator.cc (right):
>
> http://codereview.chromium.org/2001009/diff/26001/1025#newcode37
> chrome/service/gaia/service_gaia_authenticator.cc:37: GURL post_url =
> temp.ReplaceComponents(replacements);
> I try to discourage building URLs by hand. The old code managed to get
> the worse parts of the "replacement" approach, while also building the
> initial URL by hand.
>
> I'm not really sure what the goal of this is. Are you trying to strip
> the query and ref? Are you trying to build an https URL from an http
> one? Both?
>
> If this is the first:
>
> GURL::Replacements replacements;
> replacements.ClearQuery();
> replacements.ClearRef();
> GURL post_url = url.ReplaceComponents(replacements);
>
> If this is the second:
>
> GURL::Replacements replacements;
> replacements.SetScheme("https");
> replacements.ClearPort();
> GURL post_url = url.ReplaceComponents(replacements);
>
> If the URL is from some random place, you may also want to clear the
> username and the password. If the URL is from some reasonable known
> source, you don't need to worry about those.
>
>
> http://codereview.chromium.org/2001009/show
>

[sanjeevr, 2010-05-12 17:45:35]

Please take another look.

http://codereview.chromium.org/2001009/diff/26001/1025
File chrome/service/gaia/service_gaia_authenticator.cc (right):

http://codereview.chromium.org/2001009/diff/26001/1025#newcode37
chrome/service/gaia/service_gaia_authenticator.cc:37: GURL post_url =
temp.ReplaceComponents(replacements);
On 2010/05/12 05:11:39, brettw wrote:
> I try to discourage building URLs by hand. The old code managed to get the
worse
> parts of the "replacement" approach, while also building the initial URL by
> hand.
> 
> I'm not really sure what the goal of this is. Are you trying to strip the
query
> and ref? Are you trying to build an https URL from an http one? Both?
> 
> If this is the first:
> 
> GURL::Replacements replacements;
> replacements.ClearQuery();
> replacements.ClearRef();
> GURL post_url = url.ReplaceComponents(replacements);
> 
> If this is the second:
> 
> GURL::Replacements replacements;
> replacements.SetScheme("https");
> replacements.ClearPort();
> GURL post_url = url.ReplaceComponents(replacements);
> 
> If the URL is from some random place, you may also want to clear the username
> and the password. If the URL is from some reasonable known source, you don't
> need to worry about those.

I just used the passed in URL. I just added a DCHECK for SchemeIsSecure.

http://codereview.chromium.org/2001009/diff/36001/37009#newcode84
chrome/service/gaia/service_gaia_authenticator.cc:84: int response_code, const
ResponseCookies &cookies,
On 2010/05/12 05:10:51, darin wrote:
> are you sure this is necessary?   i think URLFetcher is designed to support
> being deleted within OnURLFetchComplete as the guts of it are kept as a
> reference counted Core object.  if this were not necessary, then you could
just
> make the fetcher be a member variable of ServiceGaiaAuthenticator such that
the
> URLFetcher instance gets destroyed in ~ServiceGaiaAuthenticator.

Done.

http://codereview.chromium.org/2001009/diff/36001/37009#newcode87
chrome/service/gaia/service_gaia_authenticator.cc:87: http_response_code_ =
response_code;
On 2010/05/12 05:10:51, darin wrote:
> normally, we just use scoped_refptr for this:
> 
>   scoped_refptr<ServiceGaiaAuthenticator> keep_alive(this);

Done.

[darin (slow to review), 2010-05-12 18:03:06]

LGTM