Improve large tab strip by leveraging touch icons when present

chrome/browser/ui/touch/tabs/touch_tab_strip_controller.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_strip_controller.h"
+
+#include "chrome/browser/extensions/extension_tab_helper.h"
+#include "chrome/browser/favicon/favicon_tab_helper.h"
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/tab_contents/tab_contents_wrapper.h"
+#include "chrome/browser/ui/touch/tabs/touch_tab.h"
+#include "chrome/browser/ui/views/tabs/base_tab_strip.h"
+#include "chrome/browser/ui/views/tabs/tab_renderer_data.h"
+#include "skia/ext/image_operations.h"
+#include "ui/gfx/codec/png_codec.h"
+#include "ui/gfx/favicon_size.h"
+
+// static

[sky, 2011/06/16 15:43:41]

Only use '// static' for class member definitions.

[Emmanuel Saint-loubert-Bié, 2011/06/17 00:01:44]

Done. My bad this was a cut and paste from ui/gfx/favicon_size.h

+static void calc_touch_icon_target_size(int* width, int* height) {

[tfarina, 2011/06/16 15:52:44]

Also wrap these static/private functions into an unnamed namespace, i.e:

namespace {

void CalcTouchIconTargetSize(...) {
}

...

}  // namespace

[Emmanuel Saint-loubert-Bié, 2011/06/17 00:01:44]

Thanks, also a cut and paste from ui/gfx/favicon_size.h

+  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);
+    }
+  }
+}
+
+// static
+static GURL gurl_without_fragment(const GURL& gurl)

[tfarina, 2011/06/16 15:52:44]

the function name should be CamelCase.

[Emmanuel Saint-loubert-Bié, 2011/06/17 00:01:44]

Done.

