Create a new autocomplete popup for touch. This CL depends on

chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc

Index: chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc
diff --git a/chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc b/chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc
new file mode 100644
index 0000000000000000000000000000000000000000..63c4ebdb6becd7c92d9e3278e4791f1bd15bb4ab
--- /dev/null
+++ b/chrome/browser/ui/views/autocomplete/touch_autocomplete_popup_contents_view.cc
@@ -0,0 +1,165 @@
+// 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 "touch_autocomplete_popup_contents_view.h"
+
+#include "chrome/browser/autocomplete/autocomplete_edit_view.h"
+#include "chrome/browser/profiles/profile.h"
+#include "ui/gfx/canvas.h"
+#include "ui/gfx/canvas_skia.h"
+#include "ui/gfx/font.h"
+#include "ui/gfx/path.h"
+#include "ui/gfx/rect.h"
+#include "ui/gfx/size.h"
+#include "third_party/skia/include/core/SkPaint.h"
+#include "views/view.h"
+
+const int kMaxResultViews = 4;
+
+////////////////////////////////////////////////////////////////////////////////

[Peter Kasting, 2011/02/16 00:50:35]

Nit: In new code, I ask people to avoid these big banners over each different
access-type section.  I suggest just putting a single banner over the entire
class, like:

// TouchAutocompleteResultView ----------------------- [79 chars]

...and then two blank lines between the end of the class and the banner for the
next one.

[varunjain, 2011/02/16 18:48:10]

Done.

+// TouchAutocompleteResultView public
+
+TouchAutocompleteResultView::TouchAutocompleteResultView(
+    AutocompleteResultViewModel* model,
+    int model_index,
+    const gfx::Font& font,
+    const gfx::Font& bold_font)
+    : AutocompleteResultView(model, model_index, font, bold_font) {
+}
+
+TouchAutocompleteResultView::~TouchAutocompleteResultView() {
+}
+
+void TouchAutocompleteResultView::Layout() {
+  AutocompleteResultView::Layout();
+  int text_height =
+      2 * std::max(normal_font().GetHeight(), bold_font().GetHeight());

[Peter Kasting, 2011/02/16 00:50:35]

Why "2 *"?

[varunjain, 2011/02/16 18:48:10]

added comment to explain.

+  set_text_bounds(gfx::Rect(text_bounds().x(),
+                            std::max(0, (height() - text_height) / 2),
+                            text_bounds().width(),
+                            text_height));
+}
+
+gfx::Size TouchAutocompleteResultView::GetPreferredSize() {
+  int text_height =
+      2 * (std::max(normal_font().GetHeight(), bold_font().GetHeight()) +
+          text_vertical_padding_);
+  int icon_height = AutocompleteResultView::icon_size_ +
+      (icon_vertical_padding_ * 2);
+  return gfx::Size(0, std::max(icon_height, text_height));
+}
+
+////////////////////////////////////////////////////////////////////////////////
+// TouchAutocompleteResultView protected
+
+void TouchAutocompleteResultView::PaintMatch(gfx::Canvas* canvas,
+                                             const AutocompleteMatch& match,
+                                             int x) {
+  DrawString(canvas, match.contents, match.contents_class, false, x,
+             text_bounds().y());
+
+  if (!match.description.empty()) {
+    int y = text_bounds().y()
+        + std::max(normal_font().GetHeight(), bold_font().GetHeight());

[Peter Kasting, 2011/02/16 00:50:35]

Nit: + goes at end of previous line

[varunjain, 2011/02/16 18:48:10]

not required anymore

+    DrawString(canvas, match.description, match.description_class, true, x, y);
+  }
+}
+
+////////////////////////////////////////////////////////////////////////////////
+// TouchAutocompletePopupContentsView public.
+
+TouchAutocompletePopupContentsView::TouchAutocompletePopupContentsView(
+    const gfx::Font& font,
+    AutocompleteEditView* edit_view,
+    AutocompleteEditModel* edit_model,
+    Profile* profile,
+    const views::View* location_bar)
+    : AutocompletePopupContentsView(font, edit_view, edit_model, profile,
+                                    location_bar) {
+}
+
+TouchAutocompletePopupContentsView::~TouchAutocompletePopupContentsView() {
+}
+
+void TouchAutocompletePopupContentsView::UpdatePopupAppearance() {
+  AutocompletePopupContentsView::UpdatePopupAppearance();
+  Layout();

[Peter Kasting, 2011/02/16 00:50:35]

Why is this necessary when it's not necessary in the base class?

[varunjain, 2011/02/16 18:48:10]

Layout happens in the base class implicitly because whenever the number of
suggestions change, the popup changes bounds (height) resulting in a bounds
changed event (that causes layout). In touch case, the height of the popup never
changes as the suggestions are added horizontally. Hence, bounds never change...
so I have to explicitly call layout.

[Peter Kasting, 2011/02/16 21:50:22]

Then you're not doing things correctly in the Touch version.  Since you're
adding views horizontally, you should change CalculatePopupHeight() to calculate
a rect instead, and then have the animation code animate you horizontally
instead of vertically.  This way, you'll get the same logical animation (just in
a different physical direction) and have the same "bounds changed" event
sequences.

