Fix extension scrollbar regression bug, and add a regression test to

chrome/browser/ui/views/extensions/extension_install_dialog_view.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 "chrome/browser/ui/views/extensions/extension_install_dialog_view.h"
+
 #include <vector>
 
 #include "base/basictypes.h"
 #include "base/command_line.h"
 #include "base/compiler_specific.h"
 #include "base/i18n/rtl.h"
 #include "base/metrics/histogram.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/extensions/bundle_installer.h"
@@ skipping 27 matching lines @@
 #include "ui/views/controls/link.h"
 #include "ui/views/controls/link_listener.h"
 #include "ui/views/controls/scroll_view.h"
 #include "ui/views/controls/separator.h"
 #include "ui/views/layout/box_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_client_view.h"
 #include "ui/views/window/dialog_delegate.h"

[Finnur, 2014/07/16 10:36:11]

Looks like lots of includes were copied from .cc to .h but should be moved
instead (see comment in .h file first, though).

[Chris Thompson, 2014/07/16 20:13:28]

Shuffled around includes and forward declarations. See reply to comment in .h
file.

 
 using content::OpenURLParams;
 using content::Referrer;
 using extensions::BundleInstaller;
 
-namespace {

[Finnur, 2014/07/16 10:36:12]

Why remove this?

[Chris Thompson, 2014/07/16 20:13:28]

Hmm, good point. Added back in for the "static" definitions in this file.

+// Width of bullet column in BulletedView.

[Finnur, 2014/07/16 10:36:11]

nit: s/of/of the/

[Chris Thompson, 2014/07/16 20:13:28]

Done.

+const int kBulletWidth = 20;
 
 // Size of extension icon in top left of dialog.
 const int kIconSize = 64;
 
 // We offset the icon a little bit from the right edge of the dialog, to make it
 // align with the button below it.
 const int kIconOffset = 16;
 
 // The dialog will resize based on its content, but this sets a maximum height
 // before overflowing a scrollbar.
@@ skipping 22 matching lines @@
 // experiment. This is used to group the actions in UMA histograms named
 // Extensions.InstallPromptExperiment.ShowDetails and
 // Extensions.InstallPromptExperiment.ShowPermissions.
 enum ExperimentLinkAction {
   LINK_SHOWN = 0,
   LINK_NOT_SHOWN,
   LINK_CLICKED,
   NUM_LINK_ACTIONS
 };
 
-typedef std::vector<base::string16> PermissionDetails;
-class ExpandableContainerView;
-
 void AddResourceIcon(const gfx::ImageSkia* skia_image, void* data) {
   views::View* parent = static_cast<views::View*>(data);
   views::ImageView* image_view = new views::ImageView();
   image_view->SetImage(*skia_image);
   parent->AddChildView(image_view);
 }
 
 // Creates a string for displaying |message| to the user. If it has to look
 // like a entry in a bullet point list, one is added.
 base::string16 PrepareForDisplay(const base::string16& message,
                                  bool bullet_point) {
   return bullet_point ? l10n_util::GetStringFUTF16(
       IDS_EXTENSION_PERMISSION_LINE,
       message) : message;
 }
 
-// A custom scrollable view implementation for the dialog.
-class CustomScrollableView : public views::View {
- public:
-  CustomScrollableView();
-  virtual ~CustomScrollableView();
-
- private:
-  virtual void Layout() OVERRIDE;
-
-  DISALLOW_COPY_AND_ASSIGN(CustomScrollableView);
-};
-
-// Implements the extension installation dialog for TOOLKIT_VIEWS.
-class ExtensionInstallDialogView : public views::DialogDelegateView,
-                                   public views::LinkListener,
-                                   public views::ButtonListener {
- public:
-  ExtensionInstallDialogView(
-      content::PageNavigator* navigator,
-      ExtensionInstallPrompt::Delegate* delegate,
-      scoped_refptr<ExtensionInstallPrompt::Prompt> prompt);
-  virtual ~ExtensionInstallDialogView();
-
-  // Called when one of the child elements has expanded/collapsed.
-  void ContentsChanged();
-
- private:
-  // views::DialogDelegateView:
-  virtual int GetDialogButtons() const OVERRIDE;
-  virtual base::string16 GetDialogButtonLabel(
-      ui::DialogButton button) const OVERRIDE;
-  virtual int GetDefaultDialogButton() const OVERRIDE;
-  virtual bool Cancel() OVERRIDE;
-  virtual bool Accept() OVERRIDE;
-  virtual ui::ModalType GetModalType() const OVERRIDE;
-  virtual base::string16 GetWindowTitle() const OVERRIDE;
-  virtual void Layout() OVERRIDE;
-  virtual gfx::Size GetPreferredSize() const OVERRIDE;
-  virtual void ViewHierarchyChanged(
-      const ViewHierarchyChangedDetails& details) OVERRIDE;
-
-  // views::LinkListener:
-  virtual void LinkClicked(views::Link* source, int event_flags) OVERRIDE;
-
-  // views::ButtonListener:
-  virtual void ButtonPressed(views::Button* sender,
-                             const ui::Event& event) OVERRIDE;
-
-  // Experimental: Toggles inline permission explanations with an animation.
-  void ToggleInlineExplanations();
-
-  // Creates a layout consisting of dialog header, extension name and icon.
-  views::GridLayout* CreateLayout(
-      views::View* parent,
-      int left_column_width,
-      int column_set_id,
-      bool single_detail_row) const;
-
-  bool is_inline_install() const {
-    return prompt_->type() == ExtensionInstallPrompt::INLINE_INSTALL_PROMPT;
-  }
-
-  bool is_bundle_install() const {
-    return prompt_->type() == ExtensionInstallPrompt::BUNDLE_INSTALL_PROMPT;
-  }
-
-  bool is_external_install() const {
-    return prompt_->type() == ExtensionInstallPrompt::EXTERNAL_INSTALL_PROMPT;
-  }
-
-  // Updates the histogram that holds installation accepted/aborted data.
-  void UpdateInstallResultHistogram(bool accepted) const;
-
-  // Updates the histogram that holds data about whether "Show details" or
-  // "Show permissions" links were shown and/or clicked.
-  void UpdateLinkActionHistogram(int action_type) const;
-
-  content::PageNavigator* navigator_;
-  ExtensionInstallPrompt::Delegate* delegate_;
-  scoped_refptr<ExtensionInstallPrompt::Prompt> prompt_;
-
-  // The scroll view containing all the details for the dialog (including all
-  // collapsible/expandable sections).
-  views::ScrollView* scroll_view_;
-
-  // The container view for the scroll view.
-  CustomScrollableView* scrollable_;
-
-  // The container for the simpler view with only the dialog header and the
-  // extension icon. Used for the experiment where the permissions are
-  // initially hidden when the dialog shows.
-  CustomScrollableView* scrollable_header_only_;
-
-  // The preferred size of the dialog.
-  gfx::Size dialog_size_;
-
-  // Experimental: "Show details" link to expand inline explanations and reveal
-  // permision dialog.
-  views::Link* show_details_link_;
-
-  // Experimental: Label for showing information about the checkboxes.
-  views::Label* checkbox_info_label_;
-
-  // Experimental: Contains pointers to inline explanation views.
-  typedef std::vector<ExpandableContainerView*> InlineExplanations;
-  InlineExplanations inline_explanations_;
-
-  // Experimental: Number of unchecked checkboxes in the permission list.
-  // If this becomes zero, the accept button is enabled, otherwise disabled.
-  int unchecked_boxes_;
-
-  DISALLOW_COPY_AND_ASSIGN(ExtensionInstallDialogView);
-};
-
-// A simple view that prepends a view with a bullet with the help of a grid
-// layout.
-class BulletedView : public views::View {
- public:
-  explicit BulletedView(views::View* view);
- private:
-  DISALLOW_COPY_AND_ASSIGN(BulletedView);
-};
-
 BulletedView::BulletedView(views::View* view) {
   views::GridLayout* layout = new views::GridLayout(this);
   SetLayoutManager(layout);
   views::ColumnSet* column_set = layout->AddColumnSet(0);
+  column_set->AddColumn(views::GridLayout::CENTER,
+                        views::GridLayout::LEADING,
+                        0,
+                        views::GridLayout::FIXED,
+                        kBulletWidth,
+                        0);
   column_set->AddColumn(views::GridLayout::LEADING,
                         views::GridLayout::LEADING,
                         0,
                         views::GridLayout::USE_PREF,
-                        0, // no fixed width
+                        0,  // no fixed width

[Finnur, 2014/07/16 10:36:11]

Nit: Not your problem, but can you captitalize the N and add a period at the
end.

[Chris Thompson, 2014/07/16 20:13:28]

Done.

                         0);
-   column_set->AddColumn(views::GridLayout::LEADING,
-                         views::GridLayout::LEADING,
-                         0,
-                         views::GridLayout::USE_PREF,
-                         0,  // no fixed width
-                         0);
   layout->StartRow(0, 0);
   layout->AddView(new views::Label(PrepareForDisplay(base::string16(), true)));
   layout->AddView(view);
 }
 
-// A simple view that prepends a view with a checkbox with the help of a grid
-// layout. Used for the permission experiment.
-// TODO(meacer): Remove once the experiment is completed.
-class CheckboxedView : public views::View {
- public:
-  CheckboxedView(views::View* view, views::ButtonListener* listener);
- private:
-  DISALLOW_COPY_AND_ASSIGN(CheckboxedView);
-};
-
 CheckboxedView::CheckboxedView(views::View* view,
                                views::ButtonListener* listener) {
   views::GridLayout* layout = new views::GridLayout(this);
   SetLayoutManager(layout);
   views::ColumnSet* column_set = layout->AddColumnSet(0);
   column_set->AddColumn(views::GridLayout::LEADING,
                         views::GridLayout::LEADING,
                         0,
                         views::GridLayout::USE_PREF,
-                        0, // no fixed width
+                        0,  // no fixed width

[Finnur, 2014/07/16 10:36:11]

Same here.

[Chris Thompson, 2014/07/16 20:13:28]

Done.

                         0);
    column_set->AddColumn(views::GridLayout::LEADING,
                          views::GridLayout::LEADING,
                          0,
                          views::GridLayout::USE_PREF,
                          0,  // no fixed width
                          0);
   layout->StartRow(0, 0);
   views::Checkbox* checkbox = new views::Checkbox(base::string16());
   checkbox->set_listener(listener);
   // Alignment needs to be explicitly set again here, otherwise the views are
   // not vertically centered.
   layout->AddView(checkbox, 1, 1,
                   views::GridLayout::LEADING, views::GridLayout::CENTER);
   layout->AddView(view, 1, 1,
                   views::GridLayout::LEADING, views::GridLayout::CENTER);
 }
 
-// A view to display text with an expandable details section.
-class ExpandableContainerView : public views::View,
-                                public views::ButtonListener,
-                                public views::LinkListener,
-                                public gfx::AnimationDelegate {
- public:
-  ExpandableContainerView(ExtensionInstallDialogView* owner,
-                          const base::string16& description,
-                          const PermissionDetails& details,
-                          int horizontal_space,
-                          bool parent_bulleted,
-                          bool show_expand_link,
-                          bool lighter_color_details);
-  virtual ~ExpandableContainerView();
-
-  // views::View:
-  virtual void ChildPreferredSizeChanged(views::View* child) OVERRIDE;
-
-  // views::ButtonListener:
-  virtual void ButtonPressed(views::Button* sender,
-                             const ui::Event& event) OVERRIDE;
-
-  // views::LinkListener:
-  virtual void LinkClicked(views::Link* source, int event_flags) OVERRIDE;
-
-  // gfx::AnimationDelegate:
-  virtual void AnimationProgressed(const gfx::Animation* animation) OVERRIDE;
-  virtual void AnimationEnded(const gfx::Animation* animation) OVERRIDE;
-
-  // Expand/Collapse the detail section for this ExpandableContainerView.
-  void ToggleDetailLevel();
-
-  // Expand the detail section without any animation.
-  // TODO(meacer): Remove once the experiment is completed.
-  void ExpandWithoutAnimation();
-
- private:
-  // A view which displays all the details of an IssueAdviceInfoEntry.
-  class DetailsView : public views::View {
-   public:
-    explicit DetailsView(int horizontal_space, bool parent_bulleted,
-                         bool lighter_color);
-    virtual ~DetailsView() {}
-
-    // views::View:
-    virtual gfx::Size GetPreferredSize() const OVERRIDE;
-
-    void AddDetail(const base::string16& detail);
-
-    // Animates this to be a height proportional to |state|.
-    void AnimateToState(double state);
-
-   private:
-    views::GridLayout* layout_;
-    double state_;
-
-    // Whether the detail text should be shown with a lighter color.
-    bool lighter_color_;
-
-    DISALLOW_COPY_AND_ASSIGN(DetailsView);
-  };
-
-  // The dialog that owns |this|. It's also an ancestor in the View hierarchy.
-  ExtensionInstallDialogView* owner_;
-
-  // A view for showing |issue_advice.details|.
-  DetailsView* details_view_;
-
-  // The 'more details' link shown under the heading (changes to 'hide details'
-  // when the details section is expanded).
-  views::Link* more_details_;
-
-  gfx::SlideAnimation slide_animation_;
-
-  // The up/down arrow next to the 'more detail' link (points up/down depending
-  // on whether the details section is expanded).
-  views::ImageButton* arrow_toggle_;
-
-  // Whether the details section is expanded.
-  bool expanded_;
-
-  DISALLOW_COPY_AND_ASSIGN(ExpandableContainerView);
-};
-
 void ShowExtensionInstallDialogImpl(
     const ExtensionInstallPrompt::ShowParams& show_params,
     ExtensionInstallPrompt::Delegate* delegate,
     scoped_refptr<ExtensionInstallPrompt::Prompt> prompt) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   CreateBrowserModalDialogViews(
       new ExtensionInstallDialogView(show_params.navigator, delegate, prompt),
       show_params.parent_window)->Show();
 }
 
