diff options
author | mkwst@chromium.org <mkwst@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2014-05-05 13:11:41 +0000 |
---|---|---|
committer | mkwst@chromium.org <mkwst@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2014-05-05 13:11:41 +0000 |
commit | 4bee44346f29b2fc58306b94f0c249e06bf1c8ff (patch) | |
tree | d3dc6b7a3f26a9539d178e57fe08cdd388e45cb9 | |
parent | e1e2da3479979769535e1025bcfe22f973cb2b5e (diff) | |
download | chromium_src-4bee44346f29b2fc58306b94f0c249e06bf1c8ff.zip chromium_src-4bee44346f29b2fc58306b94f0c249e06bf1c8ff.tar.gz chromium_src-4bee44346f29b2fc58306b94f0c249e06bf1c8ff.tar.bz2 |
Password bubble: ManagePasswordsIconView is now a BubbleIconView.
This CL introduces a new browser command to open the Manage Passwords bubble,
and converts the ManagePasswordsIconView class into a subclass of
BubbleIconView, which uses the new command to control the bubble's state.
This allows us to more easily test the view and the UI controller, as each
object's job is now more clearly defined (and the view is now doing a good
deal less work), and to independently verify that the command is doing the
right thing.
After this CL, we'll (finally!) have something approaching reasonable test
coverage for the core of the views code.
----------------------------------------------------------------------------
This is a re-land of r267195, which was reverted due to Windows errors. That
was a reland of r266859, which was reverted due to LSAN errors. The
original review is https://codereview.chromium.org/246393004/.
----------------------------------------------------------------------------
BUG=365678
TBR=vabr@chromium.org,pkasting@chromium.org,cpu@chromium.org
Review URL: https://codereview.chromium.org/264713010
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@268159 0039d316-1c4b-4281-b951-d872f2087c98
24 files changed, 513 insertions, 97 deletions
diff --git a/chrome/app/chrome_command_ids.h b/chrome/app/chrome_command_ids.h index 75a3a44..fdff03d 100644 --- a/chrome/app/chrome_command_ids.h +++ b/chrome/app/chrome_command_ids.h @@ -87,6 +87,7 @@ #define IDC_ADVANCED_PRINT 35007 #define IDC_PRINT_TO_DESTINATION 35008 #define IDC_TRANSLATE_PAGE 35009 +#define IDC_MANAGE_PASSWORDS_FOR_PAGE 35010 // When adding a new encoding to this list, be sure to append it to the // EncodingMenuController::kValidEncodingIds array in diff --git a/chrome/browser/ui/browser_command_controller.cc b/chrome/browser/ui/browser_command_controller.cc index a6978e7..354932e 100644 --- a/chrome/browser/ui/browser_command_controller.cc +++ b/chrome/browser/ui/browser_command_controller.cc @@ -559,6 +559,11 @@ void BrowserCommandController::ExecuteCommandWithDisposition( case IDC_TRANSLATE_PAGE: Translate(browser_); break; + case IDC_MANAGE_PASSWORDS_FOR_PAGE: + ManagePasswordsForPage(browser_); + break; + + // Page encoding commands case IDC_ENCODING_AUTO_DETECT: browser_->ToggleEncodingAutoDetect(); break; @@ -917,6 +922,7 @@ void BrowserCommandController::InitCommandState() { // Page-related commands command_updater_.UpdateCommandEnabled(IDC_EMAIL_PAGE_LOCATION, true); + command_updater_.UpdateCommandEnabled(IDC_MANAGE_PASSWORDS_FOR_PAGE, true); command_updater_.UpdateCommandEnabled(IDC_ENCODING_AUTO_DETECT, true); command_updater_.UpdateCommandEnabled(IDC_ENCODING_UTF8, true); command_updater_.UpdateCommandEnabled(IDC_ENCODING_UTF16LE, true); diff --git a/chrome/browser/ui/browser_commands.cc b/chrome/browser/ui/browser_commands.cc index f24bec6..b266077 100644 --- a/chrome/browser/ui/browser_commands.cc +++ b/chrome/browser/ui/browser_commands.cc @@ -775,6 +775,18 @@ void Translate(Browser* browser) { web_contents, step, TranslateErrors::NONE); } +void ManagePasswordsForPage(Browser* browser) { +// TODO(mkwst): Implement this feature on Mac: http://crbug.com/261628 +#if !defined(OS_MACOSX) + if (!browser->window()->IsActive()) + return; + + WebContents* web_contents = + browser->tab_strip_model()->GetActiveWebContents(); + chrome::ShowManagePasswordsBubble(web_contents); +#endif +} + void TogglePagePinnedToStartScreen(Browser* browser) { #if defined(OS_WIN) MetroPinTabHelper::FromWebContents( diff --git a/chrome/browser/ui/browser_commands.h b/chrome/browser/ui/browser_commands.h index 1e6fe26..fe63b0e 100644 --- a/chrome/browser/ui/browser_commands.h +++ b/chrome/browser/ui/browser_commands.h @@ -96,6 +96,7 @@ bool CanBookmarkCurrentPage(const Browser* browser); void BookmarkAllTabs(Browser* browser); bool CanBookmarkAllTabs(const Browser* browser); void Translate(Browser* browser); +void ManagePasswordsForPage(Browser* browser); void TogglePagePinnedToStartScreen(Browser* browser); void SavePage(Browser* browser); bool CanSavePage(const Browser* browser); diff --git a/chrome/browser/ui/browser_dialogs.h b/chrome/browser/ui/browser_dialogs.h index f812a13..ac688d6 100644 --- a/chrome/browser/ui/browser_dialogs.h +++ b/chrome/browser/ui/browser_dialogs.h @@ -114,6 +114,13 @@ void ShowSignedCertificateTimestampsViewer( content::WebContents* web_contents, const content::SignedCertificateTimestampIDStatusList& sct_ids_list); +#if !defined(OS_MACOSX) +// Shows the ManagePasswords bubble for a particular |web_contents|. +// +// TODO(mkwst): Implement this feature on Mac: http://crbug.com/261628 +void ShowManagePasswordsBubble(content::WebContents* web_contents); +#endif + } // namespace chrome #endif // CHROME_BROWSER_UI_BROWSER_DIALOGS_H_ diff --git a/chrome/browser/ui/passwords/manage_passwords_bubble_ui_controller.cc b/chrome/browser/ui/passwords/manage_passwords_bubble_ui_controller.cc index 987077b..88c76e6 100644 --- a/chrome/browser/ui/passwords/manage_passwords_bubble_ui_controller.cc +++ b/chrome/browser/ui/passwords/manage_passwords_bubble_ui_controller.cc @@ -170,6 +170,8 @@ void ManagePasswordsBubbleUIController::UpdateIconAndBubbleState( if (manage_passwords_bubble_needs_showing_) { DCHECK(state == ManagePasswordsIcon::PENDING_STATE); + // TODO(mkwst): Replace this with execution of a browser command once we + // can pipe a CommandUpdater down here. icon->ShowBubbleWithoutUserInteraction(); manage_passwords_bubble_needs_showing_ = false; } diff --git a/chrome/browser/ui/passwords/manage_passwords_icon.cc b/chrome/browser/ui/passwords/manage_passwords_icon.cc index 0914bba..c9758b5 100644 --- a/chrome/browser/ui/passwords/manage_passwords_icon.cc +++ b/chrome/browser/ui/passwords/manage_passwords_icon.cc @@ -14,5 +14,5 @@ void ManagePasswordsIcon::SetState(State state) { if (state_ == state) return; state_ = state; - SetStateInternal(state); + UpdateVisibleUI(); } diff --git a/chrome/browser/ui/passwords/manage_passwords_icon.h b/chrome/browser/ui/passwords/manage_passwords_icon.h index 0a44406..004c8cd 100644 --- a/chrome/browser/ui/passwords/manage_passwords_icon.h +++ b/chrome/browser/ui/passwords/manage_passwords_icon.h @@ -42,8 +42,9 @@ class ManagePasswordsIcon { ManagePasswordsIcon(); ~ManagePasswordsIcon(); - // Called from SetState() iff the icon's state has changed. - virtual void SetStateInternal(State state) = 0; + // Called from SetState() iff the icon's state has changed in order to do + // whatever platform-specific UI work is necessary given the new state. + virtual void UpdateVisibleUI() = 0; private: State state_; diff --git a/chrome/browser/ui/passwords/manage_passwords_icon_mock.cc b/chrome/browser/ui/passwords/manage_passwords_icon_mock.cc index 64bdb16..4b575e8 100644 --- a/chrome/browser/ui/passwords/manage_passwords_icon_mock.cc +++ b/chrome/browser/ui/passwords/manage_passwords_icon_mock.cc @@ -14,5 +14,5 @@ void ManagePasswordsIconMock::ShowBubbleWithoutUserInteraction() { ++bubble_shown_count_; } -void ManagePasswordsIconMock::SetStateInternal(ManagePasswordsIcon::State) { +void ManagePasswordsIconMock::UpdateVisibleUI() { } diff --git a/chrome/browser/ui/passwords/manage_passwords_icon_mock.h b/chrome/browser/ui/passwords/manage_passwords_icon_mock.h index 3781e6a..de4f814 100644 --- a/chrome/browser/ui/passwords/manage_passwords_icon_mock.h +++ b/chrome/browser/ui/passwords/manage_passwords_icon_mock.h @@ -18,7 +18,8 @@ class ManagePasswordsIconMock : public ManagePasswordsIcon { int bubble_shown_count() const { return bubble_shown_count_; } protected: - virtual void SetStateInternal(State state) OVERRIDE; + // ManagePasswordsIcon: + virtual void UpdateVisibleUI() OVERRIDE; private: int bubble_shown_count_; diff --git a/chrome/browser/ui/views/location_bar/location_bar_view.cc b/chrome/browser/ui/views/location_bar/location_bar_view.cc index 42cfd48..14b85c6 100644 --- a/chrome/browser/ui/views/location_bar/location_bar_view.cc +++ b/chrome/browser/ui/views/location_bar/location_bar_view.cc @@ -377,8 +377,8 @@ void LocationBarView::Init() { open_pdf_in_reader_view_ = new OpenPDFInReaderView(); AddChildView(open_pdf_in_reader_view_); - manage_passwords_icon_view_ = new ManagePasswordsIconView(delegate_); - manage_passwords_icon_view_->SetState(ManagePasswordsIcon::INACTIVE_STATE); + manage_passwords_icon_view_ = + new ManagePasswordsIconView(delegate_, command_updater()); AddChildView(manage_passwords_icon_view_); translate_icon_view_ = new TranslateIconView(command_updater()); diff --git a/chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc b/chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc index 654c114..e379e67 100644 --- a/chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc +++ b/chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc @@ -4,12 +4,12 @@ #include "chrome/browser/ui/views/passwords/manage_passwords_bubble_view.h" -#include "base/metrics/histogram.h" #include "chrome/browser/chrome_notification_types.h" #include "chrome/browser/ui/browser.h" #include "chrome/browser/ui/browser_finder.h" #include "chrome/browser/ui/browser_window.h" #include "chrome/browser/ui/passwords/manage_passwords_bubble_model.h" +#include "chrome/browser/ui/passwords/manage_passwords_bubble_ui_controller.h" #include "chrome/browser/ui/views/frame/browser_view.h" #include "chrome/browser/ui/views/location_bar/location_bar_view.h" #include "chrome/browser/ui/views/passwords/manage_password_item_view.h" @@ -45,33 +45,24 @@ int GetFieldWidth(FieldType type) { : kPasswordFieldSize); } -class SavePasswordRefusalComboboxModel : public ui::ComboboxModel { - public: - enum { INDEX_NOPE = 0, INDEX_NEVER_FOR_THIS_SITE = 1, }; +} // namespace - SavePasswordRefusalComboboxModel() { - items_.push_back( - l10n_util::GetStringUTF16(IDS_PASSWORD_MANAGER_CANCEL_BUTTON)); - items_.push_back( - l10n_util::GetStringUTF16(IDS_PASSWORD_MANAGER_BLACKLIST_BUTTON)); - } - virtual ~SavePasswordRefusalComboboxModel() {} - - private: - // Overridden from ui::ComboboxModel: - virtual int GetItemCount() const OVERRIDE { return items_.size(); } - virtual base::string16 GetItemAt(int index) OVERRIDE { return items_[index]; } - virtual bool IsItemSeparatorAt(int index) OVERRIDE { - return items_[index].empty(); - } - virtual int GetDefaultIndex() const OVERRIDE { return 0; } - std::vector<base::string16> items_; +// Globals -------------------------------------------------------------------- - DISALLOW_COPY_AND_ASSIGN(SavePasswordRefusalComboboxModel); -}; +namespace chrome { -} // namespace +void ShowManagePasswordsBubble(content::WebContents* web_contents) { + ManagePasswordsBubbleUIController* controller = + ManagePasswordsBubbleUIController::FromWebContents(web_contents); + ManagePasswordsBubbleView::ShowBubble( + web_contents, + controller->manage_passwords_bubble_needs_showing() ? + ManagePasswordsBubbleView::AUTOMATIC : + ManagePasswordsBubbleView::USER_ACTION); +} + +} // namespace chrome // ManagePasswordsBubbleView -------------------------------------------------- @@ -262,8 +253,8 @@ void ManagePasswordsBubbleView::Init() { layout->StartRow(0, SINGLE_VIEW_COLUMN_SET); layout->AddView(item); - refuse_combobox_ = - new views::Combobox(new SavePasswordRefusalComboboxModel()); + combobox_model_.reset(new SavePasswordRefusalComboboxModel()); + refuse_combobox_.reset(new views::Combobox(combobox_model_.get())); refuse_combobox_->set_listener(this); refuse_combobox_->SetStyle(views::Combobox::STYLE_ACTION); @@ -273,7 +264,7 @@ void ManagePasswordsBubbleView::Init() { layout->StartRowWithPadding( 0, DOUBLE_BUTTON_COLUMN_SET, 0, views::kRelatedControlVerticalSpacing); layout->AddView(save_button_); - layout->AddView(refuse_combobox_); + layout->AddView(refuse_combobox_.get()); layout->AddPaddingRow(0, views::kRelatedControlVerticalSpacing); } else { // If we have a list of passwords to store for the current site, display diff --git a/chrome/browser/ui/views/passwords/manage_passwords_bubble_view.h b/chrome/browser/ui/views/passwords/manage_passwords_bubble_view.h index 39e1e50..07609c2 100644 --- a/chrome/browser/ui/views/passwords/manage_passwords_bubble_view.h +++ b/chrome/browser/ui/views/passwords/manage_passwords_bubble_view.h @@ -7,6 +7,7 @@ #include "base/basictypes.h" #include "chrome/browser/ui/passwords/manage_passwords_bubble.h" +#include "chrome/browser/ui/views/passwords/save_password_refusal_combobox_model.h" #include "ui/views/bubble/bubble_delegate.h" #include "ui/views/controls/button/button.h" #include "ui/views/controls/combobox/combobox.h" @@ -106,13 +107,18 @@ class ManagePasswordsBubbleView : public ManagePasswordsBubble, // shown twice at the same time. static ManagePasswordsBubbleView* manage_passwords_bubble_; - // The buttons that are shown in the bubble. + // The views that are shown in the bubble. views::BlueButton* save_button_; - views::Combobox* refuse_combobox_; - views::Link* manage_link_; views::LabelButton* done_button_; + // The combobox doesn't take ownership of it's model. If we created a combobox + // we need to ensure that we delete the model here, and because the combobox + // uses the model in it's destructor, we need to make sure we delete the model + // _after_ the combobox itself is deleted. + scoped_ptr<SavePasswordRefusalComboboxModel> combobox_model_; + scoped_ptr<views::Combobox> refuse_combobox_; + DISALLOW_COPY_AND_ASSIGN(ManagePasswordsBubbleView); }; diff --git a/chrome/browser/ui/views/passwords/manage_passwords_bubble_view_browsertest.cc b/chrome/browser/ui/views/passwords/manage_passwords_bubble_view_browsertest.cc new file mode 100644 index 0000000..11f4e87 --- /dev/null +++ b/chrome/browser/ui/views/passwords/manage_passwords_bubble_view_browsertest.cc @@ -0,0 +1,117 @@ +// 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 "chrome/browser/ui/views/passwords/manage_passwords_bubble_view.h" + +#include "base/metrics/histogram_samples.h" +#include "chrome/browser/ui/browser.h" +#include "chrome/browser/ui/tabs/tab_strip_model.h" +#include "chrome/browser/ui/views/passwords/manage_passwords_view_test.h" +#include "components/password_manager/core/browser/password_manager_metrics_util.h" +#include "components/password_manager/core/browser/stub_password_manager_client.h" +#include "testing/gtest/include/gtest/gtest.h" + +namespace { + +const char kDisplayDispositionMetric[] = "PasswordBubble.DisplayDisposition"; + +} // namespace + +typedef ManagePasswordsViewTest ManagePasswordsBubbleViewTest; + +IN_PROC_BROWSER_TEST_F(ManagePasswordsBubbleViewTest, BasicOpenAndClose) { + EXPECT_FALSE(ManagePasswordsBubbleView::IsShowing()); + ManagePasswordsBubbleView::ShowBubble( + browser()->tab_strip_model()->GetActiveWebContents(), + ManagePasswordsBubble::USER_ACTION); + EXPECT_TRUE(ManagePasswordsBubbleView::IsShowing()); + ManagePasswordsBubbleView::CloseBubble(); + EXPECT_FALSE(ManagePasswordsBubbleView::IsShowing()); + + // And, just for grins, ensure that we can re-open the bubble. + ManagePasswordsBubbleView::ShowBubble( + browser()->tab_strip_model()->GetActiveWebContents(), + ManagePasswordsBubble::USER_ACTION); + EXPECT_TRUE(ManagePasswordsBubbleView::IsShowing()); + ManagePasswordsBubbleView::CloseBubble(); + EXPECT_FALSE(ManagePasswordsBubbleView::IsShowing()); +} + +// Same as 'BasicOpenAndClose', but use the command rather than the static +// method directly. +IN_PROC_BROWSER_TEST_F(ManagePasswordsBubbleViewTest, CommandControlsBubble) { + // The command only works if the icon is visible, so get into management mode. + SetupManagingPasswords(); + EXPECT_FALSE(ManagePasswordsBubbleView::IsShowing()); + ExecuteManagePasswordsCommand(); + EXPECT_TRUE(ManagePasswordsBubbleView::IsShowing()); + ManagePasswordsBubbleView::CloseBubble(); + EXPECT_FALSE(ManagePasswordsBubbleView::IsShowing()); + + // And, just for grins, ensure that we can re-open the bubble. + ExecuteManagePasswordsCommand(); + EXPECT_TRUE(ManagePasswordsBubbleView::IsShowing()); + ManagePasswordsBubbleView::CloseBubble(); + EXPECT_FALSE(ManagePasswordsBubbleView::IsShowing()); +} + +IN_PROC_BROWSER_TEST_F(ManagePasswordsBubbleViewTest, + CommandExecutionInManagingState) { + SetupManagingPasswords(); + ExecuteManagePasswordsCommand(); + + scoped_ptr<base::HistogramSamples> samples( + GetSamples(kDisplayDispositionMetric)); + EXPECT_EQ( + 0, + samples->GetCount( + password_manager::metrics_util::AUTOMATIC_WITH_PASSWORD_PENDING)); + EXPECT_EQ(0, + samples->GetCount( + password_manager::metrics_util::MANUAL_WITH_PASSWORD_PENDING)); + EXPECT_EQ(1, + samples->GetCount( + password_manager::metrics_util::MANUAL_MANAGE_PASSWORDS)); +} + +IN_PROC_BROWSER_TEST_F(ManagePasswordsBubbleViewTest, + CommandExecutionInAutomaticState) { + // Open with pending password: automagical! + SetupPendingPassword(); + + scoped_ptr<base::HistogramSamples> samples( + GetSamples(kDisplayDispositionMetric)); + EXPECT_EQ( + 1, + samples->GetCount( + password_manager::metrics_util::AUTOMATIC_WITH_PASSWORD_PENDING)); + EXPECT_EQ(0, + samples->GetCount( + password_manager::metrics_util::MANUAL_WITH_PASSWORD_PENDING)); + EXPECT_EQ(0, + samples->GetCount( + password_manager::metrics_util::MANUAL_MANAGE_PASSWORDS)); +} + +IN_PROC_BROWSER_TEST_F(ManagePasswordsBubbleViewTest, + CommandExecutionInPendingState) { + // Open once with pending password: automagical! + SetupPendingPassword(); + ManagePasswordsBubbleView::CloseBubble(); + // This opening should be measured as manual. + ExecuteManagePasswordsCommand(); + + scoped_ptr<base::HistogramSamples> samples( + GetSamples(kDisplayDispositionMetric)); + EXPECT_EQ( + 1, + samples->GetCount( + password_manager::metrics_util::AUTOMATIC_WITH_PASSWORD_PENDING)); + EXPECT_EQ(1, + samples->GetCount( + password_manager::metrics_util::MANUAL_WITH_PASSWORD_PENDING)); + EXPECT_EQ(0, + samples->GetCount( + password_manager::metrics_util::MANUAL_MANAGE_PASSWORDS)); +} diff --git a/chrome/browser/ui/views/passwords/manage_passwords_icon_view.cc b/chrome/browser/ui/views/passwords/manage_passwords_icon_view.cc index 23a5950..73368045 100644 --- a/chrome/browser/ui/views/passwords/manage_passwords_icon_view.cc +++ b/chrome/browser/ui/views/passwords/manage_passwords_icon_view.cc @@ -4,6 +4,8 @@ #include "chrome/browser/ui/views/passwords/manage_passwords_icon_view.h" +#include "chrome/app/chrome_command_ids.h" +#include "chrome/browser/command_updater.h" #include "chrome/browser/ui/passwords/manage_passwords_bubble_ui_controller.h" #include "chrome/browser/ui/view_ids.h" #include "chrome/browser/ui/views/passwords/manage_passwords_bubble_view.h" @@ -13,38 +15,45 @@ #include "ui/base/resource/resource_bundle.h" ManagePasswordsIconView::ManagePasswordsIconView( - LocationBarView::Delegate* location_bar_delegate) - : location_bar_delegate_(location_bar_delegate) { + LocationBarView::Delegate* location_bar_delegate, + CommandUpdater* command_updater) + : BubbleIconView(command_updater, IDC_MANAGE_PASSWORDS_FOR_PAGE), + location_bar_delegate_(location_bar_delegate), + command_updater_(command_updater), + icon_id_(0), + tooltip_text_id_(0) { set_id(VIEW_ID_MANAGE_PASSWORDS_ICON_BUTTON); SetAccessibilityFocusable(true); - SetState(ManagePasswordsIcon::INACTIVE_STATE); + UpdateVisibleUI(); } ManagePasswordsIconView::~ManagePasswordsIconView() {} -void ManagePasswordsIconView::SetStateInternal( - ManagePasswordsIcon::State state) { - if (state == ManagePasswordsIcon::INACTIVE_STATE) { +void ManagePasswordsIconView::UpdateVisibleUI() { + // If the icon is inactive: clear out it's image and tooltip, hide the icon, + // close any active bubble, and exit early. + if (state() == ManagePasswordsIcon::INACTIVE_STATE) { + icon_id_ = 0; + tooltip_text_id_ = 0; + SetVisible(false); if (ManagePasswordsBubbleView::IsShowing()) ManagePasswordsBubbleView::CloseBubble(); return; } - // Start with the correct values for ManagePasswordsIcon::MANAGE_STATE, and - // adjust things accordingly if we're either in BLACKLISTED_STATE or - // PENDING_STATE. - int which_icon = IDR_SAVE_PASSWORD; - int which_text = IDS_PASSWORD_MANAGER_TOOLTIP_MANAGE; - if (state == ManagePasswordsIcon::BLACKLISTED_STATE) - which_icon = IDR_SAVE_PASSWORD_BLACKLISTED; - else if (state == ManagePasswordsIcon::PENDING_STATE) - which_text = IDS_PASSWORD_MANAGER_TOOLTIP_SAVE; + // Otherwise, start with the correct values for MANAGE_STATE, and adjust + // things accordingly if we're either in BLACKLISTED_STATE or PENDING_STATE. + icon_id_ = IDR_SAVE_PASSWORD; + tooltip_text_id_ = IDS_PASSWORD_MANAGER_TOOLTIP_MANAGE; + if (state() == ManagePasswordsIcon::BLACKLISTED_STATE) + icon_id_ = IDR_SAVE_PASSWORD_BLACKLISTED; + else if (state() == ManagePasswordsIcon::PENDING_STATE) + tooltip_text_id_ = IDS_PASSWORD_MANAGER_TOOLTIP_SAVE; SetVisible(true); - SetImage( - ui::ResourceBundle::GetSharedInstance().GetImageSkiaNamed(which_icon)); - SetTooltipText(l10n_util::GetStringUTF16(which_text)); + SetImage(ui::ResourceBundle::GetSharedInstance().GetImageSkiaNamed(icon_id_)); + SetTooltipText(l10n_util::GetStringUTF16(tooltip_text_id_)); } void ManagePasswordsIconView::ShowBubbleWithoutUserInteraction() { @@ -52,36 +61,13 @@ void ManagePasswordsIconView::ShowBubbleWithoutUserInteraction() { if (location_bar_delegate_->GetToolbarModel()->input_in_progress()) return; - ManagePasswordsBubbleView::ShowBubble( - location_bar_delegate_->GetWebContents(), - ManagePasswordsBubbleView::AUTOMATIC); -} - -bool ManagePasswordsIconView::GetTooltipText(const gfx::Point& p, - base::string16* tooltip) const { - // Don't show tooltip if the password bubble is displayed. - return !ManagePasswordsBubbleView::IsShowing() && - ImageView::GetTooltipText(p, tooltip); + command_updater_->ExecuteCommand(IDC_MANAGE_PASSWORDS_FOR_PAGE); } -void ManagePasswordsIconView::OnGestureEvent(ui::GestureEvent* event) { - if (event->type() == ui::ET_GESTURE_TAP) { - ManagePasswordsBubbleView::ShowBubble( - location_bar_delegate_->GetWebContents(), - ManagePasswordsBubbleView::USER_ACTION); - event->SetHandled(); - } +bool ManagePasswordsIconView::IsBubbleShowing() const { + return ManagePasswordsBubbleView::IsShowing(); } -bool ManagePasswordsIconView::OnMousePressed(const ui::MouseEvent& event) { - // Do nothing until the mouse button is released. - return true; -} - -void ManagePasswordsIconView::OnMouseReleased(const ui::MouseEvent& event) { - if (event.IsOnlyLeftMouseButton() && HitTestPoint(event.location())) { - ManagePasswordsBubbleView::ShowBubble( - location_bar_delegate_->GetWebContents(), - ManagePasswordsBubbleView::USER_ACTION); - } +void ManagePasswordsIconView::OnExecuting( + BubbleIconView::ExecuteSource source) { } diff --git a/chrome/browser/ui/views/passwords/manage_passwords_icon_view.h b/chrome/browser/ui/views/passwords/manage_passwords_icon_view.h index 7dd791c..efcef5f 100644 --- a/chrome/browser/ui/views/passwords/manage_passwords_icon_view.h +++ b/chrome/browser/ui/views/passwords/manage_passwords_icon_view.h @@ -7,42 +7,60 @@ #include "chrome/browser/ui/passwords/manage_passwords_bubble_model.h" #include "chrome/browser/ui/passwords/manage_passwords_icon.h" +#include "chrome/browser/ui/views/location_bar/bubble_icon_view.h" #include "chrome/browser/ui/views/location_bar/location_bar_view.h" #include "ui/views/controls/image_view.h" +class CommandUpdater; class ManagePasswordsBubbleUIController; // View for the password icon in the Omnibox. class ManagePasswordsIconView : public ManagePasswordsIcon, - public views::ImageView { + public BubbleIconView { public: // Clicking on the ManagePasswordsIconView shows a ManagePasswordsBubbleView, // which requires the current WebContents. Because the current WebContents // changes as the user switches tabs, it cannot be provided in the // constructor. Instead, a LocationBarView::Delegate is passed here so that it // can be queried for the current WebContents as needed. - explicit ManagePasswordsIconView( - LocationBarView::Delegate* location_bar_delegate); + ManagePasswordsIconView(LocationBarView::Delegate* location_bar_delegate, + CommandUpdater* command_updater); virtual ~ManagePasswordsIconView(); // ManagePasswordsIcon: + // + // TODO(mkwst): Remove this once we get rid of the single call to + // ShowBubbleWithoutUserInteraction in ManagePasswordsBubbleUIController. virtual void ShowBubbleWithoutUserInteraction() OVERRIDE; + // BubbleIconView: + virtual bool IsBubbleShowing() const OVERRIDE; + virtual void OnExecuting(BubbleIconView::ExecuteSource source) OVERRIDE; + +#if defined(UNIT_TEST) + int icon_id() const { return icon_id_; } + int tooltip_text_id() const { return tooltip_text_id_; } +#endif + protected: // ManagePasswordsIcon: - virtual void SetStateInternal(ManagePasswordsIcon::State state) OVERRIDE; + virtual void UpdateVisibleUI() OVERRIDE; private: - // views::ImageView: - virtual bool GetTooltipText(const gfx::Point& p, - base::string16* tooltip) const OVERRIDE; - virtual void OnGestureEvent(ui::GestureEvent* event) OVERRIDE; - virtual bool OnMousePressed(const ui::MouseEvent& event) OVERRIDE; - virtual void OnMouseReleased(const ui::MouseEvent& event) OVERRIDE; - // The delegate used to get the currently visible WebContents. LocationBarView::Delegate* location_bar_delegate_; + // The updater used to deliver commands to the browser; we'll use this to + // pop open the bubble when necessary. + // + // TODO(mkwst): Remove this once we get rid of the single call to + // ShowBubbleWithoutUserInteraction in ManagePasswordsBubbleUIController. + CommandUpdater* command_updater_; + + // The ID of the icon and text resources that are currently displayed. + int icon_id_; + int tooltip_text_id_; + DISALLOW_COPY_AND_ASSIGN(ManagePasswordsIconView); }; diff --git a/chrome/browser/ui/views/passwords/manage_passwords_icon_view_browsertest.cc b/chrome/browser/ui/views/passwords/manage_passwords_icon_view_browsertest.cc new file mode 100644 index 0000000..e3c8ca6 --- /dev/null +++ b/chrome/browser/ui/views/passwords/manage_passwords_icon_view_browsertest.cc @@ -0,0 +1,45 @@ +// 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 "chrome/browser/ui/views/passwords/manage_passwords_icon_view.h" + +#include "chrome/browser/ui/passwords/manage_passwords_bubble_ui_controller_mock.h" +#include "chrome/browser/ui/passwords/manage_passwords_icon.h" +#include "chrome/browser/ui/views/passwords/manage_passwords_view_test.h" +#include "grit/generated_resources.h" +#include "grit/theme_resources.h" +#include "testing/gtest/include/gtest/gtest.h" + +typedef ManagePasswordsViewTest ManagePasswordsIconViewTest; + +IN_PROC_BROWSER_TEST_F(ManagePasswordsIconViewTest, DefaultStateIsInactive) { + EXPECT_EQ(ManagePasswordsIcon::INACTIVE_STATE, view()->state()); + EXPECT_FALSE(view()->visible()); + EXPECT_EQ(0, view()->icon_id()); + EXPECT_EQ(0, view()->tooltip_text_id()); +} + +IN_PROC_BROWSER_TEST_F(ManagePasswordsIconViewTest, PendingState) { + SetupPendingPassword(); + EXPECT_EQ(ManagePasswordsIcon::PENDING_STATE, view()->state()); + EXPECT_TRUE(view()->visible()); + EXPECT_EQ(IDR_SAVE_PASSWORD, view()->icon_id()); + EXPECT_EQ(IDS_PASSWORD_MANAGER_TOOLTIP_SAVE, view()->tooltip_text_id()); +} + +IN_PROC_BROWSER_TEST_F(ManagePasswordsIconViewTest, ManageState) { + SetupManagingPasswords(); + EXPECT_EQ(ManagePasswordsIcon::MANAGE_STATE, view()->state()); + EXPECT_TRUE(view()->visible()); + EXPECT_EQ(IDR_SAVE_PASSWORD, view()->icon_id()); + EXPECT_EQ(IDS_PASSWORD_MANAGER_TOOLTIP_MANAGE, view()->tooltip_text_id()); +} + +IN_PROC_BROWSER_TEST_F(ManagePasswordsIconViewTest, BlacklistedState) { + SetupBlackistedPassword(); + EXPECT_EQ(ManagePasswordsIcon::BLACKLISTED_STATE, view()->state()); + EXPECT_TRUE(view()->visible()); + EXPECT_EQ(IDR_SAVE_PASSWORD_BLACKLISTED, view()->icon_id()); + EXPECT_EQ(IDS_PASSWORD_MANAGER_TOOLTIP_MANAGE, view()->tooltip_text_id()); +} diff --git a/chrome/browser/ui/views/passwords/manage_passwords_view_test.cc b/chrome/browser/ui/views/passwords/manage_passwords_view_test.cc new file mode 100644 index 0000000..4cf4614 --- /dev/null +++ b/chrome/browser/ui/views/passwords/manage_passwords_view_test.cc @@ -0,0 +1,92 @@ +// 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 "chrome/browser/ui/views/passwords/manage_passwords_view_test.h" + +#include "base/test/statistics_delta_reader.h" +#include "chrome/app/chrome_command_ids.h" +#include "chrome/browser/ui/browser.h" +#include "chrome/browser/ui/browser_command_controller.h" +#include "chrome/browser/ui/passwords/manage_passwords_bubble_ui_controller_mock.h" +#include "chrome/browser/ui/passwords/manage_passwords_icon.h" +#include "chrome/browser/ui/tabs/tab_strip_model.h" +#include "chrome/browser/ui/views/frame/browser_view.h" +#include "chrome/browser/ui/views/passwords/manage_passwords_bubble_view.h" +#include "chrome/browser/ui/views/passwords/manage_passwords_icon_view.h" +#include "chrome/browser/ui/views/toolbar/toolbar_view.h" +#include "chrome/test/base/in_process_browser_test.h" +#include "chrome/test/base/interactive_test_utils.h" +#include "components/autofill/core/common/password_form.h" +#include "components/password_manager/core/browser/mock_password_manager_driver.h" +#include "components/password_manager/core/browser/password_form_manager.h" +#include "components/password_manager/core/browser/password_manager_driver.h" +#include "components/password_manager/core/browser/password_manager_metrics_util.h" +#include "components/password_manager/core/browser/stub_password_manager_client.h" +#include "testing/gtest/include/gtest/gtest.h" + +void ManagePasswordsViewTest::SetUpOnMainThread() { + // Create the test UIController here so that it's bound to the currently + // active WebContents. + new ManagePasswordsBubbleUIControllerMock( + browser()->tab_strip_model()->GetActiveWebContents()); +} + +ManagePasswordsBubbleUIControllerMock* ManagePasswordsViewTest::controller() { + return static_cast<ManagePasswordsBubbleUIControllerMock*>( + ManagePasswordsBubbleUIController::FromWebContents( + browser()->tab_strip_model()->GetActiveWebContents())); +} + +ManagePasswordsIconView* ManagePasswordsViewTest::view() { + BrowserView* browser_view = static_cast<BrowserView*>(browser()->window()); + return browser_view->GetToolbarView()->location_bar()-> + manage_passwords_icon_view(); +} + +void ManagePasswordsViewTest::ExecuteManagePasswordsCommand() { + // Show the window to ensure that it's active. + browser()->window()->Show(); + + CommandUpdater* updater = browser()->command_controller()->command_updater(); + EXPECT_TRUE(updater->IsCommandEnabled(IDC_MANAGE_PASSWORDS_FOR_PAGE)); + EXPECT_TRUE(updater->ExecuteCommand(IDC_MANAGE_PASSWORDS_FOR_PAGE)); + + // Wait for the command execution to pop up the bubble. + content::RunAllPendingInMessageLoop(); +} + +void ManagePasswordsViewTest::SetupManagingPasswords() { + base::string16 kTestUsername = base::ASCIIToUTF16("test_username"); + autofill::PasswordFormMap map; + map[kTestUsername] = test_form(); + controller()->OnPasswordAutofilled(map); + controller()->UpdateIconAndBubbleState(view()); +} + +void ManagePasswordsViewTest::SetupPendingPassword() { + password_manager::StubPasswordManagerClient client; + password_manager::MockPasswordManagerDriver driver; + scoped_ptr<password_manager::PasswordFormManager> test_form_manager( + new password_manager::PasswordFormManager( + NULL, &client, &driver, *test_form(), false)); + controller()->OnPasswordSubmitted(test_form_manager.release()); + + // Wait for the command execution triggered by the automatic popup to pop up + // the bubble. + content::RunAllPendingInMessageLoop(); + controller()->UpdateIconAndBubbleState(view()); +} + +void ManagePasswordsViewTest::SetupBlackistedPassword() { + controller()->OnBlacklistBlockedAutofill(); + controller()->UpdateIconAndBubbleState(view()); +} + +base::HistogramSamples* ManagePasswordsViewTest::GetSamples( + const char* histogram) { + // Ensure that everything has been properly recorded before pulling samples. + content::RunAllPendingInMessageLoop(); + return statistics_reader_.GetHistogramSamplesSinceCreation( + histogram).release(); +} diff --git a/chrome/browser/ui/views/passwords/manage_passwords_view_test.h b/chrome/browser/ui/views/passwords/manage_passwords_view_test.h new file mode 100644 index 0000000..354a192 --- /dev/null +++ b/chrome/browser/ui/views/passwords/manage_passwords_view_test.h @@ -0,0 +1,57 @@ +// 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_PASSWORDS_MANAGE_PASSWORDS_VIEW_TEST_H_ +#define CHROME_BROWSER_UI_VIEWS_PASSWORDS_MANAGE_PASSWORDS_VIEW_TEST_H_ + +#include "base/metrics/histogram_samples.h" +#include "base/test/statistics_delta_reader.h" +#include "chrome/test/base/in_process_browser_test.h" +#include "components/autofill/core/common/password_form.h" +#include "testing/gtest/include/gtest/gtest.h" + +class ManagePasswordsBubbleUIControllerMock; +class ManagePasswordsIconView; + +// Test class for the various password management view bits and pieces. Sets +// up a ManagePasswordsBubbleUIControllerMock, and provides some helper methods +// to poke at the bubble, icon, and controller's state. +class ManagePasswordsViewTest : public InProcessBrowserTest { + public: + ManagePasswordsViewTest() {} + + // InProcessBrowserTest: + virtual void SetUpOnMainThread() OVERRIDE; + + // Get the mock UI controller for the current WebContents. + ManagePasswordsBubbleUIControllerMock* controller(); + + // Get the icon view for the current WebContents. + ManagePasswordsIconView* view(); + + // Execute the browser command to open the manage passwords bubble. + void ExecuteManagePasswordsCommand(); + + // Put the controller, icon, and bubble into a managing-password state. + void SetupManagingPasswords(); + + // Put the controller, icon, and bubble into a pending-password state. + void SetupPendingPassword(); + + // Put the controller, icon, and bubble into a blacklisted state. + void SetupBlackistedPassword(); + + // Get samples for |histogram|. + base::HistogramSamples* GetSamples(const char* histogram); + + autofill::PasswordForm* test_form() { return &test_form_; } + + private: + autofill::PasswordForm test_form_; + base::StatisticsDeltaReader statistics_reader_; + + DISALLOW_COPY_AND_ASSIGN(ManagePasswordsViewTest); +}; + +#endif // CHROME_BROWSER_UI_VIEWS_PASSWORDS_MANAGE_PASSWORDS_VIEW_TEST_H_ diff --git a/chrome/browser/ui/views/passwords/save_password_refusal_combobox_model.cc b/chrome/browser/ui/views/passwords/save_password_refusal_combobox_model.cc new file mode 100644 index 0000000..2ec5998 --- /dev/null +++ b/chrome/browser/ui/views/passwords/save_password_refusal_combobox_model.cc @@ -0,0 +1,33 @@ +// 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 "chrome/browser/ui/views/passwords/save_password_refusal_combobox_model.h" + +#include "grit/generated_resources.h" +#include "ui/base/l10n/l10n_util.h" + +SavePasswordRefusalComboboxModel::SavePasswordRefusalComboboxModel() { + items_.push_back( + l10n_util::GetStringUTF16(IDS_PASSWORD_MANAGER_CANCEL_BUTTON)); + items_.push_back( + l10n_util::GetStringUTF16(IDS_PASSWORD_MANAGER_BLACKLIST_BUTTON)); +} + +SavePasswordRefusalComboboxModel::~SavePasswordRefusalComboboxModel() {} + +int SavePasswordRefusalComboboxModel::GetItemCount() const { + return items_.size(); +} + +base::string16 SavePasswordRefusalComboboxModel::GetItemAt(int index) { + return items_[index]; +} + +bool SavePasswordRefusalComboboxModel::IsItemSeparatorAt(int index) { + return items_[index].empty(); +} + +int SavePasswordRefusalComboboxModel::GetDefaultIndex() const { + return 0; +} diff --git a/chrome/browser/ui/views/passwords/save_password_refusal_combobox_model.h b/chrome/browser/ui/views/passwords/save_password_refusal_combobox_model.h new file mode 100644 index 0000000..e89f9db --- /dev/null +++ b/chrome/browser/ui/views/passwords/save_password_refusal_combobox_model.h @@ -0,0 +1,33 @@ +// 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_PASSWORDS_SAVE_PASSWORD_REFUSAL_COMBOBOX_MODEL_H_ +#define CHROME_BROWSER_UI_VIEWS_PASSWORDS_SAVE_PASSWORD_REFUSAL_COMBOBOX_MODEL_H_ + +#include <vector> +#include "base/basictypes.h" +#include "base/strings/string16.h" +#include "ui/base/models/combobox_model.h" + +class SavePasswordRefusalComboboxModel : public ui::ComboboxModel { + public: + enum { INDEX_NOPE = 0, INDEX_NEVER_FOR_THIS_SITE = 1 }; + + SavePasswordRefusalComboboxModel(); + virtual ~SavePasswordRefusalComboboxModel(); + + private: + // Overridden from ui::ComboboxModel: + virtual int GetItemCount() const OVERRIDE; + virtual base::string16 GetItemAt(int index) OVERRIDE; + virtual bool IsItemSeparatorAt(int index) OVERRIDE; + virtual int GetDefaultIndex() const OVERRIDE; + + std::vector<base::string16> items_; + + DISALLOW_COPY_AND_ASSIGN(SavePasswordRefusalComboboxModel); +}; + + +#endif // CHROME_BROWSER_UI_VIEWS_PASSWORDS_SAVE_PASSWORD_REFUSAL_COMBOBOX_MODEL_H_ diff --git a/chrome/chrome_browser_ui.gypi b/chrome/chrome_browser_ui.gypi index 5aa3781..8fa7401 100644 --- a/chrome/chrome_browser_ui.gypi +++ b/chrome/chrome_browser_ui.gypi @@ -1824,12 +1824,14 @@ 'browser/ui/views/panels/panel_view.h', 'browser/ui/views/panels/taskbar_window_thumbnailer_win.cc', 'browser/ui/views/panels/taskbar_window_thumbnailer_win.h', + 'browser/ui/views/passwords/manage_password_item_view.cc', + 'browser/ui/views/passwords/manage_password_item_view.h', 'browser/ui/views/passwords/manage_passwords_bubble_view.cc', 'browser/ui/views/passwords/manage_passwords_bubble_view.h', 'browser/ui/views/passwords/manage_passwords_icon_view.cc', 'browser/ui/views/passwords/manage_passwords_icon_view.h', - 'browser/ui/views/passwords/manage_password_item_view.cc', - 'browser/ui/views/passwords/manage_password_item_view.h', + 'browser/ui/views/passwords/save_password_refusal_combobox_model.cc', + 'browser/ui/views/passwords/save_password_refusal_combobox_model.h', 'browser/ui/views/password_generation_bubble_view.cc', 'browser/ui/views/password_generation_bubble_view.h', 'browser/ui/views/pdf_password_dialog.cc', diff --git a/chrome/chrome_tests.gypi b/chrome/chrome_tests.gypi index d6e23b0..cd3f4be 100644 --- a/chrome/chrome_tests.gypi +++ b/chrome/chrome_tests.gypi @@ -128,6 +128,10 @@ 'browser/ui/views/message_center/web_notification_tray_browsertest.cc', 'browser/ui/views/omnibox/omnibox_view_views_browsertest.cc', 'browser/ui/views/panels/panel_view_browsertest.cc', + 'browser/ui/views/passwords/manage_passwords_view_test.cc', + 'browser/ui/views/passwords/manage_passwords_view_test.h', + 'browser/ui/views/passwords/manage_passwords_bubble_view_browsertest.cc', + 'browser/ui/views/passwords/manage_passwords_icon_view_browsertest.cc', 'browser/ui/views/ssl_client_certificate_selector_browsertest.cc', 'browser/ui/views/status_icons/status_tray_state_changer_interactive_uitest_win.cc', 'browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc', diff --git a/chrome/chrome_tests_unit.gypi b/chrome/chrome_tests_unit.gypi index 0dd87b7..8b98fa2 100644 --- a/chrome/chrome_tests_unit.gypi +++ b/chrome/chrome_tests_unit.gypi @@ -201,6 +201,8 @@ 'browser/ui/fullscreen/fullscreen_controller_state_tests.h', 'browser/ui/fullscreen/fullscreen_controller_test.cc', 'browser/ui/fullscreen/fullscreen_controller_test.h', + 'browser/ui/passwords/manage_passwords_bubble_ui_controller_mock.cc', + 'browser/ui/passwords/manage_passwords_bubble_ui_controller_mock.h', 'browser/ui/test/test_confirm_bubble_model.cc', 'browser/ui/test/test_confirm_bubble_model.h', 'browser/ui/views/find_bar_host_unittest_util_views.cc', @@ -1680,7 +1682,6 @@ 'browser/ui/panels/panel_mouse_watcher_unittest.cc', 'browser/ui/passwords/manage_passwords_icon_mock.cc', 'browser/ui/passwords/manage_passwords_bubble_model_unittest.cc', - 'browser/ui/passwords/manage_passwords_bubble_ui_controller_mock.cc', 'browser/ui/passwords/manage_passwords_bubble_ui_controller_unittest.cc', 'browser/ui/passwords/password_manager_presenter_unittest.cc', 'browser/ui/search_engines/keyword_editor_controller_unittest.cc', |