Make Web Intents picker in Views conform to latest mocks

chrome/browser/ui/views/web_intent_picker_views.cc

 // Copyright (c) 2012 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 <algorithm>
 #include <vector>
 
 #include "base/command_line.h"
 #include "base/memory/scoped_vector.h"
 #include "base/time.h"
+#include "base/timer.h"
 #include "base/utf_string_conversions.h"
+#include "chrome/browser/download/download_util.h"
 #include "chrome/browser/tab_contents/tab_util.h"
 #include "chrome/browser/ui/browser_finder.h"
 #include "chrome/browser/ui/browser_navigator.h"
 #include "chrome/browser/ui/intents/web_intent_inline_disposition_delegate.h"
 #include "chrome/browser/ui/intents/web_intent_picker.h"
 #include "chrome/browser/ui/intents/web_intent_picker_delegate.h"
 #include "chrome/browser/ui/intents/web_intent_picker_model.h"
 #include "chrome/browser/ui/intents/web_intent_picker_model_observer.h"
 #include "chrome/browser/ui/tab_contents/tab_contents.h"
 #include "chrome/browser/ui/views/constrained_window_views.h"
@@ skipping 18 matching lines @@
 #include "ui/gfx/canvas.h"
 #include "ui/gfx/image/image.h"
 #include "ui/gfx/image/image_skia_operations.h"
 #include "ui/views/border.h"
 #include "ui/views/controls/button/image_button.h"
 #include "ui/views/controls/button/text_button.h"
 #include "ui/views/controls/image_view.h"
 #include "ui/views/controls/label.h"
 #include "ui/views/controls/link.h"
 #include "ui/views/controls/link_listener.h"