-}  // namespace

[Finnur, 2014/07/16 10:36:12]

Shouldn't this be moved to line 120 (of the file on the right) instead of
removed?

[Chris Thompson, 2014/07/16 20:13:28]

Done.

-
 CustomScrollableView::CustomScrollableView() {}
 CustomScrollableView::~CustomScrollableView() {}
 
 void CustomScrollableView::Layout() {
   SetBounds(x(), y(), width(), GetHeightForWidth(width()));
   views::View::Layout();
 }
 
 ExtensionInstallDialogView::ExtensionInstallDialogView(
     content::PageNavigator* navigator,
@@ skipping 221 matching lines @@
           permission_label->SetEnabledColor(kTextHighlight);
         } else if (prompt->experiment()->ShouldHighlightBackground(
                        prompt->GetPermission(i))) {
           permission_label->SetLineHeight(18);
           permission_label->set_background(
               views::Background::CreateSolidBackground(kBackgroundHighlight));
         }
 
         permission_label->SetMultiLine(true);
         permission_label->SetHorizontalAlignment(gfx::ALIGN_LEFT);
-        permission_label->SizeToFit(left_column_width);
 
         if (prompt->experiment()->show_checkboxes()) {
+          permission_label->SizeToFit(left_column_width);
           layout->AddView(new CheckboxedView(permission_label, this));
           ++unchecked_boxes_;
         } else {
+          permission_label->SizeToFit(left_column_width - kBulletWidth);
           layout->AddView(new BulletedView(permission_label));
         }
