Allow severing connection between dispatcher and injector

Allow severing connection between dispatcher and injector


R=gbillock@chromium.org
TBR=joi@chromium.org

BUG=
TEST=InternalWebIntentsPatcher.CancelAbandonsInjector, IntentInjectorTest.AbandonDeletes

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

[Greg Billock, 2012-04-25 14:26:52]

http://codereview.chromium.org/10218009/diff/1/content/browser/intents/intent...
File content/browser/intents/intent_injector.h (right):

http://codereview.chromium.org/10218009/diff/1/content/browser/intents/intent...
content/browser/intents/intent_injector.h:62: // a new provider.
Document that this self-deletes.

http://codereview.chromium.org/10218009/diff/1/content/browser/intents/intent...
content/browser/intents/intent_injector.h:85: // but is not guaranteed to
outlive it. Use weak pointers to handle lifetime
"The injector may be Abandon()ed and deleted before the dispatcher. Use weak
pointers to ensure the notification callback remains valid."

http://codereview.chromium.org/10218009/diff/1/content/browser/intents/intent...
content/browser/intents/intent_injector.h:87:
base::WeakPtrFactory<IntentInjector> weak_factory_;
weak_ptr_factory_

http://codereview.chromium.org/10218009/diff/1/content/browser/intents/intent...
File content/browser/intents/intent_injector_unittest.cc (right):

http://codereview.chromium.org/10218009/diff/1/content/browser/intents/intent...
content/browser/intents/intent_injector_unittest.cc:103: delete dispatcher_;
Instead, call the dispatcher->SendReplyMessage. That'll test explicitly that it
won't crash and also delete it.

http://codereview.chromium.org/10218009/diff/1/content/browser/intents/web_in...
File content/browser/intents/web_intents_dispatcher_impl.cc (right):

http://codereview.chromium.org/10218009/diff/1/content/browser/intents/web_in...
content/browser/intents/web_intents_dispatcher_impl.cc:50:
intent_injector_->Abandon();
An alternate way to do this is to clear reply_notifiers_ and delete
intent_injector_. We'd need to re-register the controller and anyone else
listening, then... You think this is neater? It's definitely more
self-contained.

http://codereview.chromium.org/10218009/diff/1/content/public/browser/web_int...
File content/public/browser/web_intents_dispatcher.h (right):

http://codereview.chromium.org/10218009/diff/1/content/public/browser/web_int...
content/public/browser/web_intents_dispatcher.h:58: virtual void
CancelDispatch() = 0;
How about ResetDispatch. This sounds like it'll cancel the whole effort.

[groby-ooo-7-16, 2012-04-25 16:40:10]

http://codereview.chromium.org/10218009/diff/1/content/browser/intents/intent...
File content/browser/intents/intent_injector.h (right):

http://codereview.chromium.org/10218009/diff/1/content/browser/intents/intent...
content/browser/intents/intent_injector.h:62: // a new provider.
On 2012/04/25 14:26:52, Greg Billock wrote:
> Document that this self-deletes.

Done.

http://codereview.chromium.org/10218009/diff/1/content/browser/intents/intent...
content/browser/intents/intent_injector.h:85: // but is not guaranteed to
outlive it. Use weak pointers to handle lifetime
On 2012/04/25 14:26:52, Greg Billock wrote:
> "The injector may be Abandon()ed and deleted before the dispatcher. Use weak
> pointers to ensure the notification callback remains valid."

Done.

http://codereview.chromium.org/10218009/diff/1/content/browser/intents/intent...
content/browser/intents/intent_injector.h:87:
base::WeakPtrFactory<IntentInjector> weak_factory_;
weak_factory_ seems to be the preferred naming, actually. (1000 vs. 729
weak_ptr_factory_)
On 2012/04/25 14:26:52, Greg Billock wrote:
> weak_ptr_factory_

http://codereview.chromium.org/10218009/diff/1/content/browser/intents/intent...
File content/browser/intents/intent_injector_unittest.cc (right):

http://codereview.chromium.org/10218009/diff/1/content/browser/intents/intent...
content/browser/intents/intent_injector_unittest.cc:103: delete dispatcher_;
On 2012/04/25 14:26:52, Greg Billock wrote:
> Instead, call the dispatcher->SendReplyMessage. That'll test explicitly that
it
> won't crash and also delete it.

Done.

http://codereview.chromium.org/10218009/diff/1/content/browser/intents/web_in...
File content/browser/intents/web_intents_dispatcher_impl.cc (right):

http://codereview.chromium.org/10218009/diff/1/content/browser/intents/web_in...
content/browser/intents/web_intents_dispatcher_impl.cc:50:
intent_injector_->Abandon();
The re-registering would happen in all sorts of different classes, though. I
think this is cleaner and expresses the intent better - abandon our current
attempt, so we can restart a new one.

