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/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/constrained_window_constants.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"
 #include "chrome/browser/ui/views/frame/browser_view.h"
 #include "chrome/browser/ui/views/location_bar/location_icon_view.h"
 #include "chrome/browser/ui/views/toolbar_view.h"
@@ skipping 15 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;
 
+// THe vertical padding around the UI elements in the waiting view.

[Peter Kasting, 2012/10/15 21:46:54]

Nit: The

[please use gerrit instead, 2012/10/16 00:12:23]

Done.

+const int kWaitingViewVerticalPadding = 40;
+
 // 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) {
   ui::ResourceBundle& rb = ui::ResourceBundle::GetSharedInstance();
   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;
 }
-// SarsView -------------------------------------------------------------------
+
+// Creates a label.
+views::Label* CreateLabel() {
+  views::Label* label = new views::Label();
+  label->SetEnabledColor(kEnabledLabelColor);
+  label->SetHorizontalAlignment(views::Label::ALIGN_LEFT);
+  return label;
+}
+
+// Creates a title-style label.
+views::Label* CreateTitleLabel() {
+  const int kLabelBuiltinTopPadding = 5;
+  const int kTopPadding = WebIntentPicker::kContentAreaBorder -
+      ConstrainedWindowConstants::kCloseButtonPadding - kLabelBuiltinTopPadding;
+  views::Label* label = CreateLabel();
+  label->SetFont(ui::ResourceBundle::GetSharedInstance().GetFont(
+      ui::ResourceBundle::MediumFont));
+  label->set_border(views::Border::CreateEmptyBorder(
+      kTopPadding, 0, 0, 0));
+  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);
+  return link;
+}
+
+// Creates a header for the inline disposition dialog.
+views::View* CreateInlineDispositionHeader(
+    views::ImageView* app_icon,
+    views::Label* app_title,
+    views::Link* use_another_service_link) {
+  const int kIconBuiltinTopPadding = 6;
+  const int kTopPadding =  WebIntentPicker::kContentAreaBorder -

[Peter Kasting, 2012/10/15 21:46:54]

Nit: Extra space

[please use gerrit instead, 2012/10/16 00:12:23]

Removed the extra space. Good eye!

+      ConstrainedWindowConstants::kCloseButtonPadding - kIconBuiltinTopPadding;
+  views::View* header = new views::View();
+  views::GridLayout* grid_layout = new views::GridLayout(header);
+  grid_layout->SetInsets(kTopPadding, 0, 0, 0);
+  header->SetLayoutManager(grid_layout);
+  views::ColumnSet* header_cs = grid_layout->AddColumnSet(0);
+  header_cs->AddColumn(GridLayout::LEADING, GridLayout::CENTER, 0,
+                       GridLayout::USE_PREF, 0, 0);  // App icon.
+  header_cs->AddPaddingColumn(0, WebIntentPicker::kIconTextPadding);
+  header_cs->AddColumn(GridLayout::LEADING, GridLayout::CENTER, 1,
+                       GridLayout::USE_PREF, 0, 0);  // App title.
+  header_cs->AddPaddingColumn(0, views::kUnrelatedControlHorizontalSpacing);
+  header_cs->AddColumn(GridLayout::TRAILING, GridLayout::CENTER, 0,
+                       GridLayout::USE_PREF, 0, 0);  // Use another app link.
+  grid_layout->StartRow(0, 0);
+  grid_layout->AddView(app_icon);
+  grid_layout->AddView(app_title);
+  grid_layout->AddView(use_another_service_link);
+  header->Layout();
+  return header;
+}
+
+// Checks whether the inline disposition dialog should show the link for using
+// another service.
+bool IsUseAnotherServiceVisible(WebIntentPickerModel* model) {
+  DCHECK(model);
+  return model->show_use_another_service()
+      && (model->GetInstalledServiceCount() > 1 ||

[Peter Kasting, 2012/10/15 21:46:54]

Nit: Operators go at end of line

[please use gerrit instead, 2012/10/16 00:12:23]

Fixed.

+          model->GetSuggestedExtensionCount());
+}
+
+// StarsView -------------------------------------------------------------------

[Peter Kasting, 2012/10/15 21:46:54]

Tiny nit: To make the eye more likely to see these class division comments, I
generally try to put two blank lines above them rather than one.

