Create a content public browser API around the ChildProcessSecurityPolicy class. The implementati...

Create a content public browser API around the ChildProcessSecurityPolicy class. The implementation of this
interface lives in content\browser\child_process_security_policy_impl.cc/.h.

Moved some security checks from the TabContentsDelegate implementation (chrome\browser) to the TabContents
code in content.

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

[jam, 2012-02-08 18:51:12]

lgtm, thanks! please also update chrome\browser\DEPS

http://codereview.chromium.org/9360014/diff/5005/chrome_frame/urlmon_url_requ...
File chrome_frame/urlmon_url_request.cc (right):

http://codereview.chromium.org/9360014/diff/5005/chrome_frame/urlmon_url_requ...
chrome_frame/urlmon_url_request.cc:308: if (previously_redirected.is_valid() &&
i think this change slipped into this cl by accident

http://codereview.chromium.org/9360014/diff/5005/content/browser/child_proces...
File content/browser/child_process_security_policy_impl.h (right):

http://codereview.chromium.org/9360014/diff/5005/content/browser/child_proces...
content/browser/child_process_security_policy_impl.h:5: #ifndef
CONTENT_BROWSER_CHILD_PROCESS_SECURITY_POLICY_H_
nit: update

http://codereview.chromium.org/9360014/diff/5005/content/browser/child_proces...
content/browser/child_process_security_policy_impl.h:14: #include
"base/basictypes.h"
nit: this and content_export, not needed since they're in the interface's header

http://codereview.chromium.org/9360014/diff/5005/content/browser/child_proces...
content/browser/child_process_security_policy_impl.h:29:
~ChildProcessSecurityPolicyImpl();
nit: virtual by convention?

http://codereview.chromium.org/9360014/diff/5005/content/browser/child_proces...
content/browser/child_process_security_policy_impl.h:33: virtual void
RegisterWebSafeScheme(const std::string& scheme) OVERRIDE;
nit: can you say "ChildProcessHost implementation:"

http://codereview.chromium.org/9360014/diff/5005/content/browser/tab_contents...
File content/browser/tab_contents/navigation_controller_impl.cc (right):

http://codereview.chromium.org/9360014/diff/5005/content/browser/tab_contents...
content/browser/tab_contents/navigation_controller_impl.cc:333:
ChildProcessSecurityPolicyImpl *policy =
nit: while you're touching this, "* "

http://codereview.chromium.org/9360014/diff/5005/content/browser/tab_contents...
File content/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/9360014/diff/5005/content/browser/tab_contents...
content/browser/tab_contents/tab_contents.cc:1576: if
(policy->CanReadDirectory(GetRenderProcessHost()->GetID(), path)) {
nit: rest of file doesn't use brace brackets for one line statements

http://codereview.chromium.org/9360014/diff/5005/content/public/browser/child...
File content/public/browser/child_process_security_policy.h (right):

http://codereview.chromium.org/9360014/diff/5005/content/public/browser/child...
content/public/browser/child_process_security_policy.h:9: #include <set>
nit: not needed

http://codereview.chromium.org/9360014/diff/5005/content/public/browser/child...
content/public/browser/child_process_security_policy.h:26: class CONTENT_EXPORT
ChildProcessSecurityPolicy {
nit: don't need CONTENT_EXPORT on the interface, just the static method (you'll
probably need to export the Impl class for unittests though, in which case
you'll need to use NON_EXPORTED_BASE in VS

http://codereview.chromium.org/9360014/diff/5005/content/public/browser/child...
content/public/browser/child_process_security_policy.h:67:
ChildProcessSecurityPolicy() {}
nit: this isn't needed, since one can't create an interface

http://codereview.chromium.org/9360014/diff/5005/content/renderer/render_view...
File content/renderer/render_view_impl.cc (left):

http://codereview.chromium.org/9360014/diff/5005/content/renderer/render_view...
content/renderer/render_view_impl.cc:11: 
nit: why remove this? i think this is right per the style guide

[ananta, 2012-02-08 19:36:41]

https://chromiumcodereview.appspot.com/9360014/diff/5005/content/browser/chil...
File content/browser/child_process_security_policy_impl.h (right):

https://chromiumcodereview.appspot.com/9360014/diff/5005/content/browser/chil...
content/browser/child_process_security_policy_impl.h:5: #ifndef
CONTENT_BROWSER_CHILD_PROCESS_SECURITY_POLICY_H_
On 2012/02/08 18:51:12, John Abd-El-Malek wrote:
> nit: update

Done.

https://chromiumcodereview.appspot.com/9360014/diff/5005/content/browser/chil...
content/browser/child_process_security_policy_impl.h:14: #include
"base/basictypes.h"
On 2012/02/08 18:51:12, John Abd-El-Malek wrote:
> nit: this and content_export, not needed since they're in the interface's
header

Done.

https://chromiumcodereview.appspot.com/9360014/diff/5005/content/browser/chil...
content/browser/child_process_security_policy_impl.h:29:
~ChildProcessSecurityPolicyImpl();
On 2012/02/08 18:51:12, John Abd-El-Malek wrote:
> nit: virtual by convention?

Done.

https://chromiumcodereview.appspot.com/9360014/diff/5005/content/browser/chil...
content/browser/child_process_security_policy_impl.h:33: virtual void
RegisterWebSafeScheme(const std::string& scheme) OVERRIDE;
On 2012/02/08 18:51:12, John Abd-El-Malek wrote:
> nit: can you say "ChildProcessHost implementation:"

Done.

https://chromiumcodereview.appspot.com/9360014/diff/5005/content/browser/tab_...
File content/browser/tab_contents/navigation_controller_impl.cc (right):

https://chromiumcodereview.appspot.com/9360014/diff/5005/content/browser/tab_...
content/browser/tab_contents/navigation_controller_impl.cc:333:
ChildProcessSecurityPolicyImpl *policy =
On 2012/02/08 18:51:12, John Abd-El-Malek wrote:
> nit: while you're touching this, "* "

Done.

https://chromiumcodereview.appspot.com/9360014/diff/5005/content/browser/tab_...
File content/browser/tab_contents/tab_contents.cc (right):

https://chromiumcodereview.appspot.com/9360014/diff/5005/content/browser/tab_...
content/browser/tab_contents/tab_contents.cc:1576: if
(policy->CanReadDirectory(GetRenderProcessHost()->GetID(), path)) {
On 2012/02/08 18:51:12, John Abd-El-Malek wrote:
> nit: rest of file doesn't use brace brackets for one line statements

Done.

https://chromiumcodereview.appspot.com/9360014/diff/5005/content/public/brows...
File content/public/browser/child_process_security_policy.h (right):

https://chromiumcodereview.appspot.com/9360014/diff/5005/content/public/brows...
content/public/browser/child_process_security_policy.h:9: #include <set>
On 2012/02/08 18:51:12, John Abd-El-Malek wrote:
> nit: not needed
I added that to address a lint warning. We use set below in the
RegisterDisabledScheme method.

https://chromiumcodereview.appspot.com/9360014/diff/5005/content/public/brows...
content/public/browser/child_process_security_policy.h:26: class CONTENT_EXPORT
ChildProcessSecurityPolicy {
On 2012/02/08 18:51:12, John Abd-El-Malek wrote:
> nit: don't need CONTENT_EXPORT on the interface, just the static method
(you'll
> probably need to export the Impl class for unittests though, in which case
> you'll need to use NON_EXPORTED_BASE in VS

Done.

https://chromiumcodereview.appspot.com/9360014/diff/5005/content/public/brows...
content/public/browser/child_process_security_policy.h:67:
ChildProcessSecurityPolicy() {}
On 2012/02/08 18:51:12, John Abd-El-Malek wrote:
> nit: this isn't needed, since one can't create an interface

Done.

https://chromiumcodereview.appspot.com/9360014/diff/5005/content/renderer/ren...
File content/renderer/render_view_impl.cc (left):

https://chromiumcodereview.appspot.com/9360014/diff/5005/content/renderer/ren...
content/renderer/render_view_impl.cc:11: 
On 2012/02/08 18:51:12, John Abd-El-Malek wrote:
> nit: why remove this? i think this is right per the style guide
This file made it through by mistake.

[jam, 2012-02-08 19:40:14]

https://chromiumcodereview.appspot.com/9360014/diff/5005/content/public/brows...
File content/public/browser/child_process_security_policy.h (right):

https://chromiumcodereview.appspot.com/9360014/diff/5005/content/public/brows...
content/public/browser/child_process_security_policy.h:9: #include <set>
On 2012/02/08 19:36:41, ananta wrote:
> On 2012/02/08 18:51:12, John Abd-El-Malek wrote:
> > nit: not needed
> I added that to address a lint warning. We use set below in the
> RegisterDisabledScheme method.
> 

oops, i must have searched wrong, nvm