web intents. Fix callbacks to restore tab management on service exit.

web intents. Fix callbacks to restore tab management on service exit.

Use a different weak pointer factory for the intent dispatcher so it
doesn't get cancelled when the picker dialog is hidden.

R=rouslan@chromium.org
BUG=148859
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=158177

[please use gerrit instead, 2012-09-20 16:38:30]

https://codereview.chromium.org/10969016/diff/4001/chrome/browser/ui/intents/...
File chrome/browser/ui/intents/web_intent_picker_controller.cc (right):

https://codereview.chromium.org/10969016/diff/4001/chrome/browser/ui/intents/...
chrome/browser/ui/intents/web_intent_picker_controller.cc:224:
dispatcher_factory_.InvalidateWeakPtrs();

When you say "not a well-handled case", do you mean that it crashes, that it
works 50% of the time, or that it does not look right?

If this is a band-aid, then what is the right solution? Let's add that
information to the comment, so that we can take care of this issue quickly when
needed.

https://codereview.chromium.org/10969016/diff/4001/chrome/browser/ui/intents/...
File chrome/browser/ui/intents/web_intent_picker_controller.h (right):

https://codereview.chromium.org/10969016/diff/4001/chrome/browser/ui/intents/...
chrome/browser/ui/intents/web_intent_picker_controller.h:309:
base::WeakPtrFactory<WebIntentPickerController> weak_ptr_factory_;

Comment please.

https://codereview.chromium.org/10969016/diff/4001/chrome/browser/ui/intents/...
chrome/browser/ui/intents/web_intent_picker_controller.h:316:
base::WeakPtrFactory<WebIntentPickerController> dispatcher_factory_;

What's the different between weak_ptr_factory_ and dispatcher_factory_? (They
are the same type.) Let's add that information to the comments.

[Greg Billock, 2012-09-20 16:45:18]

https://codereview.chromium.org/10969016/diff/4001/chrome/browser/ui/intents/...
File chrome/browser/ui/intents/web_intent_picker_controller.cc (right):

https://codereview.chromium.org/10969016/diff/4001/chrome/browser/ui/intents/...
chrome/browser/ui/intents/web_intent_picker_controller.cc:224:
dispatcher_factory_.InvalidateWeakPtrs();
The right solution is a "ChromeIntentsDispatcher" that has the same
query-lifetime semantics as the WebIntentsDispatcher, and we do this
RegisterReplyNotification there, not on the picker controller. We know we need
that, but we want to make changes right now that are patchable to M23, and a big
refactor isn't.


On 2012/09/20 16:38:30, Rouslan Solomakhin wrote:
> 
> When you say "not a well-handled case", do you mean that it crashes, that it
> works 50% of the time, or that it does not look right?
> 
> If this is a band-aid, then what is the right solution? Let's add that
> information to the comment, so that we can take care of this issue quickly
when
> needed.

https://codereview.chromium.org/10969016/diff/4001/chrome/browser/ui/intents/...
File chrome/browser/ui/intents/web_intent_picker_controller.h (right):

https://codereview.chromium.org/10969016/diff/4001/chrome/browser/ui/intents/...
chrome/browser/ui/intents/web_intent_picker_controller.h:309:
base::WeakPtrFactory<WebIntentPickerController> weak_ptr_factory_;
On 2012/09/20 16:38:30, Rouslan Solomakhin wrote:
> 
> Comment please.

Done.

https://codereview.chromium.org/10969016/diff/4001/chrome/browser/ui/intents/...
chrome/browser/ui/intents/web_intent_picker_controller.h:316:
base::WeakPtrFactory<WebIntentPickerController> dispatcher_factory_;
On 2012/09/20 16:38:30, Rouslan Solomakhin wrote:
> 
> What's the different between weak_ptr_factory_ and dispatcher_factory_? (They
> are the same type.) Let's add that information to the comments.

Done.

[please use gerrit instead, 2012-09-20 16:55:51]

lgtm

[commit-bot: I haz the power, 2012-09-20 16:59:02]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer or
a lowly provisional committer, _not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[Greg Billock, 2012-09-20 21:02:09]

On 2012/09/20 16:59:02, I haz the power (commit-bot) wrote:
> No LGTM from a valid reviewer yet. Only full committers are accepted.
> Even if an LGTM may have been provided, it was from a non-committer or
> a lowly provisional committer, _not_ a full super star committer.
> See http://www.chromium.org/getting-involved/become-a-committer
> Note that this has nothing to do with OWNERS files.

Rachel, can you lgtm?

[groby-ooo-7-16, 2012-09-21 02:25:21]

Not happy with the solution, but can't think of a better one. LGTM unless you
want to address my comments.

http://codereview.chromium.org/10969016/diff/5003/chrome/browser/ui/intents/w...
File chrome/browser/ui/intents/web_intent_picker_controller.cc (right):

http://codereview.chromium.org/10969016/diff/5003/chrome/browser/ui/intents/w...
chrome/browser/ui/intents/web_intent_picker_controller.cc:224:
dispatcher_factory_.InvalidateWeakPtrs();
Don't you want to invalidate _before_ you change member vars? 

(If invokation and registration is on the same thread, I guess this is safe
enough)

http://codereview.chromium.org/10969016/diff/5003/chrome/browser/ui/intents/w...
File chrome/browser/ui/intents/web_intent_picker_controller.h (right):

http://codereview.chromium.org/10969016/diff/5003/chrome/browser/ui/intents/w...
chrome/browser/ui/intents/web_intent_picker_controller.h:310: // picker model.
FYI: weak_ptr_factory_ as a member variable is a common enough idiom that it's
usually not necessary to document.

[Greg Billock, 2012-09-21 05:10:09]

http://codereview.chromium.org/10969016/diff/5003/chrome/browser/ui/intents/w...
File chrome/browser/ui/intents/web_intent_picker_controller.cc (right):

http://codereview.chromium.org/10969016/diff/5003/chrome/browser/ui/intents/w...
chrome/browser/ui/intents/web_intent_picker_controller.cc:224:
dispatcher_factory_.InvalidateWeakPtrs();
Good point. I'll do it.

On 2012/09/21 02:25:21, groby wrote:
> Don't you want to invalidate _before_ you change member vars? 
> 
> (If invokation and registration is on the same thread, I guess this is safe
> enough)

http://codereview.chromium.org/10969016/diff/5003/chrome/browser/ui/intents/w...
File chrome/browser/ui/intents/web_intent_picker_controller.h (right):

http://codereview.chromium.org/10969016/diff/5003/chrome/browser/ui/intents/w...
chrome/browser/ui/intents/web_intent_picker_controller.h:310: // picker model.
On 2012/09/21 02:25:21, groby wrote:
> FYI: weak_ptr_factory_ as a member variable is a common enough idiom that it's
> usually not necessary to document.

The intersting bit is the part about where they're used. Given that we have
three, I think there could be confusion. :-)

[commit-bot: I haz the power, 2012-09-21 16:15:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gbillock@chromium.org/10969016/8002

[commit-bot: I haz the power, 2012-09-22 01:00:29]

The commit queue went berserk retrying too often for a
seemingly flaky test. Builder is win_rel, revision is 158017

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

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gbillock@chromium.org/10969016/8002

[commit-bot: I haz the power, 2012-09-22 06:21:00]

Change committed as 158177