Disable delegation to picker controller once it is deleted.

Disable delegation to picker controller once it is deleted.

BUG=155529
R=gbillock@chromium.org


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

[Greg Billock, 2012-10-15 22:29:33]

lgtm

https://codereview.chromium.org/11141037/diff/1/chrome/browser/ui/views/web_i...
File chrome/browser/ui/views/web_intent_picker_views.cc (right):

https://codereview.chromium.org/11141037/diff/1/chrome/browser/ui/views/web_i...
chrome/browser/ui/views/web_intent_picker_views.cc:378: //
DCHECK_LE(size.width(), WebIntentPicker::kTitleLinkMaxWidth);
erase this line?

[groby-ooo-7-16, 2012-10-16 01:13:10]

Hi guys! Could you please check:

sail: cocoa OWNERS, plus double-check for CocoaPicker2 delegate handling
sky: views OWNERS
estade: Gtk OWNERS

http://codereview.chromium.org/11141037/diff/1/chrome/browser/ui/views/web_in...
File chrome/browser/ui/views/web_intent_picker_views.cc (right):

http://codereview.chromium.org/11141037/diff/1/chrome/browser/ui/views/web_in...
chrome/browser/ui/views/web_intent_picker_views.cc:378: //
DCHECK_LE(size.width(), WebIntentPicker::kTitleLinkMaxWidth);
On 2012/10/15 22:29:33, Greg Billock wrote:
> erase this line?

Done.

[sail, 2012-10-16 01:25:23]

http://codereview.chromium.org/11141037/diff/6001/chrome/browser/ui/cocoa/int...
File chrome/browser/ui/cocoa/intents/web_intent_picker_cocoa2.mm (right):

http://codereview.chromium.org/11141037/diff/6001/chrome/browser/ui/cocoa/int...
chrome/browser/ui/cocoa/intents/web_intent_picker_cocoa2.mm:88: delegate_ =
NULL;
This also requires updating web_intent_inline_service_view_controller.mm, and
web_intent_picker_view_controller.mm.

[groby-ooo-7-16, 2012-10-16 01:47:44]

http://codereview.chromium.org/11141037/diff/6001/chrome/browser/ui/cocoa/int...
File chrome/browser/ui/cocoa/intents/web_intent_picker_cocoa2.mm (right):

http://codereview.chromium.org/11141037/diff/6001/chrome/browser/ui/cocoa/int...
chrome/browser/ui/cocoa/intents/web_intent_picker_cocoa2.mm:88: delegate_ =
NULL;
Actually, InvalidateDelegate should only be called right before shutting down.
(See comment in web_intent_picker.h). To that end, all calls through delegate
not related to closing are DCHECKED on the other platforms.

Have added a DCHECK to the accessor to cover those use cases on Mac as well.

On 2012/10/16 01:25:24, sail wrote:
> This also requires updating web_intent_inline_service_view_controller.mm, and
> web_intent_picker_view_controller.mm.

[sail, 2012-10-16 01:52:41]

LGTM

I think there are some cases where we check try to access the delegate on close.

I think all of them involved the inline webview, so just updating
web_intent_inline_service_view_controller.mm might be enough.

You should also test closing the picker when showing an inline webview.

http://codereview.chromium.org/11141037/diff/8002/chrome/browser/ui/cocoa/int...
File chrome/browser/ui/cocoa/intents/web_intent_picker_cocoa2.h (right):

http://codereview.chromium.org/11141037/diff/8002/chrome/browser/ui/cocoa/int...
chrome/browser/ui/cocoa/intents/web_intent_picker_cocoa2.h:30:
DCHECK(delegate_);
I think this should move to the .mm file now that it's not a simple one line
function

[Evan Stade, 2012-10-16 03:04:41]

gtk lgtm

[sky, 2012-10-16 16:03:21]

LGTM

[commit-bot: I haz the power, 2012-10-16 19:38:26]

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

[commit-bot: I haz the power, 2012-10-16 20:14:18]

Sorry for I got bad news for ya.
Compile failed with a clobber build.
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[please use gerrit instead, 2012-10-16 20:17:52]

On 2012/10/16 20:14:18, I haz the power (commit-bot) wrote:
> Your code is likely broken or HEAD is junk.

You patch went into CQ 20 minutes after my giant patch. It's possible that these
two clashed, because they both change web_intent_picker_views.cc.

[commit-bot: I haz the power, 2012-10-18 19:21:21]

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

[commit-bot: I haz the power, 2012-10-18 20:02:26]

Retried try job too often for step(s) compile

[commit-bot: I haz the power, 2012-10-18 22:33:46]

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

[commit-bot: I haz the power, 2012-10-19 00:53:25]

Change committed as 162875