Add InvalidationStateTracker::Forget() to erase an entry from storage.

Add InvalidationStateTracker::Forget() to erase an entry from storage.

BUG=none


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

[dcheng, 2012-08-01 23:17:44]

Is there already a bug for this? Otherwise, I'm going to go ahead and create
one.

Also, I'm not able to come up with a good test for ChromeInvalidationClient. The
issue is that any attempt to trigger a call to RegistrationManager will cause
InvalidationClient to complain--to fix this, we'd need to allow
ChromeInvalidationClient to use a fake invalidation client for tests. If you're
OK with it, I can make the appropriate changes to CIC and add a new test.

[akalin, 2012-08-01 23:37:36]

On 2012/08/01 23:17:44, dcheng wrote:
> Is there already a bug for this? Otherwise, I'm going to go ahead and create
> one.
> 
> Also, I'm not able to come up with a good test for ChromeInvalidationClient.
The
> issue is that any attempt to trigger a call to RegistrationManager will cause
> InvalidationClient to complain--to fix this, we'd need to allow
> ChromeInvalidationClient to use a fake invalidation client for tests. If
you're
> OK with it, I can make the appropriate changes to CIC and add a new test.

Hold off on the CIC tests -- I have CL that reworks them and adds a fake
invalidator client.

[akalin, 2012-08-02 07:01:28]

Here's the CL: https://chromiumcodereview.appspot.com/10827133/

[akalin, 2012-08-02 07:04:30]

Also note in CL description that you use Forget() when IDs are unregistered.

http://codereview.chromium.org/10824140/diff/1/sync/notifier/chrome_invalidat...
File sync/notifier/chrome_invalidation_client.cc (right):

