Integrate invalidation API into ChromeToMobileService.

Integrate invalidation API into ChromeToMobileService.

Update chrome/browser/DEPS with new dependencies:
(google/cacheinvalidation and sync/notifier)

Observe Sync Notifier invalidation notifications:
(depend on this service for mobile list updates)
(refresh the device list on cloud print invalidation)

Remove RequestMobileListUpdate, timestamp, & account info:
(no longer needed with invalidation integration)
(just set command state and icon visibility w/HasMobiles)

Lazily init the access token, queue search/send operations:
(only get an access token as needed, add |task_queue_|)

Allow concurrent cloud print device search requests:
(handle user-triggered updates while fetching the list)

Misc cleanup (CloudPrintUrl handling, tests, etc.)

TODO(followup): Additional logging, tests, invalidation ack.

BUG=102709, 137086
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=152609

[msw, 2012-08-08 19:54:49]

Heads up on work in progress :)

[akalin, 2012-08-08 23:06:27]

few comments + questions

http://codereview.chromium.org/10834203/diff/2001/chrome/browser/chrome_to_mo...
File chrome/browser/chrome_to_mobile_service.cc (right):

http://codereview.chromium.org/10834203/diff/2001/chrome/browser/chrome_to_mo...
chrome/browser/chrome_to_mobile_service.cc:69: const char kMobileListObjectId[]
= "UCMMLST";
i think object id names have to be strings

