Remove WeakHandle from components/invalidation

Remove WeakHandle from components/invalidation

Replaces the use of WeakHandle with combinations of WeakPtr and
SingleThreadTaskRunner.  The WeakHandle encapsulates both of these
concepts, so the replacment is not much different from the original.

The removal makes the code a bit uglier.  However, the WeakHandle is not
widely used outside of sync and has no chance of making it to base/.
Its use in new code is discouraged.  Moving components/invalidation/ to
WeakPtr and SingeThreadTaskRunner will make it more future proof, and
allow us to remove its dependency on libsync.

TBR=dcheng
BUG=394925
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=288523

[rlarocque, 2014-08-07 23:30:26]

Using two variables is less pretty than using just one, but this seems to be the
way of the future.  At least it lets us drop a dependency.

Please review.

[pavely, 2014-08-08 20:08:30]

https://codereview.chromium.org/446223002/diff/60001/components/invalidation/...
File components/invalidation/invalidation.cc (right):

https://codereview.chromium.org/446223002/diff/60001/components/invalidation/...
components/invalidation/invalidation.cc:122: return !!ack_handler_task_runner_;
I think previous code will return false if ack_handler_ gets deleted. The new
code will return true. Does it matter?

https://codereview.chromium.org/446223002/diff/60001/components/invalidation/...
File components/invalidation/invalidation.h (right):

https://codereview.chromium.org/446223002/diff/60001/components/invalidation/...
components/invalidation/invalidation.h:63:
scoped_refptr<base::SequencedTaskRunner> handler_task_runner);
I've seen usage of const scoped_refptr<...>& in the code which saves you one
pair of AddRef/Release. Do you know if there is any guideline about it?

https://codereview.chromium.org/446223002/diff/60001/components/invalidation/...
File components/invalidation/invalidation_notifier.h (right):

https://codereview.chromium.org/446223002/diff/60001/components/invalidation/...
components/invalidation/invalidation_notifier.h:19: #include
"base/memory/scoped_ptr.h"
Do you need to include weak_ptr.h  and ref_counted.h as well?

https://codereview.chromium.org/446223002/diff/60001/components/invalidation/...
File components/invalidation/non_blocking_invalidator.cc (right):

https://codereview.chromium.org/446223002/diff/60001/components/invalidation/...
components/invalidation/non_blocking_invalidator.cc:104: explicit Core(const
base::WeakPtr<NonBlockingInvalidator>& delegate_observer,
With two parameters explicit is not needed anymore.

https://codereview.chromium.org/446223002/diff/60001/components/invalidation/...
File components/invalidation/sync_invalidation_listener_unittest.cc (right):

https://codereview.chromium.org/446223002/diff/60001/components/invalidation/...
components/invalidation/sync_invalidation_listener_unittest.cc:383:
scoped_refptr<base::SingleThreadTaskRunner>(),
Most of places with Post don't check task_runner. Would it be safer to pass
valid MessageLoopProxy here? Empty WeakPtr will be noop.

[rlarocque, 2014-08-08 21:19:35]

Patch updated.  PTAL.

https://codereview.chromium.org/446223002/diff/60001/components/invalidation/...
File components/invalidation/invalidation.cc (right):

https://codereview.chromium.org/446223002/diff/60001/components/invalidation/...
components/invalidation/invalidation.cc:122: return !!ack_handler_task_runner_;
On 2014/08/08 20:08:30, pavely wrote:
> I think previous code will return false if ack_handler_ gets deleted. The new
> code will return true. Does it matter?

This version is actually more correct.  SupportsAcknowledgement() was meant to
reflect how the object was constructed, not whether or not a shutdown is in
progress.

The documentation explains it a bit more.  Clients are allowed to ignore this
flag and call Acknowledge() or Drop() regardless of its value, so it shouldn't
matter too much what value it returns.

https://codereview.chromium.org/446223002/diff/60001/components/invalidation/...
File components/invalidation/invalidation.h (right):

https://codereview.chromium.org/446223002/diff/60001/components/invalidation/...
components/invalidation/invalidation.h:63:
scoped_refptr<base::SequencedTaskRunner> handler_task_runner);
On 2014/08/08 20:08:30, pavely wrote:
> I've seen usage of const scoped_refptr<...>& in the code which saves you one
> pair of AddRef/Release. Do you know if there is any guideline about it?

Some support it as an efficiency improvement.  Personally, I think the
improvements are not really measureable, so I'm indifferent.  In this CL, I've
tried to maintain the status quo.

As far as I know, there's no rule about this set out in the style guidelines.  I
guess we should just aim for consistency.  A quick 'grep' tells me that we're
pretty far from that goal.  :(

https://codereview.chromium.org/446223002/diff/60001/components/invalidation/...
File components/invalidation/invalidation_notifier.h (right):

https://codereview.chromium.org/446223002/diff/60001/components/invalidation/...
components/invalidation/invalidation_notifier.h:19: #include
"base/memory/scoped_ptr.h"
On 2014/08/08 20:08:30, pavely wrote:
> Do you need to include weak_ptr.h  and ref_counted.h as well?

According to IWYU, yes.

https://codereview.chromium.org/446223002/diff/60001/components/invalidation/...
File components/invalidation/non_blocking_invalidator.cc (right):

https://codereview.chromium.org/446223002/diff/60001/components/invalidation/...
components/invalidation/non_blocking_invalidator.cc:104: explicit Core(const
base::WeakPtr<NonBlockingInvalidator>& delegate_observer,
On 2014/08/08 20:08:30, pavely wrote:
> With two parameters explicit is not needed anymore.

Done.

https://codereview.chromium.org/446223002/diff/60001/components/invalidation/...
File components/invalidation/sync_invalidation_listener_unittest.cc (right):

https://codereview.chromium.org/446223002/diff/60001/components/invalidation/...
components/invalidation/sync_invalidation_listener_unittest.cc:383:
scoped_refptr<base::SingleThreadTaskRunner>(),
On 2014/08/08 20:08:30, pavely wrote:
> Most of places with Post don't check task_runner. Would it be safer to pass
> valid MessageLoopProxy here? Empty WeakPtr will be noop.

The task runner will be used by a bunch of Invalidation objects.  They should be
good about checking their task runners.

Actually, this is one case where SupportsAcknowledgement() is important.  I
think the code should be safe as currently written.  Let me know if I've missed
something.

[rlarocque, 2014-08-08 21:20:27]

+dcheng for the new header in a push_messing test.  His name claims he's OOO, so
I'll TBR him for this trivial change.

[pavely, 2014-08-08 21:25:24]

lgtm

[dcheng, 2014-08-08 21:53:45]

push_messaging LGTM

[rlarocque, 2014-08-08 22:03:39]

The CQ bit was checked by rlarocque@chromium.org

[commit-bot: I haz the power, 2014-08-08 22:04:37]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rlarocque@chromium.org/446223002/100001

[commit-bot: I haz the power, 2014-08-09 06:04:34]

Change committed as 288523