Content settings: allow scripts on interstitial pages.

Content settings: allow scripts on interstitial pages even if JavaScript is blocked.


BUG=104700
TEST=ChromeRenderViewTest.ContentSettingsInterstitialPages


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

[marja, 2011-12-02 11:11:59]

ptal

[jochen (gone - plz use gerrit), 2011-12-02 12:38:08]

http://codereview.chromium.org/8773035/diff/1/chrome/renderer/content_setting...
File chrome/renderer/content_settings_observer.cc (right):

http://codereview.chromium.org/8773035/diff/1/chrome/renderer/content_setting...
chrome/renderer/content_settings_observer.cc:132:
IPC_MESSAGE_HANDLER(ViewMsg_SetAsInterstitial, OnSetAsInterstitial)
I think the render view impl should handle the message, and invoke a callback in
render view observer.

The chrome render view observer would implement that callback and forward the
info to the content settings observer. Yay for APIs

http://codereview.chromium.org/8773035/diff/1/content/browser/tab_contents/in...
File content/browser/tab_contents/interstitial_page.cc (right):

http://codereview.chromium.org/8773035/diff/1/content/browser/tab_contents/in...
content/browser/tab_contents/interstitial_page.cc:206: render_view_host_->Send(
I think it would be nice to add a method to RenderViewHost like
set_is_interstitial_page() and have that method send the ipc msg

[marja, 2011-12-02 14:39:33]

http://codereview.chromium.org/8773035/diff/1/chrome/renderer/content_setting...
File chrome/renderer/content_settings_observer.cc (right):

http://codereview.chromium.org/8773035/diff/1/chrome/renderer/content_setting...
chrome/renderer/content_settings_observer.cc:132:
IPC_MESSAGE_HANDLER(ViewMsg_SetAsInterstitial, OnSetAsInterstitial)
On 2011/12/02 12:38:08, jochen wrote:
> I think the render view impl should handle the message, and invoke a callback
in
> render view observer.
> 
> The chrome render view observer would implement that callback and forward the
> info to the content settings observer. Yay for APIs

Done.

http://codereview.chromium.org/8773035/diff/1/content/browser/tab_contents/in...
File content/browser/tab_contents/interstitial_page.cc (right):

http://codereview.chromium.org/8773035/diff/1/content/browser/tab_contents/in...
content/browser/tab_contents/interstitial_page.cc:206: render_view_host_->Send(
On 2011/12/02 12:38:08, jochen wrote:
> I think it would be nice to add a method to RenderViewHost like
> set_is_interstitial_page() and have that method send the ipc msg

Not done; a note in RenderViewHost told not to do so.

[jochen (gone - plz use gerrit), 2011-12-02 14:42:18]

http://codereview.chromium.org/8773035/diff/7/chrome/renderer/chrome_render_v...
File chrome/renderer/chrome_render_view_observer.cc (right):

http://codereview.chromium.org/8773035/diff/7/chrome/renderer/chrome_render_v...
chrome/renderer/chrome_render_view_observer.cc:374:
content_settings_->AllowAllScripts();
why not also images? I'd go for something more generic.

http://codereview.chromium.org/8773035/diff/7/chrome/renderer/chrome_render_v...
File chrome/renderer/chrome_render_view_observer.h (right):

http://codereview.chromium.org/8773035/diff/7/chrome/renderer/chrome_render_v...
chrome/renderer/chrome_render_view_observer.h:115: 
why the empty line?

http://codereview.chromium.org/8773035/diff/7/content/public/renderer/render_...
File content/public/renderer/render_view_observer.h (right):

http://codereview.chromium.org/8773035/diff/7/content/public/renderer/render_...
content/public/renderer/render_view_observer.h:88: virtual void
SetAsInterstitial() {}
add a comment.

[marja, 2011-12-02 15:10:33]

Thanks for comments!

http://codereview.chromium.org/8773035/diff/7/chrome/renderer/chrome_render_v...
File chrome/renderer/chrome_render_view_observer.cc (right):

http://codereview.chromium.org/8773035/diff/7/chrome/renderer/chrome_render_v...
chrome/renderer/chrome_render_view_observer.cc:374:
content_settings_->AllowAllScripts();
On 2011/12/02 14:42:18, jochen wrote:
> why not also images? I'd go for something more generic.

Done.

http://codereview.chromium.org/8773035/diff/7/chrome/renderer/chrome_render_v...
File chrome/renderer/chrome_render_view_observer.h (right):

http://codereview.chromium.org/8773035/diff/7/chrome/renderer/chrome_render_v...
chrome/renderer/chrome_render_view_observer.h:115: 
On 2011/12/02 14:42:18, jochen wrote:
> why the empty line?

Done.

http://codereview.chromium.org/8773035/diff/7/content/public/renderer/render_...
File content/public/renderer/render_view_observer.h (right):

http://codereview.chromium.org/8773035/diff/7/content/public/renderer/render_...
content/public/renderer/render_view_observer.h:88: virtual void
SetAsInterstitial() {}
On 2011/12/02 14:42:18, jochen wrote:
> add a comment.

Done.

[jochen (gone - plz use gerrit), 2011-12-02 15:14:03]

lgtm

[marja, 2011-12-02 15:48:48]

jam, could you review the parts where you're an owner, i.e.,
content/common/*
content/public/*
content/renderer/*

Thanks!

[jam, 2011-12-02 17:13:40]

It's better to avoid adding methods to RenderViewObserver if we can, and also
the code in content which is really only done for a chrome feature (content
settings).

I think there's a way to do this without touching content: have content settings
install a TabContentsObserver and watch for DidStartLoading. then it can send a
chrome ipc directly to the content settings code in the renderer

http://codereview.chromium.org/8773035/diff/3017/content/renderer/render_view...
File content/renderer/render_view_impl.h (right):

http://codereview.chromium.org/8773035/diff/3017/content/renderer/render_view...
content/renderer/render_view_impl.h:853: void OnSetAsInterstitial();
nit: here and above, please don't put blank lines between message handler
declarations

[marja, 2011-12-05 14:05:19]

Hi & thanks for comments. I modified this CL so that it only touches chrome/. I
couldn't get it to work by hooking to DidStartLoading(), though, since at that
point the interstitial page is not yet set to RenderViewHostManager and
TabContents::showing_interstitial_page() returns false.

This version send the message in ChromeInterstitialPage::Show(). Does this look
ok?

[jochen (gone - plz use gerrit), 2011-12-06 23:11:50]

LGTM

[marja, 2011-12-08 07:53:02]

jam@, I'll commit this even though you haven't LGTMd, this doesn't any more
touch the directories where you're an OWNER.

[commit-bot: I haz the power, 2011-12-08 07:53:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/marja@chromium.org/8773035/9001

[commit-bot: I haz the power, 2011-12-08 08:52:56]

Change committed as 113579