Delegating the "are images allowed" decision to renderer.

chrome/renderer/content_settings_observer.cc

Index: chrome/renderer/content_settings_observer.cc
diff --git a/chrome/renderer/content_settings_observer.cc b/chrome/renderer/content_settings_observer.cc
index 94150e65b4ff6968ee426bb3a351b9eb13b55024..a4c47b534ab4a8939479d226bee9b6cf51be159d 100644
--- a/chrome/renderer/content_settings_observer.cc
+++ b/chrome/renderer/content_settings_observer.cc
@@ -75,6 +75,11 @@ void ContentSettingsObserver::SetDefaultContentSettings(
   default_settings_ = settings;
 }
 
+void ContentSettingsObserver::SetImageSettingRules(
+    const ContentSettingsForOneType* image_setting_rules) {
+  image_setting_rules_ = image_setting_rules;
+}
+
 ContentSetting ContentSettingsObserver::GetContentSetting(
     ContentSettingsType type) {
   // Don't call this for plug-ins.
@@ -118,7 +123,7 @@ void ContentSettingsObserver::DidCommitProvisionalLoad(
   NavigationState* state = NavigationState::FromDataSource(frame->dataSource());
   if (!state->was_within_same_page()) {
     // Clear "block" flags for the new page. This needs to happen before any of
-    // allowScripts(), allowImages(), allowPlugins() is called for the new page
+    // allowScripts(), allowImage(), allowPlugins() is called for the new page
     // so that these functions can correctly detect that a piece of content
     // flipped from "not blocked" to "blocked".
     ClearBlockedContentSettings();
@@ -198,16 +203,30 @@ bool ContentSettingsObserver::AllowFileSystem(WebFrame* frame) {
 bool ContentSettingsObserver::AllowImage(WebFrame* frame,
                                          bool enabled_per_settings,
                                          const WebURL& image_url) {
-  if (enabled_per_settings &&
-      AllowContentType(CONTENT_SETTINGS_TYPE_IMAGES)) {
-    return true;
-  }
-
   if (IsWhitelistedForContentSettings(frame))
     return true;
 
-  DidBlockContentType(CONTENT_SETTINGS_TYPE_IMAGES, std::string());
-  return false;  // Other protocols fall through here.
+  bool allow = false;

[Bernhard Bauer, 2011/10/20 09:21:53]

I think we may want to default to allow?

[marja, 2011/10/20 11:44:22]

Shouldn't we default to enabled_per_settings instead?

[Bernhard Bauer, 2011/10/20 11:58:24]

OK (although in practice |enabled_per_settings| is going to be true unless
someone turns off images for the whole browser, and I'm not even sure that's
possible).

[marja, 2011/10/20 14:41:15]

Done.

+  GURL top_url(frame->top()->document().securityOrigin().toString());

[Bernhard Bauer, 2011/10/20 09:21:53]

Argh, I just noticed that per http://crbug.com/100353 we return an empty
security origin for file: URLs. I think that would mean that all images would be
blocked on file: pages? :-/

Could you manually check that? If they are indeed blocked, we should use
|document.url()| instead and I'm gonna yell at Jochen.

[marja, 2011/10/20 11:44:22]

There is the gotcha that "a unique security origins is not the same as an empty
security origin". So even if
GURL(frame->top()->document().securityOrigin().toString()) is "" (it's unique),
frame->top()->document().securityOrigin().isEmpty() is false.

I also checked manually, and the images get displayed correctly for a document
loaded by the URL file:///somewhere.

It's a bit ugly though that frame->top()->document().securityOrigin().toString()
is "null" in that case, and GURL("null") gets converted to GURL(""). So, this
only works by a coincidence.

Unluckily, WebSecurityOrigin doesn't expose isUnique(). 

I changed this so that it handles the "null" case separately. Does it look less
ugly?

[Bernhard Bauer, 2011/10/20 11:58:24]

Plus, it means that file:/// content setting patterns aren't going to apply.

I still think that using |document.url()| is the right thing to do here, as it
preserves the most information. We might want to sync with Jochen though.

[marja, 2011/10/20 14:41:15]

I changed this to be frame->document().url() and pinged Jochen.

[jochen (gone - plz use gerrit), 2011/10/20 15:32:15]

I'd prefer if we'd stick to the security origin. Otherwise, a page could open an
about:blank frame and then display images in it.

What about falling back to document.url() if the security origin is empty  and
document.url is a file url?

[Bernhard Bauer, 2011/10/20 15:37:09]

How would that work if we use the main frame URL?

[jochen (gone - plz use gerrit), 2011/10/20 19:01:21]

sorry for being unclear, I meant opening the frame in a new window.

[marja, 2011/10/21 09:52:58]

I uploaded a version which falls back to document.url() if 
frame->top()->document().securityOrigin() is unique (*). Bernhard, WDYT?

(*) It won't be empty, only unique, but I guess you meant that.

+  GURL image_gurl(image_url);
+  if (image_setting_rules_ &&
+      !frame->document().securityOrigin().isEmpty() &&
+      !frame->top()->document().securityOrigin().isEmpty() &&
+      enabled_per_settings) {
+    ContentSettingsForOneType::const_iterator it;
+    for (it = image_setting_rules_->begin();
+         it != image_setting_rules_->end(); ++it) {
+      if (it->primary_pattern.Matches(top_url) &&
+          it->secondary_pattern.Matches(image_gurl)) {
+        allow = (it->setting != CONTENT_SETTING_BLOCK);
+        break;
+      }
+    }
+  }
+
+  if (!allow)
+    DidBlockContentType(CONTENT_SETTINGS_TYPE_IMAGES, std::string());
+  return allow;
 }
 
 bool ContentSettingsObserver::AllowIndexedDB(WebFrame* frame,