Cloud print proxy management UI.

Cloud print proxy management UI.
Based in the Under-the-Hood section of options, this UI allows mere mortals to turn on and off the cloud print proxy.  Currently working on Windows only, behind a flag.

BUG=none
TEST=Open up the options, turn CPP on and off.

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

[Scott Byer, 2010-09-22 00:56:01]

Here is the code for implementing the UI for the proxy.  Splitting the IPC on
getting the enabled e-mail address worked well, and the UI is behaving.  It's
all under a flag for now.

[sanjeevr, 2010-09-22 23:49:37]

http://codereview.chromium.org/3450021/diff/16001/17001
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/3450021/diff/16001/17001#newcode7149
chrome/app/generated_resources.grd:7149: Cloud Print
Check whether we should localize Cloud Print.

http://codereview.chromium.org/3450021/diff/16001/17004
File chrome/browser/printing/cloud_print/cloud_print_setup_flow.cc (right):

http://codereview.chromium.org/3450021/diff/16001/17004#newcode114
chrome/browser/printing/cloud_print/cloud_print_setup_flow.cc:114: //
Synchronous messages require a timeout.
This comment is obsolete.

http://codereview.chromium.org/3450021/diff/16001/17004#newcode152
chrome/browser/printing/cloud_print/cloud_print_setup_flow.cc:152: return NULL;
When we are being invoked from the notification handler, there may be no browser
window.

http://codereview.chromium.org/3450021/diff/16001/17004#newcode168
chrome/browser/printing/cloud_print/cloud_print_setup_flow.cc:168: if
(process_control->is_connected())
We should probably invoke the service process here if it isn't running.

http://codereview.chromium.org/3450021/diff/16001/17004#newcode284
chrome/browser/printing/cloud_print/cloud_print_setup_flow.cc:284:
GaiaConstants::kSyncService);
We don't need this call.

http://codereview.chromium.org/3450021/diff/16001/17004#newcode294
chrome/browser/printing/cloud_print/cloud_print_setup_flow.cc:294: sync_token_ =
auth_token;
We don't need the sync token.

http://codereview.chromium.org/3450021/diff/16001/17004#newcode321
chrome/browser/printing/cloud_print/cloud_print_setup_flow.cc:321: if
(process_control_->is_connected()) {
Why not else?

http://codereview.chromium.org/3450021/diff/16001/17004#newcode354
chrome/browser/printing/cloud_print/cloud_print_setup_flow.cc:354: // Method
called by CloudPrintServicePRocessHelper
Nit: Typo

http://codereview.chromium.org/3450021/diff/16001/17004#newcode362
chrome/browser/printing/cloud_print/cloud_print_setup_flow.cc:362: // Save the
preference that we have completed the setup of remoting.
remoting=>Cloud Print

http://codereview.chromium.org/3450021/diff/16001/17005
File chrome/browser/printing/cloud_print/cloud_print_setup_flow.h (right):

http://codereview.chromium.org/3450021/diff/16001/17005#newcode25
chrome/browser/printing/cloud_print/cloud_print_setup_flow.h:25: // This class
is responsible for showing a remoting setup dialog and perform
remoting=>Cloud Print

http://codereview.chromium.org/3450021/diff/16001/17009
File chrome/browser/service/service_process_control.h (right):

http://codereview.chromium.org/3450021/diff/16001/17009#newcode131
chrome/browser/service/service_process_control.h:131: scoped_ptr<Callback2<bool,
std::string>::Type> cloud_print_status_callback_;
Wouldn't it be safer to use a scoped_refptr and make the callback refounted?

http://codereview.chromium.org/3450021/diff/16001/17012
File chrome/common/net/gaia/gaia_constants.cc (right):

http://codereview.chromium.org/3450021/diff/16001/17012#newcode24
chrome/common/net/gaia/gaia_constants.cc:24: const char kCloudPrintService[] =
"cloudprint";
And this

http://codereview.chromium.org/3450021/diff/16001/17013
File chrome/common/net/gaia/gaia_constants.h (right):

http://codereview.chromium.org/3450021/diff/16001/17013#newcode21
chrome/common/net/gaia/gaia_constants.h:21: extern const char
kCloudPrintService[];
You should not need this any more

[Scott Byer, 2010-09-23 19:02:33]

http://codereview.chromium.org/3450021/diff/16001/17001
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/3450021/diff/16001/17001#newcode7149
chrome/app/generated_resources.grd:7149: Cloud Print
On 2010/09/22 23:49:37, sanjeevr wrote:
> Check whether we should localize Cloud Print.

Done.

http://codereview.chromium.org/3450021/diff/16001/17004
File chrome/browser/printing/cloud_print/cloud_print_setup_flow.cc (right):

http://codereview.chromium.org/3450021/diff/16001/17004#newcode114
chrome/browser/printing/cloud_print/cloud_print_setup_flow.cc:114: //
Synchronous messages require a timeout.
On 2010/09/22 23:49:37, sanjeevr wrote:
> This comment is obsolete.

Done.

http://codereview.chromium.org/3450021/diff/16001/17004#newcode152
chrome/browser/printing/cloud_print/cloud_print_setup_flow.cc:152: return NULL;
On 2010/09/22 23:49:37, sanjeevr wrote:
> When we are being invoked from the notification handler, there may be no
browser
> window.

Done.

http://codereview.chromium.org/3450021/diff/16001/17004#newcode168
chrome/browser/printing/cloud_print/cloud_print_setup_flow.cc:168: if
(process_control->is_connected())
On 2010/09/22 23:49:37, sanjeevr wrote:
> We should probably invoke the service process here if it isn't running.

Done.

http://codereview.chromium.org/3450021/diff/16001/17004#newcode284
chrome/browser/printing/cloud_print/cloud_print_setup_flow.cc:284:
GaiaConstants::kSyncService);
On 2010/09/22 23:49:37, sanjeevr wrote:
> We don't need this call.