On 2012/04/25 14:26:52, Greg Billock wrote:
> An alternate way to do this is to clear reply_notifiers_ and delete
> intent_injector_. We'd need to re-register the controller and anyone else
> listening, then... You think this is neater? It's definitely more
> self-contained.

http://codereview.chromium.org/10218009/diff/1/content/public/browser/web_int...
File content/public/browser/web_intents_dispatcher.h (right):

http://codereview.chromium.org/10218009/diff/1/content/public/browser/web_int...
content/public/browser/web_intents_dispatcher.h:58: virtual void
CancelDispatch() = 0;
On 2012/04/25 14:26:52, Greg Billock wrote:
> How about ResetDispatch. This sounds like it'll cancel the whole effort.

Done.

[Greg Billock, 2012-04-25 17:04:49]

On 2012/04/25 16:40:10, groby wrote:
>
http://codereview.chromium.org/10218009/diff/1/content/browser/intents/intent...
> File content/browser/intents/intent_injector.h (right):
> 
>
http://codereview.chromium.org/10218009/diff/1/content/browser/intents/intent...
> content/browser/intents/intent_injector.h:62: // a new provider.
> On 2012/04/25 14:26:52, Greg Billock wrote:
> > Document that this self-deletes.
> 
> Done.
> 
>
http://codereview.chromium.org/10218009/diff/1/content/browser/intents/intent...
> content/browser/intents/intent_injector.h:85: // but is not guaranteed to
> outlive it. Use weak pointers to handle lifetime
> On 2012/04/25 14:26:52, Greg Billock wrote:
> > "The injector may be Abandon()ed and deleted before the dispatcher. Use weak
> > pointers to ensure the notification callback remains valid."
> 
> Done.
> 
>
http://codereview.chromium.org/10218009/diff/1/content/browser/intents/intent...
> content/browser/intents/intent_injector.h:87:
> base::WeakPtrFactory<IntentInjector> weak_factory_;
> weak_factory_ seems to be the preferred naming, actually. (1000 vs. 729
> weak_ptr_factory_)
> On 2012/04/25 14:26:52, Greg Billock wrote:
> > weak_ptr_factory_
> 
>
http://codereview.chromium.org/10218009/diff/1/content/browser/intents/intent...
> File content/browser/intents/intent_injector_unittest.cc (right):
> 
>
http://codereview.chromium.org/10218009/diff/1/content/browser/intents/intent...
> content/browser/intents/intent_injector_unittest.cc:103: delete dispatcher_;
> On 2012/04/25 14:26:52, Greg Billock wrote:
> > Instead, call the dispatcher->SendReplyMessage. That'll test explicitly that
> it
> > won't crash and also delete it.
> 
> Done.
> 
>
http://codereview.chromium.org/10218009/diff/1/content/browser/intents/web_in...
> File content/browser/intents/web_intents_dispatcher_impl.cc (right):
> 
>
http://codereview.chromium.org/10218009/diff/1/content/browser/intents/web_in...
> content/browser/intents/web_intents_dispatcher_impl.cc:50:
> intent_injector_->Abandon();
> The re-registering would happen in all sorts of different classes, though. I
> think this is cleaner and expresses the intent better - abandon our current
> attempt, so we can restart a new one.
> 
> On 2012/04/25 14:26:52, Greg Billock wrote:
> > An alternate way to do this is to clear reply_notifiers_ and delete
> > intent_injector_. We'd need to re-register the controller and anyone else
> > listening, then... You think this is neater? It's definitely more
> > self-contained.
> 
>
http://codereview.chromium.org/10218009/diff/1/content/public/browser/web_int...
> File content/public/browser/web_intents_dispatcher.h (right):
> 
>
http://codereview.chromium.org/10218009/diff/1/content/public/browser/web_int...
> content/public/browser/web_intents_dispatcher.h:58: virtual void
> CancelDispatch() = 0;
> On 2012/04/25 14:26:52, Greg Billock wrote:
> > How about ResetDispatch. This sounds like it'll cancel the whole effort.
> 
> Done.

lgtm

[commit-bot: I haz the power, 2012-04-25 17:08:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/groby@chromium.org/10218009/7001

[commit-bot: I haz the power, 2012-04-25 17:08:39]

Presubmit check for 10218009-7001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Messages **
If this change has an associated bug, add BUG=[bug number].

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    content/public/browser

Presubmit checks took 1.4s to calculate.

[commit-bot: I haz the power, 2012-04-25 17:44:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/groby@chromium.org/10218009/7001

[commit-bot: I haz the power, 2012-04-25 18:06:41]

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

[commit-bot: I haz the power, 2012-04-25 23:11:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/groby@chromium.org/10218009/3004

[commit-bot: I haz the power, 2012-04-26 00:39:43]

Change committed as 134024