[please use gerrit instead, 2012/10/16 00:12:23]

Done. I hope that we will split these into separate files in the near future. If
there's logical grouping, the files should be in a folder and in a namespace,
not in the same gigantic file.

 
 // 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 {
+ public:
+  SpinnerProgressIndicator();

[Peter Kasting, 2012/10/15 21:46:54]

Nit: I suggest adding a blank line below the constructor, and explicitly
declaring a (virtual) destructor (2 places)

[please use gerrit instead, 2012/10/16 00:12:23]

Declared the virtual destructor here. Where is the second place?

[Peter Kasting, 2012/10/16 00:55:06]

WaitingView.

[please use gerrit instead, 2012/10/16 00:57:20]

Done.

+  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 percent_done_;
+  int indeterminate_;
+
+  base::TimeTicks start_time_;
+  base::RepeatingTimer<SpinnerProgressIndicator> timer_;
+};

[Peter Kasting, 2012/10/15 21:46:54]

Nit: DISALLOW_COPY_AND_ASSIGN

[please use gerrit instead, 2012/10/16 00:12:23]

Added.

+
+SpinnerProgressIndicator::SpinnerProgressIndicator()
+  : percent_done_(0),

[Peter Kasting, 2012/10/15 21:46:54]

Nit: Indent 4

[please use gerrit instead, 2012/10/16 00:12:23]

Done.

+    indeterminate_(true) {}
+
+void SpinnerProgressIndicator::SetPercentDone(int percent) {
+  percent_done_ = percent;
+  SchedulePaint();
+  UpdateTimer();
+}
+
+void SpinnerProgressIndicator::SetIndeterminate(bool indetereminate) {
+  indeterminate_ = 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(),
+      indeterminate_ ? -1 : percent_done_);
+}
+
+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() || !indeterminate_) {
+    timer_.Stop();
+    return;
+  }
+
+  if (!timer_.IsRunning()) {
+    start_time_ = base::TimeTicks::Now();
+    timer_.Start(FROM_HERE, base::TimeDelta::FromMilliseconds(kTimerIntervalMs),
+        this, &SpinnerProgressIndicator::SchedulePaint);
+  }
+}
+
+int SpinnerProgressIndicator::GetProgressAngle() {
+  if (!indeterminate_)
+    return download_util::kStartAngleDegrees;
+  base::TimeDelta delta = base::TimeTicks::Now() - start_time_;
+  int angle = delta.InSecondsF() * kSpinRateDegreesPerSecond;
+  return angle % 360;
+}
+
 // WaitingView ----------------------------------------------------------
 class WaitingView : public views::View {
  public:
   WaitingView(views::ButtonListener* listener, bool use_close_button);
 
  private:
   DISALLOW_COPY_AND_ASSIGN(WaitingView);
 };
 
