Remove SandboxedPages and SandboxedCSP from Extension Class

Remove SandboxedPages and SandboxedCSP from Extension Class

BUG=159265
TBR=ben@chromium.org for gypis, chrome_content_renderer_client

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=190865

[Devlin, 2013-03-20 17:56:33]

Sorry for the delay on these; hoping to be caught up soon.

[Yoyo Zhou, 2013-03-20 22:53:52]

Please adjust the CL description so it's clearer that the CSP is related to just
the sandboxed pages. Also, you don't need :: in the name of a class.

https://codereview.chromium.org/12886023/diff/1/chrome/browser/extensions/ext...
File chrome/browser/extensions/extension_process_manager.cc (right):

https://codereview.chromium.org/12886023/diff/1/chrome/browser/extensions/ext...
chrome/browser/extensions/extension_process_manager.cc:185: (new
extensions::SandboxedHandler)->Register();
Does this really belong in ExtensionProcessManager (which some have argued isn't
even a good place for BackgroundManifestHandler)? It seems out of place here.
Wherever the other CSP-related handlers are registered seems more appropriate.

https://codereview.chromium.org/12886023/diff/1/chrome/browser/extensions/san...
File chrome/browser/extensions/sandboxed_pages_apitest.cc (right):

https://codereview.chromium.org/12886023/diff/1/chrome/browser/extensions/san...
chrome/browser/extensions/sandboxed_pages_apitest.cc:15: (new
SandboxedHandler)->Register();
I don't think we need these in browser tests.

https://codereview.chromium.org/12886023/diff/1/chrome/common/extensions/sand...
File chrome/common/extensions/sandboxed_handler.cc (right):

https://codereview.chromium.org/12886023/diff/1/chrome/common/extensions/sand...
chrome/common/extensions/sandboxed_handler.cc:64: const std::vector<std::string>
SandboxedHandler::Keys() const {
Define these in the same order as the header file.

https://codereview.chromium.org/12886023/diff/1/chrome/common/extensions/sand...
File chrome/common/extensions/sandboxed_handler.h (right):

https://codereview.chromium.org/12886023/diff/1/chrome/common/extensions/sand...
chrome/common/extensions/sandboxed_handler.h:16: struct SandboxedInfo : public
Extension::ManifestData {
I'd prefer a name like SandboxedPageInfo.

Since most users of this are going to refer to the Info rather than the Handler,
how about renaming this file to sandboxed_page_info?

https://codereview.chromium.org/12886023/diff/1/chrome/common/extensions/sand...
chrome/common/extensions/sandboxed_handler.h:29: // origin).
either "...with a different CSP" or delete the parenthetical

[Devlin, 2013-03-23 22:26:24]

https://codereview.chromium.org/12886023/diff/1/chrome/browser/extensions/ext...
File chrome/browser/extensions/extension_process_manager.cc (right):

https://codereview.chromium.org/12886023/diff/1/chrome/browser/extensions/ext...
chrome/browser/extensions/extension_process_manager.cc:185: (new
extensions::SandboxedHandler)->Register();
On 2013/03/20 22:53:53, Yoyo Zhou wrote:
> Does this really belong in ExtensionProcessManager (which some have argued
isn't
> even a good place for BackgroundManifestHandler)? It seems out of place here.
> Wherever the other CSP-related handlers are registered seems more appropriate.

Yeah, I wasn't really sure where to put this one... it's with the other CSP ones
now.

https://codereview.chromium.org/12886023/diff/1/chrome/browser/extensions/san...
File chrome/browser/extensions/sandboxed_pages_apitest.cc (right):

https://codereview.chromium.org/12886023/diff/1/chrome/browser/extensions/san...
chrome/browser/extensions/sandboxed_pages_apitest.cc:15: (new
SandboxedHandler)->Register();
On 2013/03/20 22:53:53, Yoyo Zhou wrote:
> I don't think we need these in browser tests.

Done.

https://codereview.chromium.org/12886023/diff/1/chrome/common/extensions/sand...
File chrome/common/extensions/sandboxed_handler.cc (right):

https://codereview.chromium.org/12886023/diff/1/chrome/common/extensions/sand...
chrome/common/extensions/sandboxed_handler.cc:64: const std::vector<std::string>
SandboxedHandler::Keys() const {
On 2013/03/20 22:53:53, Yoyo Zhou wrote:
> Define these in the same order as the header file.

Done.

https://codereview.chromium.org/12886023/diff/1/chrome/common/extensions/sand...
File chrome/common/extensions/sandboxed_handler.h (right):

https://codereview.chromium.org/12886023/diff/1/chrome/common/extensions/sand...
chrome/common/extensions/sandboxed_handler.h:16: struct SandboxedInfo : public
Extension::ManifestData {
On 2013/03/20 22:53:53, Yoyo Zhou wrote:
> I'd prefer a name like SandboxedPageInfo.
> 
> Since most users of this are going to refer to the Info rather than the
Handler,
> how about renaming this file to sandboxed_page_info?

Done.

https://codereview.chromium.org/12886023/diff/1/chrome/common/extensions/sand...
chrome/common/extensions/sandboxed_handler.h:29: // origin).
On 2013/03/20 22:53:53, Yoyo Zhou wrote:
> either "...with a different CSP" or delete the parenthetical

Done.

[Yoyo Zhou, 2013-03-25 17:16:01]

LGTM

https://chromiumcodereview.appspot.com/12886023/diff/8001/chrome/common/exten...
File chrome/common/extensions/extension.cc (right):

https://chromiumcodereview.appspot.com/12886023/diff/8001/chrome/common/exten...
chrome/common/extensions/extension.cc:368: std::string
Extension::GetResourceContentSecurityPolicy(
Can this function be moved to CSPInfo? I think it could even make sense to
combine the CSP and sandboxed page manifest handlers, but you don't have to do
that in this CL.

[Devlin, 2013-03-26 23:00:15]

https://chromiumcodereview.appspot.com/12886023/diff/8001/chrome/common/exten...
File chrome/common/extensions/extension.cc (right):

https://chromiumcodereview.appspot.com/12886023/diff/8001/chrome/common/exten...
chrome/common/extensions/extension.cc:368: std::string
Extension::GetResourceContentSecurityPolicy(
On 2013/03/25 17:16:01, Yoyo Zhou wrote:
> Can this function be moved to CSPInfo? I think it could even make sense to
> combine the CSP and sandboxed page manifest handlers, but you don't have to do
> that in this CL.

Done.

[commit-bot: I haz the power, 2013-03-26 23:01:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rdevlin.cronin@chromium.org/12886023/1...

[commit-bot: I haz the power, 2013-03-27 00:02:08]

Retried try job too often on linux_rel for step(s) content_browsertests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-03-27 04:50:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rdevlin.cronin@chromium.org/12886023/1...

[commit-bot: I haz the power, 2013-03-27 08:44:44]

Change committed as 190865