http://codereview.chromium.org/10824140/diff/1/sync/notifier/chrome_invalidat...
sync/notifier/chrome_invalidation_client.cc:282:
invalidation_state_tracker_.Call(
you trigger a post on another thread for each ID, each of which rewrites a pref.
 Perhaps Forget() should take a set instead?

[dcheng, 2012-08-02 15:09:53]

On 2012/08/02 07:04:30, akalin wrote:
> Also note in CL description that you use Forget() when IDs are unregistered.
> 
>
http://codereview.chromium.org/10824140/diff/1/sync/notifier/chrome_invalidat...
> File sync/notifier/chrome_invalidation_client.cc (right):
> 
>
http://codereview.chromium.org/10824140/diff/1/sync/notifier/chrome_invalidat...
> sync/notifier/chrome_invalidation_client.cc:282:
> invalidation_state_tracker_.Call(
> you trigger a post on another thread for each ID, each of which rewrites a
pref.
>  Perhaps Forget() should take a set instead?

There's another problem too. Unregistering a handler will cause all of its
entries to be forgotten, which is probably not what we want...

[akalin, 2012-08-02 20:01:39]

On 2012/08/02 15:09:53, dcheng wrote:
> On 2012/08/02 07:04:30, akalin wrote:
> > Also note in CL description that you use Forget() when IDs are unregistered.
> > 
> >
>
http://codereview.chromium.org/10824140/diff/1/sync/notifier/chrome_invalidat...
> > File sync/notifier/chrome_invalidation_client.cc (right):
> > 
> >
>
http://codereview.chromium.org/10824140/diff/1/sync/notifier/chrome_invalidat...
> > sync/notifier/chrome_invalidation_client.cc:282:
> > invalidation_state_tracker_.Call(
> > you trigger a post on another thread for each ID, each of which rewrites a
> pref.
> >  Perhaps Forget() should take a set instead?
> 
> There's another problem too. Unregistering a handler will cause all of its
> entries to be forgotten, which is probably not what we want...

Yeah. :/ There's another problem with our current API -- we essentially
unregister everything on shutdown, which is not what we want (since Tango can
persist registrations).

Will file a bug...

[akalin, 2012-08-13 22:11:19]

I think this is ready to rebase onto trunk

[akalin, 2012-08-20 20:51:11]

On 2012/08/13 22:11:19, akalin wrote:
> I think this is ready to rebase onto trunk

going through my old CLs.  ping!

[dcheng, 2012-08-21 23:46:28]

I've updated this patch now. Please take another look =)

[akalin, 2012-08-22 19:57:30]

yay, tests

http://codereview.chromium.org/10824140/diff/9009/sync/notifier/chrome_invali...
File sync/notifier/chrome_invalidation_client.cc (right):

http://codereview.chromium.org/10824140/diff/9009/sync/notifier/chrome_invali...
sync/notifier/chrome_invalidation_client.cc:294:
invalidation_state_tracker_.Call(
resurrecting my old comment, make Forget take a set<ObjectId> so you only have
to do one Call() here?

http://codereview.chromium.org/10824140/diff/9009/sync/notifier/fake_invalida...
File sync/notifier/fake_invalidation_state_tracker.h (right):

http://codereview.chromium.org/10824140/diff/9009/sync/notifier/fake_invalida...
sync/notifier/fake_invalidation_state_tracker.h:26: int64 GetMaxVersion(const
invalidation::ObjectId& id) const;
can you move this above the overrides?

http://codereview.chromium.org/10824140/diff/9009/sync/notifier/registration_...
File sync/notifier/registration_manager.h (right):

http://codereview.chromium.org/10824140/diff/9009/sync/notifier/registration_...
sync/notifier/registration_manager.h:73: ObjectIdSet UpdateRegisteredIds(const
ObjectIdSet& ids);
what do you think of making GetRegisteredIdsForTest() not be just for test, and
moving the calculation of the types to forget to the caller?  I feel like the
return value of this function isn't very intuitive.

Also, I have another CL that needs GetRegisteredIds(), anyway.

http://codereview.chromium.org/10824140/diff/9009/sync/notifier/registration_...
sync/notifier/registration_manager.h:84: void DisableId(const
invalidation::ObjectId& id);
should we also Forget the ID if we call this?

[dcheng, 2012-08-24 01:35:24]

http://codereview.chromium.org/10824140/diff/9009/sync/notifier/chrome_invali...
File sync/notifier/chrome_invalidation_client.cc (right):

http://codereview.chromium.org/10824140/diff/9009/sync/notifier/chrome_invali...
sync/notifier/chrome_invalidation_client.cc:294:
invalidation_state_tracker_.Call(
On 2012/08/22 19:57:30, akalin wrote:
> resurrecting my old comment, make Forget take a set<ObjectId> so you only have
> to do one Call() here?

Done.

http://codereview.chromium.org/10824140/diff/9009/sync/notifier/fake_invalida...
File sync/notifier/fake_invalidation_state_tracker.h (right):

http://codereview.chromium.org/10824140/diff/9009/sync/notifier/fake_invalida...
sync/notifier/fake_invalidation_state_tracker.h:26: int64 GetMaxVersion(const
invalidation::ObjectId& id) const;
On 2012/08/22 19:57:30, akalin wrote:
> can you move this above the overrides?

Done.

http://codereview.chromium.org/10824140/diff/9009/sync/notifier/registration_...
File sync/notifier/registration_manager.h (right):

http://codereview.chromium.org/10824140/diff/9009/sync/notifier/registration_...
sync/notifier/registration_manager.h:73: ObjectIdSet UpdateRegisteredIds(const
ObjectIdSet& ids);
On 2012/08/22 19:57:30, akalin wrote:
> what do you think of making GetRegisteredIdsForTest() not be just for test,
and
> moving the calculation of the types to forget to the caller?  I feel like the
> return value of this function isn't very intuitive.
> 
> Also, I have another CL that needs GetRegisteredIds(), anyway.

I'm not opposed to exposing it publicly.

However, making the caller makes the flow kind of funky:
Caller gets the registered ids and saves it somewhere.
Callee gets the registered ids, then diffs it against the set passed in, updates
registrations, etc.
Caller then does the exact same diff and gets the exact same result set.

I do agree with comment that it's not intuitive though. I'll think about it more
and see if I can come up with something that is better.

http://codereview.chromium.org/10824140/diff/9009/sync/notifier/registration_...
sync/notifier/registration_manager.h:84: void DisableId(const
invalidation::ObjectId& id);
On 2012/08/22 19:57:30, akalin wrote:
> should we also Forget the ID if we call this?

I think we probably still want to keep it in this case. From the documentation
for InvalidationListener:
"For permanent failures, it must not automatically retry without fixing the
situation (e.g., by presenting a dialog box to the user)"

So it seems like permanent failures can still be addressed and fixed (maybe the
user changed his password or something like that)

[akalin, 2012-08-29 17:33:54]

rebase?

http://codereview.chromium.org/10824140/diff/9009/sync/notifier/registration_...
File sync/notifier/registration_manager.h (right):

http://codereview.chromium.org/10824140/diff/9009/sync/notifier/registration_...
sync/notifier/registration_manager.h:73: ObjectIdSet UpdateRegisteredIds(const
ObjectIdSet& ids);
On 2012/08/24 01:35:25, dcheng wrote:
> On 2012/08/22 19:57:30, akalin wrote:
> > what do you think of making GetRegisteredIdsForTest() not be just for test,
> and
> > moving the calculation of the types to forget to the caller?  I feel like
the
> > return value of this function isn't very intuitive.
> > 
> > Also, I have another CL that needs GetRegisteredIds(), anyway.
> 
> I'm not opposed to exposing it publicly.
> 
> However, making the caller makes the flow kind of funky:
> Caller gets the registered ids and saves it somewhere.
> Callee gets the registered ids, then diffs it against the set passed in,
updates
> registrations, etc.
> Caller then does the exact same diff and gets the exact same result set.
> 
> I do agree with comment that it's not intuitive though. I'll think about it
more
> and see if I can come up with something that is better.

Yeah, you're right about the funky flow.  Actually, since this is just the
RegistrationManager, which is pretty much an internal class, I think keeping the
unregistered set as a return value is the least bad option.

http://codereview.chromium.org/10824140/diff/9009/sync/notifier/registration_...
sync/notifier/registration_manager.h:84: void DisableId(const
invalidation::ObjectId& id);
On 2012/08/24 01:35:25, dcheng wrote:
> On 2012/08/22 19:57:30, akalin wrote:
> > should we also Forget the ID if we call this?
> 
> I think we probably still want to keep it in this case. From the documentation
> for InvalidationListener:
> "For permanent failures, it must not automatically retry without fixing the
> situation (e.g., by presenting a dialog box to the user)"
> 
> So it seems like permanent failures can still be addressed and fixed (maybe
the
> user changed his password or something like that)

Maybe add a comment to that effect when this is called in
chrome_invalidation_client.cc

[dcheng, 2012-08-31 05:56:46]

http://codereview.chromium.org/10824140/diff/9009/sync/notifier/registration_...
File sync/notifier/registration_manager.h (right):

http://codereview.chromium.org/10824140/diff/9009/sync/notifier/registration_...
sync/notifier/registration_manager.h:84: void DisableId(const
invalidation::ObjectId& id);
On 2012/08/29 17:33:54, akalin wrote:
> On 2012/08/24 01:35:25, dcheng wrote:
> > On 2012/08/22 19:57:30, akalin wrote:
> > > should we also Forget the ID if we call this?
> > 
> > I think we probably still want to keep it in this case. From the
documentation
> > for InvalidationListener:
> > "For permanent failures, it must not automatically retry without fixing the
> > situation (e.g., by presenting a dialog box to the user)"
> > 
> > So it seems like permanent failures can still be addressed and fixed (maybe
> the
> > user changed his password or something like that)
> 
> Maybe add a comment to that effect when this is called in
> chrome_invalidation_client.cc

Done.

[akalin, 2012-08-31 19:46:25]

lgtm!

[commit-bot: I haz the power, 2012-08-31 21:11:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dcheng@chromium.org/10824140/23001

[commit-bot: I haz the power, 2012-08-31 21:41:33]

Try job failure for 10824140-23001 (retry) on mac for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac&number...

[commit-bot: I haz the power, 2012-09-04 18:59:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dcheng@chromium.org/10824140/23002

[commit-bot: I haz the power, 2012-09-04 19:14:54]

Try job failure for 10824140-23002 (retry) on linux_clang for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...

[commit-bot: I haz the power, 2012-09-04 19:59:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dcheng@chromium.org/10824140/25017

[commit-bot: I haz the power, 2012-09-04 22:22:19]

Change committed as 154826