-WaitingView::WaitingView(views::ButtonListener* listener,
-                         bool use_close_button) {
+WaitingView::WaitingView(
+    views::ButtonListener* listener,

[Peter Kasting, 2012/10/15 21:46:54]

Nit: Old indenting was fine

[please use gerrit instead, 2012/10/16 00:12:23]

Returned to the old formatting style.

+    bool use_close_button) {
+

[Peter Kasting, 2012/10/15 21:46:54]

Nit: Extra blank line

[please use gerrit instead, 2012/10/16 00:12:23]

Removed the extra blank line.

+  views::ImageButton* close_button = CreateCloseButton(listener);
+
+  const int kThrobberTopPadding = kWaitingViewVerticalPadding -

[Peter Kasting, 2012/10/15 21:46:54]

Nit: Don't need to declare consts/enums atop the function; I would generally
suggest declaring directly above where these are used.  Sometimes this also
allows you to eliminate the declaration of a constant without losing clarity;
example from elsewhere in this file:

  const int kHeaderBuiltinBottomPadding = 4;
  const int kSeparatorTopPadding = ConstrainedWindowConstants::kRowPadding -
      kHeaderBuiltinBottomPadding;
  ...
  grid_layout->AddPaddingRow(0, kSeparatorTopPadding);

-->

  const int kHeaderBuiltinBottomPadding = 4;
  grid_layout->AddPaddingRow(0,
      ConstrainedWindowConstants::kRowPadding - kHeaderBuiltinBottomPadding);

(several places across file)

[please use gerrit instead, 2012/10/16 00:12:23]

I combed over this file to get rid of unnecessary constants and move the
necessary constants closer to where they are used. Done.

+      ConstrainedWindowConstants::kCloseButtonPadding -
+      close_button->GetPreferredSize().height();
+
+  const int kMessageBuiltinTopPadding = 5;
+  const int kThrobberMessagePadding = ConstrainedWindowConstants::kRowPadding -
+      kMessageBuiltinTopPadding;
+
+  const int kMessageBuiltinBottomPadding = 3;
+  const int kBottomPadding = kWaitingViewVerticalPadding -
+      kMessageBuiltinBottomPadding;
+
+  enum {

[Peter Kasting, 2012/10/15 21:46:54]

Nit: In addition to declaring right above where it's first used, I suggest also
giving the enum a name:

  enum GridLayoutColumnSets { ...

[please use gerrit instead, 2012/10/16 00:12:23]

Done.

+    kHeaderRow,

[Peter Kasting, 2012/10/15 21:46:54]

Nit: Chromium style guide says to use MACRO_STYLE for enum values rather than
kConstantStyle (as here) or CamelCase (as elsewhere)

[please use gerrit instead, 2012/10/16 00:12:23]

Done.

+    kContentRow,
+  };
+
   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(ConstrainedWindowConstants::kCloseButtonPadding, 0,
+                    kBottomPadding, 0);
   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.
-  }
-  cs->AddPaddingColumn(0, views::kPanelHorizIndentation);
-  cs->AddColumn(GridLayout::CENTER, GridLayout::CENTER,
-                1, GridLayout::USE_PREF, 0, 0);
-  cs->AddPaddingColumn(0, views::kPanelHorizIndentation);
+  views::ColumnSet* header_cs = layout->AddColumnSet(kHeaderRow);
+  header_cs->AddPaddingColumn(1, 1);
+  header_cs->AddColumn(GridLayout::TRAILING, GridLayout::LEADING, 0,
+                       GridLayout::USE_PREF, 0, 0);
+  header_cs->AddPaddingColumn(
+      0, ConstrainedWindowConstants::kCloseButtonPadding);
 
-  // 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();
+  views::ColumnSet* content_cs = layout->AddColumnSet(kContentRow);
+  content_cs->AddPaddingColumn(0, views::kPanelHorizIndentation);
+  content_cs->AddColumn(GridLayout::CENTER, GridLayout::CENTER, 1,
+                        GridLayout::USE_PREF, 0, 0);
+  content_cs->AddPaddingColumn(0, views::kPanelHorizIndentation);
 
-  // Create text.
-  views::Label* label = new views::Label();
+  // Close button
+  layout->StartRow(0, kHeaderRow);
+  layout->AddView(close_button);
+  close_button->SetVisible(use_close_button);
+
+  layout->AddPaddingRow(0, kThrobberTopPadding);
+
+  // Throbber
+  layout->StartRow(0, kContentRow);
+  SpinnerProgressIndicator* throbber = new SpinnerProgressIndicator();
+  layout->AddView(throbber);
+
+  layout->AddPaddingRow(0, kThrobberMessagePadding);
+
+  // Message
+  layout->StartRow(0, kContentRow);
+  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->AddView(label);
 
-  // Layout the view.
-  if (use_close_button) {
-    layout->StartRow(0, 1);
-    layout->AddView(CreateCloseButton(listener));
-  }
-
-  layout->AddPaddingRow(0, views::kUnrelatedControlLargeVerticalSpacing);
-  layout->StartRow(0, 0);
-  layout->AddView(throbber);
-  layout->AddPaddingRow(0, views::kUnrelatedControlLargeVerticalSpacing);
-  layout->StartRow(0, 0);
-  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;

[Peter Kasting, 2012/10/15 21:46:54]

Nit: Does this need to be a member?

[please use gerrit instead, 2012/10/16 00:12:23]

I moved it right next to the place where it is used inside of
IntentRowView::CreateHandlerRow(). Done.

+
   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 54 matching lines @@
                                         WebIntentPicker::kTitleLinkMaxWidth,
                                         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 = CreateLabel();
+    label->SetText(elided_title);
   } else {
     view = new IntentRowView(ACTION_INSTALL, tag);
     view->extension_id_ = extension->id;
     icon = extension->icon.ToImageSkia();
-    views::Link* link = new views::Link(elided_title);
+    views::Link* link = CreateLink();
+    link->SetText(elided_title);
     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::kIconTextPadding);
+  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,

[Peter Kasting, 2012/10/15 21:46:54]

Nit: Old wrapping was more correct

[please use gerrit instead, 2012/10/16 00:12:23]

Returned to the original wrapping style. Done.

+      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 105 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);
+  const int kAppRowVerticalSpacing = 10;
+  views::BoxLayout* layout = new views::BoxLayout(views::BoxLayout::kVertical,
+                                                  0, 0, kAppRowVerticalSpacing);
   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 35 matching lines @@
   EnableChildViews(this, enabled());
   View::OnEnabledChanged();
 }
 
 }  // namespace
 
 // WebIntentPickerViews --------------------------------------------------------
 
 // Views implementation of WebIntentPicker.
 class WebIntentPickerViews : public views::ButtonListener,