+{

[sky, 2011/06/16 15:43:41]

{ on 35.

[Emmanuel Saint-loubert-Bié, 2011/06/17 00:01:44]

Done.

+  url_canon::Replacements<char> replacements;
+  replacements.ClearUsername();
+  replacements.ClearPassword();
+  replacements.ClearQuery();
+  replacements.ClearRef();
+  return gurl.ReplaceComponents(replacements);
+}
+
+TouchTabStripController::TouchTabStripController(Browser* browser,
+                                                 TabStripModel* model)
+    : BrowserTabStripController(browser, model) {
+}
+
+TouchTabStripController::~TouchTabStripController() {
+}
+
+void TouchTabStripController::TabChangedAt(TabContentsWrapper* contents,
+                                           int model_index,
+                                           TabChangeType change_type) {
+  // Clear the large icon if we are loading a different URL in the same tab.
+  if (change_type == LOADING_ONLY) {
+    BaseTab* tab = tabstrip_->GetBaseTabAtModelIndex(model_index);

[sky, 2011/06/16 15:43:41]

You do this cast a couple of times. You should add a GetTouchTabAtModelIndex
method to TouchTabStrip that does this for you.

[Emmanuel Saint-loubert-Bié, 2011/06/17 00:01:44]

Done.

+    TouchTab* touch_tab = static_cast<TouchTab*>(tab);
+    if (touch_tab && !touch_tab->get_touch_icon().isNull()) {

[sky, 2011/06/16 15:43:41]

You shouldn't have to check touch_tab. If you feel you want a check, use a
DCHECK.

[Emmanuel Saint-loubert-Bié, 2011/06/17 00:01:44]

Done.

+      GURL existing_tab_url = gurl_without_fragment(touch_tab->data().url);
+      GURL page_url = gurl_without_fragment(contents->tab_contents()->GetURL());
+      // reset touch icon if the url are different
+      if (existing_tab_url != page_url)
+        touch_tab->set_touch_icon(SkBitmap());
+    }
+  }
+
+  // Always call parent's method.
+  BrowserTabStripController::TabChangedAt(contents, model_index, change_type);
+}
+
+void TouchTabStripController::SetTabRendererDataFromModel(
+    TabContents* contents,
+    int model_index,
+    TabRendererData* data,
+    bool new_tab) {
+  // Call parent first.
+  BrowserTabStripController::SetTabRendererDataFromModel(contents, model_index,
+      data, new_tab);

[sky, 2011/06/16 15:43:41]

indent to align with ( on 79.

[Emmanuel Saint-loubert-Bié, 2011/06/17 00:01:44]

Done.

+  if (new_tab)
+    return;
+
+  // Use the touch icon if any.
+  BaseTab* tab =  tabstrip_->GetBaseTabAtModelIndex(model_index);
+  TouchTab* touch_tab = static_cast<TouchTab*>(tab);
+  if (touch_tab && !touch_tab->get_touch_icon().isNull()) {
+    data->favicon = touch_tab->get_touch_icon();
+    return;
+  }
+
+  // Request touch icon.
+  TabContentsWrapper* wrapper =
+      TabContentsWrapper::GetCurrentWrapperForContents(contents);
+  GURL page_url = gurl_without_fragment(wrapper->tab_contents()->GetURL());

[sky, 2011/06/16 15:43:41]

Remove wrapper and change this to:
gurl_wihout_fragment(contents->GetURL());

[Emmanuel Saint-loubert-Bié, 2011/06/17 00:01:44]

Done.

+  FaviconService* favicon_service = profile()->GetFaviconService(
+      Profile::EXPLICIT_ACCESS);
+  if (favicon_service) {
+    CancelableRequestProvider::Handle h =
+        favicon_service->GetFaviconForURL(
+            page_url,
+            history::TOUCH_ICON | history::TOUCH_PRECOMPOSED_ICON,
+            &consumer_,
+            NewCallback(this, &TouchTabStripController::OnTouchIconAvailable));
+    consumer_.SetClientData(favicon_service, h, touch_tab);
+  }
+}
+
+void TouchTabStripController::OnTouchIconAvailable(
+    FaviconService::Handle h,
+    history::FaviconData favicon) {
+  // Abandon the request when there is no valid favicon.
+  if (!favicon.is_valid())
+    return;
+
+  // Retrieve the model_index from the TouchTab pointer received.
+  FaviconService* favicon_service = profile()->GetFaviconService(
+      Profile::EXPLICIT_ACCESS);
+  TouchTab* touch_tab = consumer_.GetClientData(favicon_service, h);

[sky, 2011/06/16 15:43:41]

117-119 can be replaced with consumer_.GetClientDataForCurrentRequest();

[Emmanuel Saint-loubert-Bié, 2011/06/17 00:01:44]

Done.

+  int model_index = tabstrip_->GetModelIndexOfBaseTab(touch_tab);
+  if (model_index == -1 || !IsValidIndex(model_index))
+    return;
+
+  // Try to decode the favicon, return on failure.
+  SkBitmap bitmap;
+  gfx::PNGCodec::Decode(favicon.image_data->front(),
+      favicon.image_data->size(),

[sky, 2011/06/16 15:43:41]

nit: align 127,128 with ( on 126.

[Emmanuel Saint-loubert-Bié, 2011/06/17 00:01:44]

Done.

+      &bitmap);
+  if (bitmap.isNull())
+    return;
+
+  // Rescale output, if needed, and assign to the TouchTab instance.
+  int width = bitmap.width();
+  int height = bitmap.height();
+  if (width == kTouchTargetIconSize && height == kTouchTargetIconSize) {
+    touch_tab->set_touch_icon(bitmap);
+  } else {
+    calc_touch_icon_target_size(&width, &height);
+    touch_tab->set_touch_icon(skia::ImageOperations::Resize(bitmap,
+        skia::ImageOperations::RESIZE_BEST, width, height));
+  }
+
+  // Refresh UI since favicon changed
+  browser_->GetTabContentsAt(model_index)->NotifyNavigationStateChanged(

[sky, 2011/06/16 15:43:41]

I believe instead of this you want:
touch_tab->SchedulePaint();

[Emmanuel Saint-loubert-Bié, 2011/06/17 00:01:44]

Actually that does not cause the refresh, so I left the code as is since it is
working.

+      TabContents::INVALIDATE_TAB);
+}
+