+#include "ui/views/controls/separator.h"
 #include "ui/views/controls/throbber.h"
 #include "ui/views/controls/webview/webview.h"
 #include "ui/views/layout/box_layout.h"
 #include "ui/views/layout/fill_layout.h"
 #include "ui/views/layout/grid_layout.h"
 #include "ui/views/layout/layout_constants.h"
 #include "ui/views/view.h"
 #include "ui/views/widget/widget.h"
 #include "ui/views/window/dialog_delegate.h"
 #include "ui/views/window/non_client_view.h"
 
 using content::WebContents;
 using views::GridLayout;
 
 namespace {
 
 // The color used to dim disabled elements.
 const SkColor kHalfOpacityWhite = SkColorSetARGB(128, 255, 255, 255);
 
+// The color used to display an enabled label.
+const SkColor kEnabledLabelColor = SkColorSetRGB(51, 51, 51);
+
+// The color used to display an enabled link.
+const SkColor kEnabledLinkColor = SkColorSetRGB(17, 85, 204);
+
 // The color used to display a disabled link.
 const SkColor kDisabledLinkColor = SkColorSetRGB(128, 128, 128);
 
 // The time between successive throbber frames in milliseconds.
 const int kThrobberFrameTimeMs = 50;
 
 // Width of IntentView action button in pixels
 const int kButtonWidth = 130;
 
 // Minimum number of action buttons - fill up with suggestions as needed.
 const int kMinRowCount = 4;
 
 // Maximum number of action buttons - do not add suggestions to reach.
 const int kMaxRowCount = 8;
 
+// Custom client insets for the constrained window.
+const int kConstrainedWindowClientTopInset = 10;
+const int kConstrainedWindowClientLeftInset = 0;
+const int kConstrainedWindowClientBottomInset = 0;
+const int kConstrainedWindowClientRightInset = 0;
+
 // Enables or disables all child views of |view|.
 void EnableChildViews(views::View* view, bool enabled) {
   for (int i = 0; i < view->child_count(); ++i) {
     views::View* child = view->child_at(i);
     child->SetEnabled(enabled);
   }
 }
 
-// Create a "close" button.
-views::ImageButton* CreateCloseButton(views::ButtonListener* listener) {
+// Creates a label.
+views::Label* CreateLabel() {
+  views::Label* label = new views::Label();
+  label->SetEnabledColor(kEnabledLabelColor);
+  label->SetHorizontalAlignment(views::Label::ALIGN_LEFT);

[Peter Kasting, 2012/10/12 03:23:32]

Wow, I'm amazed this isn't the default.  I don't know where on earth we actually
want centered labels.

Consider filing a bug that someone should clean up Label to make its default
alignment "ALIGN_LEFT" and then fix any cases that actually want ALIGN_CENTER to
specify that.

[please use gerrit instead, 2012/10/14 18:40:45]

Filed http://crbug.com/155526.

+  return label;
+}
+
+// Creates a title-style label.
+views::Label* CreateTitleLabel() {
+  views::Label* label = CreateLabel();
   ui::ResourceBundle& rb = ui::ResourceBundle::GetSharedInstance();

[Peter Kasting, 2012/10/12 03:23:32]

Nit: Or just inline into next line (I know this is a common pattern for resource
bundle usage but I never like using non-const refs)

[please use gerrit instead, 2012/10/14 18:40:45]

Done.

-  views::ImageButton* close_button = new views::ImageButton(listener);
-  close_button->SetImage(views::CustomButton::BS_NORMAL,
-                         rb.GetImageSkiaNamed(IDR_SHARED_IMAGES_X));
-  close_button->SetImage(views::CustomButton::BS_HOT,
-                         rb.GetImageSkiaNamed(IDR_SHARED_IMAGES_X_HOVER));
-  close_button->SetImage(views::CustomButton::BS_PUSHED,
-                         rb.GetImageSkiaNamed(IDR_SHARED_IMAGES_X_HOVER));
-  return close_button;
+  label->SetFont(rb.GetFont(ui::ResourceBundle::MediumFont));
+  return label;
 }
+
+// Creates a link.
+views::Link* CreateLink() {
+  views::Link* link = new views::Link();
+  link->SetEnabledColor(kEnabledLinkColor);
+  link->SetDisabledColor(kDisabledLinkColor);
+  link->SetHorizontalAlignment(views::Label::ALIGN_LEFT);
+  link->SetUnderlineOnHover(true);
+  return link;
+}
+
 // SarsView -------------------------------------------------------------------

[Peter Kasting, 2012/10/12 03:23:32]

Nit: While here, can you fix Sars -> Stars?  "SarsView" sounds scary :)

[please use gerrit instead, 2012/10/14 18:40:45]

Done.

 
 // A view that displays 5 stars: empty, full or half full, given a rating in
 // the range [0,5].
 class StarsView : public views::View {
  public:
   explicit StarsView(double rating);
   virtual ~StarsView();
 
  private:
   // The star rating to display, in the range [0,5].
@@ skipping 147 matching lines @@
   this_frame_ = gfx::ImageSkiaOperations::ExtractSubset(*frames_, subset_rect);
   return this_frame_;
 }
 
 void ThrobberNativeTextButton::Run() {
   DCHECK(running_);
 
   SchedulePaint();
 }
 
+// SpinnerProgressIndicator ----------------------------------------------------
+class SpinnerProgressIndicator : public views::View {

[Peter Kasting, 2012/10/12 03:23:32]

How does this differ from the similar-looking progress indicator in the download
shelf/downloads tab?  Please let's not define Yet Another Progress Indicator. 
We need them all to be the same.  If this means changing the way that indicator
looks, so be it.

[please use gerrit instead, 2012/10/14 18:40:45]

The downloads tab is Javascript:

this.canvasProgress_.arc(Download.Progress.centerX,
                               Download.Progress.centerY,
                               Download.Progress.radius,
                               Download.Progress.base,
                               Download.Progress.base + Math.PI * 0.02 *
                               Number(this.percent_),
                               false);

The download shelf is a DownloadItemView object that paints the download
progress bar on a small part of itself:

.-------------------------------+-.
| [prog] Filename               |v|
| [ress] Status                 | |
`-------------------------------+-'

Good thing is both DownloadItemView and SpinnerProgressIndicator here use the
same utility function to paint the progress on canvas:
download_util::PaintCustomDownloadProgress().

Perhaps a better solution is to move SpinnerProgressIndicator to
chrome/browser/ui/views/ and make the DownloadItemView use it?

[Peter Kasting, 2012/10/15 18:24:23]

Yeah, having the code here call download_util seems wrong.  Moving things to a
common location makes sense.  That can be done as a separate change if you want.

[please use gerrit instead, 2012/10/15 20:24:04]

Filed for follow up work for myself: http://crbug.com/155913

+ public:
+  SpinnerProgressIndicator();
+  void SetPercentDone(int percent);
+  void SetIndeterminate(bool indetereminate);
+
+  // Overridden from views::View.
+  virtual void Paint(gfx::Canvas* canvas) OVERRIDE;
+  virtual gfx::Size GetPreferredSize() OVERRIDE;
+
+ private:
+  void UpdateTimer();
+  int GetProgressAngle();
+
+  static const int kTimerIntervalMs = 1000 / 30;
+  static const int kSpinRateDegreesPerSecond = 270;
+
+  int percentDone_;

[Peter Kasting, 2012/10/12 03:23:32]

Never use camelCase member variables please

[please use gerrit instead, 2012/10/14 18:40:45]

Done.

+  int isIndeterminate_;
+
+  base::TimeTicks startTime_;
+  base::RepeatingTimer<SpinnerProgressIndicator> timer_;
+};
+
+SpinnerProgressIndicator::SpinnerProgressIndicator()
+  : percentDone_(0),
+    isIndeterminate_(true) {}
+
+void SpinnerProgressIndicator::SetPercentDone(int percent) {
+  percentDone_ = percent;
+  SchedulePaint();
+  UpdateTimer();
+}
+
+void SpinnerProgressIndicator::SetIndeterminate(bool indetereminate) {
+  isIndeterminate_ = indetereminate;
+  SchedulePaint();
+  UpdateTimer();
+}
+
+void SpinnerProgressIndicator::Paint(gfx::Canvas* canvas) {
+  ui::ResourceBundle& rb = ui::ResourceBundle::GetSharedInstance();
+  gfx::ImageSkia* fg = rb.GetImageSkiaNamed(IDR_WEB_INTENT_PROGRESS_FOREGROUND);
+  gfx::ImageSkia* bg = rb.GetImageSkiaNamed(IDR_WEB_INTENT_PROGRESS_BACKGROUND);
+  download_util::PaintCustomDownloadProgress(
+      canvas,
+      *bg,
+      *fg,
+      fg->width(),
+      bounds(),
+      GetProgressAngle(),
+      isIndeterminate_ ? -1 : percentDone_);
+}
+
+gfx::Size SpinnerProgressIndicator::GetPreferredSize() {
+  ui::ResourceBundle& rb = ui::ResourceBundle::GetSharedInstance();
+  gfx::ImageSkia* fg = rb.GetImageSkiaNamed(IDR_WEB_INTENT_PROGRESS_FOREGROUND);
+  return fg->size();
+}
+
+void SpinnerProgressIndicator::UpdateTimer() {
+  if (!parent() || !isIndeterminate_) {
+    timer_.Stop();
+    return;
+  }
+
+  if (!timer_.IsRunning()) {
+    startTime_ = base::TimeTicks::Now();
+    timer_.Start(FROM_HERE, base::TimeDelta::FromMilliseconds(kTimerIntervalMs),
+        this, &SpinnerProgressIndicator::SchedulePaint);
+  }
+}
+
+int SpinnerProgressIndicator::GetProgressAngle() {
+  if (!isIndeterminate_)
+    return download_util::kStartAngleDegrees;
+  base::TimeDelta delta = base::TimeTicks::Now() - startTime_;
+  int angle = delta.InSecondsF() * kSpinRateDegreesPerSecond;
+  return angle % 360;
+}
+
 // WaitingView ----------------------------------------------------------
 class WaitingView : public views::View {
  public:
-  WaitingView(views::ButtonListener* listener, bool use_close_button);
+  WaitingView();
 
  private:
+  static const int kTopBorder = 16;
+  static const int kBottomBorder = 37;
+  static const int kLeftBorder = 0;
+  static const int kRightBorder = 0;
+  static const int kThrobberTextPadding = 15;
+
   DISALLOW_COPY_AND_ASSIGN(WaitingView);
 };
 
-WaitingView::WaitingView(views::ButtonListener* listener,
-                         bool use_close_button) {
+WaitingView::WaitingView() {
+  enum {
+    kContentRowColumnSet, // Column set for the main content.
+  };
+
   views::GridLayout* layout = new views::GridLayout(this);
   layout->set_minimum_size(gfx::Size(WebIntentPicker::kWindowMinWidth, 0));
-  layout->SetInsets(WebIntentPicker::kContentAreaBorder,
-                    WebIntentPicker::kContentAreaBorder,
-                    WebIntentPicker::kContentAreaBorder,
-                    WebIntentPicker::kContentAreaBorder);
+  layout->SetInsets(kTopBorder, kLeftBorder, kBottomBorder, kRightBorder);
   SetLayoutManager(layout);
 
-  views::ColumnSet* cs = layout->AddColumnSet(0);
-  views::ColumnSet* header_cs = NULL;
-  if (use_close_button) {
-    header_cs = layout->AddColumnSet(1);
-    header_cs->AddPaddingColumn(1, views::kUnrelatedControlHorizontalSpacing);
-    header_cs->AddColumn(GridLayout::CENTER, GridLayout::CENTER, 0,
-                         GridLayout::USE_PREF, 0, 0);  // Close Button.
-  }
+  views::ColumnSet* cs = layout->AddColumnSet(kContentRowColumnSet);
   cs->AddPaddingColumn(0, views::kPanelHorizIndentation);
-  cs->AddColumn(GridLayout::CENTER, GridLayout::CENTER,
-                1, GridLayout::USE_PREF, 0, 0);
+  cs->AddColumn(GridLayout::CENTER, GridLayout::CENTER, 1,
+                GridLayout::USE_PREF, 0, 0);
   cs->AddPaddingColumn(0, views::kPanelHorizIndentation);
 
   // Create throbber.
-  ui::ResourceBundle& rb = ui::ResourceBundle::GetSharedInstance();
-  const gfx::ImageSkia* frames =
-      rb.GetImageNamed(IDR_SPEECH_INPUT_SPINNER).ToImageSkia();
-  views::Throbber* throbber = new views::Throbber(kThrobberFrameTimeMs, true);
-  throbber->SetFrames(frames);
-  throbber->Start();
+  SpinnerProgressIndicator* throbber = new SpinnerProgressIndicator();
 
   // Create text.
-  views::Label* label = new views::Label();
+  views::Label* label = CreateLabel();
   label->SetHorizontalAlignment(views::Label::ALIGN_CENTER);
-  label->SetFont(rb.GetFont(ui::ResourceBundle::MediumBoldFont));
   label->SetText(l10n_util::GetStringUTF16(IDS_INTENT_PICKER_WAIT_FOR_CWS));
 
   // Layout the view.
-  if (use_close_button) {
-    layout->StartRow(0, 1);
-    layout->AddView(CreateCloseButton(listener));
-  }
+  layout->StartRow(0, kContentRowColumnSet);
+  layout->AddView(throbber);
 
-  layout->AddPaddingRow(0, views::kUnrelatedControlLargeVerticalSpacing);
-  layout->StartRow(0, 0);
-  layout->AddView(throbber);
-  layout->AddPaddingRow(0, views::kUnrelatedControlLargeVerticalSpacing);
-  layout->StartRow(0, 0);
+  layout->AddPaddingRow(0, kThrobberTextPadding);
+
+  layout->StartRow(0, kContentRowColumnSet);
   layout->AddView(label);
-  layout->AddPaddingRow(0, views::kUnrelatedControlLargeVerticalSpacing);
-}
 
-// SuggestedExtensionsLayout ---------------------------------------------------
-
-// TODO(groby): Extremely fragile code, relies on order and number of fields.
-// Would probably be better off as GridLayout or similar. Also see review
-// comments on http://codereview.chromium.org/10909183
-
-// A LayoutManager used by a row of the IntentsView. It is similar
-// to a BoxLayout, but it right aligns the rightmost view (which is the install
-// button). It also uses the maximum height of all views in the row as a
-// preferred height so it doesn't change when the install button is hidden.
-class SuggestedExtensionsLayout : public views::LayoutManager {
- public:
-  SuggestedExtensionsLayout();
-  virtual ~SuggestedExtensionsLayout();
-
-  // Implementation of views::LayoutManager.
-  virtual void Layout(views::View* host) OVERRIDE;
-  virtual gfx::Size GetPreferredSize(views::View* host) OVERRIDE;
-
- private:
-  DISALLOW_COPY_AND_ASSIGN(SuggestedExtensionsLayout);
-};
-
-SuggestedExtensionsLayout::SuggestedExtensionsLayout() {
-}
-
-SuggestedExtensionsLayout::~SuggestedExtensionsLayout() {
-}
-
-void SuggestedExtensionsLayout::Layout(views::View* host) {
-  gfx::Rect child_area(host->GetLocalBounds());
-  child_area.Inset(host->GetInsets());
-  int x = child_area.x();
-  int y = child_area.y();
-
-  for (int i = 0; i < host->child_count(); ++i) {
-    views::View* child = host->child_at(i);
-    if (!child->visible())
-      continue;
-    gfx::Size size(child->GetPreferredSize());
-    gfx::Rect child_bounds(x, y, size.width(), child_area.height());
-    if (i == host->child_count() - 1) {
-      // Last child (the install button) should be right aligned.
-      child_bounds.set_x(std::max(child_area.width() - size.width(), x));
-    } else if (i == 1) {
-      // Label is considered fixed width, to align ratings widget.
-      DCHECK_LE(size.width(), WebIntentPicker::kTitleLinkMaxWidth);
-      x += WebIntentPicker::kTitleLinkMaxWidth +
-          views::kRelatedControlHorizontalSpacing;
-    } else {
-      x += size.width() + views::kRelatedControlHorizontalSpacing;
-    }
-    // Clamp child view bounds to |child_area|.
-    child->SetBoundsRect(child_bounds.Intersect(child_area));
-  }
-}
-
-gfx::Size SuggestedExtensionsLayout::GetPreferredSize(views::View* host) {
-  int width = 0;
-  int height = 0;
-  for (int i = 0; i < host->child_count(); ++i) {
-    views::View* child = host->child_at(i);
-    gfx::Size size(child->GetPreferredSize());
-    // The preferred height includes visible and invisible children. This
-    // prevents jank when a child is hidden.
-    height = std::max(height, size.height());
-    if (!child->visible())
-      continue;
-    if (i != 0)
-      width += views::kRelatedControlHorizontalSpacing;
-  }
-  gfx::Insets insets(host->GetInsets());
-  return gfx::Size(width + insets.width(), height + insets.height());
+  // Start the throbber.
+  throbber->SetIndeterminate(true);
 }
 
 // IntentRowView --------------------------------------------------
 
 // A view for each row in the IntentsView. It displays information
 // for both installed and suggested intent handlers.
 class IntentRowView : public views::View,
                       public views::ButtonListener,
                       public views::LinkListener {
  public:
@@ skipping 49 matching lines @@
   // Stop the throbber for this row, and show the star rating and the install
   // button.
   void StopThrobber();
 
  protected:
   virtual void OnEnabledChanged() OVERRIDE;
 
   virtual void PaintChildren(gfx::Canvas* canvas) OVERRIDE;
 
  private:
+  static const int kStarRatingHorizontalSpacing = 20;
+
   IntentRowView(ActionType type, size_t tag);
 
   // Gets the proper message string associated with |type_|.
   string16 GetActionButtonMessage();
 
   // Identifier for the suggested extension displayed in this row.
   std::string extension_id_;
 
   // A delegate to respond to button presses and clicked links.
   Delegate* delegate_;
@@ skipping 55 matching lines @@
                                         ui::ELIDE_AT_END);
 
   const gfx::ImageSkia* icon = NULL;
   StarsView* stars = NULL;
   views::Label* label = NULL;
   IntentRowView* view;
   if (service != NULL) {
     view = new IntentRowView(ACTION_INVOKE, tag);
     icon = service->favicon.ToImageSkia();
     label = new views::Label(elided_title);
+    label->SetEnabledColor(kEnabledLabelColor);
   } else {
     view = new IntentRowView(ACTION_INSTALL, tag);
     view->extension_id_ = extension->id;
     icon = extension->icon.ToImageSkia();
     views::Link* link = new views::Link(elided_title);
+    link->SetUnderlineOnHover(true);
+    link->SetEnabledColor(kEnabledLinkColor);
     link->set_listener(view);
     label = link;
     stars = new StarsView(extension->average_rating);
   }
 
   view->delegate_ = delegate;
 
-  view->SetLayoutManager(new SuggestedExtensionsLayout);
+  views::GridLayout* grid_layout = new views::GridLayout(view);
+  view->SetLayoutManager(grid_layout);
+
+  views::ColumnSet* columns = grid_layout->AddColumnSet(0);
+  columns->AddColumn(GridLayout::LEADING, GridLayout::CENTER, 0,
+                     GridLayout::USE_PREF, 0, 0); // Icon.
+  columns->AddPaddingColumn(0, WebIntentPicker::kIconLabelHorizontalSpacing);
+  columns->AddColumn(GridLayout::LEADING, GridLayout::CENTER, 1,
+                     GridLayout::FIXED, WebIntentPicker::kTitleLinkMaxWidth, 0);
+  columns->AddPaddingColumn(0, kStarRatingHorizontalSpacing);
+  if (stars != NULL) {
+    columns->AddColumn(GridLayout::TRAILING, GridLayout::CENTER, 0,
+                       GridLayout::USE_PREF, 0, 0); // Star rating.
+    columns->AddPaddingColumn(0, kStarRatingHorizontalSpacing);
+  }
+  columns->AddColumn(GridLayout::TRAILING, GridLayout::CENTER, 0,
+                     GridLayout::FIXED, preferred_width, 0); // Button.
+
+  grid_layout->StartRow(0, 0);
 
   view->icon_ = new views::ImageView();
-
   view->icon_->SetImage(icon);
-  view->AddChildView(view->icon_);
+  grid_layout->AddView(view->icon_);
 
   view->title_link_ = label;
-  view->AddChildView(view->title_link_);
+  grid_layout->AddView(view->title_link_);
 
   if (stars != NULL) {
     view->stars_ = stars;
-    view->AddChildView(view->stars_);
+    grid_layout->AddView(view->stars_);
   }
 
-  view->install_button_ = new ThrobberNativeTextButton(
-      view, view->GetActionButtonMessage());
+  view->install_button_ = new ThrobberNativeTextButton(view,
+      view->GetActionButtonMessage());
   view->install_button_->set_preferred_width(preferred_width);
-  view->AddChildView(view->install_button_);
+  grid_layout->AddView(view->install_button_);
 
   return view;
 }
 
 IntentRowView::~IntentRowView() {}
 
 void IntentRowView::ButtonPressed(views::Button* sender,
                                   const ui::Event& event) {
   if (type_ == ACTION_INSTALL)
     delegate_->OnExtensionInstallClicked(extension_id_);
@@ skipping 73 matching lines @@
   // Hide the install throbber. This function re-enables all buttons and links.
   void StopThrobber();
 
   // Adjusts a given width to account for language-specific strings.
   int AdjustWidth(int old_width);
 
  protected:
   virtual void OnEnabledChanged() OVERRIDE;
 
  private:
+  static const int kAppRowVerticalSpacing = 10;

[Peter Kasting, 2012/10/12 03:23:32]

Nit: Just declare this in the function it's used in, avoid members except when
necessary

[please use gerrit instead, 2012/10/14 18:40:45]

Done.

+
   const WebIntentPickerModel* model_;
   IntentRowView::Delegate* delegate_;
 
   // Width for the action button, adjusted to be wide enough for all possible
   // strings.
   int button_width_;
 
   DISALLOW_COPY_AND_ASSIGN(IntentsView);
 };
 
@@ skipping 12 matching lines @@
 void IntentsView::Update() {
   RemoveAllChildViews(true);
 
   ThrobberNativeTextButton size_helper(
       NULL, l10n_util::GetStringUTF16(IDS_INTENT_PICKER_INSTALL_EXTENSION));
   size_helper.SetText(
       l10n_util::GetStringUTF16(IDS_INTENT_PICKER_SELECT_INTENT));
   button_width_ = std::max(
       kButtonWidth, size_helper.GetPreferredSize().width());
 
-  views::BoxLayout* layout = new views::BoxLayout(
-      views::BoxLayout::kVertical, 0, 0, views::kRelatedControlVerticalSpacing);
+  views::BoxLayout* layout = new views::BoxLayout(views::BoxLayout::kVertical,
+      0, 0, kAppRowVerticalSpacing);

[Peter Kasting, 2012/10/12 03:23:32]

Nit: Align with first arg in line above or else move that arg onto the beginning
of this line

[please use gerrit instead, 2012/10/14 18:40:45]

Done.

   SetLayoutManager(layout);
 
   int available_rows = kMaxRowCount;
 
   for (size_t i = 0;
       available_rows > 0 && i < model_->GetInstalledServiceCount();
       ++i, --available_rows) {
     const WebIntentPickerModel::InstalledService& service =
       model_->GetInstalledServiceAt(i);
     AddChildView(IntentRowView::CreateHandlerRow(&service, NULL, i,
@@ skipping 57 matching lines @@
   // This method is called when the user cancels the picker dialog.
   virtual void ButtonPressed(views::Button* sender,
                              const ui::Event& event) OVERRIDE;
 
   // views::DialogDelegate implementation.
   virtual void WindowClosing() OVERRIDE;
   virtual void DeleteDelegate() OVERRIDE;
   virtual views::Widget* GetWidget() OVERRIDE;
   virtual const views::Widget* GetWidget() const OVERRIDE;
   virtual views::View* GetContentsView() OVERRIDE;
+  virtual views::View* GetHeaderView() OVERRIDE;

[Peter Kasting, 2012/10/12 03:23:32]

I don't think we should add this concept of a "header view" to dialog delegate. 
We can just lay things out appropriately in the contents, see below.

[please use gerrit instead, 2012/10/14 18:40:45]

Done.

   virtual int GetDialogButtons() const OVERRIDE;
   virtual bool Cancel() OVERRIDE;
 
   // LinkListener implementation.
   virtual void LinkClicked(views::Link* source, int event_flags) OVERRIDE;
 
   // WebIntentPicker implementation.
   virtual void Close() OVERRIDE;
   virtual void SetActionString(const string16& action) OVERRIDE;
   virtual void OnExtensionInstallSuccess(const std::string& id) OVERRIDE;
@@ skipping 19 matching lines @@
       const std::string& extension_id,
       WindowOpenDisposition disposition) OVERRIDE;
   virtual void OnActionButtonClicked(
       IntentRowView::ActionType type,
       size_t tag) OVERRIDE;
 
  private:
   // Update picker contents to reflect the current state of the model.
   void UpdateContents();
 
+  // Shows a spinner and notifies the user that we are waiting for information
+  // from the Chrome Web Store.
+  void ShowWaitingForSuggestions();
+
   // Updates the dialog with the list of available services, suggestions,
   // and a nice link to CWS to find more suggestions. This is the "Main"
   // view of the picker.
   void ShowAvailableServices();
 
   // Informs the user that there are no services available to handle
   // the intent, and that there are no suggestions from the Chrome Web Store.
   void ShowNoServicesMessage();
 
   // Restore the contents of the picker to the initial contents.
@@ skipping 13 matching lines @@
   // chooses a service or cancels.
   WebIntentPickerDelegate* delegate_;
 
   // A weak pointer to the picker model.
   WebIntentPickerModel* model_;
 
   // A weak pointer to the action string label.
   // Created locally, owned by Views.
   views::Label* action_label_;
 
+  // Label for the no-services dialog.
+  views::Label* no_services_title_;
+
   // A weak pointer to the header label for the extension suggestions.
   // Created locally, owned by Views.
   views::Label* suggestions_label_;
 
   // A weak pointer to the intents view.
   // Created locally, owned by Views view hierarchy.
   IntentsView* extensions_;
 
   // Delegate for inline disposition tab contents.
   scoped_ptr<WebIntentInlineDispositionDelegate> inline_disposition_delegate_;
 
   // A weak pointer to the TabContents this picker is in.
   TabContents* tab_contents_;
 
   // A weak pointer to the WebView that hosts the WebContents being displayed.
   // Created locally, owned by Views.
   views::WebView* webview_;
 
   // A weak pointer to the view that contains all other views in the picker.
   // Created locally, owned by Views.
   views::View* contents_;
 
+  // The view that contains the header for the picker.
+  views::View* header_;
+
   // A weak pointer to the constrained window.
   // Created locally, owned by Views.
   ConstrainedWindowViews* window_;
 
   // A weak pointer to the more suggestions link.
   // Created locally, owned by Views.
   views::Link* more_suggestions_link_;
 
   // A weak pointer to the choose another service link.
   // Created locally, owned by Views.
   views::Link* choose_another_service_link_;
 
+  // The icon for the inline intent.
+  views::ImageView* inline_intent_icon_;
+
+  // The label for the inline intent.
+  views::Label* inline_intent_label_;
+
   // Weak pointer to "Waiting for CWS" display. Owned by parent view.
   WaitingView* waiting_view_;
 
   // Set to true when displaying the inline disposition web contents. Used to
   // prevent laying out the inline disposition widgets twice.
   bool displaying_web_contents_;
 
   // The text for the current action.
   string16 action_text_;
 
   // Ownership of the WebContents we are displaying in the inline disposition.
   scoped_ptr<WebContents> inline_web_contents_;
 
-  // Indicate if dialog should display its own close button.
-  // TODO(groby): Only relevant until new ConstrainedWindow is implemented,
-  // from then on always true.
-  bool use_close_button_;
-
   // Signals if the picker can be closed. False during extension install.
   bool can_close_;
 
   DISALLOW_COPY_AND_ASSIGN(WebIntentPickerViews);
 };
 
 // static
 WebIntentPicker* WebIntentPicker::Create(content::WebContents* web_contents,
                                          WebIntentPickerDelegate* delegate,
                                          WebIntentPickerModel* model) {
   TabContents* tab_contents = TabContents::FromWebContents(web_contents);
   return new WebIntentPickerViews(tab_contents, delegate, model);
 }
 
 WebIntentPickerViews::WebIntentPickerViews(TabContents* tab_contents,
                                            WebIntentPickerDelegate* delegate,
                                            WebIntentPickerModel* model)
     : delegate_(delegate),
       model_(model),
-      action_label_(NULL),
       suggestions_label_(NULL),
       extensions_(NULL),
       tab_contents_(tab_contents),
       webview_(new views::WebView(tab_contents->profile())),
       window_(NULL),
       more_suggestions_link_(NULL),
       choose_another_service_link_(NULL),
       waiting_view_(NULL),
       displaying_web_contents_(false),
       can_close_(true) {
-  bool enable_chrome_style = CommandLine::ForCurrentProcess()->HasSwitch(
-      switches::kEnableFramelessConstrainedDialogs);
-  use_close_button_ = enable_chrome_style;
-
   model_->set_observer(this);
   contents_ = new views::View();
+  header_ = new views::View();
+
+  action_label_ = CreateTitleLabel();
+  action_label_->SetText(GetActionTitle());
+  no_services_title_ = CreateTitleLabel();
+  inline_intent_icon_ = new views::ImageView();
+  inline_intent_label_ = CreateLabel();
+  choose_another_service_link_ = CreateLink();
+
+  gfx::Insets client_insets(kConstrainedWindowClientTopInset,
+                            kConstrainedWindowClientLeftInset,
+                            kConstrainedWindowClientBottomInset,
+                            kConstrainedWindowClientRightInset);
   // Show the dialog.
-  window_ = new ConstrainedWindowViews(tab_contents->web_contents(),
-                                       this,
-                                       enable_chrome_style);
-
+  window_ = new ConstrainedWindowViews(tab_contents->web_contents(), this, true,
+                                       client_insets);
   UpdateContents();
 }
 
 WebIntentPickerViews::~WebIntentPickerViews() {
   model_->set_observer(NULL);
 }
 
 void WebIntentPickerViews::ButtonPressed(views::Button* sender,
                                          const ui::Event& event) {
   delegate_->OnUserCancelledPickerDialog();
@@ skipping 12 matching lines @@
 }
 
 const views::Widget* WebIntentPickerViews::GetWidget() const {
   return contents_->GetWidget();
 }
 
 views::View* WebIntentPickerViews::GetContentsView() {
   return contents_;
 }
 
+views::View* WebIntentPickerViews::GetHeaderView() {
+  return header_;
+}
+
 int WebIntentPickerViews::GetDialogButtons() const {
   return ui::DIALOG_BUTTON_NONE;
 }
 
 bool WebIntentPickerViews::Cancel() {
   return can_close_;
 }
 
 void WebIntentPickerViews::LinkClicked(views::Link* source, int event_flags) {
   if (source == more_suggestions_link_) {
     delegate_->OnSuggestionsLinkClicked(
         chrome::DispositionFromEventFlags(event_flags));
   } else if (source == choose_another_service_link_) {
     // Signal cancellation of inline disposition.
     delegate_->OnChooseAnotherService();
     ResetContents();
   } else {
     NOTREACHED();
   }
 }
 
 void WebIntentPickerViews::Close() {
   window_->CloseConstrainedWindow();
 }
 
 void WebIntentPickerViews::SetActionString(const string16& action) {
   action_text_ = action;
 
-  if (action_label_)
-    action_label_->SetText(GetActionTitle());
-    contents_->Layout();
-    SizeToContents();
+  action_label_->SetText(GetActionTitle());
+
+  contents_->Layout();
+  SizeToContents();
 }
 
 void WebIntentPickerViews::OnExtensionInstallSuccess(const std::string& id) {
   can_close_ = true;
 }
 
 void WebIntentPickerViews::OnExtensionInstallFailure(const std::string& id) {
   extensions_->StopThrobber();
   more_suggestions_link_->SetEnabled(true);
   can_close_ = true;
   contents_->Layout();
   SizeToContents();
 
   // TODO(binji): What to display to user on failure?
 }
 
 void WebIntentPickerViews::OnInlineDispositionAutoResize(
     const gfx::Size& size) {
   webview_->SetPreferredSize(size);
   contents_->Layout();
   SizeToContents();
 }
 
 void WebIntentPickerViews::OnPendingAsyncCompleted() {
   UpdateContents();
 }
 
 void WebIntentPickerViews::ShowNoServicesMessage() {
+  static const int kTopPadding = 20;

[Peter Kasting, 2012/10/12 03:23:32]

Nit: I'd just have:

  const int kPadding = 20;
  const int kHeaderTopPadding = 11;

...and drop the other constants.

Also, regarding constants in general.  It's really odd to have 1, 7, 11, 13,
etc. all over.  It's normally OK to adjust padding values given in the mocks by
a few pixels; more important is to use a small number of consistent values. 
Look at the layout constants header file and wherever possible pull from there,
otherwise try and choose a small number of constants that are used across
multiple dialogs.  If the constants differ because we're aligning different-size
objects to have e.g. a common baseline or something, calculate the precise
constants needed on the fly based on the font size or image size or whatever. 
This will make the dialog layout robust if we start changing the assets instead
of incredibly brittle and needing tons of small tweaks all the time, which sucks
since we _will_ change these dialogs' appearance over time.

[please use gerrit instead, 2012/10/14 18:40:45]

Partially done. (1) I am now using as many global constants from
layout_constants.h, web_intent_picker.h, and constrained_window_constants.h as
possible. (2) I've moved as many as possible of the remaining layout constants
into the unnamed namespace at the top of web_intent_picker_views.cc to be
defined in one place and shared among methods in this file.

UX wants some very specific paddings for Web Intents views. I don't think we can
convince them otherwise...

[Peter Kasting, 2012/10/15 18:24:23]

Normally weird paddings happen because we want some particular consistent visual
appearance and then we need to adjust different elements by different amounts in
order to achieve it.  Like if we want to align two objects but one has some
built-in padding, we need different external padding for them.

In cases like this, even if we need two padding values, we can do something like
"desired indent = x, built in padding of object 2 = y" and then pad by x and (x
- y) instead of just having two different padding values.  That's easier to
understand and maintain later because for example if we want to change the
overall padding we can just change x and not y.  Added bonus if after this sort
of transformation x becomes one of our common layout constants and then we just
need to specially define y.

The point of all that was, even if we do need some custom values here, just make
sure they're as few in number and as maintainable as possible.

[please use gerrit instead, 2012/10/15 20:24:04]

Done. I am now using mostly paddings from ConstarinedWindowConstants with
offsets for builtin paddings of icons and labels.

+  static const int kLeftPadding = 20;
+  static const int kBottomPadding = 20;
+  static const int kRightPadding = 20;
+
+  static const int kHeaderTopPadding = 11;
+  static const int kHeaderLeftPadding = 0;
+  static const int kHeaderBottomPadding = 0;
+  static const int kHeaderRightPadding = 0;
+
   ClearContents();
 
-  views::GridLayout* grid_layout = new views::GridLayout(contents_);
-  contents_->SetLayoutManager(grid_layout);
+  // Header.
+  no_services_title_->SetText(l10n_util::GetStringUTF16(
+      IDS_INTENT_PICKER_NO_SERVICES_TITLE));
+  views::GridLayout* header_layout = new views::GridLayout(header_);
+  header_->SetLayoutManager(header_layout);
+  header_layout->SetInsets(kHeaderTopPadding, kHeaderLeftPadding,
+      kHeaderBottomPadding, kHeaderRightPadding);
+  header_layout->AddColumnSet(0)->AddColumn(GridLayout::LEADING,
+      GridLayout::CENTER, 1, GridLayout::USE_PREF, 0, 0);
+  header_layout->StartRow(0, 0);
+  header_layout->AddView(no_services_title_);
 
-  grid_layout->SetInsets(kContentAreaBorder, kContentAreaBorder,
-                         kContentAreaBorder, kContentAreaBorder);
-  views::ColumnSet* main_cs = grid_layout->AddColumnSet(0);
-  main_cs->AddColumn(GridLayout::FILL, GridLayout::LEADING, 1,
-                     GridLayout::USE_PREF, 0, 0);
+  // Contents.
+  views::GridLayout* layout = new views::GridLayout(contents_);

[Peter Kasting, 2012/10/12 03:23:32]

Let's kill the header view entirely and avoid having multiple grid layouts. 
That makes it harder to refactor to use DialogDelegateView later anyway. 
Instead of using two sets of insets we can use multiple column sets, some of
which contain padding columns/rows.

[please use gerrit instead, 2012/10/14 18:40:45]

Done.

+  layout->set_minimum_size(gfx::Size(WebIntentPicker::kWindowMinWidth, 0));
+  layout->SetInsets(kTopPadding, kLeftPadding, kBottomPadding, kRightPadding);
+  contents_->SetLayoutManager(layout);
 
-  ui::ResourceBundle& rb = ui::ResourceBundle::GetSharedInstance();
+  layout->AddColumnSet(0)->AddColumn(GridLayout::FILL, GridLayout::CENTER, 1,
+      GridLayout::USE_PREF, 0, 0);
 
-  grid_layout->StartRow(0, 0);
-  views::Label* header = new views::Label();
-  header->SetHorizontalAlignment(views::Label::ALIGN_LEFT);
-  header->SetFont(rb.GetFont(ui::ResourceBundle::MediumFont));
-  header->SetText(l10n_util::GetStringUTF16(
-      IDS_INTENT_PICKER_NO_SERVICES_TITLE));
-  grid_layout->AddView(header);
+  // Content row.
+  layout->StartRow(0, 0);
+  views::Label* body = CreateLabel();
+  body->SetMultiLine(true);
+  body->SetText(l10n_util::GetStringUTF16(IDS_INTENT_PICKER_NO_SERVICES));
+  layout->AddView(body);
 
-  grid_layout->AddPaddingRow(0, views::kRelatedControlVerticalSpacing);
-
-  grid_layout->StartRow(0, 0);
-  views::Label* body = new views::Label();
-  body->SetMultiLine(true);
-  body->SetHorizontalAlignment(views::Label::ALIGN_LEFT);
-  body->SetText(l10n_util::GetStringUTF16(IDS_INTENT_PICKER_NO_SERVICES));
-  grid_layout->AddView(body);
-
-  int height = contents_->GetHeightForWidth(kWindowMinWidth);
-  contents_->SetSize(gfx::Size(kWindowMinWidth, height));
+  int height = contents_->GetHeightForWidth(WebIntentPicker::kWindowMinWidth);
+  contents_->SetSize(gfx::Size(WebIntentPicker::kWindowMinWidth, height));
+  contents_->Layout();
 }
 
 void WebIntentPickerViews::OnInlineDispositionWebContentsLoaded(
     content::WebContents* web_contents) {
+  enum {
+    kSeparatorRowColumnSet, // Column set for separator layout.
+    kWebContentsRowColumnSet, // Column set for main content layout.
+  };
+
   if (displaying_web_contents_)
     return;
 
+  const int kHeaderTopPadding = 7;
+  const int kHeaderLeftPadding = 0;
+  const int kHeaderBottomPadding = 0;
+  const int kHeaderRightPadding = 1;
+
+  const int kTopPadding = 13;
+  const int kLeftPadding = 0;
+  const int kBottomPadding = 20;
+  const int kRightPadding = 0;
+
+  const int kWebContentsTopPadding = 3;
+  const int kWebContentsLeftPadding = 1;
+  const int kWebContentsRightPadding = 1;
+
+  bool show_use_another = (model_->GetInstalledServiceCount() > 1 ||
+                           model_->GetSuggestedExtensionCount());

[Peter Kasting, 2012/10/12 03:23:32]

Nit: Strange indenting, only indent even for comma-separated arg lists, indent 4
otherwise

[please use gerrit instead, 2012/10/14 18:40:45]

Done.

+
   // Replace the picker with the inline disposition.
   ClearContents();
 
-  views::GridLayout* grid_layout = new views::GridLayout(contents_);
-  contents_->SetLayoutManager(grid_layout);
-
-  grid_layout->SetInsets(kContentAreaBorder, kContentAreaBorder,
-                         kContentAreaBorder, kContentAreaBorder);
-  views::ColumnSet* header_cs = grid_layout->AddColumnSet(0);
-  header_cs->AddColumn(GridLayout::CENTER, GridLayout::CENTER, 0,
-                       GridLayout::USE_PREF, 0, 0);  // Icon.
-  header_cs->AddPaddingColumn(0, 4);
-  header_cs->AddColumn(GridLayout::CENTER, GridLayout::CENTER, 0,
-                       GridLayout::USE_PREF, 0, 0);  // Title.
-  header_cs->AddPaddingColumn(0, 4);
-  header_cs->AddColumn(GridLayout::CENTER, GridLayout::CENTER, 0,
-                       GridLayout::USE_PREF, 0, 0);  // Link.
-  header_cs->AddPaddingColumn(1, views::kUnrelatedControlHorizontalSpacing);
-  if (use_close_button_) {
-    header_cs->AddColumn(GridLayout::CENTER, GridLayout::CENTER, 0,
-                         GridLayout::USE_PREF, 0, 0);  // Close Button.
-  }
-
-  views::ColumnSet* full_cs = grid_layout->AddColumnSet(1);
-  full_cs->AddColumn(GridLayout::FILL, GridLayout::FILL, 1.0,
-                     GridLayout::USE_PREF, 0, 0);
-
   const WebIntentPickerModel::InstalledService* service =
       model_->GetInstalledServiceWithURL(model_->inline_disposition_url());
 
-  // Header row.
-  grid_layout->StartRow(0, 0);
-  views::ImageView* icon = new views::ImageView();
-  icon->SetImage(service->favicon.ToImageSkia());
-  grid_layout->AddView(icon);
+  // Header.
+  views::GridLayout* header_layout = new views::GridLayout(header_);
+  header_layout->SetInsets(kHeaderTopPadding, kHeaderLeftPadding,
+                           kHeaderBottomPadding, kHeaderRightPadding);
+  header_->SetLayoutManager(header_layout);
+  views::ColumnSet* header_view_cs = header_layout->AddColumnSet(0);
+  header_view_cs->AddColumn(GridLayout::LEADING, GridLayout::CENTER, 0,
+                       GridLayout::USE_PREF, 0, 0);  // Icon.
+  header_view_cs->AddPaddingColumn(0, kIconLabelHorizontalSpacing);
+  header_view_cs->AddColumn(GridLayout::LEADING, GridLayout::CENTER, 1,
+                            GridLayout::USE_PREF, 0, 0);  // Title.
+  if (show_use_another) {
+    header_view_cs->AddPaddingColumn(0,
+        views::kUnrelatedControlHorizontalSpacing);
+    header_view_cs->AddColumn(GridLayout::TRAILING, GridLayout::CENTER, 0,
+                              GridLayout::USE_PREF, 0, 0);  // Use another app.
+  }
 
-  string16 elided_title = ui::ElideText(
-      service->title, gfx::Font(), kTitleLinkMaxWidth, ui::ELIDE_AT_END);
-  views::Label* title = new views::Label(elided_title);
-  grid_layout->AddView(title);
+  header_layout->StartRow(0, 0);
+  inline_intent_icon_->SetImage(service->favicon.ToImageSkia());
+  header_layout->AddView(inline_intent_icon_);
+  inline_intent_label_->SetText(ui::ElideText(service->title,
+      inline_intent_label_->font(), kTitleLinkMaxWidth, ui::ELIDE_AT_END));
+  header_layout->AddView(inline_intent_label_);
   // Add link for "choose another service" if other suggestions are available
   // or if more than one (the current) service is installed.
-  if (model_->GetInstalledServiceCount() > 1 ||
-       model_->GetSuggestedExtensionCount()) {
-    choose_another_service_link_ = new views::Link(
+  if (show_use_another) {
+    choose_another_service_link_->SetText(
         l10n_util::GetStringUTF16(IDS_INTENT_PICKER_USE_ALTERNATE_SERVICE));
-    grid_layout->AddView(choose_another_service_link_);
     choose_another_service_link_->set_listener(this);
+    header_layout->AddView(choose_another_service_link_);
   }
 
-  if (use_close_button_)
-    grid_layout->AddView(CreateCloseButton(this));
+  // Content.
+  views::GridLayout* grid_layout = new views::GridLayout(contents_);
+  grid_layout->SetInsets(kTopPadding, kLeftPadding, kBottomPadding,
+                         kRightPadding);
+  contents_->SetLayoutManager(grid_layout);
+
+  grid_layout->AddColumnSet(kSeparatorRowColumnSet)->AddColumn(
+      GridLayout::FILL, GridLayout::CENTER, 1, GridLayout::USE_PREF, 0, 0);
+
+  views::ColumnSet* full_cs =
+      grid_layout->AddColumnSet(kWebContentsRowColumnSet);
+  full_cs->AddPaddingColumn(0, kWebContentsLeftPadding);
+  full_cs->AddColumn(GridLayout::CENTER, GridLayout::CENTER, 1,
+                     GridLayout::USE_PREF, 0, 0);
+  full_cs->AddPaddingColumn(0, kWebContentsRightPadding);
+
+  // Separator row.
+  grid_layout->StartRow(0, kSeparatorRowColumnSet);
+  grid_layout->AddView(new views::Separator());
+
+  // Padding row.
+  grid_layout->AddPaddingRow(0, kWebContentsTopPadding);
 
   // Inline web contents row.
-  grid_layout->StartRow(0, 1);
-  grid_layout->AddView(webview_, 1, 1, GridLayout::CENTER,
-                       GridLayout::CENTER, 0, 0);
+  grid_layout->StartRow(0, kWebContentsRowColumnSet);
+  grid_layout->AddView(webview_);
+
   contents_->Layout();
   SizeToContents();
   displaying_web_contents_ = true;
 }
 
 void WebIntentPickerViews::OnModelChanged(WebIntentPickerModel* model) {
   if (waiting_view_ && !model->IsWaitingForSuggestions())
     UpdateContents();
 
+  string16 suggestions_label_text = model->GetSuggestionsLinkText();
   if (suggestions_label_) {
-    string16 label_text = model->GetSuggestionsLinkText();
-    suggestions_label_->SetText(label_text);
-    suggestions_label_->SetVisible(!label_text.empty());
+    suggestions_label_->SetText(suggestions_label_text);
+    suggestions_label_->SetVisible(!suggestions_label_text.empty());

[Peter Kasting, 2012/10/12 03:23:32]

Nit: It's really weird to have a view that is both optionally NULL _and_
optionally visible.  I suggest eliminating one of those two optionals.

[please use gerrit instead, 2012/10/14 18:40:45]

Done.

   }
 
   if (extensions_)
     extensions_->Update();
   contents_->Layout();
   SizeToContents();
 }
 
 void WebIntentPickerViews::OnFaviconChanged(WebIntentPickerModel* model,
                                             size_t index) {
@@ skipping 59 matching lines @@
 void WebIntentPickerViews::OnActionButtonClicked(
     IntentRowView::ActionType type, size_t tag) {
   DCHECK_EQ(IntentRowView::ACTION_INVOKE, type);
   const WebIntentPickerModel::InstalledService& service =
       model_->GetInstalledServiceAt(tag);
   delegate_->OnServiceChosen(service.url, service.disposition);
 }
 
 void WebIntentPickerViews::UpdateContents() {
   if (model_ && model_->IsWaitingForSuggestions()) {
-    ClearContents();
-    contents_->SetLayoutManager(new views::FillLayout());
-    waiting_view_ = new WaitingView(this, use_close_button_);
-    contents_->AddChildView(waiting_view_);
-    int height = contents_->GetHeightForWidth(kWindowMinWidth);
-    contents_->SetSize(gfx::Size(kWindowMinWidth, height));
-    contents_->Layout();
+    ShowWaitingForSuggestions();
   } else if (model_->GetInstalledServiceCount() ||
       model_->GetSuggestedExtensionCount()) {
     ShowAvailableServices();
   } else {
     ShowNoServicesMessage();
   }
   SizeToContents();
 }
 
+void WebIntentPickerViews::ShowWaitingForSuggestions() {
+  ClearContents();
+  contents_->SetLayoutManager(new views::FillLayout());
+  waiting_view_ = new WaitingView();
+  contents_->AddChildView(waiting_view_);
+  int height = contents_->GetHeightForWidth(kWindowMinWidth);
+  contents_->SetSize(gfx::Size(kWindowMinWidth, height));
+  contents_->Layout();
+}
+
 const string16 WebIntentPickerViews::GetActionTitle() {
-  return (!action_text_.empty()) ?
-      action_text_ :
+  return (!action_text_.empty()) ? action_text_ :
       l10n_util::GetStringUTF16(IDS_INTENT_PICKER_CHOOSE_SERVICE);
 }
 
 void WebIntentPickerViews::ShowAvailableServices() {
-  enum {
-    kHeaderRowColumnSet,  // Column set for header layout.
-    kFullWidthColumnSet,  // Column set with a single full-width column.
-    kIndentedFullWidthColumnSet,  // Single full-width column, indented.
-  };
+  const int kHeaderTopBorder = 8;
+  const int kHeaderLeftBorder = 0;
+  const int kHeaderBottomBorder = 0;
+  const int kHeaderRightBorder = 0;
+
+  const int kTopPadding = 13;
+  const int kLeftPadding = 20;
+  const int kBottomPadding = 16;
+  const int kRightPadding = 20;
+
+  const int kAppsSuggestionsSpacing = 14;
 
   ClearContents();
   displaying_web_contents_ = false;
-
   extensions_ = new IntentsView(model_, this);
 
+  // Header view.
+  action_label_->SetText(GetActionTitle());
+
+  views::GridLayout* header_layout = new views::GridLayout(header_);
+  header_->SetLayoutManager(header_layout);
+  header_layout->SetInsets(kHeaderTopBorder, kHeaderLeftBorder,
+                           kHeaderBottomBorder, kHeaderRightBorder);
+  header_layout->AddColumnSet(0)->AddColumn(GridLayout::LEADING,
+      GridLayout::CENTER, 1, GridLayout::USE_PREF, 0, 0); // Action label
+  header_layout->StartRow(0, 0);
+  header_layout->AddView(action_label_);
+
+  // Content view.
   views::GridLayout* grid_layout = new views::GridLayout(contents_);
+  grid_layout->set_minimum_size(
+      gfx::Size(extensions_->AdjustWidth(kWindowMinWidth), 0));
+  grid_layout->SetInsets(kTopPadding, kLeftPadding, kBottomPadding,
+                         kRightPadding);
   contents_->SetLayoutManager(grid_layout);
 
-  grid_layout->set_minimum_size(
-      gfx::Size(extensions_->AdjustWidth(kWindowMinWidth), 0));
-  grid_layout->SetInsets(kContentAreaBorder, kContentAreaBorder,
-                         kContentAreaBorder, kContentAreaBorder);
-  views::ColumnSet* header_cs = grid_layout->AddColumnSet(kHeaderRowColumnSet);
-  header_cs->AddColumn(GridLayout::LEADING, GridLayout::CENTER, 1,
-                       GridLayout::USE_PREF, 0, 0);  // Title.
-  if (use_close_button_) {
-    header_cs->AddPaddingColumn(0, views::kUnrelatedControlHorizontalSpacing);
-    header_cs->AddColumn(GridLayout::CENTER, GridLayout::CENTER, 0,
-                         GridLayout::USE_PREF, 0, 0);  // Close Button.
-  }
+  grid_layout->AddColumnSet(0)->AddColumn(GridLayout::FILL, GridLayout::CENTER,
+      1, GridLayout::USE_PREF, 0, 0); // Content.
 
-  views::ColumnSet* full_cs = grid_layout->AddColumnSet(kFullWidthColumnSet);
-  full_cs->AddColumn(GridLayout::FILL, GridLayout::CENTER, 1,
-                     GridLayout::USE_PREF, 0, 0);
+  // Row with extension suggestions.
+  grid_layout->StartRow(0, 0);
+  grid_layout->AddView(extensions_);
 
-  views::ColumnSet* indent_cs =
-      grid_layout->AddColumnSet(kIndentedFullWidthColumnSet);
-  indent_cs->AddPaddingColumn(0, views::kUnrelatedControlHorizontalSpacing);
-  indent_cs->AddColumn(GridLayout::FILL, GridLayout::CENTER, 1,
-                       GridLayout::USE_PREF, 0, 0);
+  // Padding row.
+  grid_layout->AddPaddingRow(0, kAppsSuggestionsSpacing);
 
   ui::ResourceBundle& rb = ui::ResourceBundle::GetSharedInstance();
 
-  // Header row.
-  grid_layout->StartRow(0, kHeaderRowColumnSet);
-  action_label_ = new views::Label(GetActionTitle());
-  action_label_->SetHorizontalAlignment(views::Label::ALIGN_LEFT);
-  action_label_->SetFont(rb.GetFont(ui::ResourceBundle::MediumFont));
-  grid_layout->AddView(action_label_);
-
-  if (use_close_button_)
-    grid_layout->AddView(CreateCloseButton(this));
-
-  // Padding row.
-  grid_layout->AddPaddingRow(0, views::kRelatedControlVerticalSpacing);
-
-  // Row with app suggestions label.
-  grid_layout->StartRow(0, kIndentedFullWidthColumnSet);
-  suggestions_label_ = new views::Label();
-  suggestions_label_->SetVisible(false);
-  suggestions_label_->SetMultiLine(true);
-  suggestions_label_->SetHorizontalAlignment(views::Label::ALIGN_LEFT);
-  grid_layout->AddView(suggestions_label_);
-
-  // Padding row.
-  grid_layout->AddPaddingRow(0, views::kRelatedControlVerticalSpacing);
-
-  // Row with extension suggestions.
-  grid_layout->StartRow(0, kIndentedFullWidthColumnSet);
-  grid_layout->AddView(extensions_);
-
-  // Padding row.
-  grid_layout->AddPaddingRow(0, views::kRelatedControlVerticalSpacing);
-
   // Row with "more suggestions" link.
-  grid_layout->StartRow(0, kFullWidthColumnSet);
+  grid_layout->StartRow(0, 0);
   views::View* more_view = new views::View();
   more_view->SetLayoutManager(
       new views::BoxLayout(views::BoxLayout::kHorizontal, 0, 0,
-                           views::kRelatedControlHorizontalSpacing));
+                           kIconLabelHorizontalSpacing));
   views::ImageView* icon = new views::ImageView();
   icon->SetImage(rb.GetImageNamed(IDR_WEBSTORE_ICON_16).ToImageSkia());
   more_view->AddChildView(icon);
-  more_suggestions_link_ = new views::Link(
-    l10n_util::GetStringUTF16(IDS_FIND_MORE_INTENT_HANDLER_MESSAGE));
-  more_suggestions_link_->SetDisabledColor(kDisabledLinkColor);
+  more_suggestions_link_ = CreateLink();
+  more_suggestions_link_->SetText(
+      l10n_util::GetStringUTF16(IDS_FIND_MORE_INTENT_HANDLER_MESSAGE));
   more_suggestions_link_->set_listener(this);
   more_view->AddChildView(more_suggestions_link_);
   grid_layout->AddView(more_view, 1, 1,
                        GridLayout::LEADING, GridLayout::CENTER);
+
   contents_->Layout();
 }
 
 void WebIntentPickerViews::ResetContents() {
   // Abandon both web contents and webview.
   webview_->SetWebContents(NULL);
   inline_web_contents_.reset();
   webview_ = NULL;
 
   // Re-initialize the UI.
   UpdateContents();
 
   // Restore previous state.
   extensions_->Update();
   action_label_->SetText(action_text_);
   contents_->Layout();
   SizeToContents();
 }
 
 void WebIntentPickerViews::SizeToContents() {
   gfx::Size client_size = contents_->GetPreferredSize();
   gfx::Rect client_bounds(client_size);
   gfx::Rect new_window_bounds = window_->non_client_view()->frame_view()->
       GetWindowBoundsForClientBounds(client_bounds);
   window_->CenterWindow(new_window_bounds.size());
 }
 
 void WebIntentPickerViews::ClearContents() {
   DCHECK(contents_);
+  DCHECK(header_);
   // The call RemoveAllChildViews(true) deletes all children of |contents|. If
   // we do not set our weak pointers to NULL, then they will continue to point
   // to where the deleted objects used to be, i.e. unitialized memory. This
   // would cause hard-to-explain crashes.
   contents_->RemoveAllChildViews(true);
-  action_label_ = NULL;
+  header_->RemoveAllChildViews(false);
   suggestions_label_ = NULL;
   extensions_ = NULL;
   more_suggestions_link_ = NULL;
-  choose_another_service_link_ = NULL;
 }