-                             public views::DialogDelegate,
+                             public views::WidgetDelegate,
                              public views::LinkListener,
                              public WebIntentPicker,
                              public WebIntentPickerModelObserver,
                              public IntentRowView::Delegate {
  public:
   WebIntentPickerViews(TabContents* tab_contents,
                        WebIntentPickerDelegate* delegate,
                        WebIntentPickerModel* model);
   virtual ~WebIntentPickerViews();
 
   // views::ButtonListener implementation.
   // 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.
+  // views::WidgetDelegate 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 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;
   virtual void OnExtensionInstallFailure(const std::string& id) OVERRIDE;
   virtual void OnInlineDispositionAutoResize(const gfx::Size& size) OVERRIDE;
@@ skipping 14 matching lines @@
   virtual void OnExtensionInstallClicked(
       const std::string& extension_id) OVERRIDE;
   virtual void OnExtensionLinkClicked(
       const std::string& extension_id,
       WindowOpenDisposition disposition) OVERRIDE;
   virtual void OnActionButtonClicked(
       IntentRowView::ActionType type,
       size_t tag) OVERRIDE;
 
  private:
+  enum {
+    Initial,
+    Waiting,
+    NoServices,
+    ListServices,
+    InlineService,
+  } state_;
+
   // 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.
   void ResetContents();
 
   // Resize the constrained window to the size of its contents.
   void SizeToContents();
 
   // Clear the contents of the picker.
   void ClearContents();
 
   // Returns the service selection question text used in the title
   // of the picker.
   const string16 GetActionTitle();
 
+  // Refresh the icon for the inline disposition service that is being
+  // displayed.
+  void RefreshInlineServiceIcon();
+
   // A weak pointer to the WebIntentPickerDelegate to notify when the user
   // 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_;
 
-  // 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_;
 
   // 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_;
 
+  // The icon for the inline disposition service.
+  views::ImageView* inline_service_icon_;
+
   // A weak pointer to the choose another service link.
   // Created locally, owned by Views.
   views::Link* choose_another_service_link_;
 
   // 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),
+    : state_(Initial),
+      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),
+      inline_service_icon_(NULL),
       choose_another_service_link_(NULL),
       waiting_view_(NULL),
