Allow slightly larger browser and page action icons.

This allows 19px icons to be able to be used for both
browser and page icons.

I think it looks nicer with the page actions slightly smaller, and that is what we also usually do in Chrome,
but some Chrome location bar icons use 18px for soft edges,
so I guess this will just have to be something we advise
developers on.

We can actually fit up to 21 (whoa nelly) pixels on Windows,
but apparently the space is slightly smaller on mac.

Also minor layout fix. We were sizing the browser action
buttons 1px too short.

BUG=24881
TEST=Load chrome/test/data/extensions/samples/icon_size_test. Icons should be 17px for the page action and 19px for the browser action and centered nicely in the space.

Review URL: http://codereview.chromium.org/286001

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@29365 0039d316-1c4b-4281-b951-d872f2087c98

10 files changed, 70 insertions, 31 deletions

diff --git a/chrome/browser/extensions/image_loading_tracker.cc b/chrome/browser/extensions/image_loading_tracker.cc
index a2e4281..10bbea3 100644
--- a/chrome/browser/extensions/image_loading_tracker.cc
+++ b/chrome/browser/extensions/image_loading_tracker.cc
@@ -4,8 +4,8 @@
 
 #include "chrome/browser/extensions/image_loading_tracker.h"
 
-#include "app/gfx/favicon_size.h"
 #include "base/file_util.h"
+#include "base/gfx/size.h"
 #include "base/logging.h"
 #include "base/message_loop.h"
 #include "base/scoped_ptr.h"
