[Web Intents] Fix crash when invoking intent in incognito mode.

[Web Intents] Fix crash when invoking intent in incognito mode.

BUG=117497
TEST=WebIntentPickerControllerIncognitoBrowserTest.*


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

[groby-ooo-7-16, 2012-03-22 00:20:24]

http://codereview.chromium.org/9805005/diff/1/chrome/browser/ui/intents/web_i...
File chrome/browser/ui/intents/web_intent_picker_controller.cc (right):

http://codereview.chromium.org/9805005/diff/1/chrome/browser/ui/intents/web_i...
chrome/browser/ui/intents/web_intent_picker_controller.cc:128: if
(wrapper_->profile()->IsOffTheRecord())
We probably need to do more than just remove that check - that'd get us back to
crashing state, no? 

// TODO(binji): Clarify what to do when intents are invoked from incognito mode

instead?

http://codereview.chromium.org/9805005/diff/1/chrome/browser/ui/intents/web_i...
File chrome/browser/ui/intents/web_intent_picker_controller.h (right):

http://codereview.chromium.org/9805005/diff/1/chrome/browser/ui/intents/web_i...
chrome/browser/ui/intents/web_intent_picker_controller.h:77: friend class
WebIntentPickerControllerBrowserIncognitoTest;
FRIEND_TEST_ALL_PREFIXES(WebIntentPickerControllerIncognitoBrowserTest,
ShowDialogShouldntCrash)

I know, we have other friend statements for tests in here that don't do that
either... Mind hitting them too? ;)

http://codereview.chromium.org/9805005/diff/1/chrome/browser/ui/intents/web_i...
File chrome/browser/ui/intents/web_intent_picker_controller_browsertest.cc
(right):

http://codereview.chromium.org/9805005/diff/1/chrome/browser/ui/intents/web_i...
chrome/browser/ui/intents/web_intent_picker_controller_browsertest.cc:367:
virtual void SetUpOnMainThread() OVERRIDE {
Can we just have a WebIntentPickerControllerBrowserTest and pass the browser in
at  SetupOnMainThread?

http://codereview.chromium.org/9805005/diff/1/chrome/browser/ui/intents/web_i...
chrome/browser/ui/intents/web_intent_picker_controller_browsertest.cc:381: //
This should do nothing for now.
Does this test even work? It seems we never call the real picker anyways, just
the mock?

(We could test via side effects - if we don't early out, the async_call_pending
should be non-zero)

[binji, 2012-03-22 00:38:40]

http://codereview.chromium.org/9805005/diff/1/chrome/browser/ui/intents/web_i...
File chrome/browser/ui/intents/web_intent_picker_controller.cc (right):

http://codereview.chromium.org/9805005/diff/1/chrome/browser/ui/intents/web_i...
chrome/browser/ui/intents/web_intent_picker_controller.cc:128: if
(wrapper_->profile()->IsOffTheRecord())
On 2012/03/22 00:20:24, groby wrote:
> We probably need to do more than just remove that check - that'd get us back
to
> crashing state, no? 
> 
> // TODO(binji): Clarify what to do when intents are invoked from incognito
mode
> 
> instead?

Done.

http://codereview.chromium.org/9805005/diff/1/chrome/browser/ui/intents/web_i...
File chrome/browser/ui/intents/web_intent_picker_controller.h (right):

http://codereview.chromium.org/9805005/diff/1/chrome/browser/ui/intents/web_i...
chrome/browser/ui/intents/web_intent_picker_controller.h:77: friend class
WebIntentPickerControllerBrowserIncognitoTest;
The individual tests don't need to be friends, just the test class. This seems
to be common:
http://code.google.com/p/chromium/source/search?q=friend%5C+class%5C+.*Test&o...

http://codereview.chromium.org/9805005/diff/1/chrome/browser/ui/intents/web_i...
File chrome/browser/ui/intents/web_intent_picker_controller_browsertest.cc
(right):

http://codereview.chromium.org/9805005/diff/1/chrome/browser/ui/intents/web_i...
chrome/browser/ui/intents/web_intent_picker_controller_browsertest.cc:367:
virtual void SetUpOnMainThread() OVERRIDE {
On 2012/03/22 00:20:24, groby wrote:
> Can we just have a WebIntentPickerControllerBrowserTest and pass the browser
in
> at  SetupOnMainThread?

I can't change the signature of SetUpOnMainThread, it is declared in
InProcessBrowserTest. This seemed the easiest way to keep most of the setup.

http://codereview.chromium.org/9805005/diff/1/chrome/browser/ui/intents/web_i...
chrome/browser/ui/intents/web_intent_picker_controller_browsertest.cc:381: //
This should do nothing for now.
On 2012/03/22 00:20:24, groby wrote:
> Does this test even work? It seems we never call the real picker anyways, just
> the mock?
> 
> (We could test via side effects - if we don't early out, the
async_call_pending
> should be non-zero)

The crash is in the picker controller, not the picker. But I can add a simple
pending_async_count test.

[groby-ooo-7-16, 2012-03-22 00:45:14]

lgtm

[commit-bot: I haz the power, 2012-03-22 02:10:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/binji@chromium.org/9805005/6001

[commit-bot: I haz the power, 2012-03-22 03:16:55]

Change committed as 128153