-      displaying_web_contents_(false),
       can_close_(true) {
   bool enable_chrome_style = chrome::IsFramelessConstrainedDialogEnabled();
   use_close_button_ = enable_chrome_style;
 
   model_->set_observer(this);
   contents_ = new views::View();
+  contents_->set_background(views::Background::CreateSolidBackground(
+      ConstrainedWindow::GetBackgroundColor()));
+
   // Show the dialog.
-  window_ = new ConstrainedWindowViews(tab_contents->web_contents(),
-                                       this,
-                                       enable_chrome_style);
-
+  window_ = new ConstrainedWindowViews(tab_contents->web_contents(), this,
+                                       enable_chrome_style, gfx::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_;
 }
 
-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_)
+  if (action_label_) {
     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();
+  extensions_->SetEnabled(true);
   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() {
+  const int kHeaderBuiltinBottomPadding = 4;
+  const int kContentBuiltinTopPadding = 5;
+  const int kHeaderContentPadding = ConstrainedWindowConstants::kRowPadding -
+      kHeaderBuiltinBottomPadding - kContentBuiltinTopPadding;
+
+  const int kContentBuiltinBottomPadding = 3;
+  const int kContentBottomPadding =
+      ConstrainedWindowConstants::kClientBottomPadding -
+      kContentBuiltinBottomPadding;
+
+  enum {
+    kHeaderRow,
+    kContentRow,
+  };
+
+  if (state_ == NoServices)
+    return;
+  state_ = NoServices;
+
   ClearContents();
-
-  views::GridLayout* grid_layout = new views::GridLayout(contents_);
-  contents_->SetLayoutManager(grid_layout);
-
-  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);
-
-  ui::ResourceBundle& rb = ui::ResourceBundle::GetSharedInstance();
-
-  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(
+  views::GridLayout* layout = new views::GridLayout(contents_);
+  layout->set_minimum_size(gfx::Size(WebIntentPicker::kWindowMinWidth, 0));
+  layout->SetInsets(ConstrainedWindowConstants::kCloseButtonPadding, 0,
+                    kContentBottomPadding, 0);
+  contents_->SetLayoutManager(layout);
+
+  views::ColumnSet* header_cs = layout->AddColumnSet(kHeaderRow);
+  header_cs->AddPaddingColumn(
+      0, ConstrainedWindowConstants::kHorizontalPadding);
+  header_cs->AddColumn(GridLayout::LEADING, GridLayout::CENTER, 1,
+                       GridLayout::USE_PREF, 0, 0); // Title
+  header_cs->AddColumn(GridLayout::TRAILING, GridLayout::LEADING, 0,
+                       GridLayout::USE_PREF, 0, 0); // Close button
+  header_cs->AddPaddingColumn(
+      0, ConstrainedWindowConstants::kCloseButtonPadding);
+
+  views::ColumnSet* content_cs = layout->AddColumnSet(kContentRow);
+  content_cs->AddPaddingColumn(
+      0, ConstrainedWindowConstants::kHorizontalPadding);
+  content_cs->AddColumn(GridLayout::FILL, GridLayout::CENTER, 1,
+                        GridLayout::USE_PREF, 0, 0); // Body
+  content_cs->AddPaddingColumn(
+      0, ConstrainedWindowConstants::kHorizontalPadding);
+
+  // Header
+  layout->StartRow(0, kHeaderRow);
+  views::Label* title = CreateTitleLabel();
+  title->SetText(l10n_util::GetStringUTF16(
       IDS_INTENT_PICKER_NO_SERVICES_TITLE));
-  grid_layout->AddView(header);
-
-  grid_layout->AddPaddingRow(0, views::kRelatedControlVerticalSpacing);
-
-  grid_layout->StartRow(0, 0);
-  views::Label* body = new views::Label();
+  layout->AddView(title);
+
+  views::ImageButton* close_button = CreateCloseButton(this);
+  layout->AddView(close_button);
+  close_button->SetVisible(use_close_button_);
+
+  // Padding
+  layout->AddPaddingRow(0, kHeaderContentPadding);
+
+  // Content
+  layout->StartRow(0, kContentRow);
+  views::Label* body = CreateLabel();
   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));
+  layout->AddView(body);
+
+  int height = contents_->GetHeightForWidth(WebIntentPicker::kWindowMinWidth);
+  contents_->SetSize(gfx::Size(WebIntentPicker::kWindowMinWidth, height));
+  contents_->Layout();
 }
 
 void WebIntentPickerViews::OnInlineDispositionWebContentsLoaded(
     content::WebContents* web_contents) {
-  if (displaying_web_contents_)
+  const int kHeaderBuiltinBottomPadding = 4;
+  const int kSeparatorTopPadding = ConstrainedWindowConstants::kRowPadding -
+      kHeaderBuiltinBottomPadding;
+
+  const int kSeparatorBottomPadding = 3;
+
+  enum {
+    kHeaderRow,
+    kSeparatorRow,
+    kWebContentsRow,
+  };
+
+  if (state_ == InlineService)
     return;
+  state_ = InlineService;
 
   // Replace the picker with the inline disposition.
   ClearContents();
-
   views::GridLayout* grid_layout = new views::GridLayout(contents_);
+  grid_layout->set_minimum_size(gfx::Size(WebIntentPicker::kWindowMinWidth, 0));
+  grid_layout->SetInsets(ConstrainedWindowConstants::kCloseButtonPadding, 0,
+                         ConstrainedWindowConstants::kClientBottomPadding, 0);
   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);
