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

Fix extension scrollbar regression bug, and add a regression test to catch similar issues in the future.

This changes the extension_install_dialog_view permission strings layout to have a fixed width for the bullet, centered, and subtract that width from the width used to size the permission string label.

This also adds two browser tests: one to test that a scrollbar _does_ show for a very long set of permissions, and one for the specific regression case from the bug.

BUG=385570

Review URL: https://codereview.chromium.org/363523002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@283968 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/ui/views/extensions/extension_install_dialog_view.h

+// Copyright 2014 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.
+
+#ifndef CHROME_BROWSER_UI_VIEWS_EXTENSIONS_EXTENSION_INSTALL_DIALOG_VIEW_H_
+#define CHROME_BROWSER_UI_VIEWS_EXTENSIONS_EXTENSION_INSTALL_DIALOG_VIEW_H_
+
+#include "chrome/browser/extensions/extension_install_prompt.h"
+#include "ui/gfx/animation/animation_delegate.h"
+#include "ui/gfx/animation/slide_animation.h"
+#include "ui/views/controls/button/button.h"
+#include "ui/views/controls/link_listener.h"
+#include "ui/views/view.h"
+#include "ui/views/window/dialog_delegate.h"
+
+typedef std::vector<base::string16> PermissionDetails;
+class ExpandableContainerView;
+
+namespace content {
+class PageNavigator;
+}
+
+namespace views {
+class GridLayout;
+class ImageButton;
+class Label;
+class Link;
+}
+
+// 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();
+
+  // Returns the interior ScrollView of the dialog. This allows us to inspect
+  // the contents of the DialogView.
+  const views::ScrollView* scroll_view() const { return scroll_view_; }
+
+  // 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);
+};
+
+// 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);
+};
+
+// 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);
+
+#endif  // CHROME_BROWSER_UI_VIEWS_EXTENSIONS_EXTENSION_INSTALL_DIALOG_VIEW_H_

chrome/browser/ui/views/extensions/extension_install_dialog_view_browsertest.cc