@@ -27,14 +27,18 @@ class ImageLoadingTracker::LoadImageTask : public Task {
  public:
   // Constructor for the LoadImageTask class. |tracker| is the object that
   // we use to communicate back to the entity that wants the image after we
-  // decode it. |path| is the path to load the image from. |index| is an
-  // identifier for the image that we pass back to the caller.
+  // decode it. |path| is the path to load the image from. |max_size| is the
+  // maximum size for the loaded image. It will be resized to fit this if
+  // larger. |index| is an identifier for the image that we pass back to the
+  // caller.
   LoadImageTask(ImageLoadingTracker* tracker,
                 const ExtensionResource& resource,
+                const gfx::Size& max_size,
                 size_t index)
     : callback_loop_(MessageLoop::current()),
       tracker_(tracker),
       resource_(resource),
+      max_size_(max_size),
       index_(index) {}
 
   void ReportBack(SkBitmap* image) {
@@ -56,7 +60,7 @@ class ImageLoadingTracker::LoadImageTask : public Task {
     // Decode the image using WebKit's image decoder.
     const unsigned char* data =
         reinterpret_cast<const unsigned char*>(file_contents.data());
-    webkit_glue::ImageDecoder decoder(gfx::Size(kFavIconSize, kFavIconSize));
+    webkit_glue::ImageDecoder decoder;
     scoped_ptr<SkBitmap> decoded(new SkBitmap());
     *decoded = decoder.Decode(data, file_contents.length());
     if (decoded->empty()) {
@@ -64,15 +68,12 @@ class ImageLoadingTracker::LoadImageTask : public Task {
       return;  // Unable to decode.
     }
 
-    if (decoded->width() != kFavIconSize || decoded->height() != kFavIconSize) {
-      // The bitmap is not the correct size, re-sample.
-      int new_width = decoded->width();
-      int new_height = decoded->height();
-      // Calculate what dimensions to use within the constraints (16x16 max).
-      calc_favicon_target_size(&new_width, &new_height);
+    if (decoded->width() > max_size_.width() ||
+        decoded->height() > max_size_.height()) {
+      // The bitmap is too big, re-sample.
       *decoded = skia::ImageOperations::Resize(
           *decoded, skia::ImageOperations::RESIZE_LANCZOS3,
-          new_width, new_height);
+          max_size_.width(), max_size_.height());
     }
 
     ReportBack(decoded.release());
@@ -88,6 +89,9 @@ class ImageLoadingTracker::LoadImageTask : public Task {
   // The image resource to load asynchronously.
   ExtensionResource resource_;
 
+  // The max size for the loaded image.
+  gfx::Size max_size_;
+
   // The index of the icon being loaded.
   size_t index_;
 };
@@ -95,9 +99,10 @@ class ImageLoadingTracker::LoadImageTask : public Task {
 ////////////////////////////////////////////////////////////////////////////////
 // ImageLoadingTracker
 
-void ImageLoadingTracker::PostLoadImageTask(const ExtensionResource& resource) {
+void ImageLoadingTracker::PostLoadImageTask(const ExtensionResource& resource,
+                                            const gfx::Size& max_size) {
   MessageLoop* file_loop = g_browser_process->file_thread()->message_loop();
-  file_loop->PostTask(FROM_HERE, new LoadImageTask(this, resource,
+  file_loop->PostTask(FROM_HERE, new LoadImageTask(this, resource, max_size,
                                                    posted_count_++));
 }
 
diff --git a/chrome/browser/extensions/image_loading_tracker.h b/chrome/browser/extensions/image_loading_tracker.h
index 9732a00..a56aaeb 100644
--- a/chrome/browser/extensions/image_loading_tracker.h
+++ b/chrome/browser/extensions/image_loading_tracker.h
@@ -10,6 +10,10 @@
 class ExtensionResource;
 class SkBitmap;
 
+namespace gfx {
+class Size;
+}
+
 // The views need to load their icons asynchronously but might be deleted before
 // the images have loaded. This class stays alive while the request is in
 // progress (manages its own lifetime) and keeps track of whether the view still
@@ -44,8 +48,10 @@ class ImageLoadingTracker
 
   // Specify image resource to load.  This method must be called a number of
   // times equal to the |image_count| arugment to the constructor.  Calling it
-  // any more or less than that is an error.
-  void PostLoadImageTask(const ExtensionResource& resource);
+  // any more or less than that is an error. If the loaded image is larger than
+  // |max_size| it will be resized to those dimensions.
+  void PostLoadImageTask(const ExtensionResource& resource,
+                         const gfx::Size& max_size);
 
  private:
   class LoadImageTask;
diff --git a/chrome/browser/gtk/browser_actions_toolbar_gtk.cc b/chrome/browser/gtk/browser_actions_toolbar_gtk.cc
index 1e7d877..2e23629 100644
--- a/chrome/browser/gtk/browser_actions_toolbar_gtk.cc
+++ b/chrome/browser/gtk/browser_actions_toolbar_gtk.cc
@@ -41,8 +41,10 @@ class BrowserActionButton : public NotificationObserver,
     tracker_ = new ImageLoadingTracker(this, browser_action_icons_.size());
     for (size_t i = 0; i < extension->browser_action()->icon_paths().size();
          ++i) {
-      tracker_->PostLoadImageTask(extension->GetResource(
-          extension->browser_action()->icon_paths()[i]));
+      tracker_->PostLoadImageTask(
+          extension->GetResource(extension->browser_action()->icon_paths()[i]),
+          gfx::Size(Extension::kBrowserActionIconMaxSize,
+                    Extension::kBrowserActionIconMaxSize));
     }
 
     OnStateUpdated();
diff --git a/chrome/browser/gtk/location_bar_view_gtk.cc b/chrome/browser/gtk/location_bar_view_gtk.cc
index 78bbd04..e36c8d3 100644
--- a/chrome/browser/gtk/location_bar_view_gtk.cc
+++ b/chrome/browser/gtk/location_bar_view_gtk.cc
@@ -698,7 +698,10 @@ LocationBarViewGtk::PageActionViewGtk::PageActionViewGtk(
   tracker_ = new ImageLoadingTracker(this, icon_paths.size());
   for (std::vector<std::string>::const_iterator iter = icon_paths.begin();
        iter != icon_paths.end(); ++iter) {
-    tracker_->PostLoadImageTask(extension->GetResource(*iter));
+    tracker_->PostLoadImageTask(
+        extension->GetResource(*iter),
+        gfx::Size(Extension::kPageActionIconMaxSize,
+                  Extension::kPageActionIconMaxSize));
   }
 }
 
diff --git a/chrome/browser/views/browser_actions_container.cc b/chrome/browser/views/browser_actions_container.cc
index aac8caf..b4a2b77 100644
--- a/chrome/browser/views/browser_actions_container.cc
+++ b/chrome/browser/views/browser_actions_container.cc
@@ -26,8 +26,8 @@
 #include "views/controls/button/menu_button.h"
 #include "views/controls/button/text_button.h"
 
-// The size of the icon for page actions.
-static const int kIconSize = 29;
+// The size (both dimensions) of the buttons for page actions.
+static const int kButtonSize = 29;
 
 // The padding between the browser actions and the omnibox/page menu.
 static const int kHorizontalPadding = 4;
@@ -139,7 +139,10 @@ BrowserActionButton::BrowserActionButton(
     tracker_ = new ImageLoadingTracker(this, icon_paths.size());
     for (std::vector<std::string>::const_iterator iter = icon_paths.begin();
          iter != icon_paths.end(); ++iter) {
-      tracker_->PostLoadImageTask(extension->GetResource(*iter));
+      tracker_->PostLoadImageTask(
+          extension->GetResource(*iter),
+          gfx::Size(Extension::kBrowserActionIconMaxSize,
+                    Extension::kBrowserActionIconMaxSize));
     }
   }
 
@@ -291,8 +294,7 @@ BrowserActionView::BrowserActionView(ExtensionAction* browser_action,
 }
 
 void BrowserActionView::Layout() {
-  button_->SetBounds(0, kControlVertOffset, width(),
-                     height() - 2 * kControlVertOffset);
+  button_->SetBounds(0, kControlVertOffset, width(), kButtonSize);
 }
 
 void BrowserActionView::PaintChildren(gfx::Canvas* canvas) {
@@ -511,16 +513,16 @@ gfx::Size BrowserActionsContainer::GetPreferredSize() {
   if (browser_action_views_.empty())
     return gfx::Size(0, 0);
   int width = kHorizontalPadding * 2 +
-      browser_action_views_.size() * kIconSize;
-  return gfx::Size(width, kIconSize);
+      browser_action_views_.size() * kButtonSize;
+  return gfx::Size(width, kButtonSize);
 }
 
 void BrowserActionsContainer::Layout() {
   for (size_t i = 0; i < browser_action_views_.size(); ++i) {
     BrowserActionView* view = browser_action_views_[i];
-    int x = kHorizontalPadding + i * kIconSize;
-    if (x + kIconSize <= width()) {
-      view->SetBounds(x, 0, kIconSize, height());
+    int x = kHorizontalPadding + i * kButtonSize;
+    if (x + kButtonSize <= width()) {
+      view->SetBounds(x, 0, kButtonSize, height());
       view->SetVisible(true);
     } else {
       view->SetVisible(false);
@@ -573,12 +575,12 @@ int BrowserActionsContainer::GetClippedPreferredWidth(int available_width) {
   // We have at least one browser action. Make some of them sticky.
   int min_width = kHorizontalPadding * 2 +
       std::min(static_cast<int>(browser_action_views_.size()),
-               kMinimumNumberOfVisibleBrowserActions) * kIconSize;
+               kMinimumNumberOfVisibleBrowserActions) * kButtonSize;
 
   // Even if available_width is <= 0, we still return at least the |min_width|.
   if (available_width <= 0)
     return min_width;
 
-  return std::max(min_width, available_width - available_width % kIconSize +
+  return std::max(min_width, available_width - available_width % kButtonSize +
                   kHorizontalPadding * 2);
 }
diff --git a/chrome/browser/views/location_bar_view.cc b/chrome/browser/views/location_bar_view.cc
index 437862f..e5e4ece 100644
--- a/chrome/browser/views/location_bar_view.cc
+++ b/chrome/browser/views/location_bar_view.cc
@@ -1198,7 +1198,10 @@ LocationBarView::PageActionImageView::PageActionImageView(
   tracker_ = new ImageLoadingTracker(this, icon_paths.size());
   for (std::vector<std::string>::const_iterator iter = icon_paths.begin();
        iter != icon_paths.end(); ++iter) {
-    tracker_->PostLoadImageTask(extension->GetResource(*iter));
+    tracker_->PostLoadImageTask(
+        extension->GetResource(*iter),
+        gfx::Size(Extension::kPageActionIconMaxSize,
+                  Extension::kPageActionIconMaxSize));
   }
 }
 
diff --git a/chrome/common/extensions/extension.cc b/chrome/common/extensions/extension.cc
index fee0e89..4ead417 100644
--- a/chrome/common/extensions/extension.cc
+++ b/chrome/common/extensions/extension.cc
@@ -96,6 +96,9 @@ const int Extension::kIconSizes[] = {
   EXTENSION_ICON_BITTY
 };
 
+const int Extension::kPageActionIconMaxSize = 19;
+const int Extension::kBrowserActionIconMaxSize = 19;
+
 const char* Extension::kPermissionNames[] = {
   "tabs",
   "bookmarks",
diff --git a/chrome/common/extensions/extension.h b/chrome/common/extensions/extension.h
index c96ec0a..f506ffd 100644
--- a/chrome/common/extensions/extension.h
+++ b/chrome/common/extensions/extension.h
@@ -65,6 +65,10 @@ class Extension {
   // Icon sizes used by the extension system.
   static const int kIconSizes[];
 
+  // Max size (both dimensions) for browser and page actions.
+  static const int kPageActionIconMaxSize;
+  static const int kBrowserActionIconMaxSize;
+
   // Each permission is a module that the extension is permitted to use.
   static const char* kPermissionNames[];
   static const size_t kNumPermissions;
diff --git a/chrome/test/data/extensions/samples/icon_size_test/icon.png b/chrome/test/data/extensions/samples/icon_size_test/icon.png
new file mode 100755
index 0000000..c791499
--- /dev/null
+++ b/chrome/test/data/extensions/samples/icon_size_test/icon.png
diff --git a/chrome/test/data/extensions/samples/icon_size_test/manifest.json b/chrome/test/data/extensions/samples/icon_size_test/manifest.json
new file mode 100755
index 0000000..4c6ecb2
--- /dev/null
+++ b/chrome/test/data/extensions/samples/icon_size_test/manifest.json
@@ -0,0 +1,11 @@
+{
+  "name": "A browser action with a large icon",
+  "version": "1.0",
+  "permissions": [
+    "tabs", "http://*/*"
+  ],
+  "browser_action": {
+      "name": "Make this page red",
+      "icons": ["icon.png"]
+  }
+}