Fix crash in AppNotifyChannelSetup when incognito profiles are used.

Fix crash in AppNotifyChannelSetup when incognito profiles are used.

BUG=118258
TEST=see bug

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

[Mihai Parparita -not on Chrome, 2012-03-21 00:48:47]

http://codereview.chromium.org/9737001/diff/1/chrome/browser/extensions/app_n...
File chrome/browser/extensions/app_notify_channel_setup.cc (right):

http://codereview.chromium.org/9737001/diff/1/chrome/browser/extensions/app_n...
chrome/browser/extensions/app_notify_channel_setup.cc:80: :
profile_(profile->GetOriginalProfile()),
As discussed, this is fine for incognito spanning mode, but for split mode, the
API should throw an error instead.

[jstritar, 2012-03-22 18:58:00]

http://codereview.chromium.org/9737001/diff/1/chrome/browser/extensions/app_n...
File chrome/browser/extensions/app_notify_channel_setup.cc (right):

http://codereview.chromium.org/9737001/diff/1/chrome/browser/extensions/app_n...
chrome/browser/extensions/app_notify_channel_setup.cc:80: :
profile_(profile->GetOriginalProfile()),
On 2012/03/21 00:48:48, Mihai Parparita wrote:
> As discussed, this is fine for incognito spanning mode, but for split mode,
the
> API should throw an error instead.

Done.

[Mihai Parparita -not on Chrome, 2012-03-22 19:44:20]

Antony should take a look at this too, since he knows the app notifications API
better.

http://codereview.chromium.org/9737001/diff/8001/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/app/app_api.cc (right):

http://codereview.chromium.org/9737001/diff/8001/chrome/browser/extensions/ap...
chrome/browser/extensions/api/app/app_api.cc:31: if (!include_incognito() &&
profile_->IsOffTheRecord()) {
There are actually two app notification APIs:
1) The (experimental) extension API, which allows an extension to display
notifications on behalf of an app (this was added mainly for the sake of
testing/prototyping)
2) The chrome.appNotifications.getChannel, which is what apps actually use. The
incognito check would need to happen in AppBindings::GetAppNotifyChannel and/or
in ExtensionTabHelper::OnGetAppNotifyChannel.

http://codereview.chromium.org/9737001/diff/8001/chrome/test/data/extensions/...
File chrome/test/data/extensions/api_test/app_notifications/launch.html (right):

http://codereview.chromium.org/9737001/diff/8001/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/app_notifications/launch.html:6: <h1>Launch
page!</h1
Nit: missing >

[jstritar, 2012-03-22 20:21:10]

http://codereview.chromium.org/9737001/diff/8001/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/app/app_api.cc (right):

http://codereview.chromium.org/9737001/diff/8001/chrome/browser/extensions/ap...
chrome/browser/extensions/api/app/app_api.cc:31: if (!include_incognito() &&
profile_->IsOffTheRecord()) {
Oh yeah, done. Do you know why this needs such custom bindings?

http://codereview.chromium.org/9737001/diff/8001/chrome/test/data/extensions/...
File chrome/test/data/extensions/api_test/app_notifications/launch.html (right):

http://codereview.chromium.org/9737001/diff/8001/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/app_notifications/launch.html:6: <h1>Launch
page!</h1
On 2012/03/22 19:44:20, Mihai Parparita wrote:
> Nit: missing >

Done.

http://codereview.chromium.org/9737001/diff/7012/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_tab_helper.cc (right):

http://codereview.chromium.org/9737001/diff/7012/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_tab_helper.cc:33: const char
kIncognitoError[] =
Not sure where I should extract the two copies of this string. Maybe the
extension_misc section of extension_constants.h?

[asargent_no_longer_on_chrome, 2012-03-22 20:58:10]

LGTM

http://codereview.chromium.org/9737001/diff/8001/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/app/app_api.cc (right):

http://codereview.chromium.org/9737001/diff/8001/chrome/browser/extensions/ap...
chrome/browser/extensions/api/app/app_api.cc:31: if (!include_incognito() &&
profile_->IsOffTheRecord()) {
On 2012/03/22 20:21:10, jstritar wrote:
> Oh yeah, done. Do you know why this needs such custom bindings?

Because it's exposed to hosted apps, which the regular extensions bindings
aren't.

http://codereview.chromium.org/9737001/diff/7012/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_tab_helper.cc (right):

http://codereview.chromium.org/9737001/diff/7012/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_tab_helper.cc:33: const char
kIncognitoError[] =
On 2012/03/22 20:21:10, jstritar wrote:
> Not sure where I should extract the two copies of this string. Maybe the
> extension_misc section of extension_constants.h?

Sure, putting it in extension_constants.h seems fine to me.

[jstritar, 2012-03-23 15:31:43]

http://codereview.chromium.org/9737001/diff/8001/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/app/app_api.cc (right):

http://codereview.chromium.org/9737001/diff/8001/chrome/browser/extensions/ap...
chrome/browser/extensions/api/app/app_api.cc:31: if (!include_incognito() &&
profile_->IsOffTheRecord()) {
On 2012/03/22 20:58:11, Antony Sargent wrote:
> On 2012/03/22 20:21:10, jstritar wrote:
> > Oh yeah, done. Do you know why this needs such custom bindings?
> 
> Because it's exposed to hosted apps, which the regular extensions bindings
> aren't.

Thanks for the explanation! We might be able to get rid of the custom binding
soon then, with the work kalman and aa have been doing.

http://codereview.chromium.org/9737001/diff/7012/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_tab_helper.cc (right):

http://codereview.chromium.org/9737001/diff/7012/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_tab_helper.cc:33: const char
kIncognitoError[] =
On 2012/03/22 20:58:11, Antony Sargent wrote:
> On 2012/03/22 20:21:10, jstritar wrote:
> > Not sure where I should extract the two copies of this string. Maybe the
> > extension_misc section of extension_constants.h?
> 
> Sure, putting it in extension_constants.h seems fine to me. 

Done.

[commit-bot: I haz the power, 2012-03-23 16:07:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jstritar@chromium.org/9737001/6017

[commit-bot: I haz the power, 2012-03-23 19:55:31]

Change committed as 128560