Make it so that allow_js_access: false can be used with background pages created by window.open.

Make it so that allow_js_access: false can be used with background pages created by window.open.

We want attempts to create those windows to succeed, but the window.open call
should still return null. This is accomplished by opening the background contents
in another process, in the same manner as r125180.

BUG=120446
R=creis@chromium.org

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

[Mihai Parparita -not on Chrome, 2012-03-24 01:03:11]

This still needs some work (a test, better behavior when calling
window.open when a background page already exists), but I wanted to make
sure I was on the right track, especially with the implementation of
window.open returning null.

Mihai

On Fri, Mar 23, 2012 at 6:01 PM, <mihaip@chromium.org> wrote:

> Reviewers: creis,
>
> Description:
> Make it so that allow_js_access: false can be used with background pages
> created
> by window.open.
>
> We want attempts to create those windows to succeed, but the window.open
> call
> should still return null. This is accomplished by having
> RenderViewImpl::createView do all the regular work, but omit its return
> value.
>
> R=creis@chromium.org
>
>
> Please review this at
https://chromiumcodereview.**appspot.com/9837074/<https://chromiumcodereview....
>
> SVN Base:
svn://svn.chromium.org/chrome/**trunk/src<http://svn.chromium.org/chrome/trunk/src>
>
> Affected files:
>  M chrome/browser/chrome_content_**browser_client.h
>  M chrome/browser/chrome_content_**browser_client.cc
>  M chrome/common/extensions/**extension.cc
>  M chrome/common/extensions/**extension_manifest_constants.h
>  M chrome/common/extensions/**extension_manifest_constants.**cc
>  M content/browser/renderer_host/**render_message_filter.h
>  M content/browser/renderer_host/**render_message_filter.cc
>  M content/common/view_messages.h
>  M content/public/browser/**content_browser_client.h
>  M content/renderer/render_view_**impl.cc
>
>
> Index: chrome/browser/chrome_content_**browser_client.cc
> diff --git a/chrome/browser/chrome_**content_browser_client.cc
> b/chrome/browser/chrome_**content_browser_client.cc
> index 76de2316615e11e70554fe75e7c8a6**a46b0b08d2..**
> a7c6f3a6dc4911c6fe2809611c2635**7596eb76b0 100644
> --- a/chrome/browser/chrome_**content_browser_client.cc
> +++ b/chrome/browser/chrome_**content_browser_client.cc
> @@ -1259,7 +1259,8 @@ void ChromeContentBrowserClient::**
> CancelDesktopNotification(
>  #endif
>  }
>
> -bool ChromeContentBrowserClient::**CanCreateWindow(
> +content::**ContentBrowserClient::**CanCreateWindowResult
> +ChromeContentBrowserClient::**CanCreateWindow(
>     const GURL& opener_url,
>     const GURL& source_origin,
>     WindowContainerType container_type,
> @@ -1272,23 +1273,23 @@ bool ChromeContentBrowserClient::**
> CanCreateWindow(
>     ProfileIOData* io_data = ProfileIOData::**
> FromResourceContext(context);
>     ExtensionInfoMap* map = io_data->GetExtensionInfoMap()**;
>
> -    // If the opener is not allowed to script its background window, then
> return
> -    // false so that the window.open call returns null.  In this case,
> only
> -    // the manifest is permitted to create a background window.
> +    if (!map->**SecurityOriginHasAPIPermission**(
> +        source_origin, render_process_id,
ExtensionAPIPermission::**kBackground))
> {
> +      return CANNOT_CREATE_WINDOW;
> +    }
> +
>     // Note: this use of GetExtensionOrAppByURL is safe but imperfect.  It
> may
>     // return a recently installed Extension even if this CanCreateWindow
> call
>     // was made by an old copy of the page in a normal web process.
>  That's ok,
> -    // because the permission check below will still fail.  We must use
> the
> -    // full URL to find hosted apps, though, and not just the origin.
> +    // because the permission check above would have caused an early
> return
> +    // already. We must use the full URL to find hosted apps, though, and
> not
> +    // just the origin.
>     const Extension* extension = map->extensions().**
> GetExtensionOrAppByURL(
>         ExtensionURLInfo(opener_url));
>     if (extension && !extension->allow_background_**js_access())
> -      return false;
> -
> -    return map->**SecurityOriginHasAPIPermission**(
> -        source_origin, render_process_id, ExtensionAPIPermission::**
> kBackground);
> +      return CAN_CREATE_WINDOW_NO_JS_**ACCESS;
>   }
> -  return true;
> +  return CAN_CREATE_WINDOW;
>  }
>
>  std::string ChromeContentBrowserClient::**GetWorkerProcessTitle(
> Index: chrome/browser/chrome_content_**browser_client.h
> diff --git a/chrome/browser/chrome_**content_browser_client.h
> b/chrome/browser/chrome_**content_browser_client.h
> index 79278fc8f2326ffcf394d54f711301**ce9feeecc0..**
> 4d70e069e50edc2005442ae5868ea6**0a1593872e 100644
> --- a/chrome/browser/chrome_**content_browser_client.h
> +++ b/chrome/browser/chrome_**content_browser_client.h
> @@ -140,7 +140,7 @@ class ChromeContentBrowserClient : public
> content::ContentBrowserClient {
>       int render_process_id,
>       int render_view_id,
>       int notification_id) OVERRIDE;
> -  virtual bool CanCreateWindow(
> +  virtual CanCreateWindowResult CanCreateWindow(
>       const GURL& opener_url,
>       const GURL& source_origin,
>       WindowContainerType container_type,
> Index: chrome/common/extensions/**extension.cc
> diff --git a/chrome/common/extensions/**extension.cc
> b/chrome/common/extensions/**extension.cc
> index 549566daf80e743e92acbd07db1fcd**b1bc31a9c5..**
> b4d4733c14bdda459bee90c58a0581**49ad444d76 100644
> --- a/chrome/common/extensions/**extension.cc
> +++ b/chrome/common/extensions/**extension.cc
> @@ -1828,11 +1828,6 @@ bool Extension::**LoadBackgroundAllowJSAccess(
>     return false;
>   }
>
> -  if (!has_background_page()) {
> -    *error = ASCIIToUTF16(errors::**kInvalidBackgroundAllowJsAcces**
> sNoPage);
> -    return false;
> -  }
> -
>   return true;
>  }
>
> Index: chrome/common/extensions/**extension_manifest_constants.**cc
> diff --git a/chrome/common/extensions/**extension_manifest_constants.**cc
> b/chrome/common/extensions/**extension_manifest_constants.**cc
> index d1fa89eea4aea30401ed018fbafc28**17f11bc03f..**
> a9cf15d5e30d3bba2f326f7de1cfcb**b6dc52307d 100644
> --- a/chrome/common/extensions/**extension_manifest_constants.**cc
> +++ b/chrome/common/extensions/**extension_manifest_constants.**cc
> @@ -180,9 +180,6 @@ const char kInvalidBackground[] =
>     "Invalid value for 'background_page'.";
>  const char kInvalidBackgroundAllowJsAcces**s[] =
>     "Invalid value for 'background.allow_js_access'."**;
> -const char kInvalidBackgroundAllowJsAcces**sNoPage[] =
> -    "Must specify one of background.page or background.scripts to use"
> -    " background.allow_js_access.";
>  const char kInvalidBackgroundCombination[**] =
>     "The background.page and background.scripts properties cannot be used
> at "
>     "the same time.";
> Index: chrome/common/extensions/**extension_manifest_constants.h
> diff --git a/chrome/common/extensions/**extension_manifest_constants.h
> b/chrome/common/extensions/**extension_manifest_constants.h
> index 83928583c359ecb0155f9348a6b67d**4cda39052e..**
> 4284e04dea765253b04429ee32ebe2**6f5933ec6f 100644
> --- a/chrome/common/extensions/**extension_manifest_constants.h
> +++ b/chrome/common/extensions/**extension_manifest_constants.h
> @@ -168,7 +168,6 @@ namespace extension_manifest_errors {
>   extern const char kInvalidAllFrames[];
>   extern const char kInvalidBackground[];
>   extern const char kInvalidBackgroundAllowJsAcces**s[];
> -  extern const char kInvalidBackgroundAllowJsAcces**sNoPage[];
>   extern const char kInvalidBackgroundCombination[**];
>   extern const char kInvalidBackgroundScript[];
>   extern const char kInvalidBackgroundScripts[];
> Index: content/browser/renderer_host/**render_message_filter.cc
> diff --git a/content/browser/renderer_**host/render_message_filter.cc
> b/content/browser/renderer_**host/render_message_filter.cc
> index 0684b0ffe77e6c5e71c9089403cfcb**020a1b142e..**
> 6a407720b487d230c1ec81099e0bb5**40d152962d 100644
> --- a/content/browser/renderer_**host/render_message_filter.cc
> +++ b/content/browser/renderer_**host/render_message_filter.cc
> @@ -401,16 +401,23 @@ void RenderMessageFilter::**OnMsgCreateWindow(
>     const ViewHostMsg_CreateWindow_**Params& params,
>     int* route_id,
>     int* surface_id,
> -    int64* cloned_session_storage_**namespace_id) {
> -  if (!content::GetContentClient()-**>browser()->CanCreateWindow(
> +    int64* cloned_session_storage_**namespace_id,
> +    bool* no_js_access) {
> +  content::ContentBrowserClient:**:CanCreateWindowResult result =
> +      content::GetContentClient()->**browser()->CanCreateWindow(
>           GURL(params.opener_url), GURL(params.opener_security_**origin),
>           params.window_container_type, resource_context_,
> -          render_process_id_)) {
> +          render_process_id_);
> +
> +  if (result == content::ContentBrowserClient:**:CANNOT_CREATE_WINDOW) {
>     *route_id = MSG_ROUTING_NONE;
>     *surface_id = 0;
>     return;
>   }
>
> +  *no_js_access =
> +      result == content::ContentBrowserClient:**:CAN_CREATE_WINDOW_NO_JS_
> **ACCESS;
> +
>  #ifdef ENABLE_NEW_DOM_STORAGE_BACKEND
>   // TODO(michaeln): Fix this.
>   // This is a bug in the existing impl, session storage is effectively
> Index: content/browser/renderer_host/**render_message_filter.h
> diff --git a/content/browser/renderer_**host/render_message_filter.h
> b/content/browser/renderer_**host/render_message_filter.h
> index fff19638f0a67bc2780a62eba7946d**cfc218ff2f..**
> 3a485c3950f8f2b0499a1a90938251**1910259461 100644
> --- a/content/browser/renderer_**host/render_message_filter.h
> +++ b/content/browser/renderer_**host/render_message_filter.h
> @@ -104,7 +104,8 @@ class RenderMessageFilter : public
> content::BrowserMessageFilter {
>   void OnMsgCreateWindow(const ViewHostMsg_CreateWindow_**Params& params,
>                          int* route_id,
>                          int* surface_id,
> -                         int64* cloned_session_storage_**namespace_id);
> +                         int64* cloned_session_storage_**namespace_id,
> +                         bool* no_js_access);
>   void OnMsgCreateWidget(int opener_id,
>                          WebKit::WebPopupType popup_type,
>                          int* route_id,
> Index: content/common/view_messages.h
> diff --git a/content/common/view_**messages.h b/content/common/view_**
> messages.h
> index 2eb279e1b1e4b922ef34a4a9df16c9**693b006a5f..**
> 7356f46aecfa4550fd67efc182de19**5257ef6fd8 100644
> --- a/content/common/view_**messages.h
> +++ b/content/common/view_**messages.h
> @@ -1181,11 +1181,12 @@ IPC_MESSAGE_CONTROL1(ViewMsg_**TempCrashWithData,
>  // Sent by the renderer when it is creating a new window.  The browser
> creates
>  // a tab for it and responds with a ViewMsg_CreatingNew_ACK.  If route_id
> is
>  // MSG_ROUTING_NONE, the view couldn't be created.
> -IPC_SYNC_MESSAGE_CONTROL1_3(**ViewHostMsg_CreateWindow,
> +IPC_SYNC_MESSAGE_CONTROL1_4(**ViewHostMsg_CreateWindow,
>                             ViewHostMsg_CreateWindow_**Params,
>                             int /* route_id */,
>                             int32 /* surface_id */,
> -                            int64 /* cloned_session_storage_**namespace_id
> */)
> +                            int64 /* cloned_session_storage_**namespace_id
> */,
> +                            bool /* no_js_access */)
>
>  // Similar to ViewHostMsg_CreateWindow, except used for sub-widgets, like
>  // <select> dropdowns.  This message is sent to the TabContents that
> Index: content/public/browser/**content_browser_client.h
> diff --git a/content/public/browser/**content_browser_client.h
> b/content/public/browser/**content_browser_client.h
> index c9303d93a919ac054531146147151a**5625e623ee..**
> fcff765f5d1912de1aef4cb0e8a6a2**b0b1816443 100644
> --- a/content/public/browser/**content_browser_client.h
> +++ b/content/public/browser/**content_browser_client.h
> @@ -91,6 +91,12 @@ class ContentBrowserClient {
>  public:
>   virtual ~ContentBrowserClient() {}
>
> +  enum CanCreateWindowResult {
> +    CANNOT_CREATE_WINDOW,
> +    CAN_CREATE_WINDOW,
> +    CAN_CREATE_WINDOW_NO_JS_ACCESS
> +  };
> +
>   // Allows the embedder to set any number of custom BrowserMainParts
>   // implementations for the browser startup code. See comments in
>   // browser_main_parts.h.
> @@ -333,7 +339,7 @@ class ContentBrowserClient {
>   // Returns true if the given page is allowed to open a window of the
> given
>   // type.
>   // This is called on the IO thread.
> -  virtual bool CanCreateWindow(
> +  virtual CanCreateWindowResult CanCreateWindow(
>       const GURL& opener_url,
>       const GURL& source_origin,
>       WindowContainerType container_type,
> Index: content/renderer/render_view_**impl.cc
> diff --git a/content/renderer/render_**view_impl.cc
> b/content/renderer/render_**view_impl.cc
> index 5a016ac64d916b62eba02156930446**3e33bbb5b8..**
> 98ebc5ab0ee946c10fbefbd93f3144**b5e489a5b8 100644
> --- a/content/renderer/render_**view_impl.cc
> +++ b/content/renderer/render_**view_impl.cc
> @@ -1481,12 +1481,14 @@ WebView* RenderViewImpl::createView(
>   int32 routing_id = MSG_ROUTING_NONE;
>   int32 surface_id = 0;
>   int64 cloned_session_storage_**namespace_id;
> +  bool no_js_access;
>
>   RenderThread::Get()->Send(
>       new ViewHostMsg_CreateWindow(**params,
>                                    &routing_id,
>                                    &surface_id,
> -                                   &cloned_session_storage_**
> namespace_id));
> +                                   &cloned_session_storage_**
> namespace_id,
> +                                   &no_js_access));
>   if (routing_id == MSG_ROUTING_NONE)
>     return NULL;
>
> @@ -1521,7 +1523,7 @@ WebView* RenderViewImpl::createView(
>   // the new render view (we don't need the browser to send the URL back
> down).
>   view->alternate_error_page_**url_ = alternate_error_page_url_;
>
> -  return view->webview();
> +  return no_js_access ? NULL : view->webview();
>  }
>
>  WebWidget* RenderViewImpl::**createPopupMenu(WebKit::**WebPopupType
> popup_type) {
>
>
>

[Andrew T Wilson (Slow), 2012-03-26 15:28:10]

Looks like this is the right approach. A few nits.

https://chromiumcodereview.appspot.com/9837074/diff/1/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client.cc (right):

https://chromiumcodereview.appspot.com/9837074/diff/1/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.cc:1277: source_origin,
render_process_id, ExtensionAPIPermission::kBackground)) {
nit: 80 chars

https://chromiumcodereview.appspot.com/9837074/diff/1/content/browser/rendere...
File content/browser/renderer_host/render_message_filter.cc (right):

https://chromiumcodereview.appspot.com/9837074/diff/1/content/browser/rendere...
content/browser/renderer_host/render_message_filter.cc:418: *no_js_access =
I'd move this to the check up above just to make sure no_js_access is always
initialized. Otherwise, we have to document that it's only initialized in some
case.

https://chromiumcodereview.appspot.com/9837074/diff/1/content/renderer/render...
File content/renderer/render_view_impl.cc (right):

https://chromiumcodereview.appspot.com/9837074/diff/1/content/renderer/render...
content/renderer/render_view_impl.cc:1495: // TODO(fsamuel): The host renderer
needs to be able to control whether
Naive question - what happens if we don't do all of this (what if the above
check read):

if (routing_id == MSG_ROUTING_NONE || no_js_access) return NULL;

So no RenderViewImpl is created? Since it's a background page, do we need to
create the RenderView?

[Charlie Reis, 2012-03-26 17:56:50]

https://chromiumcodereview.appspot.com/9837074/diff/1/chrome/common/extension...
File chrome/common/extensions/extension.cc (left):

https://chromiumcodereview.appspot.com/9837074/diff/1/chrome/common/extension...
chrome/common/extensions/extension.cc:1832: *error =
ASCIIToUTF16(errors::kInvalidBackgroundAllowJsAccessNoPage);
Normally, we expect people to set allow_js_access to false, since it defaults to
true.

However, what if the manifest explicitly sets allow_js_access to true, without
specifying a background page?  Are we ok with the app creating background pages
manually and scripting them?  (Note that they will have process-per-site in that
case.)

If so, do we also allow an empty "background" block (with no mention of page or
allow_js_access) in the manifest to get this behavior?  Or should we keep it an
error for allow_js_access to be true without specifying a background page?

https://chromiumcodereview.appspot.com/9837074/diff/1/content/renderer/render...
File content/renderer/render_view_impl.cc (right):

https://chromiumcodereview.appspot.com/9837074/diff/1/content/renderer/render...
content/renderer/render_view_impl.cc:1495: // TODO(fsamuel): The host renderer
needs to be able to control whether
On 2012/03/26 15:28:10, Andrew T Wilson wrote:
> Naive question - what happens if we don't do all of this (what if the above
> check read):
> 
> if (routing_id == MSG_ROUTING_NONE || no_js_access) return NULL;
> 
> So no RenderViewImpl is created? Since it's a background page, do we need to
> create the RenderView?

Wouldn't that mean that no WebKit Frame is created for it?  I don't know much
about BackgroundContents, but they do have a RenderViewHost, right?

I agree that it feels weird to wire up a connected RenderView when we don't want
the page to be able to script it, and I'm not yet convinced that this current
approach will be enough.  (There might still be some ways for a reference to
leak through, since the routing_id is valid.)

The alternative is doing something similar to http://crbug.com/69267, where we
open the background page in a new BrowsingInstance (and thus a separate
process).  Then we're guaranteed to have no references leak.  Not sure if that's
the process model behavior we want in this case or not.

See this CL for context:
https://chromiumcodereview.appspot.com/9325082

[Mihai Parparita -not on Chrome, 2012-03-27 01:16:07]

PTAL. I've added tests as well.

+John for content API changes.

https://chromiumcodereview.appspot.com/9837074/diff/1/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client.cc (right):

https://chromiumcodereview.appspot.com/9837074/diff/1/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.cc:1277: source_origin,
render_process_id, ExtensionAPIPermission::kBackground)) {
On 2012/03/26 15:28:10, Andrew T Wilson wrote:
> nit: 80 chars

Done.

https://chromiumcodereview.appspot.com/9837074/diff/1/chrome/common/extension...
File chrome/common/extensions/extension.cc (left):

https://chromiumcodereview.appspot.com/9837074/diff/1/chrome/common/extension...
chrome/common/extensions/extension.cc:1832: *error =
ASCIIToUTF16(errors::kInvalidBackgroundAllowJsAccessNoPage);
On 2012/03/26 17:56:51, creis wrote:
> Normally, we expect people to set allow_js_access to false, since it defaults
to
> true.
> 
> However, what if the manifest explicitly sets allow_js_access to true, without
> specifying a background page?  Are we ok with the app creating background
pages
> manually and scripting them?  (Note that they will have process-per-site in
that
> case.)
> 
> If so, do we also allow an empty "background" block (with no mention of page
or
> allow_js_access) in the manifest to get this behavior?  Or should we keep it
an
> error for allow_js_access to be true without specifying a background page?

Not quite sure I understand this. Being able to create a background page not
tied to the "background" section in the manifest,  but instead to the
"background" permission. Apps today can already create same-process background
pages and script them, and I don't think we can change that.

https://chromiumcodereview.appspot.com/9837074/diff/1/content/renderer/render...
File content/renderer/render_view_impl.cc (right):

https://chromiumcodereview.appspot.com/9837074/diff/1/content/renderer/render...
content/renderer/render_view_impl.cc:1495: // TODO(fsamuel): The host renderer
needs to be able to control whether
On 2012/03/26 17:56:51, creis wrote:
> The alternative is doing something similar to http://crbug.com/69267, where we
> open the background page in a new BrowsingInstance (and thus a separate
> process).  Then we're guaranteed to have no references leak.  Not sure if
that's
> the process model behavior we want in this case or not.
> 
> See this CL for context:
> https://chromiumcodereview.appspot.com/9325082

Thanks for the pointer to this. I've changed this CL to use that approach
instead.

[Charlie Reis, 2012-03-27 23:19:27]

LGTM

https://chromiumcodereview.appspot.com/9837074/diff/1/chrome/common/extension...
File chrome/common/extensions/extension.cc (left):

https://chromiumcodereview.appspot.com/9837074/diff/1/chrome/common/extension...
chrome/common/extensions/extension.cc:1832: *error =
ASCIIToUTF16(errors::kInvalidBackgroundAllowJsAccessNoPage);
On 2012/03/27 01:16:07, Mihai Parparita wrote:
> Not quite sure I understand this. Being able to create a background page not
> tied to the "background" section in the manifest,  but instead to the
> "background" permission. Apps today can already create same-process background
> pages and script them, and I don't think we can change that.

Here's what I meant.  Suppose the manifest looked like this:
  "permissions": ["background"],
  "background": {
    "allow_js_access": true
  }

That's equivalent to:
  "permissions": ["background"],
  "background": {
  }

I think this used to be illegal syntax, but now we appear to be saying it's ok. 
I just wanted to confirm that it's legal.  (It means we don't create a
background page for the app until it calls window.open for one, but when we do,
it stays in process and can be scripted.  Seems ok to me, but I think it wasn't
a case we supported before.)

https://chromiumcodereview.appspot.com/9837074/diff/1002/chrome/browser/ui/br...
File chrome/browser/ui/browser.cc (right):

https://chromiumcodereview.appspot.com/9837074/diff/1002/chrome/browser/ui/br...
chrome/browser/ui/browser.cc:2333: // If script access is not allowed, created
the the background contents in a
nit: create

https://chromiumcodereview.appspot.com/9837074/diff/1002/content/public/brows...
File content/public/browser/content_browser_client.h (right):

https://chromiumcodereview.appspot.com/9837074/diff/1002/content/public/brows...
content/public/browser/content_browser_client.h:95: // The given window canot be
created.
nit: cannot

https://chromiumcodereview.appspot.com/9837074/diff/1002/content/public/brows...
File content/public/browser/web_contents_delegate.h (right):

https://chromiumcodereview.appspot.com/9837074/diff/1002/content/public/brows...
content/public/browser/web_contents_delegate.h:276: // true to allow the
creation. Default is to allow it.
Should we mention that |target_url| isn't normally used, unless the delegate
intercepts the navigation and handles it itself?

[jam, 2012-03-28 02:38:26]

http://codereview.chromium.org/9837074/diff/1002/content/public/browser/conte...
File content/public/browser/content_browser_client.h (right):

http://codereview.chromium.org/9837074/diff/1002/content/public/browser/conte...
content/public/browser/content_browser_client.h:94: enum CanCreateWindowResult {
nit: see hte other enums in content API to see the convention, i.e. this would
be CAN_CREATE_WINDOW_RESULTS_DISALLOW, ...ALLOW, ...ALLOW_NO_JS_ACCESS.

but can we avoid the enum altogether? because if you add it and are passing it
around in content, by convention to the API this needs to be in a separate file
which seems nice to avoid (we try to keep the files in the public directories to
a minimum). so another boolean out parameter on the function that is
"no_javascript_access" (we also don't abbreviate per the style guide)

[Mihai Parparita -not on Chrome, 2012-03-28 20:29:01]

+Sky, for browser/ui OWNERS

http://codereview.chromium.org/9837074/diff/1/chrome/common/extensions/extens...
File chrome/common/extensions/extension.cc (left):

http://codereview.chromium.org/9837074/diff/1/chrome/common/extensions/extens...
chrome/common/extensions/extension.cc:1832: *error =
ASCIIToUTF16(errors::kInvalidBackgroundAllowJsAccessNoPage);
On 2012/03/27 23:19:27, creis wrote:
> On 2012/03/27 01:16:07, Mihai Parparita wrote:
> > Not quite sure I understand this. Being able to create a background page not
> > tied to the "background" section in the manifest,  but instead to the
> > "background" permission. Apps today can already create same-process
background
> > pages and script them, and I don't think we can change that.
> 
> Here's what I meant.  Suppose the manifest looked like this:
>   "permissions": ["background"],
>   "background": {
>     "allow_js_access": true
>   }
> 
> That's equivalent to:
>   "permissions": ["background"],
>   "background": {
>   }
> 
> I think this used to be illegal syntax, but now we appear to be saying it's
ok. 

I'm pretty sure this was legal, and that manifest does load in M18. We never
check for the presence/absence of the background section itself, just keys
within it (kBackgroundPage, kBackgroundScripts, etc.).

> I just wanted to confirm that it's legal.  (It means we don't create a
> background page for the app until it calls window.open for one, but when we
do,
> it stays in process and can be scripted.  Seems ok to me, but I think it
wasn't
> a case we supported before.)

This how window.open(..., "background") has worked since it was introduced in
Chrome 10 (same-process, scriptable).

http://codereview.chromium.org/9837074/diff/1002/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/9837074/diff/1002/chrome/browser/ui/browser.cc...
chrome/browser/ui/browser.cc:2333: // If script access is not allowed, created
the the background contents in a
On 2012/03/27 23:19:27, creis wrote:
> nit: create

Done.

http://codereview.chromium.org/9837074/diff/1002/content/public/browser/conte...
File content/public/browser/content_browser_client.h (right):

http://codereview.chromium.org/9837074/diff/1002/content/public/browser/conte...
content/public/browser/content_browser_client.h:94: enum CanCreateWindowResult {
On 2012/03/28 02:38:26, John Abd-El-Malek wrote:
> nit: see hte other enums in content API to see the convention, i.e. this would
> be CAN_CREATE_WINDOW_RESULTS_DISALLOW, ...ALLOW, ...ALLOW_NO_JS_ACCESS.
> 
> but can we avoid the enum altogether? because if you add it and are passing it
> around in content, by convention to the API this needs to be in a separate
file
> which seems nice to avoid (we try to keep the files in the public directories
to
> a minimum). so another boolean out parameter on the function that is
> "no_javascript_access" (we also don't abbreviate per the style guide)

Went with the boolean out parameter approach.

http://codereview.chromium.org/9837074/diff/1002/content/public/browser/conte...
content/public/browser/content_browser_client.h:95: // The given window canot be
created.
On 2012/03/27 23:19:27, creis wrote:
> nit: cannot

Obsolete, this enum is gone per John's comments.

http://codereview.chromium.org/9837074/diff/1002/content/public/browser/web_c...
File content/public/browser/web_contents_delegate.h (right):

http://codereview.chromium.org/9837074/diff/1002/content/public/browser/web_c...
content/public/browser/web_contents_delegate.h:276: // true to allow the
creation. Default is to allow it.
On 2012/03/27 23:19:27, creis wrote:
> Should we mention that |target_url| isn't normally used, unless the delegate
> intercepts the navigation and handles it itself?

In theory, a delegate could also make decisions based on the target URL (it just
hasn't needed to so far), but I've added a comment about the URL being used if
the delegate handles creation/navigation.

[sky, 2012-03-28 20:43:13]

http://codereview.chromium.org/9837074/diff/6028/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/9837074/diff/6028/chrome/browser/ui/browser.cc...
chrome/browser/ui/browser.cc:2360: content::PAGE_TRANSITION_LINK,
Are you sure this is the right transition type?

[Mihai Parparita -not on Chrome, 2012-03-28 20:47:50]

http://codereview.chromium.org/9837074/diff/6028/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/9837074/diff/6028/chrome/browser/ui/browser.cc...
chrome/browser/ui/browser.cc:2360: content::PAGE_TRANSITION_LINK,
On 2012/03/28 20:43:13, sky wrote:
> Are you sure this is the right transition type?

This is how BackgroundService labels its navigations:
http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/background/...

None of the other transition types seem any more appropriate.

[sky, 2012-03-28 20:50:16]

Fair enough, LGTM

[jam, 2012-03-28 21:29:10]

lgtm

[Andrew T Wilson (Slow), 2012-03-28 22:01:30]

drive by LGTM w/nits.

http://codereview.chromium.org/9837074/diff/6030/chrome/browser/chrome_conten...
File chrome/browser/chrome_content_browser_client.cc (right):

http://codereview.chromium.org/9837074/diff/6030/chrome/browser/chrome_conten...
chrome/browser/chrome_content_browser_client.cc:1285: source_origin,
nit:I think this indentation is incorrect (should indent from left-paren + 4
spaces, not from the if statement). I could be wrong though (style guide seems
to be vague on this point).

http://codereview.chromium.org/9837074/diff/6030/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/9837074/diff/6030/chrome/browser/ui/browser.cc...
chrome/browser/ui/browser.cc:2358: target_url,
nit:Indent is incorrect - should be start-of-block+4 (aligned with 'e' in
'contents').

[Mihai Parparita -not on Chrome, 2012-03-28 23:00:45]

http://codereview.chromium.org/9837074/diff/6030/chrome/browser/chrome_conten...
File chrome/browser/chrome_content_browser_client.cc (right):

http://codereview.chromium.org/9837074/diff/6030/chrome/browser/chrome_conten...
chrome/browser/chrome_content_browser_client.cc:1285: source_origin,
On 2012/03/28 22:01:30, Andrew T Wilson wrote:
> nit:I think this indentation is incorrect (should indent from left-paren + 4
> spaces, not from the if statement). I could be wrong though (style guide seems
> to be vague on this point).

left-paren + 4 seems more readable to me, switched to that.

http://codereview.chromium.org/9837074/diff/6030/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/9837074/diff/6030/chrome/browser/ui/browser.cc...
chrome/browser/ui/browser.cc:2358: target_url,
On 2012/03/28 22:01:30, Andrew T Wilson wrote:
> nit:Indent is incorrect - should be start-of-block+4 (aligned with 'e' in
> 'contents').

Done.

[commit-bot: I haz the power, 2012-03-29 01:40:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mihaip@chromium.org/9837074/18001

[commit-bot: I haz the power, 2012-03-29 04:03:04]

Change committed as 129574