Done.

http://codereview.chromium.org/3450021/diff/16001/17004#newcode294
chrome/browser/printing/cloud_print/cloud_print_setup_flow.cc:294: sync_token_ =
auth_token;
On 2010/09/22 23:49:37, sanjeevr wrote:
> We don't need the sync token.

Done.

http://codereview.chromium.org/3450021/diff/16001/17004#newcode321
chrome/browser/printing/cloud_print/cloud_print_setup_flow.cc:321: if
(process_control_->is_connected()) {
On 2010/09/22 23:49:37, sanjeevr wrote:
> Why not else?

Investigating, it looks like the semantics of Launch changed, so that the task
is always run as long as the the process control can get connected again.  This
line was no longer needed, removed.

http://codereview.chromium.org/3450021/diff/16001/17004#newcode354
chrome/browser/printing/cloud_print/cloud_print_setup_flow.cc:354: // Method
called by CloudPrintServicePRocessHelper
On 2010/09/22 23:49:37, sanjeevr wrote:
> Nit: Typo

Done.

http://codereview.chromium.org/3450021/diff/16001/17004#newcode362
chrome/browser/printing/cloud_print/cloud_print_setup_flow.cc:362: // Save the
preference that we have completed the setup of remoting.
On 2010/09/22 23:49:37, sanjeevr wrote:
> remoting=>Cloud Print

Done.

http://codereview.chromium.org/3450021/diff/16001/17005
File chrome/browser/printing/cloud_print/cloud_print_setup_flow.h (right):

http://codereview.chromium.org/3450021/diff/16001/17005#newcode25
chrome/browser/printing/cloud_print/cloud_print_setup_flow.h:25: // This class
is responsible for showing a remoting setup dialog and perform
On 2010/09/22 23:49:37, sanjeevr wrote:
> remoting=>Cloud Print

Done.

http://codereview.chromium.org/3450021/diff/16001/17009
File chrome/browser/service/service_process_control.h (right):

http://codereview.chromium.org/3450021/diff/16001/17009#newcode131
chrome/browser/service/service_process_control.h:131: scoped_ptr<Callback2<bool,
std::string>::Type> cloud_print_status_callback_;
On 2010/09/22 23:49:37, sanjeevr wrote:
> Wouldn't it be safer to use a scoped_refptr and make the callback refounted?

The callback being handed in is a scoped callback (holding a weak ptr to the
contained object); Safe.

http://codereview.chromium.org/3450021/diff/16001/17013
File chrome/common/net/gaia/gaia_constants.h (right):

http://codereview.chromium.org/3450021/diff/16001/17013#newcode21
chrome/common/net/gaia/gaia_constants.h:21: extern const char
kCloudPrintService[];
On 2010/09/22 23:49:37, sanjeevr wrote:
> You should not need this any more

You would think, no?  It turns out that using the one from cloud_print_consts.h
doesn't work for most of the tests - link error.  Should I just have the service
point back to using this one instead?

[sanjeevr, 2010-09-23 21:59:39]

LGTM with 2 nits:

1. There are other instances of "Cloud Print" in the GRD file which we should
probably not localize.

2. Should we localize the manage URL?

[Scott Byer, 2010-09-25 01:31:53]

Enough changed with the browser lifetime fix, if you could take one last look. 
Thanks!

-Scott

On 2010/09/23 21:59:39, sanjeevr wrote:
> LGTM with 2 nits:
> 
> 1. There are other instances of "Cloud Print" in the GRD file which we should
> probably not localize.
> 
> 2. Should we localize the manage URL?

[sanjeevr, 2010-09-25 04:26:21]

LGTM with nits.

http://codereview.chromium.org/3450021/diff/40001/41004
File chrome/browser/printing/cloud_print/cloud_print_setup_flow.cc (right):

http://codereview.chromium.org/3450021/diff/40001/41004#newcode195
chrome/browser/printing/cloud_print/cloud_print_setup_flow.cc:195: // If a
browser doesn't exist, create one (sans window) to run the
This comment is invalid

http://codereview.chromium.org/3450021/diff/40001/41005
File chrome/browser/printing/cloud_print/cloud_print_setup_flow.h (right):

http://codereview.chromium.org/3450021/diff/40001/41005#newcode87
chrome/browser/printing/cloud_print/cloud_print_setup_flow.h:87:
CloudPrintSetupFlow(const std::string& args, Browser* profile);
Nit: Browser* browser. Also please add a comment that the class takes ownership
of browser.

http://codereview.chromium.org/3450021/diff/40001/41010
File chrome/browser/views/options/advanced_contents_view.cc (right):

http://codereview.chromium.org/3450021/diff/40001/41010#newcode1432
chrome/browser/views/options/advanced_contents_view.cc:1432: // lost behind
other windows.
Wrong comment.

http://codereview.chromium.org/3450021/diff/40001/41010#newcode1433
chrome/browser/views/options/advanced_contents_view.cc:1433: Browser* browser =
Browser::Create(profile());
I presume in this case, we don't need to delete browser because we are opening a
tab?