[varunjain, 2011/02/16 23:12:37]

Not quiet. The touch popup never changes size (unless the height of a result
somehow changes). The width of the popup is constant (equal to the width of
location bar) and the width of the results vary based on the number of results.
So there is no animation required.

+}
+
+void TouchAutocompletePopupContentsView::PaintChildren(
+    gfx::CanvasSkia* canvas) {
+  AutocompletePopupContentsView::PaintChildren(canvas);
+
+  // Draw divider lines.
+  int visible_child_count = GetVisibleChildCount();
+  for (int i = 0, j = 0; i < child_count() && j < visible_child_count - 1;
+      ++i) {
+    View* v = GetChildViewAt(i);
+    if (v->IsVisible()) {
+      canvas->DrawLineInt(AutocompleteResultView::GetColor(
+          AutocompleteResultView::NORMAL, AutocompleteResultView::DIMMED_TEXT),
+          v->x() + v->width(), v->y(),
+          v->x() + v->width(), v->y() + v->height());
+      ++j;
+    }
+  }
+}
+
+void TouchAutocompletePopupContentsView::LayoutChildren() {
+  gfx::Rect contents_rect = GetContentsBounds();
+  int visible_child_count = GetVisibleChildCount();
+  int child_width = contents_rect.width() / visible_child_count;
+
+  int max_child_height = 0;
+  for (int i = 0, j = 0; i < child_count() && j < visible_child_count; ++i) {
+    View* v = GetChildViewAt(i);
+    if (v->IsVisible()) {
+      int child_height = v->GetPreferredSize().height();
+      v->SetBounds(contents_rect.x() + (j++) * child_width, contents_rect.y(),
+          child_width, child_height);
+      if (max_child_height < child_height)

[Peter Kasting, 2011/02/16 00:50:35]

Nit: How about just:

max_child_height = std::max(max_child_height, child_height);

[varunjain, 2011/02/16 18:48:10]

Done.

+        max_child_height = child_height;
+    }
+  }
+}
+
+////////////////////////////////////////////////////////////////////////////////
+// TouchAutocompletePopupContentsView protected.
+
+int TouchAutocompletePopupContentsView::CalculatePopupHeight() {
+  int popup_height = 0;
+  int result_size = model()->result().size();

[Peter Kasting, 2011/02/16 00:50:35]

You'll want to update this function to look like the newest version of the
non-touch code, once you check in that other change.

[varunjain, 2011/02/16 18:48:10]

Done.

+  for (int i = 0; i < result_size && i < child_count(); ++i) {
+    AutocompleteResultView* result_view =
+        static_cast<AutocompleteResultView*>(GetChildViewAt(i));
+    int child_height = result_view->GetPreferredSize().height();
+    if (popup_height < child_height)
+      popup_height = child_height;
+  }
+  if (opt_in_view() &&
+      popup_height < opt_in_view()->GetPreferredSize().height())
+    popup_height = opt_in_view()->GetPreferredSize().height();
+  return popup_height;
+}
+
+AutocompleteResultView* TouchAutocompletePopupContentsView::CreateResultView(

[Peter Kasting, 2011/02/16 00:50:35]

So, how come we use a factory function in this case, but an #if in
AutocompleteEditViewViews' constructor where we actually create the popup view? 
Shouldn't we be consistent?

[varunjain, 2011/02/16 18:48:10]

we cannot use a factory function in AutocompleteEditViewViews' constructor
because we are not subclassing AutocompleteEditViewViews so there is no way to
decide whether we want the touch implementation or the normal one.
I did not use the #if here in the greater effort of minimizing #if usage in
chrome.
Another option I considered instead of using #if in AutocompleteEditViewViews,
we create a PopupViewProvider and call PopupViewProvider.provide() in
AutocompleteEditViewViews' constructor. PopupViewProvider will have separate
implementation for touch and normal and the correct implementation can be chose
through restrictions in gyp files. But I think thats a total overkill for this
purpose. I welcome other ideas you may have.

[oshima, 2011/02/16 20:00:27]

At some point, we need to merge touch and chromeos build (so that we have one
image,
and possibly a user can switch between two modes), and we will need a factory
method/object to create one of contents views. Here are a couple of options.
1) Create a factory method and uses #ifdef TOUCH_UI inside factory method. This
  still requires if/def but it's better than putting #ifdef in initializer list
