Improve large tab strip by leveraging touch icons when present

chrome/browser/ui/touch/tabs/touch_tab.cc

Index: chrome/browser/ui/touch/tabs/touch_tab.cc
diff --git a/chrome/browser/ui/touch/tabs/touch_tab.cc b/chrome/browser/ui/touch/tabs/touch_tab.cc
index dca408a783e84522e451556d230f89f5d80e80a8..9ab00e07b37c1a0574a30155ccf5cd83ada34527 100644
--- a/chrome/browser/ui/touch/tabs/touch_tab.cc
+++ b/chrome/browser/ui/touch/tabs/touch_tab.cc
@@ -9,6 +9,7 @@
 #include "grit/app_resources.h"
 #include "grit/theme_resources.h"
 #include "grit/theme_resources_standard.h"
+#include "skia/ext/image_operations.h"
 #include "ui/base/resource/resource_bundle.h"
 #include "ui/gfx/canvas_skia.h"
 #include "ui/gfx/favicon_size.h"
@@ -19,14 +20,29 @@ static const int kLeftPadding = 16;
 static const int kRightPadding = 15;
 static const int kDropShadowHeight = 2;
 
-// The size of the favicon touch area. This generally would be the same as
-// kFaviconSize in ui/gfx/favicon_size.h
-static const int kTouchTabIconSize = 32;
+// The size of the favicon touch area. This is not the same as kFaviconSize
+// which is defined in ui/gfx/favicon_size.h.
+static const int kTouchTargetIconSize = 32;
 
 TouchTab::TouchTabImage TouchTab::tab_alpha = {0};
 TouchTab::TouchTabImage TouchTab::tab_active = {0};
 TouchTab::TouchTabImage TouchTab::tab_inactive = {0};
 
+// static
+void calc_touch_icon_target_size(int* width, int* height) {

[sky, 2011/06/09 16:53:00]

This should really be static.

[Emmanuel Saint-loubert-Bié, 2011/06/10 02:21:45]

Yes of course, sorry for this cut and paste error

+  if (*width > kTouchTargetIconSize || *height > kTouchTargetIconSize) {
+    // Too big, resize it maintaining the aspect ratio.
+    float aspect_ratio = static_cast<float>(*width) /
+                         static_cast<float>(*height);
+    *height = kTouchTargetIconSize;
+    *width = static_cast<int>(aspect_ratio * *height);
+    if (*width > kTouchTargetIconSize) {
+      *width = kTouchTargetIconSize;
+      *height = static_cast<int>(*width / aspect_ratio);
+    }
+  }
+}
+
 ////////////////////////////////////////////////////////////////////////////////
 // TouchTab, public:
 
