[Web Intents, Mac] Allow re-open after cancellation/close of picker bubble

Allow re-open after cancellation/close of picker bubble



Picker cancellation doesn't prevent re-open any more


R=rsesek@chromium.org
BUG=110415, 90458
TEST=Open picker, cancel, open again
DIFFBASE= 9310074

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

[groby-ooo-7-16, 2012-02-03 19:28:33]

Please review.

[Robert Sesek, 2012-02-03 19:45:06]

I think unit test would be very good with this change :).

[groby-ooo-7-16, 2012-02-03 22:44:45]

I doubt the usefulness of the tests, but I'd rather land the patch than debate
it :)

[Robert Sesek, 2012-02-03 23:26:51]

On 2012/02/03 22:44:45, groby wrote:
> I doubt the usefulness of the tests, but I'd rather land the patch than debate
> it :)

I was expecting a test to verify that the picker bubble could be re-opened after
close, which is what this change says it's doing. I agree that these tests don't
provide much value.

http://codereview.chromium.org/9307086/diff/3001/chrome/browser/ui/cocoa/web_...
File chrome/browser/ui/cocoa/web_intent_bubble_controller_unittest.mm (right):

http://codereview.chromium.org/9307086/diff/3001/chrome/browser/ui/cocoa/web_...
chrome/browser/ui/cocoa/web_intent_bubble_controller_unittest.mm:40:
(void)picker_.release();  // Closing |controller_| will destroy |picker_|
nit: Comments end with a .

http://codereview.chromium.org/9307086/diff/3001/chrome/browser/ui/cocoa/web_...
chrome/browser/ui/cocoa/web_intent_bubble_controller_unittest.mm:146:
(void)picker_.release();  // Closing |picker_| will self-destruct it.
ignore_result() in base/basictypes.h.

[groby-ooo-7-16, 2012-02-04 01:57:43]

Well, the root cause was that WebIntentPickerCocoa didn't call OnClosing() -
which is now tested.

The only alternative to this test would've been to basically set up a test with
WebIntentPickerController, which would require mocking everything and the
kitchen sink. (WebDataService, FavIconService, Profile, Browser,....)

If I'm missing something and you see another alternative, please let me know.
I'd love to get a bit more testing on this.

http://codereview.chromium.org/9307086/diff/3001/chrome/browser/ui/cocoa/web_...
File chrome/browser/ui/cocoa/web_intent_bubble_controller_unittest.mm (right):

http://codereview.chromium.org/9307086/diff/3001/chrome/browser/ui/cocoa/web_...
chrome/browser/ui/cocoa/web_intent_bubble_controller_unittest.mm:40:
(void)picker_.release();  // Closing |controller_| will destroy |picker_|
On 2012/02/03 23:26:51, rsesek wrote:
> nit: Comments end with a .

Done.

http://codereview.chromium.org/9307086/diff/3001/chrome/browser/ui/cocoa/web_...
chrome/browser/ui/cocoa/web_intent_bubble_controller_unittest.mm:146:
(void)picker_.release();  // Closing |picker_| will self-destruct it.
On 2012/02/03 23:26:51, rsesek wrote:
> ignore_result() in base/basictypes.h.

Done.

[Robert Sesek, 2012-02-07 00:28:17]

Just one Q.

http://codereview.chromium.org/9307086/diff/2002/chrome/browser/ui/cocoa/web_...
File chrome/browser/ui/cocoa/web_intent_picker_cocoa.mm (right):

http://codereview.chromium.org/9307086/diff/2002/chrome/browser/ui/cocoa/web_...
chrome/browser/ui/cocoa/web_intent_picker_cocoa.mm:168: delete this;
Is this safe? Could anything access |this| after this method? When the stack is
being reaped back down to the event loop, is |this| still on the stack? If so,
MessageLoop::DeleteSoon().

[groby-ooo-7-16, 2012-02-08 03:27:42]

https://chromiumcodereview.appspot.com/9307086/diff/2002/chrome/browser/ui/co...
File chrome/browser/ui/cocoa/web_intent_picker_cocoa.mm (right):

https://chromiumcodereview.appspot.com/9307086/diff/2002/chrome/browser/ui/co...
chrome/browser/ui/cocoa/web_intent_picker_cocoa.mm:168: delete this;
On 2012/02/07 00:28:17, rsesek wrote:
> Is this safe? Could anything access |this| after this method? When the stack
is
> being reaped back down to the event loop, is |this| still on the stack? If so,
> MessageLoop::DeleteSoon().

Done.

[Robert Sesek, 2012-02-08 15:54:47]

LGTM if you fix the easy nits in the test.

https://chromiumcodereview.appspot.com/9307086/diff/10001/chrome/browser/ui/c...
File chrome/browser/ui/cocoa/web_intent_bubble_controller_unittest.mm (right):

https://chromiumcodereview.appspot.com/9307086/diff/10001/chrome/browser/ui/c...
chrome/browser/ui/cocoa/web_intent_bubble_controller_unittest.mm:20:
MOCK_METHOD2(OnServiceChosen,void(size_t index, Disposition disposition));
nit: space after ',' and throughout this file

[Robert Sesek, 2012-02-08 19:03:56]

lgtm

[commit-bot: I haz the power, 2012-02-08 21:49:25]

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

[commit-bot: I haz the power, 2012-02-08 23:18:34]

Try job failure for 9307086-12006 (retry) on mac_rel for step "unit_tests".
It's a second try, previously, step "unit_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-02-09 00:08:58]

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

[commit-bot: I haz the power, 2012-02-09 02:08:33]

Change committed as 121158