Move the content settings code out of RenderView, since it belongs in the Chrome layer.

Move the content settings code out of RenderView, since it belongs in the Chrome layer.

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

[pastarmovj, 2011-04-18 16:09:06]

On 2011/04/18 01:44:48, John Abd-El-Malek wrote:

LGTM, just a few minor comments from me.

[pastarmovj, 2011-04-18 16:09:23]

and the comments :)

http://codereview.chromium.org/6873040/diff/6001/chrome/renderer/content_sett...
File chrome/renderer/content_settings_observer.cc (right):

http://codereview.chromium.org/6873040/diff/6001/chrome/renderer/content_sett...
chrome/renderer/content_settings_observer.cc:119: if (WebView* opener_view =
frame->opener()->view()) {
Do we encourage putting assignments in conditional statements in the coding
style?

http://codereview.chromium.org/6873040/diff/6001/chrome/renderer/content_sett...
File chrome/renderer/content_settings_observer.h (right):

http://codereview.chromium.org/6873040/diff/6001/chrome/renderer/content_sett...
chrome/renderer/content_settings_observer.h:55: // Helper method that returns if
the user wants to block content of type
nit: Indentation

http://codereview.chromium.org/6873040/diff/6001/content/renderer/render_view.cc
File content/renderer/render_view.cc (right):

http://codereview.chromium.org/6873040/diff/6001/content/renderer/render_view...
content/renderer/render_view.cc:1064: // Top-level navigation.
Do you want to preserve one empty line here.

[jam, 2011-04-18 16:50:10]

http://codereview.chromium.org/6873040/diff/6001/chrome/renderer/content_sett...
File chrome/renderer/content_settings_observer.cc (right):

http://codereview.chromium.org/6873040/diff/6001/chrome/renderer/content_sett...
chrome/renderer/content_settings_observer.cc:119: if (WebView* opener_view =
frame->opener()->view()) {
On 2011/04/18 16:09:23, pastarmovj wrote:
> Do we encourage putting assignments in conditional statements in the coding
> style?

I don't think it's against the style guide, but either way, this is code that's
being moved around.

http://codereview.chromium.org/6873040/diff/6001/chrome/renderer/content_sett...
File chrome/renderer/content_settings_observer.h (right):

http://codereview.chromium.org/6873040/diff/6001/chrome/renderer/content_sett...
chrome/renderer/content_settings_observer.h:55: // Helper method that returns if
the user wants to block content of type
On 2011/04/18 16:09:23, pastarmovj wrote:
> nit: Indentation

Done.

http://codereview.chromium.org/6873040/diff/6001/content/renderer/render_view.cc
File content/renderer/render_view.cc (right):

http://codereview.chromium.org/6873040/diff/6001/content/renderer/render_view...
content/renderer/render_view.cc:1064: // Top-level navigation.
On 2011/04/18 16:09:23, pastarmovj wrote:
> Do you want to preserve one empty line here.

Done.

[Bernhard Bauer, 2011-04-18 21:37:24]

http://codereview.chromium.org/6873040/diff/6001/chrome/renderer/content_sett...
File chrome/renderer/content_settings_observer.cc (right):

http://codereview.chromium.org/6873040/diff/6001/chrome/renderer/content_sett...
chrome/renderer/content_settings_observer.cc:119: if (WebView* opener_view =
frame->opener()->view()) {
On 2011/04/18 16:50:10, John Abd-El-Malek wrote:
> On 2011/04/18 16:09:23, pastarmovj wrote:
> > Do we encourage putting assignments in conditional statements in the coding
> > style?
> 
> I don't think it's against the style guide, but either way, this is code
that's
> being moved around.

I think these days most compilers are smart enough to warn when it's ambiguous,
but here it's not, because |opener_view| is declared here, so an == comparison
wouldn't make sense.

http://codereview.chromium.org/6873040/diff/10001/chrome/renderer/content_set...
chrome/renderer/content_settings_observer.cc:178: CONTENT_SETTING_BLOCK;
Nit: Indent two more spaces.

(But as you've already committed it, and it's not worth a followup IMO, I'll
just preserve this nit for eternity here ;-)

[jam, 2011-04-18 22:19:12]

http://codereview.chromium.org/6873040/diff/10001/chrome/renderer/content_set...
File chrome/renderer/content_settings_observer.cc (right):

http://codereview.chromium.org/6873040/diff/10001/chrome/renderer/content_set...
chrome/renderer/content_settings_observer.cc:178: CONTENT_SETTING_BLOCK;
On 2011/04/18 21:37:25, Bernhard Bauer wrote:
> Nit: Indent two more spaces.
> 
> (But as you've already committed it, and it's not worth a followup IMO, I'll
> just preserve this nit for eternity here ;-)

oops, I'll fix this in a followup, thanks