@@ -180,7 +196,6 @@ void TouchTab::PaintActiveTabBackground(gfx::Canvas* canvas) {
 }
 
 void TouchTab::PaintIcon(gfx::Canvas* canvas) {
-  // TODO(wyck): use thumbnailer to get better page images
   int x = favicon_bounds_.x();
   int y = favicon_bounds_.y();
 
@@ -190,8 +205,8 @@ void TouchTab::PaintIcon(gfx::Canvas* canvas) {
   x += x_base;
 
   if (base::i18n::IsRTL()) {
-    x = width() - x -
-        (data().favicon.isNull() ? kFaviconSize : data().favicon.width());
+    x = width() - x - (data().favicon.isNull()
+        ? kFaviconSize : data().favicon.width());

[sky, 2011/06/09 16:53:00]

Shouldn't this be kTouchTargetIconSize always?

[Emmanuel Saint-loubert-Bié, 2011/06/10 02:21:45]

Yes good catch, and it will matter even more now that we returned kFaviconSize
to 16 

   }
 
   int favicon_x = x;

[sky, 2011/06/09 16:53:00]

I don't think 212-214 are needed.

[Emmanuel Saint-loubert-Bié, 2011/06/10 02:21:45]

Indeed, seems like old cruft

@@ -200,13 +215,14 @@ void TouchTab::PaintIcon(gfx::Canvas* canvas) {
 
   if (data().network_state != TabRendererData::NETWORK_STATE_NONE) {
     ui::ThemeProvider* tp = GetThemeProvider();
-    SkBitmap frames(*tp->GetBitmapNamed(
-        (data().network_state == TabRendererData::NETWORK_STATE_WAITING) ?
-        IDR_THROBBER_WAITING : IDR_THROBBER));
+    SkBitmap frames(
+        *tp->GetBitmapNamed(
+            (data().network_state == TabRendererData::NETWORK_STATE_WAITING)
+                ? IDR_THROBBER_WAITING : IDR_THROBBER));
     int image_size = frames.height();
     int image_offset = loading_animation_frame() * image_size;
-    canvas->DrawBitmapInt(frames, image_offset, 0, image_size, image_size, x, y,
-                          kTouchTabIconSize, kTouchTabIconSize, false);
+    canvas->DrawBitmapInt(frames, image_offset, 0, image_size, image_size, x,
+        y, kTouchTargetIconSize, kTouchTargetIconSize, false);

[sky, 2011/06/09 16:53:00]

Is the throbber size 32x32 so that it lines up nicely with the  scaled image?

[Emmanuel Saint-loubert-Bié, 2011/06/10 02:21:45]

Yes it has been 32x32 for a while. No change needed here.

   } else {
     canvas->Save();
     canvas->ClipRectInt(0, 0, width(), height());
@@ -215,32 +231,22 @@ void TouchTab::PaintIcon(gfx::Canvas* canvas) {
       SkBitmap crashed_favicon(*rb.GetBitmapNamed(IDR_SAD_FAVICON));

[Emmanuel Saint-loubert-Bié, 2011/06/10 02:21:45]

On the other hand the IDR_SAD_FAVICON is not 32x32, so I will get that fixed.

       canvas->DrawBitmapInt(crashed_favicon, 0, 0, crashed_favicon.width(),
           crashed_favicon.height(), x, y + favicon_hiding_offset(),
-          kTouchTabIconSize, kTouchTabIconSize, true);
+          kTouchTargetIconSize, kTouchTargetIconSize, true);
     } else {
       if (!data().favicon.isNull()) {
-
-        if ((data().favicon.width() == kTouchTabIconSize) &&
-            (data().favicon.height() == kTouchTabIconSize)) {
-          canvas->DrawBitmapInt(data().favicon, 0, 0,
-                                data().favicon.width(), data().favicon.height(),
-                                x, y + favicon_hiding_offset(),
-                                kTouchTabIconSize, kTouchTabIconSize, true);
+        int width = data().favicon.width();
+        int height = data().favicon.height();
+        if (width != kTouchTargetIconSize || height != kTouchTargetIconSize) {
+          calc_touch_icon_target_size(&width, &height);
+          SkBitmap scaled_image = skia::ImageOperations::Resize(data().favicon,

[sky, 2011/06/09 16:53:00]

Seems painful to resize the image each time through. Can't you resize once and
cache that?

[Emmanuel Saint-loubert-Bié, 2011/06/10 02:21:45]

That is a great point. We did not cache before, but now that we do I will move
that code.

+              skia::ImageOperations::RESIZE_BEST, width, height);
+          canvas->DrawBitmapInt(scaled_image, 0, 0, scaled_image.width(),

[sky, 2011/06/09 16:53:00]

If the favicon width is not the same as the height, then won't this result in
stretching? Shouldn't you use the width/height you calculate for where to draw
it?

[Emmanuel Saint-loubert-Bié, 2011/06/10 02:21:45]

Done.

+              scaled_image.height(), x, y + favicon_hiding_offset(),
+              kTouchTargetIconSize, kTouchTargetIconSize, true);
         } else {
-          // Draw a background around target touch area in case the favicon
-          // is smaller than touch area (e.g www.google.com is 16x16 now)
-          canvas->DrawRectInt(
-              GetThemeProvider()->GetColor(
-                  ThemeService::COLOR_BUTTON_BACKGROUND),
-              x, y, kTouchTabIconSize, kTouchTabIconSize);
-
-          // We center the image
-          // TODO(saintlou): later request larger image from HistoryService
           canvas->DrawBitmapInt(data().favicon, 0, 0, data().favicon.width(),
-              data().favicon.height(),
-              x + ((kTouchTabIconSize - data().favicon.width()) / 2),
-              y + ((kTouchTabIconSize - data().favicon.height()) / 2) +
-                                                     favicon_hiding_offset(),
-              data().favicon.width(), data().favicon.height(), true);
+              data().favicon.height(), x, y + favicon_hiding_offset(),
+              kTouchTargetIconSize, kTouchTargetIconSize, true);
         }
       }
     }