Cloud print connector policy.

Cloud print connector policy.

This implements the policy inside the browser process, shutting down the cloud
print connector if the policy is set to disabled. This isn't a complete
solution, as the browser needs to be running or be launched for the policy to
take effect. (It will take a lot more refactoring for the service process to
use the policy code). The hole without the refactoring is that if you enable
the connector, quit Chromium, and set the policy, the connector will stay alive
until the next launch of Chromium.

The browser process checks the policy on startup, and listens for it changing
as long as it is up. You can sit on the Under the Hood page and watch the
button change state on Windows as you fiddle with the policy.

BUG=59769
TEST=Set Cloud Print Proxy policy to disabled, bring up browser, Options/Under
the Hood - Sign in to Google Cloud print will be disabled.  Unset policy, wait,
button becomes active.  Log in to cloud print.  Quit Chromium, note service
process hangs around for more than a minute.  Set policy, launch and quit
Chromium. Note that the service process quits within a minute.


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

[Peter Kasting, 2011-11-01 22:54:09]

http://codereview.chromium.org/8438020/diff/1/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/8438020/diff/1/chrome/browser/ui/browser.cc#ne...
chrome/browser/ui/browser.cc:4634: UpdateCloudPrintConnectorState();
This seems like the wrong place for this.  This function is only for
enabling/disabling commands, not other random services.

[Mattias Nissler (ping if slow), 2011-11-02 10:55:56]

http://codereview.chromium.org/8438020/diff/1/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/8438020/diff/1/chrome/browser/ui/browser.cc#ne...
chrome/browser/ui/browser.cc:4634: UpdateCloudPrintConnectorState();
On 2011/11/01 22:54:10, Peter Kasting wrote:
> This seems like the wrong place for this.  This function is only for
> enabling/disabling commands, not other random services.

Agree with Peter. This should probably be hooked off BrowserProcess somehow.

http://codereview.chromium.org/8438020/diff/1/chrome/browser/ui/browser.cc#ne...
chrome/browser/ui/browser.cc:4793: if (!email.empty())
need curlies.

[Scott Byer, 2011-11-02 21:33:25]

Ok, moved over to browser_process_impl. It does make more sense there.

http://codereview.chromium.org/8438020/diff/1/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/8438020/diff/1/chrome/browser/ui/browser.cc#ne...
chrome/browser/ui/browser.cc:4634: UpdateCloudPrintConnectorState();
Makes sense - it was a bit trickier, but not too bad, done.

On 2011/11/02 10:55:56, Mattias Nissler wrote:
> On 2011/11/01 22:54:10, Peter Kasting wrote:
> > This seems like the wrong place for this.  This function is only for
> > enabling/disabling commands, not other random services.
> 
> Agree with Peter. This should probably be hooked off BrowserProcess somehow.

http://codereview.chromium.org/8438020/diff/1/chrome/browser/ui/browser.cc#ne...
chrome/browser/ui/browser.cc:4793: if (!email.empty())
On 2011/11/02 10:55:56, Mattias Nissler wrote:
> need curlies.

Done.

[Peter Kasting, 2011-11-02 22:09:59]

LGTM

http://codereview.chromium.org/8438020/diff/5001/chrome/browser/browser_proce...
File chrome/browser/browser_process_impl.cc (right):