+
         // If we have more details to provide, show them in collapsed form.
         if (!prompt->GetPermissionsDetails(i).empty()) {
           layout->StartRow(0, column_set_id);
           PermissionDetails details;
           details.push_back(
               PrepareForDisplay(prompt->GetPermissionsDetails(i), false));
           ExpandableContainerView* details_container =
               new ExpandableContainerView(
                   this, base::string16(), details, left_column_width,
                   true, true, false);
@@ skipping 400 matching lines @@
         NUM_LINK_ACTIONS);
   } else {
     // The clickable link in the UI is "Show Details".
     UMA_HISTOGRAM_ENUMERATION(
         "Extensions.InstallPromptExperiment.ShowDetails",
         action_type,
         NUM_LINK_ACTIONS);
   }
 }
 
-// static
-ExtensionInstallPrompt::ShowDialogCallback
-ExtensionInstallPrompt::GetDefaultShowDialogCallback() {
-  return base::Bind(&ShowExtensionInstallDialogImpl);
-}
-
 // ExpandableContainerView::DetailsView ----------------------------------------
 
 ExpandableContainerView::DetailsView::DetailsView(int horizontal_space,
                                                   bool parent_bulleted,
                                                   bool lighter_color)
     : layout_(new views::GridLayout(this)),
       state_(0),
       lighter_color_(lighter_color) {
   SetLayoutManager(layout_);
   views::ColumnSet* column_set = layout_->AddColumnSet(0);
@@ skipping 193 matching lines @@
   if (slide_animation_.IsShowing())
     slide_animation_.Hide();
   else
     slide_animation_.Show();
 }
 
 void ExpandableContainerView::ExpandWithoutAnimation() {
   expanded_ = true;
   details_view_->AnimateToState(1.0);
 }
+
+// static
+ExtensionInstallPrompt::ShowDialogCallback
+ExtensionInstallPrompt::GetDefaultShowDialogCallback() {
+  return base::Bind(&ShowExtensionInstallDialogImpl);
+}