Improve large tab strip by leveraging touch icons when present

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

 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/ui/touch/tabs/touch_tab.h"
 
 #include "base/logging.h"
 #include "chrome/browser/themes/theme_service.h"
 #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"
 #include "ui/gfx/path.h"
 #include "ui/gfx/skbitmap_operations.h"
 
 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:
 
 TouchTab::TouchTab(TabController* controller)
     : BaseTab(controller) {
   InitTabResources();
 }
 
 TouchTab::~TouchTab() {
 }
@@ skipping 133 matching lines @@
 
   // Now draw the highlights/shadows around the tab edge.
   canvas->DrawBitmapInt(*tab_image->image_l, 0, y_base);
   canvas->TileImageInt(*tab_image->image_c, tab_image->l_width, y_base,
       width() - tab_image->l_width - tab_image->r_width, image_height);
   canvas->DrawBitmapInt(*tab_image->image_r, width() - tab_image->r_width,
                         y_base);
 }
 
 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();
 
   TouchTabImage* tab_image = &tab_active;
   int x_base = tab_image->image_l->width();
 
   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

   if (!data().favicon.isNull() && data().favicon.width() != kFaviconSize)
     favicon_x += (data().favicon.width() - kFaviconSize) / 2;
 
   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());
     if (should_display_crashed_favicon()) {
       ResourceBundle& rb = ResourceBundle::GetSharedInstance();
       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);
         }
       }
     }
     canvas->Restore();
   }
 }
 
 // static
 void TouchTab::InitTabResources() {
   static bool initialized = false;
@@ skipping 20 matching lines @@
   tab_active.image_r = rb.GetBitmapNamed(IDR_TAB_ACTIVE_RIGHT);
   tab_active.l_width = tab_active.image_l->width();
   tab_active.r_width = tab_active.image_r->width();
 
   tab_inactive.image_l = rb.GetBitmapNamed(IDR_TAB_INACTIVE_LEFT);
   tab_inactive.image_c = rb.GetBitmapNamed(IDR_TAB_INACTIVE_CENTER);
   tab_inactive.image_r = rb.GetBitmapNamed(IDR_TAB_INACTIVE_RIGHT);
   tab_inactive.l_width = tab_inactive.image_l->width();
   tab_inactive.r_width = tab_inactive.image_r->width();
 }