http://codereview.chromium.org/8438020/diff/5001/chrome/browser/browser_proce...
chrome/browser/browser_process_impl.cc:699: } else if (type ==
chrome::NOTIFICATION_PROFILE_CREATED) {
Nit: I would either do:

if (type == chrome::NOTIFICATION_PREF_CHANGED) {
  ...
  return;
}
DCHECK_EQ(chrome::NOTIFICATION_PROFILE_CREATED);
...

or else

switch (type) {
  case chrome::NOTIFICATION_PREF_CHANGED:
    ...;
    break;

  case chrome::NOTIFICATION_PROFILE_CREATED:
    ...;
    break;

  default:
    NOTREACHED();
}

http://codereview.chromium.org/8438020/diff/5001/chrome/browser/browser_proce...
chrome/browser/browser_process_impl.cc:702: if (profile)
Can this ever be NULL?  If not then don't add a conditional here.

http://codereview.chromium.org/8438020/diff/5001/chrome/browser/browser_proce...
chrome/browser/browser_process_impl.cc:989: this,
chrome::NOTIFICATION_PROFILE_CREATED,
Nit: These args can go on the prior line

http://codereview.chromium.org/8438020/diff/5001/chrome/browser/browser_proce...
chrome/browser/browser_process_impl.cc:1095: ProfileManager* pm =
profile_manager();
Nit: Could just roll this into the next line

http://codereview.chromium.org/8438020/diff/5001/chrome/browser/browser_proce...
chrome/browser/browser_process_impl.cc:1097: for (size_t i = 0; i <
profiles.size(); ++i)
Nit: I suggest using an iterator (rest of this file seems to generally prefer
iterators to indexes where possible).  Feel free to add a typedef for
vector<Profile*> to profile_manager.h if it makes life easier.

[Scott Byer, 2011-11-03 20:22:28]

Mattias, PTAL? I need an LGTM for the policy_templates.json part. Thanks!

[Mattias Nissler (ping if slow), 2011-11-04 00:19:15]

The code you have works, but I think a different solution is better after all.
Sorry for not realizing the full picture when I first looked at this. Also, it
would be helpful if we could have a unit test that makes sure the connector gets
properly disabled when the policy is present and that we switch state correctly
on policy changes (TestingProfile sports a TestingPrefService that allows for
easy adjustment of policy settings for testing).

http://codereview.chromium.org/8438020/diff/5002/chrome/browser/browser_proce...
File chrome/browser/browser_process_impl.cc (right):

http://codereview.chromium.org/8438020/diff/5002/chrome/browser/browser_proce...
chrome/browser/browser_process_impl.cc:1096: // Search the profiles for any
enabled connectors and shut them down.
So the cloud print connector is per-profile (I wasn't aware of that)? If that is
the case, we should probably move the policy handling code to some place that is
hooked off profile after all. CloudPrintProxyService comes to mind, it can read
prefs::kCloudPrintProxyEnabled from profile->GetPrefs() (you'll need to change
pref registration, but that should be all), and CloudPrintProxyService can
directly listen for notifications on pref updates. That also saves you the
trouble to go through all profiles here. Sorry for directing you to
BrowserProcessImpl first, I just didn't know enough context.

[Scott Byer, 2011-11-11 23:56:34]

Mattias, I agree with you; I've moved the watching of the preference over to the
cloud print proxy service itself, the code seems a bit more straightforward that
way, and it allowed for the addition of unit tests. It was a good chance for me
to work on my gMock skills!

PTAL, I really appreciate the time.

-Scott

On 2011/11/04 00:19:15, Mattias Nissler wrote:
> The code you have works, but I think a different solution is better after all.
> Sorry for not realizing the full picture when I first looked at this. Also, it
> would be helpful if we could have a unit test that makes sure the connector
gets
> properly disabled when the policy is present and that we switch state
correctly
> on policy changes (TestingProfile sports a TestingPrefService that allows for
> easy adjustment of policy settings for testing).
> 
>
http://codereview.chromium.org/8438020/diff/5002/chrome/browser/browser_proce...
> File chrome/browser/browser_process_impl.cc (right):
> 
>
http://codereview.chromium.org/8438020/diff/5002/chrome/browser/browser_proce...
> chrome/browser/browser_process_impl.cc:1096: // Search the profiles for any
> enabled connectors and shut them down.
> So the cloud print connector is per-profile (I wasn't aware of that)? If that
is
> the case, we should probably move the policy handling code to some place that
is
> hooked off profile after all. CloudPrintProxyService comes to mind, it can
read
> prefs::kCloudPrintProxyEnabled from profile->GetPrefs() (you'll need to change
> pref registration, but that should be all), and CloudPrintProxyService can
> directly listen for notifications on pref updates. That also saves you the
> trouble to go through all profiles here. Sorry for directing you to
> BrowserProcessImpl first, I just didn't know enough context.

[Mattias Nissler (ping if slow), 2011-11-14 08:33:06]

Nice unit tests! Not much to complain about, but I put a couple of suggestions
and nits.

http://codereview.chromium.org/8438020/diff/13002/chrome/browser/printing/clo...
File chrome/browser/printing/cloud_print/cloud_print_proxy_service.cc (right):

http://codereview.chromium.org/8438020/diff/13002/chrome/browser/printing/clo...
chrome/browser/printing/cloud_print/cloud_print_proxy_service.cc:91:
DCHECK(profile_->GetPrefs()->GetBoolean(prefs::kCloudPrintProxyEnabled));
nit: Since EnableForUser() is in the public interface, I'd upgrade this check to
not post the task if it's disabled. That's just my gut feeling however, you know
the code better, so I leave the decision to you.

http://codereview.chromium.org/8438020/diff/13002/chrome/browser/printing/clo...
chrome/browser/printing/cloud_print/cloud_print_proxy_service.cc:167: if
(profile_->GetPrefs()->HasPrefPath(prefs::kCloudPrintEmail)) {
You might as well avoid the HasPrefPath call and just ask for GetString() (if
there's no pref value set, you'll get the default, which appears to be the empty
string).

http://codereview.chromium.org/8438020/diff/13002/chrome/browser/printing/clo...
File chrome/browser/printing/cloud_print/cloud_print_proxy_service_unittest.cc
(right):

http://codereview.chromium.org/8438020/diff/13002/chrome/browser/printing/clo...
chrome/browser/printing/cloud_print/cloud_print_proxy_service_unittest.cc:20:
using content::BrowserThread;
you could kill this declaration and just spell out the full namespace on the
sole user below.

http://codereview.chromium.org/8438020/diff/13002/chrome/browser/printing/clo...
chrome/browser/printing/cloud_print/cloud_print_proxy_service_unittest.cc:25:
using ::testing::Mock;
needed?

http://codereview.chromium.org/8438020/diff/13002/chrome/browser/printing/clo...
chrome/browser/printing/cloud_print/cloud_print_proxy_service_unittest.cc:28:
using ::testing::StrictMock;
not used.

http://codereview.chromium.org/8438020/diff/13002/chrome/browser/printing/clo...
chrome/browser/printing/cloud_print/cloud_print_proxy_service_unittest.cc:141:
process_control_ = new MockServiceProcessControl();
how about just making this a regular class member variable (non-pointer)?

http://codereview.chromium.org/8438020/diff/13002/chrome/browser/printing/clo...
chrome/browser/printing/cloud_print/cloud_print_proxy_service_unittest.cc:185:
scoped_ptr<TestCloudPrintProxyService> service(
Any reason to wrap this in a scoped_ptr? Regular non-pointer variable should do.

http://codereview.chromium.org/8438020/diff/13002/chrome/browser/printing/clo...
chrome/browser/printing/cloud_print/cloud_print_proxy_service_unittest.cc:202:
scoped_ptr<TestCloudPrintProxyService> service(
ditto (and also below).

http://codereview.chromium.org/8438020/diff/13002/chrome/browser/printing/clo...
chrome/browser/printing/cloud_print/cloud_print_proxy_service_unittest.cc:273:
Value::CreateBooleanValue(false));
validate here that it's still disabled?

http://codereview.chromium.org/8438020/diff/13002/chrome/browser/printing/clo...
chrome/browser/printing/cloud_print/cloud_print_proxy_service_unittest.cc:340:
EXPECT_EQ(std::string(), prefs->GetString(prefs::kCloudPrintEmail));
I guess an additional test that first sets the policy, then starts and verifies
that the service is disabled, followed by removing the policy and verifying that
the service starts would be also nice to have.

[Scott Byer, 2011-11-15 01:42:23]

http://codereview.chromium.org/8438020/diff/13002/chrome/browser/printing/clo...
File chrome/browser/printing/cloud_print/cloud_print_proxy_service.cc (right):

http://codereview.chromium.org/8438020/diff/13002/chrome/browser/printing/clo...
chrome/browser/printing/cloud_print/cloud_print_proxy_service.cc:91:
DCHECK(profile_->GetPrefs()->GetBoolean(prefs::kCloudPrintProxyEnabled));
On 2011/11/14 08:33:06, Mattias Nissler wrote:
> nit: Since EnableForUser() is in the public interface, I'd upgrade this check
to
> not post the task if it's disabled. That's just my gut feeling however, you
know
> the code better, so I leave the decision to you.

Done, and in EnableForUserWithRobot as well.

http://codereview.chromium.org/8438020/diff/13002/chrome/browser/printing/clo...
chrome/browser/printing/cloud_print/cloud_print_proxy_service.cc:167: if
(profile_->GetPrefs()->HasPrefPath(prefs::kCloudPrintEmail)) {
On 2011/11/14 08:33:06, Mattias Nissler wrote:
> You might as well avoid the HasPrefPath call and just ask for GetString() (if
> there's no pref value set, you'll get the default, which appears to be the
empty
> string).

Makes sense. Done.

http://codereview.chromium.org/8438020/diff/13002/chrome/browser/printing/clo...
File chrome/browser/printing/cloud_print/cloud_print_proxy_service_unittest.cc
(right):

http://codereview.chromium.org/8438020/diff/13002/chrome/browser/printing/clo...
chrome/browser/printing/cloud_print/cloud_print_proxy_service_unittest.cc:20:
using content::BrowserThread;
On 2011/11/14 08:33:06, Mattias Nissler wrote:
> you could kill this declaration and just spell out the full namespace on the
> sole user below.

Done.

http://codereview.chromium.org/8438020/diff/13002/chrome/browser/printing/clo...
chrome/browser/printing/cloud_print/cloud_print_proxy_service_unittest.cc:25:
using ::testing::Mock;
On 2011/11/14 08:33:06, Mattias Nissler wrote:
> needed?

Nope. Done.

http://codereview.chromium.org/8438020/diff/13002/chrome/browser/printing/clo...
chrome/browser/printing/cloud_print/cloud_print_proxy_service_unittest.cc:28:
using ::testing::StrictMock;
On 2011/11/14 08:33:06, Mattias Nissler wrote:
> not used.

Done.

http://codereview.chromium.org/8438020/diff/13002/chrome/browser/printing/clo...
chrome/browser/printing/cloud_print/cloud_print_proxy_service_unittest.cc:141:
process_control_ = new MockServiceProcessControl();
On 2011/11/14 08:33:06, Mattias Nissler wrote:
> how about just making this a regular class member variable (non-pointer)?

Done.

http://codereview.chromium.org/8438020/diff/13002/chrome/browser/printing/clo...
chrome/browser/printing/cloud_print/cloud_print_proxy_service_unittest.cc:185:
scoped_ptr<TestCloudPrintProxyService> service(
On 2011/11/14 08:33:06, Mattias Nissler wrote:
> Any reason to wrap this in a scoped_ptr? Regular non-pointer variable should
do.

Done.

http://codereview.chromium.org/8438020/diff/13002/chrome/browser/printing/clo...
chrome/browser/printing/cloud_print/cloud_print_proxy_service_unittest.cc:202:
scoped_ptr<TestCloudPrintProxyService> service(
On 2011/11/14 08:33:06, Mattias Nissler wrote:
> ditto (and also below).

Done.

http://codereview.chromium.org/8438020/diff/13002/chrome/browser/printing/clo...
chrome/browser/printing/cloud_print/cloud_print_proxy_service_unittest.cc:273:
Value::CreateBooleanValue(false));
On 2011/11/14 08:33:06, Mattias Nissler wrote:
> validate here that it's still disabled?

Done.

http://codereview.chromium.org/8438020/diff/13002/chrome/browser/printing/clo...
chrome/browser/printing/cloud_print/cloud_print_proxy_service_unittest.cc:340:
EXPECT_EQ(std::string(), prefs->GetString(prefs::kCloudPrintEmail));
On 2011/11/14 08:33:06, Mattias Nissler wrote:
> I guess an additional test that first sets the policy, then starts and
verifies
> that the service is disabled, followed by removing the policy and verifying
that
> the service starts would be also nice to have.

Thank you! At some point in the middle of the day I had thought about writing
it, but got distracted before I could make a note. Added, plus another.

[Mattias Nissler (ping if slow), 2011-11-15 09:06:05]

LGTM. Thanks for bearing with me :)

[Scott Byer, 2011-11-15 16:49:37]

Peter, do you want to take another look since things have changed a bit (read:
completely) since your OK?

On 2011/11/02 22:09:59, Peter Kasting wrote:
> LGTM
> 
>
http://codereview.chromium.org/8438020/diff/5001/chrome/browser/browser_proce...
> File chrome/browser/browser_process_impl.cc (right):
> 
>
http://codereview.chromium.org/8438020/diff/5001/chrome/browser/browser_proce...
> chrome/browser/browser_process_impl.cc:699: } else if (type ==
> chrome::NOTIFICATION_PROFILE_CREATED) {
> Nit: I would either do:
> 
> if (type == chrome::NOTIFICATION_PREF_CHANGED) {
>   ...
>   return;
> }
> DCHECK_EQ(chrome::NOTIFICATION_PROFILE_CREATED);
> ...
> 
> or else
> 
> switch (type) {
>   case chrome::NOTIFICATION_PREF_CHANGED:
>     ...;
>     break;
> 
>   case chrome::NOTIFICATION_PROFILE_CREATED:
>     ...;
>     break;
> 
>   default:
>     NOTREACHED();
> }
> 
>
http://codereview.chromium.org/8438020/diff/5001/chrome/browser/browser_proce...
> chrome/browser/browser_process_impl.cc:702: if (profile)
> Can this ever be NULL?  If not then don't add a conditional here.
> 
>
http://codereview.chromium.org/8438020/diff/5001/chrome/browser/browser_proce...
> chrome/browser/browser_process_impl.cc:989: this,
> chrome::NOTIFICATION_PROFILE_CREATED,
> Nit: These args can go on the prior line
> 
>
http://codereview.chromium.org/8438020/diff/5001/chrome/browser/browser_proce...
> chrome/browser/browser_process_impl.cc:1095: ProfileManager* pm =
> profile_manager();
> Nit: Could just roll this into the next line
> 
>
http://codereview.chromium.org/8438020/diff/5001/chrome/browser/browser_proce...
> chrome/browser/browser_process_impl.cc:1097: for (size_t i = 0; i <
> profiles.size(); ++i)
> Nit: I suggest using an iterator (rest of this file seems to generally prefer
> iterators to indexes where possible).  Feel free to add a typedef for
> vector<Profile*> to profile_manager.h if it makes life easier.

[commit-bot: I haz the power, 2011-11-16 17:36:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/scottbyer@chromium.org/8438020/16002

[commit-bot: I haz the power, 2011-11-16 18:40:59]

Change committed as 110332

[Peter Kasting, 2011-11-21 20:51:32]

Much-belated LGTM, just a few tiny nits that you can write a followup for if you
want.

http://codereview.chromium.org/8438020/diff/16002/chrome/browser/printing/clo...
File chrome/browser/printing/cloud_print/cloud_print_proxy_service.cc (right):

http://codereview.chromium.org/8438020/diff/16002/chrome/browser/printing/clo...
chrome/browser/printing/cloud_print/cloud_print_proxy_service.cc:168:
std::string email;
Nit: Combine this line with the next

http://codereview.chromium.org/8438020/diff/16002/chrome/browser/printing/clo...
chrome/browser/printing/cloud_print/cloud_print_proxy_service.cc:186: if (type
== chrome::NOTIFICATION_PREF_CHANGED) {
Nit: Convert this conditional to a simple:

  DCHECK_EQ(chrome::NOTIFICATION_PREF_CHANGED, type);

http://codereview.chromium.org/8438020/diff/16002/chrome/browser/service/serv...
File chrome/browser/service/service_process_control.cc (right):

http://codereview.chromium.org/8438020/diff/16002/chrome/browser/service/serv...
chrome/browser/service/service_process_control.cc:82: return channel_.get() !=
NULL;
Nit: I think this still works without ".get()"

http://codereview.chromium.org/8438020/diff/16002/chrome/browser/service/serv...
File chrome/browser/service/service_process_control.h (right):

http://codereview.chromium.org/8438020/diff/16002/chrome/browser/service/serv...
chrome/browser/service/service_process_control.h:54: virtual bool is_connected()
const;
Nit: virtual functions must not be named in unix_hacker() style.