alternate ntp: implement searchbox api for instant overlay to adopt themes

chrome/browser/ui/browser_instant_controller.cc

Index: chrome/browser/ui/browser_instant_controller.cc
diff --git a/chrome/browser/ui/browser_instant_controller.cc b/chrome/browser/ui/browser_instant_controller.cc
index 89ae408be8f9084d72c09ad2ae08b4e70596a89a..f7004eabbf1baba5e8cf9fa0787de548f8b0ed72 100644
--- a/chrome/browser/ui/browser_instant_controller.cc
+++ b/chrome/browser/ui/browser_instant_controller.cc
@@ -4,11 +4,16 @@
 
 #include "chrome/browser/ui/browser_instant_controller.h"
 
+#include "base/string_number_conversions.h"
+#include "base/stringprintf.h"
+#include "base/utf_string_conversions.h"
 #include "chrome/browser/browser_shutdown.h"
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/instant/instant_controller.h"
 #include "chrome/browser/prefs/pref_service.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/themes/theme_service.h"
+#include "chrome/browser/themes/theme_service_factory.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_tabstrip.h"
 #include "chrome/browser/ui/browser_window.h"
@@ -22,9 +27,19 @@
 #include "chrome/common/pref_names.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/web_contents.h"
+#include "grit/theme_resources.h"
+#include "ui/base/theme_provider.h"
+#include "ui/gfx/color_utils.h"
+#include "ui/gfx/sys_color_change_listener.h"
+
 
 namespace {
 
+const char kCSSBackgroundImageFormat[] =
+    "-webkit-image-set(url(chrome://theme/IDR_THEME_BACKGROUND?%s) 1x)";

[palmer, 2012/11/19 18:26:51]

String interpolation is generally a security danger-sign. See below for details.

[kuan, 2012/11/19 22:06:07]

Done.

+
+const char kCSSBackgroundColorFormat[] = "rgba(%d,%d,%d,%s)";
+
 // Returns true iff the search model for |tab| is in an NTP state, that is, a
 // state in which the Instant overlay may be showing custom NTP content in
 // EXTENDED mode.
@@ -45,12 +60,21 @@ namespace chrome {
 
 BrowserInstantController::BrowserInstantController(Browser* browser)
     : browser_(browser),
-      instant_unload_handler_(browser) {
+      instant_unload_handler_(browser),
+      theme_image_alignment_(0),
+      theme_area_height_(0) {
   profile_pref_registrar_.Init(browser_->profile()->GetPrefs());
   profile_pref_registrar_.Add(prefs::kInstantEnabled, this);
   ResetInstant();
   browser_->tab_strip_model()->AddObserver(this);
   browser_->search_model()->AddObserver(this);
+
+#if defined(ENABLE_THEMES)
+  // Listen for theme installation.
+  registrar_.Add(this, chrome::NOTIFICATION_BROWSER_THEME_CHANGED,
+                 content::Source<ThemeService>(
+                     ThemeServiceFactory::GetForProfile(browser_->profile())));
+#endif  // defined(ENABLE_THEMES)
 }
 
 BrowserInstantController::~BrowserInstantController() {
@@ -119,6 +143,21 @@ TabContents* BrowserInstantController::GetActiveTabContents() const {
   return chrome::GetActiveTabContents(browser_);
 }
 
+void BrowserInstantController::SetContentHeight(int height) {
+  OnThemeAreaHeightChanged(height);
+}
+
+void BrowserInstantController::UpdateThemeRelatedInfoForPreview() {
+  // Update theme background info and theme area height.
+  // |theme_info_.color_rgba| must always be filled, so if it's empty,
+  // |theme_info_| hasn't been initialized.
+  // |OnThemeChanged| also updates theme area height.
+  if (theme_info_.color_rgba.length() == 0)
+    OnThemeChanged(ThemeServiceFactory::GetForProfile(browser_->profile()));
+  else
+    OnThemeChanged(NULL);
+}
+
 ////////////////////////////////////////////////////////////////////////////////
 // BrowserInstantController, PrefObserver implementation:
 
@@ -156,13 +195,28 @@ void BrowserInstantController::TabStripEmpty() {
 
 void BrowserInstantController::ModeChanged(const search::Mode& old_mode,
                                            const search::Mode& new_mode) {
-  if (instant())
+  if (instant()) {
     instant_->OnActiveTabModeChanged(new_mode);
+
+    // If mode is now |NTP|, send theme-related information to instant.
+    if (new_mode.is_ntp())
+      OnThemeChanged(NULL);
+  }
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 // BrowserInstantController, private:
 
+void BrowserInstantController::Observe(
+    int type,
+    const content::NotificationSource& source,
+    const content::NotificationDetails& details) {
+#if defined(ENABLE_THEMES)
+  DCHECK_EQ(chrome::NOTIFICATION_BROWSER_THEME_CHANGED, type);
+  OnThemeChanged(content::Source<ThemeService>(source).ptr());
+#endif  // defined(ENABLE_THEMES)
+}
+
 void BrowserInstantController::ResetInstant() {
   instant_.reset(
       !browser_shutdown::ShuttingDownWithoutClosingBrowsers() &&
@@ -176,4 +230,99 @@ void BrowserInstantController::ResetInstant() {
       content::NotificationService::NoDetails());
 }
 
+void BrowserInstantController::OnThemeChanged(
+    ui::ThemeProvider* theme_provider) {
+  if (theme_provider) {  // Get theme information from theme provider.
+    theme_info_ = ThemeBackgroundInfo();
+    theme_image_alignment_ = 0;
+
+    // Set theme background color.
+    SkColor background_color =
+        theme_provider->GetColor(ThemeService::COLOR_NTP_BACKGROUND);
+    if (gfx::IsInvertedColorScheme())
+      background_color = color_utils::InvertColor(background_color);
+    theme_info_.color_rgba = UTF8ToUTF16(
+        // Convert the alpha using DoubleToString because StringPrintf will use
+        // locale specific formatters (e.g., use , instead of . in German).
+        base::StringPrintf(
+            kCSSBackgroundColorFormat,
+            SkColorGetR(background_color),
+            SkColorGetG(background_color),
+            SkColorGetB(background_color),
+            base::DoubleToString(SkColorGetA(background_color) / 255.0).

[palmer, 2012/11/19 18:26:51]

This is you creating an honest message, but generally we consider that IPCs
could come from any source, potentially malicious. CSS injection could happen to
receiver of a message like this.

[kuan, 2012/11/19 22:06:07]

Done.

+                c_str()));
+
+    if (theme_provider->HasCustomImage(IDR_THEME_NTP_BACKGROUND)) {
+      // Set theme background image url.
+      theme_info_.image_url = UTF8ToUTF16(
+          base::StringPrintf(kCSSBackgroundImageFormat,

[palmer, 2012/11/19 18:26:51]

For example:

-webkit-image-set(url(chrome://theme/IDR_THEME_BACKGROUND?abc123)
1x);evil-css-here(url:javascript:blah
blah);-webkit-image-set(url(chrome://theme/IDR_THEME_BACKGROUND?abc123) 1x)

I.e. the injection string would be "abc123)
1x);evil-css-here(url:javascript:blah
blah);-webkit-image-set(url(chrome://theme/IDR_THEME_BACKGROUND?abc123) ".

(I don't know if that is the best way to neutralize the trailing "1x)" that
kCSSBackgroundImageFormat specifies, but hopefully you see the point.)

Getting JavaScript execution from CSS injection is harder than regular XSS, but
it has been possible. Generally, we treat CSS as code and don't like to
interpolate inputs from untrustworthy sources into CSS.

[kuan, 2012/11/19 22:06:07]

Done.

+                             browser_->profile()->GetPrefs()->GetString(
+                                 prefs::kCurrentThemeID).c_str()));
+
+      // Set theme background image horizontal alignment.
+      theme_provider->GetDisplayProperty(
+          ThemeService::NTP_BACKGROUND_ALIGNMENT, &theme_image_alignment_);
+      std::string alignment;
+      if (theme_image_alignment_ & ThemeService::ALIGN_LEFT)
+        alignment = ThemeService::kAlignmentLeft;
+      else if (theme_image_alignment_& ThemeService::ALIGN_RIGHT)
+        alignment = ThemeService::kAlignmentRight;
+      else  // ALIGN_CENTER
+        alignment = ThemeService::kAlignmentCenter;
+      theme_info_.image_horizontal_alignment = UTF8ToUTF16(alignment);

[palmer, 2012/11/19 18:26:51]

These conversions are another efficiency reason to prefer primitive types when
packing and unpacking these structs and IPCs.

[kuan, 2012/11/19 22:06:07]

Done.

+
+      // Set theme background image vertical alignment.
+      if (theme_image_alignment_& ThemeService::ALIGN_TOP)

[palmer, 2012/11/19 18:26:51]

Oh, you've already got enums for this. So ignore my example enums, and use these
in the IPC messages, packed by the sender here:

if (theme_image_alignment_& ThemeService::ALIGN_TOP)
    theme_info_.image_vertical_alignment = ThemeService::ALIGN_TOP;
else if ( /* bottom */)
    // ...
else
    // CENTER...

(I.e. this packing code would become simpler and smaller)

...and unpacked and interpreted by the receiver:

switch (theme_background_info.image_vertical_alignment) {
    case ThemeService::ALIGN_BOTTOM:
        alignment = ThemeService::kAlignmentBottom;
    //...
    default:
        // rather than explode, use ALIGN_CENTER as default
}

It is now safe to interpolate the static strings ThemeService::kAlignment* into
your CSS, because the attacker cannot influence them. We selected which one to
use from a simple switch statement with a reasonable default. We did not use an
arbitrary string from across the IPC boundary.

[kuan, 2012/11/19 22:06:07]

i did as per ur suggestion.  it works, EXCEPT when i try to upload the cl, i get
a presubmit error:
Illegal include: "chrome/browser/themes/theme_service.h"
Because of "-chrome" from chrome's include_rules.

i've a feeling i can't include chrome/browser files in chrome/renderer files??
does this i mean i have to repeat the definitions of ThemeService enum values
for IPC?

On 2012/11/19 18:26:51, Chris P. wrote:

+        alignment = ThemeService::kAlignmentTop;
+      else if (theme_image_alignment_& ThemeService::ALIGN_BOTTOM)
+        alignment = ThemeService::kAlignmentBottom;
+      else  // ALIGN_CENTER
+        alignment = ThemeService::kAlignmentCenter;
+      theme_info_.image_vertical_alignment = UTF8ToUTF16(alignment);
+
+      // Set tiling of theme background image.
+      int repeat_mode = 0;
+      theme_provider->GetDisplayProperty(
+          ThemeService::NTP_BACKGROUND_TILING, &repeat_mode);
+      theme_info_.image_tiling = UTF8ToUTF16(
+          ThemeService::TilingToString(repeat_mode));
+
+      // Set theme background image height.
+      gfx::ImageSkia* image = theme_provider->GetImageSkiaNamed(
+          IDR_THEME_NTP_BACKGROUND);
+      DCHECK(image);
+      theme_info_.image_height = image->height();
+    }
+  }
+
+  if (instant() && browser_->search_model()->mode().is_ntp()) {
+    instant()->OnThemeChanged(theme_info_);

[sreeram, 2012/11/19 18:12:02]

Nit: instant() -> instant_ (it works on both lines above).

[kuan, 2012/11/19 22:06:07]

Done.

+
+    // Theme area height is only sent to preview for non-top-aligned images;
+    // new theme may have a different alignment that requires preview to know
+    // theme area height.
+    OnThemeAreaHeightChanged(theme_area_height_);
+  }
+}
+
+void BrowserInstantController::OnThemeAreaHeightChanged(int height) {
+  theme_area_height_ = height;
+
+  // Notify preview if mode is |NTP| and theme background image is not top-
+  // aligned; top-aligned images don't need theme area height to determine which
+  // part of the image overlay should draw, 'cos the origin is top-left.
+  bool notify = instant() && browser_->search_model()->mode().is_ntp() &&
+      theme_info_.image_url.length() > 0;
+  if (!notify)
+    return;
+  if (theme_image_alignment_ & ThemeService::ALIGN_TOP)
+    notify = false;
+  else if (theme_image_alignment_ & ThemeService::ALIGN_BOTTOM)
+    notify = true;
+  else // ALIGN_CENTER
+    notify = true;
+  if (notify)
+    instant()->OnThemeAreaHeightChanged(theme_area_height_);

[sreeram, 2012/11/19 18:12:02]

Why bother with the notify variable? Why not just this:

theme_area_height_ = height;
if (!instant_ || browser_->... || theme_info_.image_url... ||
theme_image_alignment & ThemeService::ALIGN_TOP)
  return;

instant_->OnThemeAreaHeightChanged(theme_area_height_);

[kuan, 2012/11/19 22:06:07]

Done.

+}
+
 }  // namespace chrome