+  views::ColumnSet* header_cs = grid_layout->AddColumnSet(kHeaderRow);
+  header_cs->AddPaddingColumn(
+      0, ConstrainedWindowConstants::kHorizontalPadding);
+  header_cs->AddColumn(GridLayout::FILL, GridLayout::CENTER, 1,
+                       GridLayout::USE_PREF, 0, 0); // Icon, title, link.
+  header_cs->AddPaddingColumn(0, views::kRelatedControlHorizontalSpacing);
+  header_cs->AddColumn(GridLayout::TRAILING, GridLayout::LEADING, 0,
+                       GridLayout::USE_PREF, 0, 0); // Close button.
+  header_cs->AddPaddingColumn(
+      0, ConstrainedWindowConstants::kCloseButtonPadding);
+
+  views::ColumnSet* sep_cs = grid_layout->AddColumnSet(kSeparatorRow);
+  sep_cs->AddColumn(GridLayout::FILL, GridLayout::CENTER, 1,
+                    GridLayout::USE_PREF, 0, 0); // Separator.
+
+  views::ColumnSet* contents_cs = grid_layout->AddColumnSet(kWebContentsRow);
+  contents_cs->AddPaddingColumn(0, 1);
+  contents_cs->AddColumn(GridLayout::CENTER, GridLayout::CENTER, 1,
+                         GridLayout::USE_PREF, 0, 0); // Web contents.
+  contents_cs->AddPaddingColumn(0, 1);
+
+  // Header.
+  grid_layout->StartRow(0, kHeaderRow);
 
   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);
-
-  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);
-  // 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(
-        l10n_util::GetStringUTF16(IDS_INTENT_PICKER_USE_ALTERNATE_SERVICE));
-    grid_layout->AddView(choose_another_service_link_);
-    choose_another_service_link_->set_listener(this);
-  }
-
-  if (use_close_button_)
-    grid_layout->AddView(CreateCloseButton(this));
-
-  // Inline web contents row.
-  grid_layout->StartRow(0, 1);
-  grid_layout->AddView(webview_, 1, 1, GridLayout::CENTER,
-                       GridLayout::CENTER, 0, 0);
+  if (!inline_service_icon_)
+    inline_service_icon_ = new views::ImageView();
+  inline_service_icon_->SetImage(service->favicon.ToImageSkia());
+
+  views::Label* title = CreateLabel();
+  title->SetText(ui::ElideText(
+      service->title, title->font(), kTitleLinkMaxWidth, ui::ELIDE_AT_END));
+
+  if (!choose_another_service_link_)
+    choose_another_service_link_ = CreateLink();
+  choose_another_service_link_->SetText(l10n_util::GetStringUTF16(
+      IDS_INTENT_PICKER_USE_ALTERNATE_SERVICE));
+  choose_another_service_link_->set_listener(this);
+
+  grid_layout->AddView(CreateInlineDispositionHeader(
+      inline_service_icon_, title, choose_another_service_link_));
+  choose_another_service_link_->SetVisible(IsUseAnotherServiceVisible(model_));
+
+  views::ImageButton* close_button = CreateCloseButton(this);
+  grid_layout->AddView(close_button);
+  close_button->SetVisible(use_close_button_);
+
+  // Padding.

[Peter Kasting, 2012/10/15 21:46:54]

Nit: I'd probably just put the AddPaddingRow() atop the block marked
"Separator."  (several places across file)

[please use gerrit instead, 2012/10/16 00:12:23]

I combed through the file and moved the AddPaddingRow() calls into the blocks
with the beef. Done.

+  grid_layout->AddPaddingRow(0, kSeparatorTopPadding);
+
+  // Separator.
+  grid_layout->StartRow(0, kSeparatorRow);
+  grid_layout->AddView(new views::Separator());
+
+  // Padding.
+  grid_layout->AddPaddingRow(0, kSeparatorBottomPadding);
+
+  // Inline web contents.
+  grid_layout->StartRow(0, kWebContentsRow);
+  grid_layout->AddView(webview_);
+
   contents_->Layout();
   SizeToContents();