http://codereview.chromium.org/10834203/diff/2001/chrome/browser/chrome_to_mo...
chrome/browser/chrome_to_mobile_service.cc:192:
prefs->RegisterStringPref(prefs::kChromeToMobileTimestamp, "",
just to be clear, this is different from the invalidation timestamp/version?

http://codereview.chromium.org/10834203/diff/2001/chrome/browser/chrome_to_mo...
chrome/browser/chrome_to_mobile_service.cc:219: // TODO(msw): Check if
sync_invalidation_enabled_ directly here?
Hmm, I guess you need some way to check if notifications are enabled.

http://codereview.chromium.org/10834203/diff/2001/chrome/browser/chrome_to_mo...
File chrome/browser/chrome_to_mobile_service.h (right):

http://codereview.chromium.org/10834203/diff/2001/chrome/browser/chrome_to_mo...
chrome/browser/chrome_to_mobile_service.h:26: #include
"sync/notifier/chrome_invalidation_client.h"
chrome_invalidation_client shouldn't be included.

[dcheng, 2012-08-08 23:11:10]

http://codereview.chromium.org/10834203/diff/2001/chrome/browser/chrome_to_mo...
File chrome/browser/chrome_to_mobile_service.cc (right):

http://codereview.chromium.org/10834203/diff/2001/chrome/browser/chrome_to_mo...
chrome/browser/chrome_to_mobile_service.cc:205: if (profile_) {
Why do you need to check profile_ here and not other places (line 244). Maybe
just DCHECK(profile) and remove the checks. Same comment on other places that
use profile_.

[msw, 2012-08-10 22:05:05]

Comments addressed and I made some additional progress, but I still have
outstanding TODOs, including testing that this actually works :)
Feel free to take a look or wait until i'm further along; thanks!

http://codereview.chromium.org/10834203/diff/2001/chrome/browser/chrome_to_mo...
File chrome/browser/chrome_to_mobile_service.cc (right):

http://codereview.chromium.org/10834203/diff/2001/chrome/browser/chrome_to_mo...
chrome/browser/chrome_to_mobile_service.cc:69: const char kMobileListObjectId[]
= "UCMMLST";
On 2012/08/08 23:06:27, akalin wrote:
> i think object id names have to be strings

I've voiced my dissent, but will go along :)
"It won't be my problem down the road..." - msw

http://codereview.chromium.org/10834203/diff/2001/chrome/browser/chrome_to_mo...
chrome/browser/chrome_to_mobile_service.cc:192:
prefs->RegisterStringPref(prefs::kChromeToMobileTimestamp, "",
On 2012/08/08 23:06:27, akalin wrote:
> just to be clear, this is different from the invalidation timestamp/version?

Removed. I was changing it to match the last received invalidation
timestamp/version to ensure that we'd only update the device list when truly
needed, but Tommy explained offline that this shouldn't be necessary.

As I understand, OnIncomingNotification should correctly trigger updates from
cloud print without Chrome To Mobile needing to store or check a
timestamp/version. Please let me know if I'm wrong!

http://codereview.chromium.org/10834203/diff/2001/chrome/browser/chrome_to_mo...
chrome/browser/chrome_to_mobile_service.cc:205: if (profile_) {
On 2012/08/08 23:11:10, dcheng wrote:
> Why do you need to check profile_ here and not other places (line 244). Maybe
> just DCHECK(profile) and remove the checks. Same comment on other places that
> use profile_.

It's for the GMock unit tests, which provide a NULL profile to this ctor and
mock out other profile-dependent locations. This isn't ideal, I clarified the
comment as a TODO pointing to http://crbug.com/122183 (for now, will follow up).

http://codereview.chromium.org/10834203/diff/2001/chrome/browser/chrome_to_mo...
chrome/browser/chrome_to_mobile_service.cc:219: // TODO(msw): Check if
sync_invalidation_enabled_ directly here?
On 2012/08/08 23:06:27, akalin wrote:
> Hmm, I guess you need some way to check if notifications are enabled.

How about profile_sync_service->IsSyncEnabled()? Not sure if you meant I should
use ShouldPushChanges() for this in http://crbug.com/137086 ?

http://codereview.chromium.org/10834203/diff/2001/chrome/browser/chrome_to_mo...
File chrome/browser/chrome_to_mobile_service.h (right):

http://codereview.chromium.org/10834203/diff/2001/chrome/browser/chrome_to_mo...
chrome/browser/chrome_to_mobile_service.h:26: #include
"sync/notifier/chrome_invalidation_client.h"
On 2012/08/08 23:06:27, akalin wrote:
> chrome_invalidation_client shouldn't be included.

Done.

[akalin, 2012-08-10 23:03:22]

http://codereview.chromium.org/10834203/diff/2001/chrome/browser/chrome_to_mo...
File chrome/browser/chrome_to_mobile_service.cc (right):

http://codereview.chromium.org/10834203/diff/2001/chrome/browser/chrome_to_mo...
chrome/browser/chrome_to_mobile_service.cc:69: const char kMobileListObjectId[]
= "UCMMLST";
On 2012/08/10 22:05:05, msw wrote:
> On 2012/08/08 23:06:27, akalin wrote:
> > i think object id names have to be strings
> 
> I've voiced my dissent, but will go along :)
> "It won't be my problem down the road..." - msw

I understand you don't like strings, but I don't know of any alternatives.  The
object ID names are part of the ticl protocol, right?

[nyquist, 2012-08-10 23:46:39]

http://codereview.chromium.org/10834203/diff/2001/chrome/browser/chrome_to_mo...
File chrome/browser/chrome_to_mobile_service.cc (right):

http://codereview.chromium.org/10834203/diff/2001/chrome/browser/chrome_to_mo...
chrome/browser/chrome_to_mobile_service.cc:69: const char kMobileListObjectId[]
= "UCMMLST";
On 2012/08/10 23:03:22, akalin wrote:
> On 2012/08/10 22:05:05, msw wrote:
> > On 2012/08/08 23:06:27, akalin wrote:
> > > i think object id names have to be strings
> > 
> > I've voiced my dissent, but will go along :)
> > "It won't be my problem down the road..." - msw
> 
> I understand you don't like strings, but I don't know of any alternatives. 
The
> object ID names are part of the ticl protocol, right?

The object ID name must match whatever the source sends, in this case Cloud
Print. There is server-side validation for this, and it is alphanumeric-based.

We should probably have some way of making sure we don't reuse object ID name
space, such as here, "CM" which is the chrome component project ID.

[akalin, 2012-08-13 22:23:58]

few more comments

http://codereview.chromium.org/10834203/diff/2003/chrome/browser/chrome_to_mo...
File chrome/browser/chrome_to_mobile_service.cc (right):

http://codereview.chromium.org/10834203/diff/2003/chrome/browser/chrome_to_mo...
chrome/browser/chrome_to_mobile_service.cc:196:
profile_sync_service->UpdateRegisteredInvalidationIds(this, ids);
what's the code path where chrome to phone is disabled?  is there one, or is it
completely tied to sync?  if it's the latter, then you may have to listen to
sync notifications and unregister if you detect that sync was disabled

http://codereview.chromium.org/10834203/diff/2003/chrome/browser/chrome_to_mo...
chrome/browser/chrome_to_mobile_service.cc:197: // TODO(msw): Use IsSyncEnabled?
Is OnNotificationsEnabled called now?
don't use IsSyncEnabled.  I plan to have OnNotifications{Enabled,Disabled}
called when you register an invalidation handler (
https://code.google.com/p/chromium/issues/detail?id=142475 )

Maybe for now assume that it's enabled until told otherwise and add a TODO to
switch to disabled?  Or just wait for my fix to land

http://codereview.chromium.org/10834203/diff/2003/chrome/browser/chrome_to_mo...
chrome/browser/chrome_to_mobile_service.cc:206: if (profile_)
i think you can omit braces only if the 'then' statement fits on one line

http://codereview.chromium.org/10834203/diff/2003/chrome/browser/chrome_to_mo...
chrome/browser/chrome_to_mobile_service.cc:368:
DCHECK(sync_invalidation_enabled_);
i don't know if this should be a DCHECK.  The API doesn't guarantee that
notifications come only when they're enabled (due to caching, etc. you may get
some notifications before it goes to a fully enabled state, etc.)

http://codereview.chromium.org/10834203/diff/2003/chrome/browser/chrome_to_mo...
File chrome/browser/chrome_to_mobile_service.h (right):

http://codereview.chromium.org/10834203/diff/2003/chrome/browser/chrome_to_mo...
chrome/browser/chrome_to_mobile_service.h:26: #include
"sync/notifier/invalidation_util.h"
i think IWYU has the exception that you don't have to include files for types
that are part of overriding an interface (in this case,
NotificationsDisabledReason, ObjectIdPayloadMap)

You probably use them in the .cc file, though.

[msw, 2012-08-16 02:41:51]

Comments addressed and more. Still a work in progress, but feel free to take a
look. I'll split out some tangential parts of this change while Tommy and folks
debug the cloud print and tango side.

https://chromiumcodereview.appspot.com/10834203/diff/2003/chrome/browser/chro...
File chrome/browser/chrome_to_mobile_service.cc (right):

https://chromiumcodereview.appspot.com/10834203/diff/2003/chrome/browser/chro...
chrome/browser/chrome_to_mobile_service.cc:196:
profile_sync_service->UpdateRegisteredInvalidationIds(this, ids);
On 2012/08/13 22:23:58, akalin wrote:
> what's the code path where chrome to phone is disabled?  is there one, or is
it
> completely tied to sync?  if it's the latter, then you may have to listen to
> sync notifications and unregister if you detect that sync was disabled

It's tied to sync invalidation (presuming commandline flags allow
ChromeToMobileService instantiation). Do you mean to unregister the handler
and/or ids if OnNotificationsDisabled is called? That seems odd.

https://chromiumcodereview.appspot.com/10834203/diff/2003/chrome/browser/chro...
chrome/browser/chrome_to_mobile_service.cc:197: // TODO(msw): Use IsSyncEnabled?
Is OnNotificationsEnabled called now?
On 2012/08/13 22:23:58, akalin wrote:
> don't use IsSyncEnabled.  I plan to have OnNotifications{Enabled,Disabled}
> called when you register an invalidation handler (
> https://code.google.com/p/chromium/issues/detail?id=142475 )
> 
> Maybe for now assume that it's enabled until told otherwise and add a TODO to
> switch to disabled?  Or just wait for my fix to land

Done. OnNotificationsEnabled seems to be called all the time already.

https://chromiumcodereview.appspot.com/10834203/diff/2003/chrome/browser/chro...
chrome/browser/chrome_to_mobile_service.cc:206: if (profile_)
On 2012/08/13 22:23:58, akalin wrote:
> i think you can omit braces only if the 'then' statement fits on one line

Done.

https://chromiumcodereview.appspot.com/10834203/diff/2003/chrome/browser/chro...
chrome/browser/chrome_to_mobile_service.cc:368:
DCHECK(sync_invalidation_enabled_);
On 2012/08/13 22:23:58, akalin wrote:
> i don't know if this should be a DCHECK.  The API doesn't guarantee that
> notifications come only when they're enabled (due to caching, etc. you may get
> some notifications before it goes to a fully enabled state, etc.)

Odd; then I'll just use it to gate the feature w/[Has|Get]Mobiles().

https://chromiumcodereview.appspot.com/10834203/diff/2003/chrome/browser/chro...
File chrome/browser/chrome_to_mobile_service.h (right):

https://chromiumcodereview.appspot.com/10834203/diff/2003/chrome/browser/chro...
chrome/browser/chrome_to_mobile_service.h:26: #include
"sync/notifier/invalidation_util.h"
On 2012/08/13 22:23:58, akalin wrote:
> i think IWYU has the exception that you don't have to include files for types
> that are part of overriding an interface (in this case,
> NotificationsDisabledReason, ObjectIdPayloadMap)
> 
> You probably use them in the .cc file, though.

Done.

[akalin, 2012-08-17 22:09:26]

On 2012/08/16 02:41:51, msw wrote:
> Comments addressed and more. Still a work in progress, but feel free to take a
> look. I'll split out some tangential parts of this change while Tommy and
folks
> debug the cloud print and tango side.
> 
>
https://chromiumcodereview.appspot.com/10834203/diff/2003/chrome/browser/chro...
> File chrome/browser/chrome_to_mobile_service.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10834203/diff/2003/chrome/browser/chro...
> chrome/browser/chrome_to_mobile_service.cc:196:
> profile_sync_service->UpdateRegisteredInvalidationIds(this, ids);
> On 2012/08/13 22:23:58, akalin wrote:
> > what's the code path where chrome to phone is disabled?  is there one, or is
> it
> > completely tied to sync?  if it's the latter, then you may have to listen to
> > sync notifications and unregister if you detect that sync was disabled
> 
> It's tied to sync invalidation (presuming commandline flags allow
> ChromeToMobileService instantiation). Do you mean to unregister the handler
> and/or ids if OnNotificationsDisabled is called? That seems odd.

Not quite.  OnNotificationsDisabled is called when the XMPP connection is
interrupted, etc.  I'm talking about when someone disables sync -- presumably
you guys want to know when that happens, right?

But ignore my comment about unregistering when sync is disabled -- it should
still be safe to leave it registered; it just won't get any new notifications.

[akalin, 2012-08-17 22:09:32]

one more

http://codereview.chromium.org/10834203/diff/5012/chrome/browser/chrome_to_mo...
File chrome/browser/chrome_to_mobile_service.cc (right):

http://codereview.chromium.org/10834203/diff/5012/chrome/browser/chrome_to_mo...
chrome/browser/chrome_to_mobile_service.cc:467:
profile_sync_service->RegisterInvalidationHandler(this);
seems iffy to have this here instead of in the constructor directly.  This
function is only called once, but if it's called from somewhere else in the
future, it'll fail because it'll try to register an invalidation handler twice.

[msw, 2012-08-18 00:09:56]

Done. Please take a look; this is ready for full review.
I may add more tests and logging, or do that in a followup.

The latest patch set ties Chrome To Mobile to the ProfileSyncService state,
moves invalidation registration back into ctor, updates tests, etc.

https://chromiumcodereview.appspot.com/10834203/diff/5012/chrome/browser/chro...
File chrome/browser/chrome_to_mobile_service.cc (right):

https://chromiumcodereview.appspot.com/10834203/diff/5012/chrome/browser/chro...
chrome/browser/chrome_to_mobile_service.cc:467:
profile_sync_service->RegisterInvalidationHandler(this);
On 2012/08/17 22:09:33, akalin wrote:
> seems iffy to have this here instead of in the constructor directly.  This
> function is only called once, but if it's called from somewhere else in the
> future, it'll fail because it'll try to register an invalidation handler
twice.

Done.

[akalin, 2012-08-20 20:50:17]

sorry for the late reply.  few more nits.

http://codereview.chromium.org/10834203/diff/12002/chrome/browser/chrome_to_m...
File chrome/browser/chrome_to_mobile_service.cc (right):

http://codereview.chromium.org/10834203/diff/12002/chrome/browser/chrome_to_m...
chrome/browser/chrome_to_mobile_service.cc:195:
ProfileSyncServiceFactory::GetForProfile(profile_);
may want to handle a NULL pss?  in case --disable-sync is used

http://codereview.chromium.org/10834203/diff/12002/chrome/browser/chrome_to_m...
chrome/browser/chrome_to_mobile_service.cc:196: profile_sync_service_enabled_ =
profile_sync_service->ShouldPushChanges();
may want to add a comment re. the unintuitive function name. maybe say that it's
recommended by the sync team?

http://codereview.chromium.org/10834203/diff/12002/chrome/browser/chrome_to_m...
chrome/browser/chrome_to_mobile_service.cc:199: syncer::ObjectIdSet ids;
add the TODO here to initialize sync_invalidation_enabled_ properly when a
getter is exposed.

http://codereview.chromium.org/10834203/diff/12002/chrome/browser/chrome_to_m...
chrome/browser/chrome_to_mobile_service.cc:215:
profile_sync_service->RemoveObserver(this);
here too

http://codereview.chromium.org/10834203/diff/12002/chrome/browser/chrome_to_m...
File chrome/browser/chrome_to_mobile_service.h (right):

http://codereview.chromium.org/10834203/diff/12002/chrome/browser/chrome_to_m...
chrome/browser/chrome_to_mobile_service.h:157: virtual void OnStateChanged()
OVERRIDE;
include compiler_specific.h for OVERRIDE

[msw, 2012-08-20 21:56:15]

Comments addressed; please take another look, thanks!

https://chromiumcodereview.appspot.com/10834203/diff/12002/chrome/browser/chr...
File chrome/browser/chrome_to_mobile_service.cc (right):

https://chromiumcodereview.appspot.com/10834203/diff/12002/chrome/browser/chr...
chrome/browser/chrome_to_mobile_service.cc:195:
ProfileSyncServiceFactory::GetForProfile(profile_);
On 2012/08/20 20:50:17, akalin wrote:
> may want to handle a NULL pss?  in case --disable-sync is used

Done.

https://chromiumcodereview.appspot.com/10834203/diff/12002/chrome/browser/chr...
chrome/browser/chrome_to_mobile_service.cc:196: profile_sync_service_enabled_ =
profile_sync_service->ShouldPushChanges();
On 2012/08/20 20:50:17, akalin wrote:
> may want to add a comment re. the unintuitive function name. maybe say that
it's
> recommended by the sync team?

Done.

https://chromiumcodereview.appspot.com/10834203/diff/12002/chrome/browser/chr...
chrome/browser/chrome_to_mobile_service.cc:199: syncer::ObjectIdSet ids;
On 2012/08/20 20:50:17, akalin wrote:
> add the TODO here to initialize sync_invalidation_enabled_ properly when a
> getter is exposed.

Done.

https://chromiumcodereview.appspot.com/10834203/diff/12002/chrome/browser/chr...
chrome/browser/chrome_to_mobile_service.cc:215:
profile_sync_service->RemoveObserver(this);
On 2012/08/20 20:50:17, akalin wrote:
> here too

Done.

https://chromiumcodereview.appspot.com/10834203/diff/12002/chrome/browser/chr...
File chrome/browser/chrome_to_mobile_service.h (right):

https://chromiumcodereview.appspot.com/10834203/diff/12002/chrome/browser/chr...
chrome/browser/chrome_to_mobile_service.h:157: virtual void OnStateChanged()
OVERRIDE;
On 2012/08/20 20:50:17, akalin wrote:
> include compiler_specific.h for OVERRIDE

Done.

[akalin, 2012-08-20 22:29:30]

sync stuff lgtm!

http://codereview.chromium.org/10834203/diff/1007/chrome/browser/chrome_to_mo...
File chrome/browser/chrome_to_mobile_service.cc (right):

http://codereview.chromium.org/10834203/diff/1007/chrome/browser/chrome_to_mo...
chrome/browser/chrome_to_mobile_service.cc:372:
ProfileSyncServiceFactory::GetForProfile(profile_)->ShouldPushChanges();
maybe dcheck return value of GetForProfile()?  I guess if we're in here, the PSS
must exist

[msw, 2012-08-20 22:43:57]

+sky for c/b/*service*, DEPS, and ui/views.
+sail for c/b/ui/cocoa.
+erg for c/b/ui/gtk.

Please take a look; thanks!

https://chromiumcodereview.appspot.com/10834203/diff/1007/chrome/browser/chro...
File chrome/browser/chrome_to_mobile_service.cc (right):

https://chromiumcodereview.appspot.com/10834203/diff/1007/chrome/browser/chro...
chrome/browser/chrome_to_mobile_service.cc:372:
ProfileSyncServiceFactory::GetForProfile(profile_)->ShouldPushChanges();
On 2012/08/20 22:29:30, akalin wrote:
> maybe dcheck return value of GetForProfile()?  I guess if we're in here, the
PSS
> must exist

Yup, this ProfileSyncServiceObserver function is only reached iff |profile_| and
its PSS were valid in the ctor. Do we support PSS becoming NULL (but still
sending state changes?) even if the factory
DependsOn(ProfileSyncServiceFactory...)? Otherwise, there's no reason to DCHECK,
since it'll crash anyway in such a fatal scenario.

[Elliot Glaysher, 2012-08-20 22:46:52]

gtk lgtm

[sail, 2012-08-20 22:48:36]

cocoa/* LGTM

[sky, 2012-08-20 23:14:03]

LGTM

https://chromiumcodereview.appspot.com/10834203/diff/1007/chrome/browser/chro...
File chrome/browser/chrome_to_mobile_service.h (right):

https://chromiumcodereview.appspot.com/10834203/diff/1007/chrome/browser/chro...
chrome/browser/chrome_to_mobile_service.h:10: #include <queue>
nit: sort

[nyquist, 2012-08-20 23:19:19]

https://chromiumcodereview.appspot.com/10834203/diff/1007/chrome/browser/chro...
File chrome/browser/chrome_to_mobile_service.cc (right):

https://chromiumcodereview.appspot.com/10834203/diff/1007/chrome/browser/chro...
chrome/browser/chrome_to_mobile_service.cc:531: if (search_request_.get())
If a user does cleanup in her printer list by deleting/renaming, etc., we might
miss the last update if we already have one in process (which might or might not
get the latest data).
Could we instead queue another search?

[nyquist, 2012-08-21 00:00:01]

https://chromiumcodereview.appspot.com/10834203/diff/1007/chrome/browser/chro...
File chrome/browser/chrome_to_mobile_service.cc (left):

https://chromiumcodereview.appspot.com/10834203/diff/1007/chrome/browser/chro...
chrome/browser/chrome_to_mobile_service.cc:183:
prefs->RegisterInt64Pref(prefs::kChromeToMobileTimestamp, 0,
Remove from chrome/common/pref_names.[h|cc]

[tim (not reviewing), 2012-08-21 00:33:47]

https://chromiumcodereview.appspot.com/10834203/diff/1007/chrome/browser/chro...
File chrome/browser/chrome_to_mobile_service.cc (right):

https://chromiumcodereview.appspot.com/10834203/diff/1007/chrome/browser/chro...
chrome/browser/chrome_to_mobile_service.cc:372:
ProfileSyncServiceFactory::GetForProfile(profile_)->ShouldPushChanges();
Out of curiosity, what exactly do you need to check here?  ShouldPushChanges is
likely sufficient, but if it's necessary I'd like to know why since we have a
few alternatives now.  Do you care if sync is (e.g.) blocked waiting for the
user to type in a password to sync encrypted types, or do you really just care
if the user has signed in to chrome (and hasn't explicitly chosen to sign out
yet)? The SigninManager or IsSyncEnabledAndLogged in could tell you that.

Or maybe you do need ShouldPushChanges?

[nyquist, 2012-08-21 00:38:58]

https://chromiumcodereview.appspot.com/10834203/diff/1007/chrome/browser/chro...
File chrome/browser/chrome_to_mobile_service.cc (right):

https://chromiumcodereview.appspot.com/10834203/diff/1007/chrome/browser/chro...
chrome/browser/chrome_to_mobile_service.cc:372:
ProfileSyncServiceFactory::GetForProfile(profile_)->ShouldPushChanges();
On 2012/08/21 00:33:47, timsteele wrote:
> Out of curiosity, what exactly do you need to check here?  ShouldPushChanges
is
> likely sufficient, but if it's necessary I'd like to know why since we have a
> few alternatives now.  Do you care if sync is (e.g.) blocked waiting for the
> user to type in a password to sync encrypted types, or do you really just care
> if the user has signed in to chrome (and hasn't explicitly chosen to sign out
> yet)? The SigninManager or IsSyncEnabledAndLogged in could tell you that.
> 
> Or maybe you do need ShouldPushChanges?

I believe the intent here is what you propose:
SigninManager::IsSyncEnabledAndLoggedIn().

[akalin, 2012-08-21 01:50:58]

http://codereview.chromium.org/10834203/diff/1007/chrome/browser/chrome_to_mo...
File chrome/browser/chrome_to_mobile_service.cc (right):

http://codereview.chromium.org/10834203/diff/1007/chrome/browser/chrome_to_mo...
chrome/browser/chrome_to_mobile_service.cc:372:
ProfileSyncServiceFactory::GetForProfile(profile_)->ShouldPushChanges();
On 2012/08/21 00:38:59, nyquist wrote:
> On 2012/08/21 00:33:47, timsteele wrote:
> > Out of curiosity, what exactly do you need to check here?  ShouldPushChanges
> is
> > likely sufficient, but if it's necessary I'd like to know why since we have
a
> > few alternatives now.  Do you care if sync is (e.g.) blocked waiting for the
> > user to type in a password to sync encrypted types, or do you really just
care
> > if the user has signed in to chrome (and hasn't explicitly chosen to sign
out
> > yet)? The SigninManager or IsSyncEnabledAndLogged in could tell you that.
> > 
> > Or maybe you do need ShouldPushChanges?
> 
> I believe the intent here is what you propose:
> SigninManager::IsSyncEnabledAndLoggedIn().

Okay I think I was confused, but now I'm not.  Basically, it sounds like we want
to show the icon exactly when sync notifications are working, for which "sync is
enabled and user is logged in" is a necessary condition.  That is, if sync is
enabled and user is logged in, sync notifications are (most likely) on.  If sync
is disabled or user is not logged in, sync notifications are definitely off.

Anyway, I think it suffices to just keep track of sync_invalidation_enabled_,
i.e. remove the registering as an observer of ProfileSyncService and the
checking of ShouldPushChanges().

My original question was whether C2P could be disabled independently from sync,
but it sounds like that's not the case.  If it were the case, then you'd need
some code like the following:

void OnC2PDisable() {
  ProfileSyncService* pss = ...;
  pss->UpdateRegisteredInvalidationIds(this, ObjectIdSet());
}

[msw, 2012-08-21 03:37:01]

Comments addressed; Fred, Tim, Tommy, please take another look.

https://chromiumcodereview.appspot.com/10834203/diff/1007/chrome/browser/chro...
File chrome/browser/chrome_to_mobile_service.cc (left):

https://chromiumcodereview.appspot.com/10834203/diff/1007/chrome/browser/chro...
chrome/browser/chrome_to_mobile_service.cc:183:
prefs->RegisterInt64Pref(prefs::kChromeToMobileTimestamp, 0,
On 2012/08/21 00:00:01, nyquist wrote:
> Remove from chrome/common/pref_names.[h|cc]

Done.

https://chromiumcodereview.appspot.com/10834203/diff/1007/chrome/browser/chro...
File chrome/browser/chrome_to_mobile_service.cc (right):

https://chromiumcodereview.appspot.com/10834203/diff/1007/chrome/browser/chro...
chrome/browser/chrome_to_mobile_service.cc:372:
ProfileSyncServiceFactory::GetForProfile(profile_)->ShouldPushChanges();
On 2012/08/21 01:50:58, akalin wrote:
> On 2012/08/21 00:38:59, nyquist wrote:
> > On 2012/08/21 00:33:47, timsteele wrote:
> > > Out of curiosity, what exactly do you need to check here? 
ShouldPushChanges
> > is
> > > likely sufficient, but if it's necessary I'd like to know why since we
have
> a
> > > few alternatives now.  Do you care if sync is (e.g.) blocked waiting for
the
> > > user to type in a password to sync encrypted types, or do you really just
> care
> > > if the user has signed in to chrome (and hasn't explicitly chosen to sign
> out
> > > yet)? The SigninManager or IsSyncEnabledAndLogged in could tell you that.
> > > 
> > > Or maybe you do need ShouldPushChanges?
> > 
> > I believe the intent here is what you propose:
> > SigninManager::IsSyncEnabledAndLoggedIn().
> 
> Okay I think I was confused, but now I'm not.  Basically, it sounds like we
want
> to show the icon exactly when sync notifications are working, for which "sync
is
> enabled and user is logged in" is a necessary condition.  That is, if sync is
> enabled and user is logged in, sync notifications are (most likely) on.  If
sync
> is disabled or user is not logged in, sync notifications are definitely off.
> 
> Anyway, I think it suffices to just keep track of sync_invalidation_enabled_,
> i.e. remove the registering as an observer of ProfileSyncService and the
> checking of ShouldPushChanges().
> 
> My original question was whether C2P could be disabled independently from
sync,
> but it sounds like that's not the case.  If it were the case, then you'd need
> some code like the following:
> 
> void OnC2PDisable() {
>   ProfileSyncService* pss = ...;
>   pss->UpdateRegisteredInvalidationIds(this, ObjectIdSet());
> }

Done. Now it's just tracking OnNotifications[En|Dis]abled.
I updated http://crbug.com/137086 for Fred to check those calls, etc.

https://chromiumcodereview.appspot.com/10834203/diff/1007/chrome/browser/chro...
chrome/browser/chrome_to_mobile_service.cc:531: if (search_request_.get())
On 2012/08/20 23:19:19, nyquist wrote:
> If a user does cleanup in her printer list by deleting/renaming, etc., we
might
> miss the last update if we already have one in process (which might or might
not
> get the latest data).
> Could we instead queue another search?

Done; good point. Some users will definitely manage to update their devices
while we're getting the URLFetcher response with old data :)

https://chromiumcodereview.appspot.com/10834203/diff/1007/chrome/browser/chro...
File chrome/browser/chrome_to_mobile_service.h (right):

https://chromiumcodereview.appspot.com/10834203/diff/1007/chrome/browser/chro...
chrome/browser/chrome_to_mobile_service.h:10: #include <queue>
On 2012/08/20 23:14:03, sky wrote:
> nit: sort

Done.

[akalin, 2012-08-21 07:47:50]

sync lgtm

https://chromiumcodereview.appspot.com/10834203/diff/5032/chrome/browser/chro...
File chrome/browser/chrome_to_mobile_service.cc (right):

https://chromiumcodereview.appspot.com/10834203/diff/5032/chrome/browser/chro...
chrome/browser/chrome_to_mobile_service.cc:210: // Unregister for profile sync
and cloud print device list notifications.
update comment

[commit-bot: I haz the power, 2012-08-21 17:14:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/msw@chromium.org/10834203/2039

[commit-bot: I haz the power, 2012-08-21 19:37:05]

Change committed as 152609

[msw, 2012-08-21 19:46:09]

(just FYI, I did update the comment)

https://chromiumcodereview.appspot.com/10834203/diff/5032/chrome/browser/chro...
File chrome/browser/chrome_to_mobile_service.cc (right):

https://chromiumcodereview.appspot.com/10834203/diff/5032/chrome/browser/chro...
chrome/browser/chrome_to_mobile_service.cc:210: // Unregister for profile sync
and cloud print device list notifications.
On 2012/08/21 07:47:50, akalin wrote:
> update comment

Done.

[msw, 2012-08-21 23:50:20]

FYI: r152609 reverted r152609 for debug test crashes, see continuation:
http://codereview.chromium.org/10869002