Add js api for hosted/pacakged apps to request notification authorization

Add js api for hosted/packaged apps to request notification authorization

BUG=98145
TEST=None (more pieces will be landing that will make it testable)


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

[Aaron Boodman, 2011-10-03 22:07:43]

Cool, I look forward to simpifying this :)

http://codereview.chromium.org/8113006/diff/1/chrome/renderer/extensions/call...
File chrome/renderer/extensions/callback_map.h (right):

http://codereview.chromium.org/8113006/diff/1/chrome/renderer/extensions/call...
chrome/renderer/extensions/callback_map.h:15: namespace extensions_v8_util {
This isn't specific to extensions - can you just put it in renderer/ instead?
Call it something like WeakV8FunctionMap or something?

http://codereview.chromium.org/8113006/diff/1/chrome/renderer/extensions/chro...
File chrome/renderer/extensions/chrome_app_bindings.cc (right):

http://codereview.chromium.org/8113006/diff/1/chrome/renderer/extensions/chro...
chrome/renderer/extensions/chrome_app_bindings.cc:89: "    this.experimental =
{};"
Hm, is this checked against --enable-experimental-extension-apis somewhere?

[asargent_no_longer_on_chrome, 2011-10-04 00:43:20]

Ready for another look.

http://codereview.chromium.org/8113006/diff/1/chrome/renderer/extensions/call...
File chrome/renderer/extensions/callback_map.h (right):

http://codereview.chromium.org/8113006/diff/1/chrome/renderer/extensions/call...
chrome/renderer/extensions/callback_map.h:15: namespace extensions_v8_util {
On 2011/10/03 22:07:43, Aaron Boodman wrote:
> This isn't specific to extensions - can you just put it in renderer/ instead?
> Call it something like WeakV8FunctionMap or something?

Done.

http://codereview.chromium.org/8113006/diff/1/chrome/renderer/extensions/chro...
File chrome/renderer/extensions/chrome_app_bindings.cc (right):

http://codereview.chromium.org/8113006/diff/1/chrome/renderer/extensions/chro...
chrome/renderer/extensions/chrome_app_bindings.cc:89: "    this.experimental =
{};"
On 2011/10/03 22:07:43, Aaron Boodman wrote:
> Hm, is this checked against --enable-experimental-extension-apis somewhere?
Yep - the check is in ExtensionMessageHandler::OnGetAppNotifyChannel

[Aaron Boodman, 2011-10-04 23:40:36]

http://codereview.chromium.org/8113006/diff/4001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_message_handler.cc (right):

http://codereview.chromium.org/8113006/diff/4001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_message_handler.cc:61: if
(!extension_service || !extension_service->is_ready())
I don't think that it is possible for either of these to be false.

http://codereview.chromium.org/8113006/diff/4001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_message_handler.cc:64:
extension_service->GetInstalledApp(requestor_url);
Can you also check that the app is running in the requesting process (via
ExtensionProcessManager) ?

[asargent_no_longer_on_chrome, 2011-10-05 05:13:25]

> Can you also check that the app is running in the requesting process (via
> ExtensionProcessManager) ?

I tried adding in a check like that, and it seems to work for packaged apps but
not for hosted ones. I notice that ExtensionProcessManager::
GetExtensionProcess(url) has a comment that it won't work correctly for hosted
apps without the background permission. Any advice on how to proceed?

[asargent_no_longer_on_chrome, 2011-10-06 03:35:32]

Added process check for packaged apps. 

I also realized that it will only be valid to call this function from within a
TabContents because there will be a confirmation UI in some cases done as an
infobar, so I moved the relevant code from ExtensionMessageHandler to
ExtensionTabHelper.

Please take another look.

http://codereview.chromium.org/8113006/diff/4001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_message_handler.cc (right):

http://codereview.chromium.org/8113006/diff/4001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_message_handler.cc:61: if
(!extension_service || !extension_service->is_ready())
On 2011/10/04 23:40:36, Aaron Boodman wrote:
> I don't think that it is possible for either of these to be false.
Ok, removed this.

http://codereview.chromium.org/8113006/diff/4001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_message_handler.cc:64:
extension_service->GetInstalledApp(requestor_url);
On 2011/10/04 23:40:36, Aaron Boodman wrote:
> Can you also check that the app is running in the requesting process (via
> ExtensionProcessManager) ?

As discussed in IRC, I've added the process check for packaged apps but not
hosted ones.

[Aaron Boodman, 2011-10-06 20:29:28]

http://codereview.chromium.org/8113006/diff/12002/chrome/browser/extensions/a...
File chrome/browser/extensions/app_notify_channel_setup.cc (right):

http://codereview.chromium.org/8113006/diff/12002/chrome/browser/extensions/a...
chrome/browser/extensions/app_notify_channel_setup.cc:46: if (delegate_)
You need to clear this when ExtensionTabHelper goes away, otherwise you will
crash.

http://codereview.chromium.org/8113006/diff/12002/chrome/browser/extensions/a...
File chrome/browser/extensions/app_notify_channel_setup.h (right):

http://codereview.chromium.org/8113006/diff/12002/chrome/browser/extensions/a...
chrome/browser/extensions/app_notify_channel_setup.h:1: // Copyright (c) 2011
The Chromium Authors. All rights reserved.
Add a unit test for this class?

http://codereview.chromium.org/8113006/diff/12002/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_tab_helper.cc (right):

http://codereview.chromium.org/8113006/diff/12002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_tab_helper.cc:175:
extension_service->GetInstalledApp(requestor_url);
Are you sure this works correctly for hosted apps? It seems like allowed will be
false for all hosted apps (the process check will fail).

http://codereview.chromium.org/8113006/diff/12002/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_tab_helper.h (right):

http://codereview.chromium.org/8113006/diff/12002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_tab_helper.h:90: // Implementing the
AppNotifyChannelSetup::Delegate interface.
Note that you can implement interfaces in C++ privately, so if you don't intend
for someone to call this, it's sometimes cleaner to make it private.

[asargent_no_longer_on_chrome, 2011-10-06 22:37:38]

http://codereview.chromium.org/8113006/diff/12002/chrome/browser/extensions/a...
File chrome/browser/extensions/app_notify_channel_setup.cc (right):

http://codereview.chromium.org/8113006/diff/12002/chrome/browser/extensions/a...
chrome/browser/extensions/app_notify_channel_setup.cc:46: if (delegate_)
On 2011/10/06 20:29:28, Aaron Boodman wrote:
> You need to clear this when ExtensionTabHelper goes away, otherwise you will
> crash.

delegate_ is a WeakPtr, so we get this for free. (The "if (delegate_)" check
will return false, because SupportsWeakPtr objects clear all their outstanding
WeakPtr's when they get deleted.) I could change this to "if (delegate_.get())"
if that makes it read more clearly though.

http://codereview.chromium.org/8113006/diff/12002/chrome/browser/extensions/a...
File chrome/browser/extensions/app_notify_channel_setup.h (right):

http://codereview.chromium.org/8113006/diff/12002/chrome/browser/extensions/a...
chrome/browser/extensions/app_notify_channel_setup.h:1: // Copyright (c) 2011
The Chromium Authors. All rights reserved.
On 2011/10/06 20:29:28, Aaron Boodman wrote:
> Add a unit test for this class?

Done.

http://codereview.chromium.org/8113006/diff/12002/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_tab_helper.cc (right):

http://codereview.chromium.org/8113006/diff/12002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_tab_helper.cc:175:
extension_service->GetInstalledApp(requestor_url);
On 2011/10/06 20:29:28, Aaron Boodman wrote:
> Are you sure this works correctly for hosted apps? It seems like allowed will
be
> false for all hosted apps (the process check will fail).

The process check below is:

(extension->is_hosted_app() ||
 process_manager->GetExtensionProcess(requestor_url) == process)

So we don't do the process check for packaged apps. I guess I could rewrite it
to be something like:

 bool allowed =
  ...
  <all the other stuff except the process check>
  ...;

 if (allowed && extension->is_packaged_app()) {
    RenderProcessHost* expected_process =
        process_manager->GetExtensionProcess(requestor_url);
   allowed = (expected_process == process);
 }

Let me know if you think it reads better this way.

http://codereview.chromium.org/8113006/diff/12002/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_tab_helper.h (right):

http://codereview.chromium.org/8113006/diff/12002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_tab_helper.h:90: // Implementing the
AppNotifyChannelSetup::Delegate interface.
On 2011/10/06 20:29:28, Aaron Boodman wrote:
> Note that you can implement interfaces in C++ privately, so if you don't
intend
> for someone to call this, it's sometimes cleaner to make it private.

Good idea. Done.

[Aaron Boodman, 2011-10-06 22:40:25]

lgtm

http://codereview.chromium.org/8113006/diff/12002/chrome/browser/extensions/a...
File chrome/browser/extensions/app_notify_channel_setup.cc (right):

http://codereview.chromium.org/8113006/diff/12002/chrome/browser/extensions/a...
chrome/browser/extensions/app_notify_channel_setup.cc:46: if (delegate_)
On 2011/10/06 22:37:38, Antony Sargent wrote:
> On 2011/10/06 20:29:28, Aaron Boodman wrote:
> > You need to clear this when ExtensionTabHelper goes away, otherwise you will
> > crash.
> 
> delegate_ is a WeakPtr, so we get this for free. (The "if (delegate_)" check
> will return false, because SupportsWeakPtr objects clear all their outstanding
> WeakPtr's when they get deleted.) I could change this to "if
(delegate_.get())"
> if that makes it read more clearly though. 

Oh, I missed that, OK. yeah, I do think it would be a teeny bit more clear with
.get(), but nbd.

http://codereview.chromium.org/8113006/diff/12002/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_tab_helper.cc (right):

http://codereview.chromium.org/8113006/diff/12002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_tab_helper.cc:175:
extension_service->GetInstalledApp(requestor_url);
On 2011/10/06 22:37:38, Antony Sargent wrote:
> On 2011/10/06 20:29:28, Aaron Boodman wrote:
> > Are you sure this works correctly for hosted apps? It seems like allowed
will
> be
> > false for all hosted apps (the process check will fail).
> 
> The process check below is:
> 
> (extension->is_hosted_app() ||
>  process_manager->GetExtensionProcess(requestor_url) == process)
> 
> So we don't do the process check for packaged apps. I guess I could rewrite it
> to be something like:
> 
>  bool allowed =
>   ...
>   <all the other stuff except the process check>
>   ...;
> 
>  if (allowed && extension->is_packaged_app()) {
>     RenderProcessHost* expected_process =
>         process_manager->GetExtensionProcess(requestor_url);
>    allowed = (expected_process == process);
>  }
> 
> Let me know if you think it reads better this way. 

I see, ok.

[commit-bot: I haz the power, 2011-10-06 23:11:19]

CQ is trying the patch. Follow status at
https://chromium-status.appspot.com/cq/asargent@chromium.org/8113006/21001

[commit-bot: I haz the power, 2011-10-07 00:49:01]

Change committed as 104410