-  displaying_web_contents_ = true;
 }
 
 void WebIntentPickerViews::OnModelChanged(WebIntentPickerModel* model) {
-  if (waiting_view_ && !model->IsWaitingForSuggestions())
+  if (state_ == Waiting && !model->IsWaitingForSuggestions())
     UpdateContents();
 
-  if (suggestions_label_) {
-    string16 label_text = model->GetSuggestionsLinkText();
-    suggestions_label_->SetText(label_text);
-    suggestions_label_->SetVisible(!label_text.empty());
-  }
-
-  if (extensions_)
+  if (choose_another_service_link_) {
+    choose_another_service_link_->SetVisible(IsUseAnotherServiceVisible(model));
+    contents_->Layout();
+    SizeToContents();
+  }
+
+  if (extensions_) {

[Peter Kasting, 2012/10/15 21:46:54]

Nit: This block also appears below.  Consider whether it should be in a helper
somewhere.

[please use gerrit instead, 2012/10/16 00:12:23]

Moved this block into WebIntentPickerViews::RefreshExtensions() and calling this
helper from both locations. Done.

     extensions_->Update();
-  contents_->Layout();
-  SizeToContents();
+    contents_->Layout();
+    SizeToContents();
+  }
 }
 
 void WebIntentPickerViews::OnFaviconChanged(WebIntentPickerModel* model,
                                             size_t index) {
   // TODO(groby): Update favicons on extensions_;
-  contents_->Layout();
-  SizeToContents();
+  if (inline_service_icon_)
+    RefreshInlineServiceIcon();
+  if (extensions_) {
+    extensions_->Update();
+    contents_->Layout();
+    SizeToContents();
+  }
 }
 
 void WebIntentPickerViews::OnExtensionIconChanged(
     WebIntentPickerModel* model,
     const std::string& extension_id) {
-  if (extensions_)
+  if (inline_service_icon_)

[Peter Kasting, 2012/10/15 21:46:54]

Nit: The contents of this fucntion and OnFaviconChanged() appear identical. 
Consider making one call the other.

[please use gerrit instead, 2012/10/16 00:12:23]

Done.

+    RefreshInlineServiceIcon();
+  if (extensions_) {
     extensions_->Update();
-
-  contents_->Layout();
-  SizeToContents();
+    contents_->Layout();
+    SizeToContents();
+  }
 }
 
 void WebIntentPickerViews::OnInlineDisposition(
     const string16&, const GURL& url) {
   if (!webview_)
     webview_ = new views::WebView(tab_contents_->profile());
 
   inline_web_contents_.reset(delegate_->CreateWebContentsForInlineDisposition(
       tab_contents_->profile(), url));
   // Does not take ownership, so we keep a scoped_ptr
@@ skipping 37 matching lines @@
     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,
                              WebIntentPickerDelegate::kEnableDefaults);
 }
 
 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() {
+  if (state_ == Waiting)
+    return;
+  state_ = Waiting;
+  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();
+}
+
 const string16 WebIntentPickerViews::GetActionTitle() {
-  return (!action_text_.empty()) ?
-      action_text_ :
+  return (!action_text_.empty()) ? action_text_ :

[Peter Kasting, 2012/10/15 21:46:54]

Nit: No parens around unary expression.  Also consider removing "!" and
reversing arms

[please use gerrit instead, 2012/10/16 00:12:23]

Done.

       l10n_util::GetStringUTF16(IDS_INTENT_PICKER_CHOOSE_SERVICE);
 }
 
 void WebIntentPickerViews::ShowAvailableServices() {
+  const int kHeaderBuiltinBottomPadding = 4;
+  const int kHeaderExtensionsPadding = ConstrainedWindowConstants::kRowPadding -
+      kHeaderBuiltinBottomPadding;
+
+  const int kIconBuiltinTopPadding = 6;
+  const int kExtensionsSuggestionsPadding =
+      ConstrainedWindowConstants::kRowPadding - kIconBuiltinTopPadding;
+
+  const int kIconBuiltinBottomPadding = 4;
+  const int kBottomPadding = ConstrainedWindowConstants::kClientBottomPadding -
+      kIconBuiltinBottomPadding;
+
   enum {
-    kHeaderRowColumnSet,  // Column set for header layout.
-    kFullWidthColumnSet,  // Column set with a single full-width column.
-    kIndentedFullWidthColumnSet,  // Single full-width column, indented.
+    kHeaderRow,
+    kContentRow,
   };
 
   ClearContents();
-  displaying_web_contents_ = false;
-
+  state_ = ListServices;
   extensions_ = new IntentsView(model_, this);
+  gfx::Size min_size = gfx::Size(extensions_->AdjustWidth(kWindowMinWidth), 0);
 
   views::GridLayout* grid_layout = new views::GridLayout(contents_);
+  grid_layout->set_minimum_size(min_size);
+  grid_layout->SetInsets(ConstrainedWindowConstants::kCloseButtonPadding, 0,
+                         kBottomPadding, 0);
   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);