+// Copyright 2014 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 "base/run_loop.h"
+#include "base/strings/utf_string_conversions.h"
+#include "chrome/browser/extensions/extension_browsertest.h"
+#include "chrome/browser/extensions/extension_icon_manager.h"
+#include "chrome/browser/extensions/extension_install_prompt.h"
+#include "chrome/browser/extensions/extension_install_prompt_experiment.h"
+#include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/tabs/tab_strip_model.h"
+#include "chrome/browser/ui/views/constrained_window_views.h"
+#include "chrome/browser/ui/views/extensions/extension_install_dialog_view.h"
+#include "chrome/browser/ui/webui/extensions/extension_settings_handler.h"
+#include "chrome/common/extensions/extension_test_util.h"
+#include "content/public/browser/browser_thread.h"
+#include "extensions/common/extension.h"
+#include "extensions/common/permissions/permissions_data.h"
+#include "extensions/common/test_util.h"
+#include "ui/views/controls/scroll_view.h"
+#include "ui/views/view.h"
+#include "ui/views/widget/widget.h"
+
+// A simple delegate implementation that counts the number of times
+// |InstallUIProceed| and |InstallUIAbort| are called.
+class MockExtensionInstallPromptDelegate
+    : public ExtensionInstallPrompt::Delegate {
+ public:
+  MockExtensionInstallPromptDelegate()
+      : proceed_count_(0),
+        abort_count_(0) {}
+
+  // ExtensionInstallPrompt::Delegate overrides.
+  virtual void InstallUIProceed() OVERRIDE;
+  virtual void InstallUIAbort(bool user_initiated) OVERRIDE;
+
+  int proceed_count() { return proceed_count_; }
+  int abort_count() { return abort_count_; }
+
+ protected:
+  int proceed_count_;
+  int abort_count_;
+};
+
+void MockExtensionInstallPromptDelegate::InstallUIProceed() {
+  ++proceed_count_;
+}
+
+void MockExtensionInstallPromptDelegate::InstallUIAbort(bool user_initiated) {
+  ++abort_count_;
+}
+
+// This lets us construct the parent for the prompt we're constructing in our
+// tests.
+class MockExtensionInstallPrompt : public ExtensionInstallPrompt {
+ public:
+  explicit MockExtensionInstallPrompt(content::WebContents* web_contents)
+      : ExtensionInstallPrompt(web_contents), prompt_(NULL) {}
+  virtual ~MockExtensionInstallPrompt() {}
+  void set_prompt(ExtensionInstallPrompt::Prompt* prompt) {
+    prompt_ = prompt;
+  }
+  ExtensionInstallPrompt::Prompt* get_prompt() {
+    return prompt_;
+  }
+
+ private:
+  ExtensionInstallPrompt::Prompt* prompt_;
+};
+
+class ScrollbarTest : public ExtensionBrowserTest {
+ protected:
+  ScrollbarTest();
+  virtual ~ScrollbarTest() {}
+
+  virtual void SetUpOnMainThread() OVERRIDE;
+
+  void SetPromptPermissions(std::vector<base::string16> permissions);
+  void SetPromptDetails(std::vector<base::string16> details);
+  void SetPromptRetainedFiles(std::vector<base::FilePath> files);
+
+  bool IsScrollbarVisible();
+
+ private:
+  const extensions::Extension* extension_;
+  MockExtensionInstallPrompt* install_prompt_;
+  scoped_refptr<ExtensionInstallPrompt::Prompt> prompt_;
+  content::WebContents* web_contents_;
+};
+
+ScrollbarTest::ScrollbarTest() :
+  extension_(NULL),
+  install_prompt_(NULL),
+  prompt_(new ExtensionInstallPrompt::Prompt(
+      ExtensionInstallPrompt::PERMISSIONS_PROMPT)),
+  web_contents_(NULL) {}
+
+void ScrollbarTest::SetUpOnMainThread() {
+  ExtensionBrowserTest::SetUpOnMainThread();
+  extension_ = ExtensionBrowserTest::LoadExtension(test_data_dir_.AppendASCII(
+      "install_prompt/permissions_scrollbar_regression"));
+
+  web_contents_ = browser()->tab_strip_model()->GetWebContentsAt(0);
+
+  install_prompt_ = new MockExtensionInstallPrompt(web_contents_);
+  install_prompt_->set_prompt(prompt_);
+  prompt_->set_experiment(ExtensionInstallPromptExperiment::ControlGroup());
+  prompt_->set_extension(extension_);
+
+  scoped_ptr<ExtensionIconManager> icon_manager(new ExtensionIconManager());
+  const SkBitmap icon_bitmap = icon_manager->GetIcon(extension_->id());
+  gfx::Image icon = gfx::Image::CreateFrom1xBitmap(icon_bitmap);
+  prompt_->set_icon(icon);
+
+  this->SetPromptPermissions(std::vector<base::string16>());
+  this->SetPromptDetails(std::vector<base::string16>());
+  this->SetPromptRetainedFiles(std::vector<base::FilePath>());
+}
+
+void ScrollbarTest::SetPromptPermissions(
+    std::vector<base::string16> permissions) {
+  prompt_->SetPermissions(permissions);
+}
+
+void ScrollbarTest::SetPromptDetails(
+    std::vector<base::string16> details) {
+  prompt_->SetPermissionsDetails(details);
+}
+
+void ScrollbarTest::SetPromptRetainedFiles(
+    std::vector<base::FilePath> files) {
+  prompt_->set_retained_files(files);
+}
+
+bool ScrollbarTest::IsScrollbarVisible() {
+  ExtensionInstallPrompt::ShowParams show_params(web_contents_);
+  MockExtensionInstallPromptDelegate delegate;
+  ExtensionInstallDialogView* dialog =
+      new ExtensionInstallDialogView(show_params.navigator, &delegate, prompt_);
+
+  // Create the modal view around the install dialog view.
+  views::Widget* modal =
+      CreateBrowserModalDialogViews(dialog, show_params.parent_window);
+  modal->Show();
+  content::BrowserThread::GetBlockingPool()->FlushForTesting();
+  base::RunLoop().RunUntilIdle();
+
+  // Check if the vertical scrollbar is visible.
+  return dialog->scroll_view()->vertical_scroll_bar()->visible();
+}
+
+// Tests that a scrollbar _is_ shown for an excessively long extension
+// install prompt.
+IN_PROC_BROWSER_TEST_F(ScrollbarTest, LongPromptScrollbar) {
+  base::string16 permission_string(base::ASCIIToUTF16("Test"));
+  std::vector<base::string16> permissions;
+  std::vector<base::string16> details;
+  for (int i = 0; i < 20; i++) {
+    permissions.push_back(permission_string);
+    details.push_back(base::string16());
+  }
+  this->SetPromptPermissions(permissions);
+  this->SetPromptDetails(details);
+  ASSERT_TRUE(IsScrollbarVisible()) << "Scrollbar is not visible";
+}
+
+// Tests that a scrollbar isn't shown for this regression case.
+// See crbug.com/385570 for details.
+IN_PROC_BROWSER_TEST_F(ScrollbarTest, ScrollbarRegression) {
+  base::string16 permission_string(base::ASCIIToUTF16(
+      "Read and modify your data on *.facebook.com"));
+  std::vector<base::string16> permissions;
+  permissions.push_back(permission_string);
+  this->SetPromptPermissions(permissions);
+  std::vector<base::string16> details;
+  details.push_back(base::string16());
+  this->SetPromptDetails(details);
+  ASSERT_FALSE(IsScrollbarVisible()) << "Scrollbar is visible";
+}

chrome/chrome_tests.gypi

@@ -1430,6 +1430,7 @@
         'browser/ui/views/autofill/autofill_dialog_view_tester_views.cc',
         'browser/ui/views/autofill/autofill_dialog_view_tester_views.h',
         'browser/ui/views/autofill/autofill_popup_base_view_browsertest.cc',
+        'browser/ui/views/extensions/extension_install_dialog_view_browsertest.cc',
         'browser/ui/views/frame/browser_non_client_frame_view_ash_browsertest.cc',
         'browser/ui/views/frame/browser_view_browsertest.cc',
         'browser/ui/views/frame/browser_window_property_manager_browsertest_win.cc',

chrome/test/data/extensions/install_prompt/permissions_scrollbar_regression/manifest.json

+{
+  "name": "Scrollbar Regression",
+  "description": "Should not show a vertical scrollbar",
+  "version": "1",
+  "manifest_version": 2,
+  "permissions": [
+    "*://*.facebook.com/*"
+  ]
+}