IMO.
2) Create a factory method, and have one implementation for touch and another
for
  touch. We switch the implementation using gyp file.
3) Create two subclass for touch and non-touch version of ContentsView, and
  include the impl of factory function there. This is probably overkill though.

My preference at this point is 1), but don't have strong feeling about it.

[varunjain, 2011/02/16 23:12:37]

Done.

+    AutocompleteResultViewModel* model,
+    int model_index,
+    const gfx::Font& font,
+    const gfx::Font& bold_font) {
+  return new TouchAutocompleteResultView(model, model_index, font, bold_font);
+}
+
+////////////////////////////////////////////////////////////////////////////////
+// TouchAutocompletePopupContentsView private.
+
+int TouchAutocompletePopupContentsView::GetVisibleChildCount() {
+  int visible_child_count = 0;
+  for (int i = 0; i < child_count(); ++i)
+    if (GetChildViewAt(i)->IsVisible())
+      visible_child_count++;
+  if (visible_child_count > kMaxResultViews)

[Peter Kasting, 2011/02/16 00:50:35]

Whoah there.  You don't want to be clamping the visible results here.  You want
to do this clamping in the AutocompleteController instead by modifying the
existing global clamp value (which is 6) and probably also the per-provider
clamp value (which is 3).

Once you do this, this function can disappear and your callers can all get
simpler, because they can simply walk through the childredn skipping non-visible
ones, not having to keep two loop iterators.

[varunjain, 2011/02/16 18:48:10]

yes... I realized that clamping here doesnot work very well as the controller
still thinks that the clamp is at 6. Removing the clamping here. But I still
need the visible child count to determine the width of a single result view and
also when drawing lines in PaintChildren()

[Peter Kasting, 2011/02/16 21:50:22]

No, you don't.  In both cases, you can simply keep the current X coordinate as a
function-scope temp and add to it every time you encounter a visible child. 
That will kill off this function and noticeably simplify both those callers.

[varunjain, 2011/02/16 23:12:37]

Sry... I am really not getting this :(
as the width of a visible child is not constant (its essentially
total_width_of_popup/number_of_chilren), I cannot simply keep a running X
cordinate and add to it (because I will not know what to add to it)

+    visible_child_count = kMaxResultViews;
+  return visible_child_count;
+}