+  views::ColumnSet* header_cs = grid_layout->AddColumnSet(kHeaderRow);
+  header_cs->AddPaddingColumn(
+      0, ConstrainedWindowConstants::kHorizontalPadding);
   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.
-  }
+                       GridLayout::USE_PREF, 0, 0); // Action title
+  header_cs->AddColumn(GridLayout::TRAILING, GridLayout::LEADING, 0,
+                       GridLayout::USE_PREF, 0, 0); // Close button
+  header_cs->AddPaddingColumn(
+      0, ConstrainedWindowConstants::kCloseButtonPadding);
 
-  views::ColumnSet* full_cs = grid_layout->AddColumnSet(kFullWidthColumnSet);
-  full_cs->AddColumn(GridLayout::FILL, GridLayout::CENTER, 1,
-                     GridLayout::USE_PREF, 0, 0);
+  views::ColumnSet* content_cs = grid_layout->AddColumnSet(kContentRow);
+  content_cs->AddPaddingColumn(
+      0, ConstrainedWindowConstants::kHorizontalPadding);
+  content_cs->AddColumn(GridLayout::FILL, GridLayout::CENTER, 1,
+                        GridLayout::USE_PREF, 0, 0); // Content.
+  content_cs->AddPaddingColumn(
+      0, ConstrainedWindowConstants::kHorizontalPadding);
 
-  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);
+  // Header.
+  grid_layout->StartRow(0, kHeaderRow);
+  if (!action_label_)
+    action_label_ = CreateTitleLabel();
+  action_label_->SetText(GetActionTitle());
+  grid_layout->AddView(action_label_);
+
+  views::ImageButton* close_button = CreateCloseButton(this);
+  grid_layout->AddView(close_button);
+  close_button->SetVisible(use_close_button_);
+
+  // Padding.
+  grid_layout->AddPaddingRow(0, kHeaderExtensionsPadding);
+
+  // Extensions.
+  grid_layout->StartRow(0, kContentRow);
+  grid_layout->AddView(extensions_);
+
+  // Padding.
+  grid_layout->AddPaddingRow(0, kExtensionsSuggestionsPadding);
 
   ui::ResourceBundle& rb = ui::ResourceBundle::GetSharedInstance();

[Peter Kasting, 2012/10/15 21:46:54]

Nit: Consider inlining into use below

[please use gerrit instead, 2012/10/16 00:12:23]

Done.

 
-  // 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, kContentRow);
   views::View* more_view = new views::View();
   more_view->SetLayoutManager(
       new views::BoxLayout(views::BoxLayout::kHorizontal, 0, 0,
-                           views::kRelatedControlHorizontalSpacing));
+                           kIconTextPadding));

[Peter Kasting, 2012/10/15 21:46:54]

Nit: Or wrap as

  more_view->SetLayoutManager(new views::BoxLayout(
      views::BoxLayout::kHorizontal, 0, 0, kIconTextPadding));

[please use gerrit instead, 2012/10/16 00:12:23]

Done.

   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);
+  if (!more_suggestions_link_)
+    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_);
+  if (extensions_)
+    extensions_->Update();
+  if (action_label_)
+    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_);
   // 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;
-  suggestions_label_ = NULL;
   extensions_ = NULL;
   more_suggestions_link_ = NULL;
+  inline_service_icon_ = NULL;
   choose_another_service_link_ = NULL;
 }
+
+void WebIntentPickerViews::RefreshInlineServiceIcon() {
+  DCHECK(inline_service_icon_);
+  const WebIntentPickerModel::InstalledService* inline_service =
+      model_->GetInstalledServiceWithURL(model_->inline_disposition_url());
+  if (inline_service)
+    inline_service_icon_->SetImage(inline_service->favicon.ToImageSkia());
+}