diff --git a/chrome/browser/chromeos/policy/device_status_collector_browsertest.cc b/chrome/browser/chromeos/policy/device_status_collector_browsertest.cc index e408233..0343a4e 100644 --- a/chrome/browser/chromeos/policy/device_status_collector_browsertest.cc +++ b/chrome/browser/chromeos/policy/device_status_collector_browsertest.cc
@@ -287,6 +287,8 @@ std::string() /* kiosk_app_update_url */), user_data_dir_override_(chrome::DIR_USER_DATA), update_engine_client_(new chromeos::FakeUpdateEngineClient) { + EXPECT_CALL(*user_manager_, Shutdown()).Times(1); + // Although this is really a unit test which runs in the browser_tests // binary, it doesn't get the unit setup which normally happens in the unit // test binary. @@ -348,14 +350,6 @@ chromeos::LoginState::Initialize(); } - void AddMountPoint(const std::string& mount_point) { - mount_point_map_.insert(DiskMountManager::MountPointMap::value_type( - mount_point, - DiskMountManager::MountPointInfo( - mount_point, mount_point, chromeos::MOUNT_TYPE_DEVICE, - chromeos::disks::MOUNT_CONDITION_NONE))); - } - ~DeviceStatusCollectorTest() override { chromeos::LoginState::Shutdown(); chromeos::CrasAudioHandler::Shutdown(); @@ -378,6 +372,14 @@ settings_helper_.RestoreProvider(); } + protected: + void AddMountPoint(const std::string& mount_point) { + mount_point_map_.insert(DiskMountManager::MountPointMap::value_type( + mount_point, DiskMountManager::MountPointInfo( + mount_point, mount_point, chromeos::MOUNT_TYPE_DEVICE, + chromeos::disks::MOUNT_CONDITION_NONE))); + } + void RestartStatusCollector( const policy::DeviceStatusCollector::VolumeInfoFetcher& volume_info, const policy::DeviceStatusCollector::CPUStatisticsFetcher& cpu_stats, @@ -479,7 +481,6 @@ manager->GetAutoLaunchAppRequiredPlatformVersion()); } - protected: // Convenience method. int64_t ActivePeriodMilliseconds() { return policy::DeviceStatusCollector::kIdlePollIntervalSeconds * 1000; @@ -762,8 +763,7 @@ TEST_F(DeviceStatusCollectorTest, ActivityTimesKeptUntilSubmittedSuccessfully) { ui::IdleState test_states[] = { - ui::IDLE_STATE_ACTIVE, - ui::IDLE_STATE_ACTIVE, + ui::IDLE_STATE_ACTIVE, ui::IDLE_STATE_ACTIVE, }; // Make sure CPU stats get reported in time. If we don't run this, the second // call to |GetStatus()| will contain these stats, but the first call won't @@ -778,10 +778,14 @@ GetActiveMilliseconds(device_status_)); em::DeviceStatusReportRequest first_status(device_status_); - // The collector returns the same status again. + // The collector returns the same activity times again. GetStatus(); - EXPECT_EQ(first_status.SerializeAsString(), - device_status_.SerializeAsString()); + int period_count = first_status.active_period_size(); + EXPECT_EQ(period_count, device_status_.active_period_size()); + for (int n = 0; n < period_count; ++n) { + EXPECT_EQ(first_status.active_period(n).SerializeAsString(), + device_status_.active_period(n).SerializeAsString()); + } // After indicating a successful submit, the submitted status gets cleared, // but what got collected meanwhile sticks around.
diff --git a/chrome/browser/ntp_snippets/content_suggestions_service_factory.cc b/chrome/browser/ntp_snippets/content_suggestions_service_factory.cc index 8d223c3..cfb80a2 100644 --- a/chrome/browser/ntp_snippets/content_suggestions_service_factory.cc +++ b/chrome/browser/ntp_snippets/content_suggestions_service_factory.cc
@@ -376,18 +376,27 @@ service->RegisterProvider(std::move(provider)); } -void SubscribeForGCMPushUpdates(PrefService* pref_service, - ContentSuggestionsService* service, - Profile* profile) { +void SubscribeForGCMPushUpdates( + PrefService* pref_service, + ContentSuggestionsService* content_suggestions_service, + Profile* profile) { + // TODO(mamir): Either pass all params from outside or pass only profile and + // create them inside the method, but be consistent. gcm::GCMDriver* gcm_driver = gcm::GCMProfileServiceFactory::GetForProfile(profile)->driver(); + OAuth2TokenService* token_service = + ProfileOAuth2TokenServiceFactory::GetForProfile(profile); + + SigninManagerBase* signin_manager = + SigninManagerFactory::GetForProfile(profile); + scoped_refptr<net::URLRequestContextGetter> request_context = content::BrowserContext::GetDefaultStoragePartition(profile) ->GetURLRequestContext(); auto subscription_manager = base::MakeUnique<SubscriptionManager>( - request_context, pref_service, + request_context, pref_service, signin_manager, token_service, GetPushUpdatesSubscriptionEndpoint(chrome::GetChannel()), GetPushUpdatesUnsubscriptionEndpoint(chrome::GetChannel())); @@ -411,9 +420,10 @@ base::FilePath database_dir( profile->GetPath().Append(ntp_snippets::kBreakingNewsDatabaseFolder)); auto provider = base::MakeUnique<BreakingNewsSuggestionsProvider>( - service, std::move(handler), base::MakeUnique<base::DefaultClock>(), + content_suggestions_service, std::move(handler), + base::MakeUnique<base::DefaultClock>(), base::MakeUnique<RemoteSuggestionsDatabase>(database_dir, task_runner)); - service->RegisterProvider(std::move(provider)); + content_suggestions_service->RegisterProvider(std::move(provider)); } } // namespace
diff --git a/chrome/browser/ui/BUILD.gn b/chrome/browser/ui/BUILD.gn index 317d597..0fe9ce05 100644 --- a/chrome/browser/ui/BUILD.gn +++ b/chrome/browser/ui/BUILD.gn
@@ -1540,6 +1540,8 @@ "views/harmony/harmony_layout_provider.h", "views/harmony/harmony_typography_provider.cc", "views/harmony/harmony_typography_provider.h", + "views/harmony/textfield_layout.cc", + "views/harmony/textfield_layout.h", "views/importer/import_lock_dialog_view.cc", "views/importer/import_lock_dialog_view.h", "views/location_bar/location_bar_bubble_delegate_view.cc",
diff --git a/chrome/browser/ui/login/login_handler.cc b/chrome/browser/ui/login/login_handler.cc index 651a0dd..18081f4 100644 --- a/chrome/browser/ui/login/login_handler.cc +++ b/chrome/browser/ui/login/login_handler.cc
@@ -13,6 +13,7 @@ #include "base/strings/string_util.h" #include "base/strings/utf_string_conversions.h" #include "base/synchronization/lock.h" +#include "build/build_config.h" #include "chrome/browser/chrome_notification_types.h" #include "chrome/browser/password_manager/chrome_password_manager_client.h" #include "chrome/browser/prerender/prerender_contents.h" @@ -507,16 +508,20 @@ auth_info.challenger, url_formatter::SchemeDisplay::SHOW)); authority_url = auth_info.challenger.GetURL(); } else { - *authority = l10n_util::GetStringFUTF16( - IDS_LOGIN_DIALOG_AUTHORITY, - url_formatter::FormatUrlForSecurityDisplay(request_url)); + *authority = url_formatter::FormatUrlForSecurityDisplay(request_url); +#if defined(OS_ANDROID) + // Android concatenates with a space rather than displaying on two separate + // lines, so it needs some surrounding text. + *authority = + l10n_util::GetStringFUTF16(IDS_LOGIN_DIALOG_AUTHORITY, *authority); +#endif authority_url = request_url; } if (!content::IsOriginSecure(authority_url)) { // TODO(asanka): The string should be different for proxies and servers. // http://crbug.com/620756 - *explanation = l10n_util::GetStringUTF16(IDS_PAGE_INFO_NOT_SECURE_SUMMARY); + *explanation = l10n_util::GetStringUTF16(IDS_LOGIN_DIALOG_NOT_PRIVATE); } else { explanation->clear(); }
diff --git a/chrome/browser/ui/login/login_handler_unittest.cc b/chrome/browser/ui/login/login_handler_unittest.cc index e136655..f4965d3 100644 --- a/chrome/browser/ui/login/login_handler_unittest.cc +++ b/chrome/browser/ui/login/login_handler_unittest.cc
@@ -6,6 +6,7 @@ #include "base/macros.h" #include "base/strings/utf_string_conversions.h" +#include "build/build_config.h" #include "chrome/browser/ui/login/login_handler.h" #include "net/base/auth.h" #include "testing/gtest/include/gtest/gtest.h" @@ -16,7 +17,7 @@ const char kHttpUrl[] = "http://example.com/foo/bar"; const char kBasicAuthScheme[] = "Basic"; const char kFooRealm[] = "Foo"; -const char kInsecureProxy[] = "Your connection to this site is not secure"; +const char kInsecureProxy[] = "Your connection to this site is not private"; enum TargetType { PROXY, SERVER }; @@ -37,76 +38,79 @@ // Insecure proxy {kHttpUrl, {PROXY, kBasicAuthScheme, kFooRealm, "http://example.com"}, - {"The proxy http://example.com requires a username and password.", - kInsecureProxy, "example.com:80/Foo"}}, + {"The proxy http://example.com", kInsecureProxy, "example.com:80/Foo"}}, // Insecure proxy on non-standard port {kHttpUrl, {PROXY, kBasicAuthScheme, kFooRealm, "http://example.com:8009"}, - {"The proxy http://example.com:8009 requires a username and password.", - kInsecureProxy, "example.com:8009/Foo"}}, + {"The proxy http://example.com:8009", kInsecureProxy, + "example.com:8009/Foo"}}, // Secure proxy {kHttpUrl, {PROXY, kBasicAuthScheme, kFooRealm, "https://example.com"}, - {"The proxy https://example.com requires a username and password.", "", - "example.com:443/Foo"}}, + {"The proxy https://example.com", "", "example.com:443/Foo"}}, // Secure proxy on non-standard port {kHttpUrl, {PROXY, kBasicAuthScheme, kFooRealm, "https://example.com:446"}, - {"The proxy https://example.com:446 requires a username and password.", "", - "example.com:446/Foo"}}, + {"The proxy https://example.com:446", "", "example.com:446/Foo"}}, // localhost {kHttpUrl, {PROXY, kBasicAuthScheme, kFooRealm, "http://localhost:7323"}, - {"The proxy http://localhost:7323 requires a username and password.", "", - "localhost:7323/Foo"}}, + {"The proxy http://localhost:7323", "", "localhost:7323/Foo"}}, // Secure server {"https://www.nowhere.org/dir/index.html", {SERVER, kBasicAuthScheme, kFooRealm, nullptr}, - {"https://www.nowhere.org requires a username and password.", "", - "https://www.nowhere.org/Foo"}}, + {"https://www.nowhere.org", "", "https://www.nowhere.org/Foo"}}, // URL uses default port. {"https://www.nowhere.org:443/dir/index.html", {SERVER, kBasicAuthScheme, kFooRealm, nullptr}, - {"https://www.nowhere.org requires a username and password.", "", - "https://www.nowhere.org/Foo"}}, + {"https://www.nowhere.org", "", "https://www.nowhere.org/Foo"}}, // URL uses non-default port. {"https://www.nowhere.org:8443/dir/index.html", {SERVER, kBasicAuthScheme, kFooRealm, nullptr}, - {"https://www.nowhere.org:8443 requires a username and password.", "", - "https://www.nowhere.org:8443/Foo"}}, + {"https://www.nowhere.org:8443", "", "https://www.nowhere.org:8443/Foo"}}, // URL has no trailing slash. {"https://www.nowhere.org", {SERVER, kBasicAuthScheme, kFooRealm, nullptr}, - {"https://www.nowhere.org requires a username and password.", "", - "https://www.nowhere.org/Foo"}}, + {"https://www.nowhere.org", "", "https://www.nowhere.org/Foo"}}, // username:password {"https://foo:bar@www.nowhere.org/dir/index.html", {SERVER, kBasicAuthScheme, kFooRealm, nullptr}, - {"https://www.nowhere.org requires a username and password.", "", - "https://www.nowhere.org/Foo"}}, + {"https://www.nowhere.org", "", "https://www.nowhere.org/Foo"}}, // query {"https://www.nowhere.org/dir/index.html?id=965362", {SERVER, kBasicAuthScheme, kFooRealm, nullptr}, - {"https://www.nowhere.org requires a username and password.", "", - "https://www.nowhere.org/Foo"}}, + {"https://www.nowhere.org", "", "https://www.nowhere.org/Foo"}}, // reference {"https://www.nowhere.org/dir/index.html#toc", {SERVER, kBasicAuthScheme, kFooRealm, nullptr}, - {"https://www.nowhere.org requires a username and password.", "", - "https://www.nowhere.org/Foo"}}, + {"https://www.nowhere.org", "", "https://www.nowhere.org/Foo"}}, }; +base::string16 ExpectedAuthority(bool is_proxy, const char* prefix) { + base::string16 str = base::ASCIIToUTF16(prefix); + // Proxies and Android have additional surrounding text. Otherwise, only the + // host URL is shown. + bool extra_text = is_proxy; +#if defined(OS_ANDROID) + extra_text = true; +#endif + if (extra_text) + str += base::ASCIIToUTF16(" requires a username and password."); + + return str; +} + } // namespace TEST(LoginHandlerTest, DialogStringsAndRealm) { @@ -131,8 +135,9 @@ LoginHandler::GetDialogStrings(request_url, *auth_info, &authority, &explanation); - EXPECT_STREQ(test_case.expected.authority, - base::UTF16ToASCII(authority).c_str()); + EXPECT_EQ(ExpectedAuthority(test_case.auth_info.target_type == PROXY, + test_case.expected.authority), + authority); EXPECT_STREQ(test_case.expected.explanation, base::UTF16ToASCII(explanation).c_str());
diff --git a/chrome/browser/ui/views/harmony/chrome_layout_provider.cc b/chrome/browser/ui/views/harmony/chrome_layout_provider.cc index 53e14c44..c292261 100644 --- a/chrome/browser/ui/views/harmony/chrome_layout_provider.cc +++ b/chrome/browser/ui/views/harmony/chrome_layout_provider.cc
@@ -4,11 +4,14 @@ #include "chrome/browser/ui/views/harmony/chrome_layout_provider.h" +#include <algorithm> + #include "base/logging.h" #include "base/memory/ptr_util.h" #include "chrome/browser/ui/views/harmony/chrome_typography.h" #include "chrome/browser/ui/views/harmony/harmony_layout_provider.h" #include "ui/base/material_design/material_design_controller.h" +#include "ui/gfx/font_list.h" // static ChromeLayoutProvider* ChromeLayoutProvider::Get() { @@ -23,12 +26,22 @@ : base::MakeUnique<ChromeLayoutProvider>(); } +// static +int ChromeLayoutProvider::GetControlHeightForFont(const gfx::FontList& font) { + return std::max(views::style::GetLineHeight(views::style::CONTEXT_LABEL, + views::style::STYLE_PRIMARY), + font.GetHeight()) + + Get()->GetDistanceMetric(DISTANCE_CONTROL_TOTAL_VERTICAL_TEXT_PADDING); +} + int ChromeLayoutProvider::GetDistanceMetric(int metric) const { switch (metric) { case DISTANCE_BUTTON_MINIMUM_WIDTH: return 48; case DISTANCE_CONTROL_LIST_VERTICAL: return GetDistanceMetric(views::DISTANCE_UNRELATED_CONTROL_VERTICAL); + case DISTANCE_CONTROL_TOTAL_VERTICAL_TEXT_PADDING: + return 6; case DISTANCE_RELATED_CONTROL_HORIZONTAL_SMALL: return 8; case DISTANCE_RELATED_CONTROL_VERTICAL_SMALL:
diff --git a/chrome/browser/ui/views/harmony/chrome_layout_provider.h b/chrome/browser/ui/views/harmony/chrome_layout_provider.h index f647d06..c5185da 100644 --- a/chrome/browser/ui/views/harmony/chrome_layout_provider.h +++ b/chrome/browser/ui/views/harmony/chrome_layout_provider.h
@@ -17,6 +17,8 @@ DISTANCE_BUTTON_MINIMUM_WIDTH = views::VIEWS_DISTANCE_END, // Vertical spacing between a list of multiple controls in one column. DISTANCE_CONTROL_LIST_VERTICAL, + // The combined vertical padding applied to text in a control. + DISTANCE_CONTROL_TOTAL_VERTICAL_TEXT_PADDING, // Smaller horizontal spacing between other controls that are logically // related. DISTANCE_RELATED_CONTROL_HORIZONTAL_SMALL, @@ -47,6 +49,10 @@ static ChromeLayoutProvider* Get(); static std::unique_ptr<views::LayoutProvider> CreateLayoutProvider(); + // Calculates the control height based on the |font|'s reported glyph height, + // the default line spacing and DISTANCE_CONTROL_TOTAL_VERTICAL_TEXT_PADDING. + static int GetControlHeightForFont(const gfx::FontList& font); + // views::LayoutProvider: int GetDistanceMetric(int metric) const override; const views::TypographyProvider& GetTypographyProvider() const override;
diff --git a/chrome/browser/ui/views/harmony/harmony_layout_provider.cc b/chrome/browser/ui/views/harmony/harmony_layout_provider.cc index 29d289b..03a1890 100644 --- a/chrome/browser/ui/views/harmony/harmony_layout_provider.cc +++ b/chrome/browser/ui/views/harmony/harmony_layout_provider.cc
@@ -36,6 +36,8 @@ // margin we need to subtract out the padding. return kVisibleMargin - kHarmonyLayoutUnit / 4; } + case DISTANCE_CONTROL_TOTAL_VERTICAL_TEXT_PADDING: + return kHarmonyLayoutUnit / 2; case views::DISTANCE_RELATED_BUTTON_HORIZONTAL: return kHarmonyLayoutUnit / 2; case views::DISTANCE_RELATED_CONTROL_HORIZONTAL:
diff --git a/chrome/browser/ui/views/harmony/textfield_layout.cc b/chrome/browser/ui/views/harmony/textfield_layout.cc new file mode 100644 index 0000000..a663d91 --- /dev/null +++ b/chrome/browser/ui/views/harmony/textfield_layout.cc
@@ -0,0 +1,63 @@ +// Copyright 2017 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/harmony/textfield_layout.h" + +#include "chrome/browser/ui/views/harmony/chrome_layout_provider.h" +#include "ui/views/controls/label.h" +#include "ui/views/controls/textfield/textfield.h" +#include "ui/views/layout/grid_layout.h" + +using views::GridLayout; + +namespace { + +// GridLayout "resize percent" constants. +constexpr float kFixed = 0.f; +constexpr float kStretchy = 1.f; + +} // namespace + +views::ColumnSet* ConfigureTextfieldStack(GridLayout* layout, + int column_set_id) { + ChromeLayoutProvider* provider = ChromeLayoutProvider::Get(); + const int between_padding = + provider->GetDistanceMetric(views::DISTANCE_RELATED_CONTROL_HORIZONTAL); + + views::ColumnSet* column_set = layout->AddColumnSet(column_set_id); + column_set->AddColumn(provider->GetControlLabelGridAlignment(), + GridLayout::CENTER, kFixed, GridLayout::USE_PREF, 0, 0); + // TODO(tapted): This column may need some additional alignment logic under + // Harmony so that its x-offset is not wholly dictated by the string length + // of labels in the first column. + column_set->AddPaddingColumn(kFixed, between_padding); + column_set->AddColumn(GridLayout::FILL, GridLayout::FILL, kStretchy, + GridLayout::USE_PREF, 0, 0); + return column_set; +} + +views::Textfield* AddFirstTextfieldRow(GridLayout* layout, + const base::string16& label_text, + int column_set_id) { + views::Textfield* textfield = new views::Textfield(); + layout->StartRow( + kFixed, column_set_id, + ChromeLayoutProvider::GetControlHeightForFont(textfield->GetFontList())); + views::Label* label = new views::Label( + label_text, views::style::CONTEXT_LABEL, views::style::STYLE_PRIMARY); + textfield->SetAccessibleName(label_text); + + layout->AddView(label); + layout->AddView(textfield); + + return textfield; +} + +views::Textfield* AddTextfieldRow(GridLayout* layout, + const base::string16& label, + int column_set_id) { + layout->AddPaddingRow(kFixed, ChromeLayoutProvider::Get()->GetDistanceMetric( + DISTANCE_CONTROL_LIST_VERTICAL)); + return AddFirstTextfieldRow(layout, label, column_set_id); +}
diff --git a/chrome/browser/ui/views/harmony/textfield_layout.h b/chrome/browser/ui/views/harmony/textfield_layout.h new file mode 100644 index 0000000..4cc8699 --- /dev/null +++ b/chrome/browser/ui/views/harmony/textfield_layout.h
@@ -0,0 +1,31 @@ +// Copyright 2017 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_HARMONY_TEXTFIELD_LAYOUT_H_ +#define CHROME_BROWSER_UI_VIEWS_HARMONY_TEXTFIELD_LAYOUT_H_ + +#include "base/strings/string16.h" + +namespace views { +class ColumnSet; +class GridLayout; +class Textfield; +} // namespace views + +// Configures a three-column ColumnSet for the standard textfield stack layout +// with: {Label, Padding, Textfield}. +views::ColumnSet* ConfigureTextfieldStack(views::GridLayout* layout, + int column_set_id); + +// Adds a views::Label and Textfield to |column_set_id| in |layout|, which has +// been configured as a Textfield stack. The returned Textfield will be owned by +// the View hosting |layout|. +views::Textfield* AddFirstTextfieldRow(views::GridLayout* layout, + const base::string16& label, + int column_set_id); +views::Textfield* AddTextfieldRow(views::GridLayout* layout, + const base::string16& label, + int column_set_id); + +#endif // CHROME_BROWSER_UI_VIEWS_HARMONY_TEXTFIELD_LAYOUT_H_
diff --git a/chrome/browser/ui/views/login_handler_views.cc b/chrome/browser/ui/views/login_handler_views.cc index b86dfcd..9f21726d 100644 --- a/chrome/browser/ui/views/login_handler_views.cc +++ b/chrome/browser/ui/views/login_handler_views.cc
@@ -46,6 +46,8 @@ void OnLoginModelDestroying() override {} // views::DialogDelegate: + bool ShouldShowCloseButton() const override { return false; } + base::string16 GetDialogButtonLabel(ui::DialogButton button) const override { if (button == ui::DIALOG_BUTTON_OK) return l10n_util::GetStringUTF16(IDS_LOGIN_DIALOG_OK_BUTTON_LABEL);
diff --git a/chrome/browser/ui/views/login_view.cc b/chrome/browser/ui/views/login_view.cc index 6fdea248..109a75b 100644 --- a/chrome/browser/ui/views/login_view.cc +++ b/chrome/browser/ui/views/login_view.cc
@@ -5,96 +5,73 @@ #include "chrome/browser/ui/views/login_view.h" #include "chrome/browser/ui/views/harmony/chrome_layout_provider.h" +#include "chrome/browser/ui/views/harmony/chrome_typography.h" +#include "chrome/browser/ui/views/harmony/textfield_layout.h" #include "components/strings/grit/components_strings.h" #include "ui/base/l10n/l10n_util.h" #include "ui/views/controls/label.h" #include "ui/views/controls/textfield/textfield.h" #include "ui/views/layout/grid_layout.h" -static const int kMessageWidth = 320; -static const int kTextfieldStackHorizontalSpacing = 30; - -using password_manager::LoginModel; using views::GridLayout; +namespace { + +constexpr int kHeaderColumnSetId = 0; +constexpr int kFieldsColumnSetId = 1; +constexpr float kFixed = 0.f; +constexpr float kStretchy = 1.f; + +// Adds a row to |layout| and puts a Label in it. +void AddHeaderLabel(GridLayout* layout, + const base::string16& text, + int text_style) { + views::Label* label = + new views::Label(text, views::style::CONTEXT_LABEL, text_style); + label->SetMultiLine(true); + label->SetHorizontalAlignment(gfx::ALIGN_LEFT); + label->SetAllowCharacterBreak(true); + layout->StartRow(kFixed, kHeaderColumnSetId); + layout->AddView(label); +} + +} // namespace + /////////////////////////////////////////////////////////////////////////////// // LoginView, public: LoginView::LoginView(const base::string16& authority, const base::string16& explanation, LoginHandler::LoginModelData* login_model_data) - : username_field_(new views::Textfield()), - password_field_(new views::Textfield()), - username_label_(new views::Label( - l10n_util::GetStringUTF16(IDS_LOGIN_DIALOG_USERNAME_FIELD))), - password_label_(new views::Label( - l10n_util::GetStringUTF16(IDS_LOGIN_DIALOG_PASSWORD_FIELD))), - authority_label_(new views::Label(authority)), - message_label_(nullptr), - login_model_(login_model_data ? login_model_data->model : nullptr) { + : login_model_(login_model_data ? login_model_data->model : nullptr) { + // TODO(tapted): When Harmony is default, this should be removed and left up + // to textfield_layout.h to decide. + constexpr int kMessageWidth = 320; ChromeLayoutProvider* provider = ChromeLayoutProvider::Get(); - password_field_->SetTextInputType(ui::TEXT_INPUT_TYPE_PASSWORD); - - authority_label_->SetMultiLine(true); - authority_label_->SetHorizontalAlignment(gfx::ALIGN_LEFT); - authority_label_->SetAllowCharacterBreak(true); // Initialize the Grid Layout Manager used for this dialog box. GridLayout* layout = GridLayout::CreatePanel(this); - - // Add the column set for the information message at the top of the dialog - // box. - const int single_column_view_set_id = 0; - views::ColumnSet* column_set = - layout->AddColumnSet(single_column_view_set_id); - column_set->AddColumn(GridLayout::FILL, GridLayout::FILL, 1, + views::ColumnSet* column_set = layout->AddColumnSet(kHeaderColumnSetId); + column_set->AddColumn(GridLayout::FILL, GridLayout::FILL, kStretchy, GridLayout::FIXED, kMessageWidth, 0); + AddHeaderLabel(layout, authority, views::style::STYLE_PRIMARY); + AddHeaderLabel(layout, explanation, STYLE_SECONDARY); + layout->AddPaddingRow(kFixed, provider->GetDistanceMetric( + DISTANCE_UNRELATED_CONTROL_VERTICAL_LARGE)); - // Add the column set for the user name and password fields and labels. - const int labels_column_set_id = 1; - column_set = layout->AddColumnSet(labels_column_set_id); - if (provider->UseExtraDialogPadding()) - column_set->AddPaddingColumn(0, kTextfieldStackHorizontalSpacing); - column_set->AddColumn(provider->GetControlLabelGridAlignment(), - GridLayout::CENTER, 0, GridLayout::USE_PREF, 0, 0); - column_set->AddPaddingColumn( - 0, - provider->GetDistanceMetric(views::DISTANCE_RELATED_CONTROL_HORIZONTAL)); - column_set->AddColumn(GridLayout::FILL, GridLayout::CENTER, 1, - GridLayout::USE_PREF, 0, 0); - if (provider->UseExtraDialogPadding()) - column_set->AddPaddingColumn(0, kTextfieldStackHorizontalSpacing); - - layout->StartRow(0, single_column_view_set_id); - layout->AddView(authority_label_); - if (!explanation.empty()) { - message_label_ = new views::Label(explanation); - message_label_->SetMultiLine(true); - message_label_->SetHorizontalAlignment(gfx::ALIGN_LEFT); - message_label_->SetAllowCharacterBreak(true); - layout->AddPaddingRow(0, provider->GetDistanceMetric( - views::DISTANCE_RELATED_CONTROL_VERTICAL)); - layout->StartRow(0, single_column_view_set_id); - layout->AddView(message_label_); - } - - layout->AddPaddingRow(0, provider->GetDistanceMetric( - DISTANCE_UNRELATED_CONTROL_VERTICAL_LARGE)); - - layout->StartRow(0, labels_column_set_id); - layout->AddView(username_label_); - layout->AddView(username_field_); - - layout->AddPaddingRow( - 0, provider->GetDistanceMetric(views::DISTANCE_RELATED_CONTROL_VERTICAL)); - - layout->StartRow(0, labels_column_set_id); - layout->AddView(password_label_); - layout->AddView(password_field_); + ConfigureTextfieldStack(layout, kFieldsColumnSetId); + username_field_ = AddFirstTextfieldRow( + layout, l10n_util::GetStringUTF16(IDS_LOGIN_DIALOG_USERNAME_FIELD), + kFieldsColumnSetId); + password_field_ = AddTextfieldRow( + layout, l10n_util::GetStringUTF16(IDS_LOGIN_DIALOG_PASSWORD_FIELD), + kFieldsColumnSetId); + password_field_->SetTextInputType(ui::TEXT_INPUT_TYPE_PASSWORD); if (provider->UseExtraDialogPadding()) { - layout->AddPaddingRow(0, provider->GetDistanceMetric( - views::DISTANCE_UNRELATED_CONTROL_VERTICAL)); + layout->AddPaddingRow(kFixed, + provider->GetDistanceMetric( + views::DISTANCE_UNRELATED_CONTROL_VERTICAL)); } if (login_model_data) {
diff --git a/chrome/browser/ui/views/login_view.h b/chrome/browser/ui/views/login_view.h index 4b6bd28..d717cc4 100644 --- a/chrome/browser/ui/views/login_view.h +++ b/chrome/browser/ui/views/login_view.h
@@ -5,15 +5,13 @@ #ifndef CHROME_BROWSER_UI_VIEWS_LOGIN_VIEW_H_ #define CHROME_BROWSER_UI_VIEWS_LOGIN_VIEW_H_ -#include "base/compiler_specific.h" #include "base/macros.h" +#include "base/strings/string16.h" #include "chrome/browser/ui/login/login_handler.h" -#include "components/autofill/core/common/password_form.h" #include "components/password_manager/core/browser/login_model.h" #include "ui/views/view.h" namespace views { -class Label; class Textfield; } @@ -39,7 +37,7 @@ const base::string16& password) override; void OnLoginModelDestroying() override; - // Used by LoginHandlerWin to set the initial focus. + // Used by LoginHandlerViews to set the initial focus. views::View* GetInitiallyFocusedView(); private: @@ -50,14 +48,6 @@ views::Textfield* username_field_; views::Textfield* password_field_; - // Button labels - views::Label* username_label_; - views::Label* password_label_; - - // Authority and security state messages. - views::Label* authority_label_; - views::Label* message_label_; - // If not null, points to a model we need to notify of our own destruction // so it doesn't try and access this when its too late. password_manager::LoginModel* login_model_;
diff --git a/components/login_dialog_strings.grdp b/components/login_dialog_strings.grdp index 1929e7b..bd4417c 100644 --- a/components/login_dialog_strings.grdp +++ b/components/login_dialog_strings.grdp
@@ -1,23 +1,36 @@ <?xml version="1.0" encoding="utf-8"?> <grit-part> - <message name="IDS_LOGIN_DIALOG_TITLE" desc="String to be displayed in the title bar of the login prompt dialog" formatter_data="android_java"> - Authentication Required - </message> + <if expr="use_titlecase"> + <message name="IDS_LOGIN_DIALOG_TITLE" desc="In title case: String to be displayed in the title bar of the login prompt dialog" formatter_data="android_java"> + Authentication Required + </message> + <message name="IDS_LOGIN_DIALOG_OK_BUTTON_LABEL" desc="In title case: The label of the 'Log In' button on the login prompt dialog" formatter_data="android_java"> + Log In + </message> + </if> + <if expr="not use_titlecase"> + <message name="IDS_LOGIN_DIALOG_TITLE" desc="In sentence case: String to be displayed in the title bar of the login prompt dialog" formatter_data="android_java"> + Authentication required + </message> + <message name="IDS_LOGIN_DIALOG_OK_BUTTON_LABEL" desc="In sentence case: The label of the 'Log in' button on the login prompt dialog" formatter_data="android_java"> + Log in + </message> + </if> <message name="IDS_LOGIN_DIALOG_AUTHORITY" desc="String to be displayed in the login prompt dialog to explain that the user needs to log in, and the name of the web site"> <ph name="DOMAIN">$1<ex>google.com</ex></ph> requires a username and password. </message> <message name="IDS_LOGIN_DIALOG_PROXY_AUTHORITY" desc="String to be displayed in the proxy login prompt dialog to explain that the user needs to log in, and the name of the proxy"> The proxy <ph name="DOMAIN">$1<ex>google.com</ex></ph> requires a username and password. </message> + <message name="IDS_LOGIN_DIALOG_NOT_PRIVATE" desc="A phrase displayed on login dialogs if the connection to the current website is not private."> + Your connection to this site is not private + </message> <message name="IDS_LOGIN_DIALOG_USERNAME_FIELD" desc="The label of the username field in the login prompt dialog" formatter_data="android_java"> - User Name: + Username </message> <message name="IDS_LOGIN_DIALOG_PASSWORD_FIELD" desc="The label of the password field in the login prompt dialog" formatter_data="android_java"> - Password: - </message> - <message name="IDS_LOGIN_DIALOG_OK_BUTTON_LABEL" desc="The label of the 'Log In' button on the login prompt dialog" formatter_data="android_java"> - Log In + Password </message> </grit-part>
diff --git a/components/ntp_snippets/BUILD.gn b/components/ntp_snippets/BUILD.gn index 7ad4db1..c8b6443c 100644 --- a/components/ntp_snippets/BUILD.gn +++ b/components/ntp_snippets/BUILD.gn
@@ -210,6 +210,7 @@ "//components/signin/core/common", "//components/strings", "//components/sync:test_support_driver", + "//components/sync_preferences:test_support", "//components/sync_sessions", "//components/variations:test_support", "//components/web_resource:web_resource",
diff --git a/components/ntp_snippets/DEPS b/components/ntp_snippets/DEPS index cd0ce25..9e6b868 100644 --- a/components/ntp_snippets/DEPS +++ b/components/ntp_snippets/DEPS
@@ -13,6 +13,7 @@ "+components/reading_list", "+components/signin", "+components/strings/grit/components_strings.h", + "+components/sync_preferences/testing_pref_service_syncable.h", "+components/sync/driver", "+components/url_formatter", "+components/variations",
diff --git a/components/ntp_snippets/breaking_news/breaking_news_gcm_app_handler.cc b/components/ntp_snippets/breaking_news/breaking_news_gcm_app_handler.cc index 9c7ca33..5a85841 100644 --- a/components/ntp_snippets/breaking_news/breaking_news_gcm_app_handler.cc +++ b/components/ntp_snippets/breaking_news/breaking_news_gcm_app_handler.cc
@@ -60,20 +60,18 @@ DCHECK_EQ(gcm_driver_->GetAppHandler(kBreakingNewsGCMAppID), this); gcm_driver_->RemoveAppHandler(kBreakingNewsGCMAppID); on_new_content_callback_ = OnNewContentCallback(); - // TODO(mamir): Check which token should be used for unsubscription when - // handling change in the token. - std::string token = pref_service_->GetString( - ntp_snippets::prefs::kBreakingNewsGCMSubscriptionTokenCache); - subscription_manager_->Unsubscribe(token); + subscription_manager_->Unsubscribe(); } void BreakingNewsGCMAppHandler::Subscribe() { - std::string token = pref_service_->GetString( - ntp_snippets::prefs::kBreakingNewsGCMSubscriptionTokenCache); + // TODO(mamir): This logic should be moved to the SubscriptionManager. + std::string token = + pref_service_->GetString(prefs::kBreakingNewsGCMSubscriptionTokenCache); // If a token has been already obtained, subscribe directly at the content - // suggestions server. + // suggestions server. Otherwise, obtain a GCM token first. if (!token.empty()) { - if (!subscription_manager_->IsSubscribed()) { + if (!subscription_manager_->IsSubscribed() || + subscription_manager_->NeedsToResubscribe()) { subscription_manager_->Subscribe(token); } return; @@ -81,19 +79,19 @@ instance_id_driver_->GetInstanceID(kBreakingNewsGCMAppID) ->GetToken(kBreakingNewsGCMSenderId, kGCMScope, - std::map<std::string, std::string>() /* options */, + /*options=*/std::map<std::string, std::string>(), base::Bind(&BreakingNewsGCMAppHandler::DidSubscribe, weak_ptr_factory_.GetWeakPtr())); } -void BreakingNewsGCMAppHandler::DidSubscribe(const std::string& subscription_id, - InstanceID::Result result) { +void BreakingNewsGCMAppHandler::DidSubscribe( + const std::string& subscription_token, + InstanceID::Result result) { switch (result) { case InstanceID::SUCCESS: - pref_service_->SetString( - ntp_snippets::prefs::kBreakingNewsGCMSubscriptionTokenCache, - subscription_id); - subscription_manager_->Subscribe(subscription_id); + pref_service_->SetString(prefs::kBreakingNewsGCMSubscriptionTokenCache, + subscription_token); + subscription_manager_->Subscribe(subscription_token); return; case InstanceID::INVALID_PARAMETER: case InstanceID::DISABLED: @@ -112,8 +110,7 @@ void BreakingNewsGCMAppHandler::ShutdownHandler() {} void BreakingNewsGCMAppHandler::OnStoreReset() { - pref_service_->ClearPref( - ntp_snippets::prefs::kBreakingNewsGCMSubscriptionTokenCache); + pref_service_->ClearPref(prefs::kBreakingNewsGCMSubscriptionTokenCache); } void BreakingNewsGCMAppHandler::OnMessage(const std::string& app_id,
diff --git a/components/ntp_snippets/breaking_news/breaking_news_gcm_app_handler.h b/components/ntp_snippets/breaking_news/breaking_news_gcm_app_handler.h index 4dcfe34..59527c3 100644 --- a/components/ntp_snippets/breaking_news/breaking_news_gcm_app_handler.h +++ b/components/ntp_snippets/breaking_news/breaking_news_gcm_app_handler.h
@@ -77,7 +77,7 @@ void Subscribe(); // Called after the subscription is obtained from the GCM server. - void DidSubscribe(const std::string& subscription_id, + void DidSubscribe(const std::string& subscription_token, instance_id::InstanceID::Result result); // Called after successfully parsing the received suggestion JSON.
diff --git a/components/ntp_snippets/breaking_news/subscription_json_request.cc b/components/ntp_snippets/breaking_news/subscription_json_request.cc index 8aec37c..187e463 100644 --- a/components/ntp_snippets/breaking_news/subscription_json_request.cc +++ b/components/ntp_snippets/breaking_news/subscription_json_request.cc
@@ -26,13 +26,7 @@ SubscriptionJsonRequest::SubscriptionJsonRequest() = default; -SubscriptionJsonRequest::~SubscriptionJsonRequest() { - if (!request_completed_callback_.is_null()) { - std::move(request_completed_callback_) - .Run(ntp_snippets::Status(ntp_snippets::StatusCode::TEMPORARY_ERROR, - "cancelled")); - } -} +SubscriptionJsonRequest::~SubscriptionJsonRequest() = default; void SubscriptionJsonRequest::Start(CompletedCallback callback) { DCHECK(request_completed_callback_.is_null()) << "Request already running!"; @@ -49,18 +43,15 @@ if (!status.is_success()) { std::move(request_completed_callback_) - .Run(ntp_snippets::Status( - ntp_snippets::StatusCode::TEMPORARY_ERROR, - base::StringPrintf("Internal Error: %d", status.error()))); + .Run(Status(StatusCode::TEMPORARY_ERROR, + base::StringPrintf("Network Error: %d", status.error()))); } else if (response != net::HTTP_OK) { std::move(request_completed_callback_) - .Run(ntp_snippets::Status( - ntp_snippets::StatusCode::PERMANENT_ERROR, - base::StringPrintf("HTTP Error: %d", response))); + .Run(Status(StatusCode::PERMANENT_ERROR, + base::StringPrintf("HTTP Error: %d", response))); } else { std::move(request_completed_callback_) - .Run(ntp_snippets::Status(ntp_snippets::StatusCode::SUCCESS, - std::string())); + .Run(Status(StatusCode::SUCCESS, std::string())); } } @@ -80,9 +71,9 @@ request->url_fetcher_ = BuildURLFetcher(request.get(), headers, body); // Log the request for debugging network issues. - VLOG(1) << "Sending a subscription request to " << url_ << ":\n" - << headers << "\n" - << body; + DVLOG(1) << "Building a subscription request to " << url_ << ":\n" + << headers << "\n" + << body; return request; } @@ -106,10 +97,20 @@ return *this; } +SubscriptionJsonRequest::Builder& +SubscriptionJsonRequest::Builder::SetAuthenticationHeader( + const std::string& auth_header) { + auth_header_ = auth_header; + return *this; +} + std::string SubscriptionJsonRequest::Builder::BuildHeaders() const { HttpRequestHeaders headers; - headers.SetHeader("Content-Type", "application/json; charset=UTF-8"); - + headers.SetHeader(HttpRequestHeaders::kContentType, + "application/json; charset=UTF-8"); + if (!auth_header_.empty()) { + headers.SetHeader(HttpRequestHeaders::kAuthorization, auth_header_); + } // Add X-Client-Data header with experiment IDs from field trials. // Note: It's OK to pass |is_signed_in| false if it's unknown, as it does // not affect transmission of experiments coming from the variations server.
diff --git a/components/ntp_snippets/breaking_news/subscription_json_request.h b/components/ntp_snippets/breaking_news/subscription_json_request.h index f2aef592..b0aa71c 100644 --- a/components/ntp_snippets/breaking_news/subscription_json_request.h +++ b/components/ntp_snippets/breaking_news/subscription_json_request.h
@@ -27,10 +27,9 @@ public: // A client can expect a message in the status only, if there was any error // during the subscription. In successful cases, it will be an empty string. - using CompletedCallback = - base::OnceCallback<void(const ntp_snippets::Status& status)>; + using CompletedCallback = base::OnceCallback<void(const Status& status)>; - // Builds non-authenticated SubscriptionJsonRequests. + // Builds non-authenticated and authenticated SubscriptionJsonRequests. class Builder { public: Builder(); @@ -44,6 +43,7 @@ Builder& SetUrl(const GURL& url); Builder& SetUrlRequestContextGetter( const scoped_refptr<net::URLRequestContextGetter>& context_getter); + Builder& SetAuthenticationHeader(const std::string& auth_header); private: std::string BuildHeaders() const; @@ -53,18 +53,21 @@ const std::string& headers, const std::string& body) const; - // GCM subscribtion token obtain from GCM driver (instanceID::getToken()) + // GCM subscription token obtained from GCM driver (instanceID::getToken()). std::string token_; - // TODO(mamir): Additional fields to be added: country, language + // TODO(mamir): Additional fields to be added: country, language. GURL url_; scoped_refptr<net::URLRequestContextGetter> url_request_context_getter_; + std::string auth_header_; + + DISALLOW_COPY_AND_ASSIGN(Builder); }; ~SubscriptionJsonRequest() override; - // Starts an async request. The callback is invoked when the request succeeds, - // fails or gets destroyed. + // Starts an async request. The callback is invoked when the request succeeds + // or fails. The callback is not called if the request is destroyed. void Start(CompletedCallback callback); private:
diff --git a/components/ntp_snippets/breaking_news/subscription_json_request_unittest.cc b/components/ntp_snippets/breaking_news/subscription_json_request_unittest.cc index cb3fe4b..0f09e8b 100644 --- a/components/ntp_snippets/breaking_news/subscription_json_request_unittest.cc +++ b/components/ntp_snippets/breaking_news/subscription_json_request_unittest.cc
@@ -95,8 +95,6 @@ GURL url("http://valid-url.test"); base::MockCallback<SubscriptionJsonRequest::CompletedCallback> callback; - ntp_snippets::Status status(StatusCode::SUCCESS, "initial"); - EXPECT_CALL(callback, Run(_)).WillOnce(SaveArg<0>(&status)); SubscriptionJsonRequest::Builder builder; std::unique_ptr<SubscriptionJsonRequest> request = @@ -126,13 +124,12 @@ EXPECT_THAT(url_fetcher->upload_data(), EqualsJSON(expected_body)); } -TEST_F(SubscriptionJsonRequestTest, InvokesCallbackWhenCancelled) { +TEST_F(SubscriptionJsonRequestTest, ShouldNotInvokeCallbackWhenCancelled) { std::string token = "1234567890"; GURL url("http://valid-url.test"); base::MockCallback<SubscriptionJsonRequest::CompletedCallback> callback; - ntp_snippets::Status status(StatusCode::SUCCESS, "initial"); - EXPECT_CALL(callback, Run(_)).WillOnce(SaveArg<0>(&status)); + EXPECT_CALL(callback, Run(_)).Times(0); SubscriptionJsonRequest::Builder builder; std::unique_ptr<SubscriptionJsonRequest> request = @@ -144,9 +141,6 @@ // Destroy the request before getting any response. request.reset(); - - EXPECT_EQ(status.code, StatusCode::TEMPORARY_ERROR); - EXPECT_EQ(status.message, "cancelled"); } TEST_F(SubscriptionJsonRequestTest, SubscribeWithoutErrors) {
diff --git a/components/ntp_snippets/breaking_news/subscription_manager.cc b/components/ntp_snippets/breaking_news/subscription_manager.cc index aba15d2..f9077a7 100644 --- a/components/ntp_snippets/breaking_news/subscription_manager.cc +++ b/components/ntp_snippets/breaking_news/subscription_manager.cc
@@ -3,13 +3,18 @@ // found in the LICENSE file. #include "components/ntp_snippets/breaking_news/subscription_manager.h" + #include "base/bind.h" +#include "base/memory/ptr_util.h" #include "base/metrics/field_trial_params.h" +#include "base/strings/stringprintf.h" #include "components/ntp_snippets/breaking_news/subscription_json_request.h" #include "components/ntp_snippets/features.h" #include "components/ntp_snippets/ntp_snippets_constants.h" #include "components/ntp_snippets/pref_names.h" #include "components/prefs/pref_service.h" +#include "components/signin/core/browser/access_token_fetcher.h" +#include "components/signin/core/browser/signin_manager_base.h" namespace ntp_snippets { @@ -22,109 +27,240 @@ // Variation parameter for chrome-push-unsubscription backend. const char kPushUnsubscriptionBackendParam[] = "push_unsubscription_backend"; -} + +const char kAuthorizationRequestHeaderFormat[] = "Bearer %s"; + +} // namespace + +class SubscriptionManager::SigninObserver : public SigninManagerBase::Observer { + public: + SigninObserver(SigninManagerBase* signin_manager, + const base::Closure& signin_status_changed_callback) + : signin_manager_(signin_manager), + signin_status_changed_callback_(signin_status_changed_callback) { + signin_manager_->AddObserver(this); + } + + ~SigninObserver() override { signin_manager_->RemoveObserver(this); } + + private: + // SigninManagerBase::Observer implementation. + void GoogleSigninSucceeded(const std::string& account_id, + const std::string& username) override { + signin_status_changed_callback_.Run(); + } + + void GoogleSignedOut(const std::string& account_id, + const std::string& username) override { + signin_status_changed_callback_.Run(); + } + + SigninManagerBase* const signin_manager_; + base::Closure signin_status_changed_callback_; +}; SubscriptionManager::SubscriptionManager( scoped_refptr<net::URLRequestContextGetter> url_request_context_getter, PrefService* pref_service, + SigninManagerBase* signin_manager, + OAuth2TokenService* access_token_service, const GURL& subscribe_url, const GURL& unsubscribe_url) : url_request_context_getter_(std::move(url_request_context_getter)), pref_service_(pref_service), + signin_manager_(signin_manager), + signin_observer_(base::MakeUnique<SigninObserver>( + signin_manager, + base::Bind(&SubscriptionManager::SigninStatusChanged, + base::Unretained(this)))), + access_token_service_(access_token_service), subscribe_url_(subscribe_url), unsubscribe_url_(unsubscribe_url) {} SubscriptionManager::~SubscriptionManager() = default; -void SubscriptionManager::Subscribe(const std::string& token) { - DCHECK(!subscription_request_); - subscription_token_ = token; +void SubscriptionManager::Subscribe(const std::string& subscription_token) { + // If there is a request in flight, cancel it. + if (request_) { + request_ = nullptr; + } + if (signin_manager_->IsAuthenticated()) { + StartAccessTokenRequest(subscription_token); + } else { + SubscribeInternal(subscription_token, /*access_token=*/std::string()); + } +} + +void SubscriptionManager::SubscribeInternal( + const std::string& subscription_token, + const std::string& access_token) { SubscriptionJsonRequest::Builder builder; - builder.SetToken(token) + builder.SetToken(subscription_token) .SetUrlRequestContextGetter(url_request_context_getter_) .SetUrl(subscribe_url_); - - subscription_request_ = builder.Build(); - subscription_request_->Start(base::BindOnce( - &SubscriptionManager::DidSubscribe, base::Unretained(this))); -} - -bool SubscriptionManager::CanSubscribeNow() { - if (subscription_request_) { - return false; + if (!access_token.empty()) { + builder.SetAuthenticationHeader(base::StringPrintf( + kAuthorizationRequestHeaderFormat, access_token.c_str())); } - return true; + + request_ = builder.Build(); + request_->Start(base::BindOnce(&SubscriptionManager::DidSubscribe, + base::Unretained(this), subscription_token, + /*is_authenticated=*/!access_token.empty())); } -void SubscriptionManager::DidSubscribe(const ntp_snippets::Status& status) { - subscription_request_.reset(); +void SubscriptionManager::StartAccessTokenRequest( + const std::string& subscription_token) { + // If there is already an ongoing token request, destroy it. + if (access_token_fetcher_) { + access_token_fetcher_ = nullptr; + } + + OAuth2TokenService::ScopeSet scopes = {kContentSuggestionsApiScope}; + access_token_fetcher_ = base::MakeUnique<AccessTokenFetcher>( + "ntp_snippets", signin_manager_, access_token_service_, scopes, + base::BindOnce(&SubscriptionManager::AccessTokenFetchFinished, + base::Unretained(this), subscription_token)); +} + +void SubscriptionManager::AccessTokenFetchFinished( + const std::string& subscription_token, + const GoogleServiceAuthError& error, + const std::string& access_token) { + // Delete the fetcher only after we leave this method (which is called from + // the fetcher itself). + std::unique_ptr<AccessTokenFetcher> access_token_fetcher_deleter( + std::move(access_token_fetcher_)); + + if (error.state() != GoogleServiceAuthError::NONE) { + // In case of error, we will retry on next Chrome restart. + return; + } + DCHECK(!access_token.empty()); + SubscribeInternal(subscription_token, access_token); +} + +void SubscriptionManager::DidSubscribe(const std::string& subscription_token, + bool is_authenticated, + const Status& status) { + // Delete the request only after we leave this method (which is called from + // the request itself). + std::unique_ptr<internal::SubscriptionJsonRequest> request_deleter( + std::move(request_)); switch (status.code) { - case ntp_snippets::StatusCode::SUCCESS: + case StatusCode::SUCCESS: // In case of successful subscription, store the same data used for - // subscription in order to be able to re-subscribe in case of data + // subscription in order to be able to resubscribe in case of data // change. - // TODO(mamir): store region and language. - pref_service_->SetString( - ntp_snippets::prefs::kBreakingNewsSubscriptionDataToken, - subscription_token_); + // TODO(mamir): Store region and language. + pref_service_->SetString(prefs::kBreakingNewsSubscriptionDataToken, + subscription_token); + pref_service_->SetBoolean( + prefs::kBreakingNewsSubscriptionDataIsAuthenticated, + is_authenticated); break; default: - // TODO(mamir): handle failure. + // TODO(mamir): Handle failure. break; } } -bool SubscriptionManager::CanUnsubscribeNow() { - if (unsubscription_request_) { - return false; - } - return true; +void SubscriptionManager::Unsubscribe() { + std::string token = + pref_service_->GetString(prefs::kBreakingNewsSubscriptionDataToken); + ResubscribeInternal(/*old_token=*/token, /*new_token=*/std::string()); } -void SubscriptionManager::Unsubscribe(const std::string& token) { - DCHECK(!unsubscription_request_); - unsubscription_token_ = token; +void SubscriptionManager::ResubscribeInternal(const std::string& old_token, + const std::string& new_token) { + // If there is an request in flight, cancel it. + if (request_) { + request_ = nullptr; + } + SubscriptionJsonRequest::Builder builder; - builder.SetToken(token) + builder.SetToken(old_token) .SetUrlRequestContextGetter(url_request_context_getter_) .SetUrl(unsubscribe_url_); - unsubscription_request_ = builder.Build(); - unsubscription_request_->Start(base::BindOnce( - &SubscriptionManager::DidUnsubscribe, base::Unretained(this))); + request_ = builder.Build(); + request_->Start(base::BindOnce(&SubscriptionManager::DidUnsubscribe, + base::Unretained(this), new_token)); } bool SubscriptionManager::IsSubscribed() { - std::string subscription_token_ = pref_service_->GetString( - ntp_snippets::prefs::kBreakingNewsSubscriptionDataToken); - return !subscription_token_.empty(); + std::string subscription_token = + pref_service_->GetString(prefs::kBreakingNewsSubscriptionDataToken); + return !subscription_token.empty(); } -void SubscriptionManager::DidUnsubscribe(const ntp_snippets::Status& status) { - unsubscription_request_.reset(); +bool SubscriptionManager::NeedsToResubscribe() { + // Check if authentication state changed after subscription. + bool is_auth_subscribe = pref_service_->GetBoolean( + prefs::kBreakingNewsSubscriptionDataIsAuthenticated); + bool is_authenticated = signin_manager_->IsAuthenticated(); + return is_auth_subscribe != is_authenticated; +} + +void SubscriptionManager::Resubscribe(const std::string& new_token) { + std::string old_token = + pref_service_->GetString(prefs::kBreakingNewsSubscriptionDataToken); + if (old_token == new_token) { + // If the token didn't change, subscribe directly. The server handles the + // unsubscription if previous subscriptions exists. + Subscribe(old_token); + } else { + ResubscribeInternal(old_token, new_token); + } +} + +void SubscriptionManager::DidUnsubscribe(const std::string& new_token, + const Status& status) { + // Delete the request only after we leave this method (which is called from + // the request itself). + std::unique_ptr<internal::SubscriptionJsonRequest> request_deleter( + std::move(request_)); switch (status.code) { - case ntp_snippets::StatusCode::SUCCESS: + case StatusCode::SUCCESS: // In case of successful unsubscription, clear the previously stored data. - // TODO(mamir): clear stored region and language. + // TODO(mamir): Clear stored region and language. + pref_service_->ClearPref(prefs::kBreakingNewsSubscriptionDataToken); pref_service_->ClearPref( - ntp_snippets::prefs::kBreakingNewsSubscriptionDataToken); + prefs::kBreakingNewsSubscriptionDataIsAuthenticated); + if (!new_token.empty()) { + Subscribe(new_token); + } break; default: - // TODO(mamir): handle failure. + // TODO(mamir): Handle failure. break; } } +void SubscriptionManager::SigninStatusChanged() { + // If subscribed already, resubscribe. + if (IsSubscribed()) { + if (request_) { + request_ = nullptr; + } + std::string token = + pref_service_->GetString(prefs::kBreakingNewsSubscriptionDataToken); + Subscribe(token); + } +} + void SubscriptionManager::RegisterProfilePrefs(PrefRegistrySimple* registry) { registry->RegisterStringPref(prefs::kBreakingNewsSubscriptionDataToken, std::string()); + registry->RegisterBooleanPref( + prefs::kBreakingNewsSubscriptionDataIsAuthenticated, false); } GURL GetPushUpdatesSubscriptionEndpoint(version_info::Channel channel) { std::string endpoint = base::GetFieldTrialParamValueByFeature( - ntp_snippets::kBreakingNewsPushFeature, kPushSubscriptionBackendParam); + kBreakingNewsPushFeature, kPushSubscriptionBackendParam); if (!endpoint.empty()) { return GURL{endpoint}; } @@ -145,7 +281,7 @@ GURL GetPushUpdatesUnsubscriptionEndpoint(version_info::Channel channel) { std::string endpoint = base::GetFieldTrialParamValueByFeature( - ntp_snippets::kBreakingNewsPushFeature, kPushUnsubscriptionBackendParam); + kBreakingNewsPushFeature, kPushUnsubscriptionBackendParam); if (!endpoint.empty()) { return GURL{endpoint}; }
diff --git a/components/ntp_snippets/breaking_news/subscription_manager.h b/components/ntp_snippets/breaking_news/subscription_manager.h index e75c241..ba5a3b3 100644 --- a/components/ntp_snippets/breaking_news/subscription_manager.h +++ b/components/ntp_snippets/breaking_news/subscription_manager.h
@@ -7,10 +7,13 @@ #include "components/ntp_snippets/breaking_news/subscription_json_request.h" #include "components/prefs/pref_registry_simple.h" +#include "components/signin/core/browser/signin_manager_base.h" #include "components/version_info/version_info.h" #include "net/url_request/url_request_context_getter.h" #include "url/gurl.h" +class AccessTokenFetcher; +class OAuth2TokenService; class PrefRegistrySimple; class PrefService; @@ -34,38 +37,67 @@ SubscriptionManager( scoped_refptr<net::URLRequestContextGetter> url_request_context_getter, PrefService* pref_service, + SigninManagerBase* signin_manager, + OAuth2TokenService* access_token_service, const GURL& subscribe_url, const GURL& unsubscribe_url); ~SubscriptionManager(); void Subscribe(const std::string& token); - bool CanSubscribeNow(); - void Unsubscribe(const std::string& token); - bool CanUnsubscribeNow(); + void Unsubscribe(); bool IsSubscribed(); + void Resubscribe(const std::string& new_token); + + // Checks if some data that has been used when subscribing has changed. For + // example, the user has signed in. + bool NeedsToResubscribe(); + static void RegisterProfilePrefs(PrefRegistrySimple* registry); private: - std::string subscription_token_; - std::string unsubscription_token_; + class SigninObserver; + + void SigninStatusChanged(); + + void DidSubscribe(const std::string& subscription_token, + bool is_authenticated, + const Status& status); + void DidUnsubscribe(const std::string& new_token, const Status& status); + void SubscribeInternal(const std::string& subscription_token, + const std::string& access_token); + + // If |new_token| is empty, this will just unsubscribe. If |new_token| is + // non-empty, a subscription request with the |new_token| will be started upon + // successful unsubscription. + void ResubscribeInternal(const std::string& old_token, + const std::string& new_token); + + // |subscription_token| is the token when subscribing after obtaining the + // access token. + void StartAccessTokenRequest(const std::string& subscription_token); + void AccessTokenFetchFinished(const std::string& subscription_token, + const GoogleServiceAuthError& error, + const std::string& access_token); // Holds the URL request context. scoped_refptr<net::URLRequestContextGetter> url_request_context_getter_; - std::unique_ptr<internal::SubscriptionJsonRequest> subscription_request_; - std::unique_ptr<internal::SubscriptionJsonRequest> unsubscription_request_; + std::unique_ptr<internal::SubscriptionJsonRequest> request_; + std::unique_ptr<AccessTokenFetcher> access_token_fetcher_; PrefService* pref_service_; + // Authentication for signed-in users. + SigninManagerBase* signin_manager_; + std::unique_ptr<SigninObserver> signin_observer_; + OAuth2TokenService* access_token_service_; + // API endpoint for subscribing and unsubscribing. const GURL subscribe_url_; const GURL unsubscribe_url_; - void DidSubscribe(const ntp_snippets::Status& status); - void DidUnsubscribe(const ntp_snippets::Status& status); - DISALLOW_COPY_AND_ASSIGN(SubscriptionManager); }; }
diff --git a/components/ntp_snippets/breaking_news/subscription_manager_unittest.cc b/components/ntp_snippets/breaking_news/subscription_manager_unittest.cc index b90d1e5..ad222d5 100644 --- a/components/ntp_snippets/breaking_news/subscription_manager_unittest.cc +++ b/components/ntp_snippets/breaking_news/subscription_manager_unittest.cc
@@ -5,30 +5,47 @@ #include "components/ntp_snippets/breaking_news/subscription_manager.h" #include "base/message_loop/message_loop.h" +#include "build/build_config.h" #include "components/ntp_snippets/pref_names.h" +#include "components/ntp_snippets/remote/test_utils.h" #include "components/prefs/testing_pref_service.h" +#include "components/signin/core/browser/fake_profile_oauth2_token_service.h" +#include "components/signin/core/browser/fake_signin_manager.h" +#include "components/signin/core/browser/test_signin_client.h" +#include "google_apis/gaia/fake_oauth2_token_service_delegate.h" #include "net/url_request/test_url_fetcher_factory.h" #include "net/url_request/url_request_test_util.h" #include "testing/gtest/include/gtest/gtest.h" namespace ntp_snippets { +const char kTestEmail[] = "test@email.com"; +const char kSubscriptionUrl[] = "http://valid-url.test/subscribe"; +const char kUnsubscriptionUrl[] = "http://valid-url.test/unsubscribe"; + class SubscriptionManagerTest : public testing::Test { public: SubscriptionManagerTest() : request_context_getter_( - new net::TestURLRequestContextGetter(message_loop_.task_runner())), - pref_service_(base::MakeUnique<TestingPrefServiceSimple>()) {} + new net::TestURLRequestContextGetter(message_loop_.task_runner())) { + } void SetUp() override { - SubscriptionManager::RegisterProfilePrefs(pref_service_->registry()); + SubscriptionManager::RegisterProfilePrefs( + utils_.pref_service()->registry()); } scoped_refptr<net::URLRequestContextGetter> GetRequestContext() { return request_context_getter_.get(); } - PrefService* GetPrefService() { return pref_service_.get(); } + PrefService* GetPrefService() { return utils_.pref_service(); } + + FakeProfileOAuth2TokenService* GetOAuth2TokenService() { + return utils_.token_service(); + } + + SigninManagerBase* GetSigninManager() { return utils_.fake_signin_manager(); } net::TestURLFetcher* GetRunningFetcher() { // All created TestURLFetchers have ID 0 by default. @@ -37,11 +54,53 @@ return url_fetcher; } - void RespondWithData(const std::string& data) { + void RespondToSubscriptionRequestSuccessfully() { + net::TestURLFetcher* url_fetcher = GetRunningFetcher(); + ASSERT_EQ(GURL(kSubscriptionUrl), url_fetcher->GetOriginalURL()); + RespondSuccessfully(); + } + + void RespondToUnsubscriptionRequestSuccessfully() { + net::TestURLFetcher* url_fetcher = GetRunningFetcher(); + ASSERT_EQ(GURL(kUnsubscriptionUrl), url_fetcher->GetOriginalURL()); + RespondSuccessfully(); + } + + void RespondToSubscriptionWithError(int error_code) { + net::TestURLFetcher* url_fetcher = GetRunningFetcher(); + ASSERT_EQ(GURL(kSubscriptionUrl), url_fetcher->GetOriginalURL()); + RespondWithError(error_code); + } + + void RespondToUnsubscriptionWithError(int error_code) { + net::TestURLFetcher* url_fetcher = GetRunningFetcher(); + ASSERT_EQ(GURL(kUnsubscriptionUrl), url_fetcher->GetOriginalURL()); + RespondWithError(error_code); + } + +#if !defined(OS_CHROMEOS) + void SignIn() { + utils_.fake_signin_manager()->SignIn(kTestEmail, "user", "pass"); + } + + void SignOut() { utils_.fake_signin_manager()->ForceSignOut(); } +#endif // !defined(OS_CHROMEOS) + + void IssueRefreshToken(FakeProfileOAuth2TokenService* auth_token_service) { + auth_token_service->GetDelegate()->UpdateCredentials(kTestEmail, "token"); + } + + void IssueAccessToken(FakeProfileOAuth2TokenService* auth_token_service) { + auth_token_service->IssueAllTokensForAccount(kTestEmail, "access_token", + base::Time::Max()); + } + + private: + void RespondSuccessfully() { net::TestURLFetcher* url_fetcher = GetRunningFetcher(); url_fetcher->set_status(net::URLRequestStatus()); url_fetcher->set_response_code(net::HTTP_OK); - url_fetcher->SetResponseString(data); + url_fetcher->SetResponseString(std::string()); // Call the URLFetcher delegate to continue the test. url_fetcher->delegate()->OnURLFetchComplete(url_fetcher); } @@ -54,63 +113,185 @@ url_fetcher->delegate()->OnURLFetchComplete(url_fetcher); } - const std::string url{"http://valid-url.test"}; - private: base::MessageLoop message_loop_; + test::RemoteSuggestionsTestUtils utils_; scoped_refptr<net::TestURLRequestContextGetter> request_context_getter_; net::TestURLFetcherFactory url_fetcher_factory_; - std::unique_ptr<TestingPrefServiceSimple> pref_service_; }; TEST_F(SubscriptionManagerTest, SubscribeSuccessfully) { - std::string token = "1234567890"; - SubscriptionManager manager(GetRequestContext(), GetPrefService(), GURL(url), - GURL(url)); - manager.Subscribe(token); - RespondWithData(""); - EXPECT_TRUE(manager.IsSubscribed()); - EXPECT_EQ(GetPrefService()->GetString( - ntp_snippets::prefs::kBreakingNewsSubscriptionDataToken), - token); + std::string subscription_token = "1234567890"; + SubscriptionManager manager(GetRequestContext(), GetPrefService(), + GetSigninManager(), GetOAuth2TokenService(), + GURL(kSubscriptionUrl), GURL(kUnsubscriptionUrl)); + manager.Subscribe(subscription_token); + RespondToSubscriptionRequestSuccessfully(); + ASSERT_TRUE(manager.IsSubscribed()); + EXPECT_EQ(subscription_token, GetPrefService()->GetString( + prefs::kBreakingNewsSubscriptionDataToken)); + EXPECT_FALSE(GetPrefService()->GetBoolean( + prefs::kBreakingNewsSubscriptionDataIsAuthenticated)); } -TEST_F(SubscriptionManagerTest, SubscribeWithErrors) { - std::string token = "1234567890"; - SubscriptionManager manager(GetRequestContext(), GetPrefService(), GURL(url), - GURL(url)); - manager.Subscribe(token); - RespondWithError(net::ERR_TIMED_OUT); +// This test is relevant only on non-ChromeOS platforms, as the flow being +// tested here is not possible on ChromeOS. +#if !defined(OS_CHROMEOS) +TEST_F(SubscriptionManagerTest, + ShouldSubscribeWithAuthenticationWhenAuthenticated) { + // Sign in. + FakeProfileOAuth2TokenService* auth_token_service = GetOAuth2TokenService(); + SignIn(); + IssueRefreshToken(auth_token_service); + + // Create manager and subscribe. + std::string subscription_token = "1234567890"; + SubscriptionManager manager(GetRequestContext(), GetPrefService(), + GetSigninManager(), auth_token_service, + GURL(kSubscriptionUrl), GURL(kUnsubscriptionUrl)); + manager.Subscribe(subscription_token); + + // Make sure that subscription is pending an access token. + ASSERT_FALSE(manager.IsSubscribed()); + ASSERT_EQ(1u, auth_token_service->GetPendingRequests().size()); + + // Issue the access token and respond to the subscription request. + IssueAccessToken(auth_token_service); + ASSERT_FALSE(manager.IsSubscribed()); + RespondToSubscriptionRequestSuccessfully(); + ASSERT_TRUE(manager.IsSubscribed()); + + // Check that we are now subscribed correctly with authentication. + EXPECT_EQ(subscription_token, GetPrefService()->GetString( + prefs::kBreakingNewsSubscriptionDataToken)); + EXPECT_TRUE(GetPrefService()->GetBoolean( + prefs::kBreakingNewsSubscriptionDataIsAuthenticated)); +} +#endif + +TEST_F(SubscriptionManagerTest, ShouldNotSubscribeIfError) { + std::string subscription_token = "1234567890"; + SubscriptionManager manager(GetRequestContext(), GetPrefService(), + GetSigninManager(), GetOAuth2TokenService(), + GURL(kSubscriptionUrl), GURL(kUnsubscriptionUrl)); + + manager.Subscribe(subscription_token); + RespondToSubscriptionWithError(net::ERR_TIMED_OUT); EXPECT_FALSE(manager.IsSubscribed()); - EXPECT_FALSE(GetPrefService()->HasPrefPath( - ntp_snippets::prefs::kBreakingNewsSubscriptionDataToken)); } TEST_F(SubscriptionManagerTest, UnsubscribeSuccessfully) { - std::string token = "1234567890"; - GetPrefService()->SetString( - ntp_snippets::prefs::kBreakingNewsSubscriptionDataToken, token); - SubscriptionManager manager(GetRequestContext(), GetPrefService(), GURL(url), - GURL(url)); - manager.Unsubscribe(token); - RespondWithData(""); + std::string subscription_token = "1234567890"; + SubscriptionManager manager(GetRequestContext(), GetPrefService(), + GetSigninManager(), GetOAuth2TokenService(), + GURL(kSubscriptionUrl), GURL(kUnsubscriptionUrl)); + manager.Subscribe(subscription_token); + RespondToSubscriptionRequestSuccessfully(); + ASSERT_TRUE(manager.IsSubscribed()); + manager.Unsubscribe(); + RespondToUnsubscriptionRequestSuccessfully(); EXPECT_FALSE(manager.IsSubscribed()); - EXPECT_FALSE(GetPrefService()->HasPrefPath( - ntp_snippets::prefs::kBreakingNewsSubscriptionDataToken)); + EXPECT_FALSE( + GetPrefService()->HasPrefPath(prefs::kBreakingNewsSubscriptionDataToken)); } -TEST_F(SubscriptionManagerTest, UnsubscribeWithErrors) { - std::string token = "1234567890"; - GetPrefService()->SetString( - ntp_snippets::prefs::kBreakingNewsSubscriptionDataToken, token); - SubscriptionManager manager(GetRequestContext(), GetPrefService(), GURL(url), - GURL(url)); - manager.Unsubscribe(token); - RespondWithError(net::ERR_TIMED_OUT); - EXPECT_TRUE(manager.IsSubscribed()); - EXPECT_EQ(GetPrefService()->GetString( - ntp_snippets::prefs::kBreakingNewsSubscriptionDataToken), - token); +TEST_F(SubscriptionManagerTest, + ShouldRemainSubscribedIfErrorDuringUnsubscribe) { + std::string subscription_token = "1234567890"; + SubscriptionManager manager(GetRequestContext(), GetPrefService(), + GetSigninManager(), GetOAuth2TokenService(), + GURL(kSubscriptionUrl), GURL(kUnsubscriptionUrl)); + manager.Subscribe(subscription_token); + RespondToSubscriptionRequestSuccessfully(); + ASSERT_TRUE(manager.IsSubscribed()); + manager.Unsubscribe(); + RespondToUnsubscriptionWithError(net::ERR_TIMED_OUT); + ASSERT_TRUE(manager.IsSubscribed()); + EXPECT_EQ(subscription_token, GetPrefService()->GetString( + prefs::kBreakingNewsSubscriptionDataToken)); +} + +// This test is relevant only on non-ChromeOS platforms, as the flow being +// tested here is not possible on ChromeOS. +#if !defined(OS_CHROMEOS) +TEST_F(SubscriptionManagerTest, ShouldResubscribeIfSignInAfterSubscription) { + // Create manager and subscribe. + FakeProfileOAuth2TokenService* auth_token_service = GetOAuth2TokenService(); + std::string subscription_token = "1234567890"; + SubscriptionManager manager(GetRequestContext(), GetPrefService(), + GetSigninManager(), auth_token_service, + GURL(kSubscriptionUrl), GURL(kUnsubscriptionUrl)); + manager.Subscribe(subscription_token); + RespondToSubscriptionRequestSuccessfully(); + ASSERT_FALSE(manager.NeedsToResubscribe()); + + // Sign in. This should trigger a resubscribe. + SignIn(); + IssueRefreshToken(auth_token_service); + ASSERT_TRUE(manager.NeedsToResubscribe()); + ASSERT_EQ(1u, auth_token_service->GetPendingRequests().size()); + IssueAccessToken(auth_token_service); + RespondToSubscriptionRequestSuccessfully(); + + // Check that we are now subscribed with authentication. + EXPECT_TRUE(GetPrefService()->GetBoolean( + prefs::kBreakingNewsSubscriptionDataIsAuthenticated)); +} +#endif + +// This test is relevant only on non-ChromeOS platforms, as the flow being +// tested here is not possible on ChromeOS. +#if !defined(OS_CHROMEOS) +TEST_F(SubscriptionManagerTest, ShouldResubscribeIfSignOutAfterSubscription) { + // Signin and subscribe. + FakeProfileOAuth2TokenService* auth_token_service = GetOAuth2TokenService(); + SignIn(); + IssueRefreshToken(auth_token_service); + std::string subscription_token = "1234567890"; + SubscriptionManager manager(GetRequestContext(), GetPrefService(), + GetSigninManager(), auth_token_service, + GURL(kSubscriptionUrl), GURL(kUnsubscriptionUrl)); + manager.Subscribe(subscription_token); + ASSERT_EQ(1u, auth_token_service->GetPendingRequests().size()); + IssueAccessToken(auth_token_service); + RespondToSubscriptionRequestSuccessfully(); + + // Signout, this should trigger a resubscribe. + SignOut(); + EXPECT_TRUE(manager.NeedsToResubscribe()); + RespondToSubscriptionRequestSuccessfully(); + + // Check that we are now subscribed without authentication. + EXPECT_FALSE(GetPrefService()->GetBoolean( + prefs::kBreakingNewsSubscriptionDataIsAuthenticated)); +} +#endif + +TEST_F(SubscriptionManagerTest, + ShouldUpdateTokenInPrefWhenResubscribeWithChangeInToken) { + // Create manager and subscribe. + std::string old_subscription_token = "1234567890"; + SubscriptionManager manager(GetRequestContext(), GetPrefService(), + GetSigninManager(), GetOAuth2TokenService(), + GURL(kSubscriptionUrl), GURL(kUnsubscriptionUrl)); + manager.Subscribe(old_subscription_token); + RespondToSubscriptionRequestSuccessfully(); + EXPECT_EQ( + old_subscription_token, + GetPrefService()->GetString(prefs::kBreakingNewsSubscriptionDataToken)); + + // Resubscribe with a new token. + std::string new_subscription_token = "0987654321"; + manager.Resubscribe(new_subscription_token); + // Resubscribe with a new token should issue an unsubscribe request before + // subscribing. + RespondToUnsubscriptionRequestSuccessfully(); + RespondToSubscriptionRequestSuccessfully(); + + // Check we are now subscribed with the new token. + EXPECT_EQ( + new_subscription_token, + GetPrefService()->GetString(prefs::kBreakingNewsSubscriptionDataToken)); } } // namespace ntp_snippets
diff --git a/components/ntp_snippets/ntp_snippets_constants.cc b/components/ntp_snippets/ntp_snippets_constants.cc index 6fd0a20..52abe08 100644 --- a/components/ntp_snippets/ntp_snippets_constants.cc +++ b/components/ntp_snippets/ntp_snippets_constants.cc
@@ -12,6 +12,9 @@ const base::FilePath::CharType kBreakingNewsDatabaseFolder[] = FILE_PATH_LITERAL("NTPBreakingNews"); +const char kContentSuggestionsApiScope[] = + "https://www.googleapis.com/auth/chrome-content-suggestions"; + const char kContentSuggestionsServer[] = "https://chromecontentsuggestions-pa.googleapis.com/v1/suggestions/fetch"; const char kContentSuggestionsStagingServer[] =
diff --git a/components/ntp_snippets/ntp_snippets_constants.h b/components/ntp_snippets/ntp_snippets_constants.h index 1ce410c8..f7ddd58a 100644 --- a/components/ntp_snippets/ntp_snippets_constants.h +++ b/components/ntp_snippets/ntp_snippets_constants.h
@@ -16,6 +16,9 @@ // TODO(mamir): Check if the same DB can be used. extern const base::FilePath::CharType kBreakingNewsDatabaseFolder[]; +// OAuth access token scope. +extern const char kContentSuggestionsApiScope[]; + // Server endpoints for fetching snippets. extern const char kContentSuggestionsServer[]; // used on stable/beta extern const char kContentSuggestionsStagingServer[]; // used on dev/canary
diff --git a/components/ntp_snippets/pref_names.cc b/components/ntp_snippets/pref_names.cc index 1583914..72851b0 100644 --- a/components/ntp_snippets/pref_names.cc +++ b/components/ntp_snippets/pref_names.cc
@@ -84,6 +84,9 @@ const char kBreakingNewsSubscriptionDataToken[] = "ntp_suggestions.breaking_news_subscription_data.token"; +const char kBreakingNewsSubscriptionDataIsAuthenticated[] = + "ntp_suggestions.breaking_news_subscription_data.is_authenticated"; + const char kBreakingNewsGCMSubscriptionTokenCache[] = "ntp_suggestions.breaking_news_gcm_subscription_token_cache";
diff --git a/components/ntp_snippets/pref_names.h b/components/ntp_snippets/pref_names.h index 75090cc..110aafe 100644 --- a/components/ntp_snippets/pref_names.h +++ b/components/ntp_snippets/pref_names.h
@@ -93,10 +93,13 @@ // The folllowing prefs hold the data used when subscribing for content // suggestions via GCM push updates. They are stored in pref such that in case // of change (e.g. the token renders invalid), re-subscription is required. +// They are stored in prefs for persisting them across Chrome restarts. /////////////////////////////////////////////////////////////////////////////// // The pref name for the subscription token used when subscription for // breaking news push updates. extern const char kBreakingNewsSubscriptionDataToken[]; +// The pref name for whether the subscription is authenticated or not. +extern const char kBreakingNewsSubscriptionDataIsAuthenticated[]; //////////////////////// End of breaking news subscription-related prefs. // The pref name for the subscription token received from the gcm server. As
diff --git a/components/ntp_snippets/remote/remote_suggestions_fetcher_impl.cc b/components/ntp_snippets/remote/remote_suggestions_fetcher_impl.cc index cf3fdb7b..5817f94 100644 --- a/components/ntp_snippets/remote/remote_suggestions_fetcher_impl.cc +++ b/components/ntp_snippets/remote/remote_suggestions_fetcher_impl.cc
@@ -13,6 +13,7 @@ #include "base/values.h" #include "components/language/core/browser/url_language_histogram.h" #include "components/ntp_snippets/category.h" +#include "components/ntp_snippets/ntp_snippets_constants.h" #include "components/ntp_snippets/user_classifier.h" #include "components/signin/core/browser/access_token_fetcher.h" #include "components/signin/core/browser/signin_manager_base.h" @@ -32,8 +33,6 @@ namespace { -const char kContentSuggestionsApiScope[] = - "https://www.googleapis.com/auth/chrome-content-suggestions"; const char kSnippetsServerNonAuthorizedFormat[] = "%s?key=%s"; const char kAuthorizationRequestHeaderFormat[] = "Bearer %s";
diff --git a/components/ntp_snippets/remote/remote_suggestions_fetcher_impl_unittest.cc b/components/ntp_snippets/remote/remote_suggestions_fetcher_impl_unittest.cc index 6fb19b3..e4c2e2eb 100644 --- a/components/ntp_snippets/remote/remote_suggestions_fetcher_impl_unittest.cc +++ b/components/ntp_snippets/remote/remote_suggestions_fetcher_impl_unittest.cc
@@ -18,6 +18,7 @@ #include "base/time/default_clock.h" #include "base/time/time.h" #include "base/values.h" +#include "build/build_config.h" #include "components/ntp_snippets/category.h" #include "components/ntp_snippets/features.h" #include "components/ntp_snippets/ntp_snippets_constants.h" @@ -298,7 +299,13 @@ fetcher_->SetClockForTesting(mock_task_runner_->GetMockClock()); } - void SignIn() { utils_.fake_signin_manager()->SignIn(kTestEmail); } + void SignIn() { +#if defined(OS_CHROMEOS) + utils_.fake_signin_manager()->SignIn(kTestEmail); +#else + utils_.fake_signin_manager()->SignIn(kTestEmail, "user", "password"); +#endif + } void IssueRefreshToken() { fake_token_service_->GetDelegate()->UpdateCredentials(kTestEmail, "token");
diff --git a/components/ntp_snippets/remote/remote_suggestions_scheduler_impl_unittest.cc b/components/ntp_snippets/remote/remote_suggestions_scheduler_impl_unittest.cc index eed873c..85678494 100644 --- a/components/ntp_snippets/remote/remote_suggestions_scheduler_impl_unittest.cc +++ b/components/ntp_snippets/remote/remote_suggestions_scheduler_impl_unittest.cc
@@ -29,8 +29,8 @@ #include "components/ntp_snippets/remote/test_utils.h" #include "components/ntp_snippets/status.h" #include "components/ntp_snippets/user_classifier.h" -#include "components/prefs/pref_registry_simple.h" #include "components/prefs/testing_pref_service.h" +#include "components/sync_preferences/testing_pref_service_syncable.h" #include "components/variations/variations_params_manager.h" #include "components/web_resource/web_resource_pref_names.h" #include "net/base/network_change_notifier.h"
diff --git a/components/ntp_snippets/remote/remote_suggestions_status_service_unittest.cc b/components/ntp_snippets/remote/remote_suggestions_status_service_unittest.cc index dcfc9b8..65ca301 100644 --- a/components/ntp_snippets/remote/remote_suggestions_status_service_unittest.cc +++ b/components/ntp_snippets/remote/remote_suggestions_status_service_unittest.cc
@@ -7,16 +7,17 @@ #include <memory> #include "base/memory/ptr_util.h" +#include "build/build_config.h" #include "components/ntp_snippets/features.h" #include "components/ntp_snippets/ntp_snippets_constants.h" #include "components/ntp_snippets/pref_names.h" #include "components/ntp_snippets/remote/test_utils.h" -#include "components/prefs/pref_registry_simple.h" #include "components/prefs/testing_pref_service.h" #include "components/signin/core/browser/account_tracker_service.h" #include "components/signin/core/browser/fake_signin_manager.h" #include "components/signin/core/browser/test_signin_client.h" #include "components/signin/core/common/signin_pref_names.h" +#include "components/sync_preferences/testing_pref_service_syncable.h" #include "components/variations/variations_params_manager.h" #include "testing/gmock/include/gmock/gmock.h" #include "testing/gtest/include/gtest/gtest.h" @@ -48,7 +49,11 @@ service->GetStatusFromDeps()); // One can still sign in. +#if defined(OS_CHROMEOS) utils_.fake_signin_manager()->SignIn("foo@bar.com"); +#else + utils_.fake_signin_manager()->SignIn("foo@bar.com", "user", "pass"); +#endif EXPECT_EQ(RemoteSuggestionsStatus::ENABLED_AND_SIGNED_IN, service->GetStatusFromDeps()); } @@ -66,7 +71,11 @@ service->GetStatusFromDeps()); // The other dependencies shouldn't matter anymore. +#if defined(OS_CHROMEOS) utils_.fake_signin_manager()->SignIn("foo@bar.com"); +#else + utils_.fake_signin_manager()->SignIn("foo@bar.com", "user", "pass"); +#endif EXPECT_EQ(RemoteSuggestionsStatus::EXPLICITLY_DISABLED, service->GetStatusFromDeps()); }
diff --git a/components/ntp_snippets/remote/test_utils.cc b/components/ntp_snippets/remote/test_utils.cc index eb63bc4..1880933 100644 --- a/components/ntp_snippets/remote/test_utils.cc +++ b/components/ntp_snippets/remote/test_utils.cc
@@ -7,10 +7,9 @@ #include <memory> #include "base/memory/ptr_util.h" -#include "components/prefs/pref_registry_simple.h" #include "components/prefs/testing_pref_service.h" #include "components/signin/core/browser/account_tracker_service.h" -#include "components/signin/core/browser/fake_signin_manager.h" +#include "components/signin/core/browser/fake_profile_oauth2_token_service.h" #include "components/signin/core/browser/test_signin_client.h" #include "components/signin/core/common/signin_pref_names.h" #include "components/sync/driver/fake_sync_service.h" @@ -48,24 +47,37 @@ } RemoteSuggestionsTestUtils::RemoteSuggestionsTestUtils() - : pref_service_(base::MakeUnique<TestingPrefServiceSimple>()) { - pref_service_->registry()->RegisterStringPref(prefs::kGoogleServicesAccountId, - std::string()); - pref_service_->registry()->RegisterStringPref( - prefs::kGoogleServicesLastAccountId, std::string()); - pref_service_->registry()->RegisterStringPref( - prefs::kGoogleServicesLastUsername, std::string()); + : pref_service_(base::MakeUnique<TestingPrefServiceSyncable>()) { + AccountTrackerService::RegisterPrefs(pref_service_->registry()); + +#if defined(OS_CHROMEOS) + SigninManagerBase::RegisterProfilePrefs(pref_service_->registry()); + SigninManagerBase::RegisterPrefs(pref_service_->registry()); +#else + SigninManager::RegisterProfilePrefs(pref_service_->registry()); + SigninManager::RegisterPrefs(pref_service_->registry()); +#endif // OS_CHROMEOS + + token_service_ = base::MakeUnique<FakeProfileOAuth2TokenService>(); signin_client_ = base::MakeUnique<TestSigninClient>(pref_service_.get()); account_tracker_ = base::MakeUnique<AccountTrackerService>(); + account_tracker_->Initialize(signin_client_.get()); fake_sync_service_ = base::MakeUnique<FakeSyncService>(); + ResetSigninManager(); } RemoteSuggestionsTestUtils::~RemoteSuggestionsTestUtils() = default; void RemoteSuggestionsTestUtils::ResetSigninManager() { +#if defined(OS_CHROMEOS) fake_signin_manager_ = base::MakeUnique<FakeSigninManagerBase>( signin_client_.get(), account_tracker_.get()); +#else + fake_signin_manager_ = base::MakeUnique<FakeSigninManager>( + signin_client_.get(), token_service_.get(), account_tracker_.get(), + /*cookie_manager_service=*/nullptr); +#endif } } // namespace test
diff --git a/components/ntp_snippets/remote/test_utils.h b/components/ntp_snippets/remote/test_utils.h index d7cdfa1..ed825a6 100644 --- a/components/ntp_snippets/remote/test_utils.h +++ b/components/ntp_snippets/remote/test_utils.h
@@ -7,14 +7,25 @@ #include <memory> +#include "build/build_config.h" +#include "components/signin/core/browser/fake_signin_manager.h" #include "components/sync/driver/fake_sync_service.h" +#include "components/sync_preferences/testing_pref_service_syncable.h" #include "testing/gtest/include/gtest/gtest.h" class AccountTrackerService; -class FakeSigninManagerBase; -class TestingPrefServiceSimple; +class FakeProfileOAuth2TokenService; class TestSigninClient; +using sync_preferences::TestingPrefServiceSyncable; + +#if defined(OS_CHROMEOS) +// ChromeOS doesn't have SigninManager. +using SigninManagerForTest = FakeSigninManagerBase; +#else +using SigninManagerForTest = FakeSigninManager; +#endif // OS_CHROMEOS + namespace ntp_snippets { namespace test { @@ -46,15 +57,19 @@ void ResetSigninManager(); FakeSyncService* fake_sync_service() { return fake_sync_service_.get(); } - FakeSigninManagerBase* fake_signin_manager() { + SigninManagerForTest* fake_signin_manager() { return fake_signin_manager_.get(); } - TestingPrefServiceSimple* pref_service() { return pref_service_.get(); } + TestingPrefServiceSyncable* pref_service() { return pref_service_.get(); } + FakeProfileOAuth2TokenService* token_service() { + return token_service_.get(); + } private: - std::unique_ptr<FakeSigninManagerBase> fake_signin_manager_; + std::unique_ptr<SigninManagerForTest> fake_signin_manager_; std::unique_ptr<FakeSyncService> fake_sync_service_; - std::unique_ptr<TestingPrefServiceSimple> pref_service_; + std::unique_ptr<TestingPrefServiceSyncable> pref_service_; + std::unique_ptr<FakeProfileOAuth2TokenService> token_service_; std::unique_ptr<TestSigninClient> signin_client_; std::unique_ptr<AccountTrackerService> account_tracker_; };
diff --git a/components/page_info_strings.grdp b/components/page_info_strings.grdp index 9a35a8b..59764dd 100644 --- a/components/page_info_strings.grdp +++ b/components/page_info_strings.grdp
@@ -8,7 +8,7 @@ <message name="IDS_PAGE_INFO_MIXED_CONTENT_SUMMARY" desc="A one-line summary at the top of the Page Info bubble (which shows when you click the security indicator) if the connection to the current website is using mainly using a secure connection but has some insecure parts (like insecurely loaded images)."> Your connection to this site is not fully secure </message> - <message name="IDS_PAGE_INFO_NOT_SECURE_SUMMARY" desc="A one-line summary at the top of the Page Info bubble (which shows when you click the security indicator) if the connection to the current website is secure."> + <message name="IDS_PAGE_INFO_NOT_SECURE_SUMMARY" desc="A one-line summary at the top of the Page Info bubble (which shows when you click the security indicator) if the connection to the current website is not secure."> Your connection to this site is not secure </message> <message name="IDS_PAGE_INFO_MALWARE_SUMMARY" desc="A one-line summary at the top of the Page Info bubble (which shows when you click the security indicator) if the current website has been flagged as containing malware.">
diff --git a/content/browser/resources/media/client_renderer.js b/content/browser/resources/media/client_renderer.js index 5e85499..acc8ef2 100644 --- a/content/browser/resources/media/client_renderer.js +++ b/content/browser/resources/media/client_renderer.js
@@ -525,6 +525,7 @@ if (this.selectedPlayer) { removeChildren(this.logTable); this.selectedPlayerLogIndex = 0; + this.drawLog_(); } }, };
diff --git a/content/browser/resources/media/media_internals.css b/content/browser/resources/media/media_internals.css index 72afd8c..ba0997c 100644 --- a/content/browser/resources/media/media_internals.css +++ b/content/browser/resources/media/media_internals.css
@@ -13,7 +13,7 @@ } tabbox { - margin-top: 10px; + padding-top: 10px; } tab {
diff --git a/ios/chrome/browser/ui/bookmarks/bookmark_home_handset_view_controller.h b/ios/chrome/browser/ui/bookmarks/bookmark_home_handset_view_controller.h index 2d09e87..e4eea61 100644 --- a/ios/chrome/browser/ui/bookmarks/bookmark_home_handset_view_controller.h +++ b/ios/chrome/browser/ui/bookmarks/bookmark_home_handset_view_controller.h
@@ -30,41 +30,13 @@ @end // Navigate/edit the bookmark hierarchy on a handset. -@interface BookmarkHomeHandsetViewController : BookmarkHomeViewController { - @protected - // The following 2 ivars both represent the set of nodes being edited. - // The set is for fast lookup. - // The vector maintains the order that edit nodes were added. - // Use the relevant instance methods to modify these two ivars in tandem. - // DO NOT modify these two ivars directly. - std::set<const bookmarks::BookmarkNode*> _editNodes; - std::vector<const bookmarks::BookmarkNode*> _editNodesOrdered; -} +@interface BookmarkHomeHandsetViewController : BookmarkHomeViewController #pragma mark - Properties Relevant To Presenters @property(nonatomic, weak) id<BookmarkHomeHandsetViewControllerDelegate> delegate; -#pragma mark - Properties -// Whether the view controller is in editing mode. -@property(nonatomic, assign, readonly) BOOL editing; -// The set of selected index paths for edition. -@property(nonatomic, strong, readonly) NSMutableArray* editIndexPaths; - -#pragma mark - Relevant Methods -// Replaces |_editNodes| and |_editNodesOrdered| with new container objects. -- (void)resetEditNodes; -// Adds |node| and |indexPath| if it isn't already present. -- (void)insertEditNode:(const bookmarks::BookmarkNode*)node - atIndexPath:(NSIndexPath*)indexPath; -// Removes |node| and |indexPath| if it's present. -- (void)removeEditNode:(const bookmarks::BookmarkNode*)node - atIndexPath:(NSIndexPath*)indexPath; - -// This method updates the property, and resets the edit nodes. -- (void)setEditing:(BOOL)editing animated:(BOOL)animated; - // Dismisses any modal interaction elements. The base implementation does // nothing. - (void)dismissModals:(BOOL)animated;
diff --git a/ios/chrome/browser/ui/bookmarks/bookmark_home_handset_view_controller.mm b/ios/chrome/browser/ui/bookmarks/bookmark_home_handset_view_controller.mm index 18f60cb..da18e55 100644 --- a/ios/chrome/browser/ui/bookmarks/bookmark_home_handset_view_controller.mm +++ b/ios/chrome/browser/ui/bookmarks/bookmark_home_handset_view_controller.mm
@@ -54,10 +54,6 @@ std::unique_ptr<bookmarks::BookmarkModelBridge> _bridge; } -// This views holds the primary content of this view controller. -// Redefined to be readwrite. -@property(nonatomic, strong, readwrite) NSMutableArray* editIndexPaths; - // Returns the parent, if all the bookmarks are siblings. // Otherwise returns the mobile_node. + (const BookmarkNode*) @@ -72,10 +68,6 @@ // property is set to nil after it is used. @property(nonatomic, strong) NSNumber* cachedContentPosition; -// The layout code in this class relies on the assumption that the editingBar -// has the same frame as the navigationBar. -@property(nonatomic, strong) BookmarkEditingBar* editingBar; - // The action sheet coordinator used when trying to edit a single bookmark. @property(nonatomic, strong) ActionSheetCoordinator* actionSheetCoordinator; @@ -88,42 +80,6 @@ // Updates the UI to reflect the new state of 'primaryMenuItem'. - (void)updatePrimaryMenuItem:(BookmarkMenuItem*)menuItem animated:(BOOL)animated; - -#pragma mark Editing related methods -// This method statelessly updates the editing top bar from |_editNodes| and -// |editing|. -- (void)updateEditingStateAnimated:(BOOL)animated; -// Shows or hides the editing bar. -- (void)showEditingBarAnimated:(BOOL)animated; -- (void)hideEditingBarAnimated:(BOOL)animated; -// Instaneously updates the shadow of the edit bar. -// This method should be called anytime: -// (1)|editing| property changes. -// (2)The primary view changes. -// (3)The primary view's collection view is scrolled. -// (2) is not necessary right now, as it is only possible to switch primary -// views when |editing| is NO. When |editing| is NO, the shadow is never shown. -- (void)updateEditBarShadow; - -#pragma mark Editing bar callbacks -// The cancel button was tapped on the editing bar. -- (void)editingBarCancel; -// The move button was tapped on the editing bar. -- (void)editingBarMove; -// The delete button was tapped on the editing bar. -- (void)editingBarDelete; -// The edit button was tapped on the editing bar. -- (void)editingBarEdit; - -#pragma mark Action sheet callbacks -// Enters into edit mode by selecting the given node and cell. -- (void)selectFirstNode:(const BookmarkNode*)node - withCell:(UICollectionViewCell*)cell; - -#pragma mark private utility methods -// Deletes the nodes, and presents a toast with an undo button. -- (void)deleteSelectedNodes; - #pragma mark Navigation bar // Updates the UI of the navigation bar with the primaryMenuItem. // This method should be called anytime: @@ -162,85 +118,26 @@ // Saves the current position and asks the delegate to open the url. - (void)delegateDismiss:(const GURL&)url; -- (NSIndexPath*)indexPathForCell:(UICollectionViewCell*)cell; - @end @implementation BookmarkHomeHandsetViewController @synthesize contentView = _contentView; @synthesize cachedContentPosition = _cachedContentPosition; -@synthesize editingBar = _editingBar; @synthesize actionSheetCoordinator = _actionSheetCoordinator; @synthesize delegate = _delegate; -@synthesize editIndexPaths = _editIndexPaths; -@synthesize editing = _editing; - (instancetype)initWithLoader:(id<UrlLoader>)loader browserState:(ios::ChromeBrowserState*)browserState { self = [super initWithLoader:loader browserState:browserState]; if (self) { - _editIndexPaths = [[NSMutableArray alloc] init]; - - [self resetEditNodes]; - _bridge.reset(new bookmarks::BookmarkModelBridge(self, self.bookmarks)); + self.sideSwipingPossible = YES; } return self; } -- (void)resetEditNodes { - _editNodes = std::set<const BookmarkNode*>(); - _editNodesOrdered = std::vector<const BookmarkNode*>(); - [self.editIndexPaths removeAllObjects]; -} - -- (void)insertEditNode:(const BookmarkNode*)node - atIndexPath:(NSIndexPath*)indexPath { - if (_editNodes.find(node) != _editNodes.end()) - return; - _editNodes.insert(node); - _editNodesOrdered.push_back(node); - if (indexPath) { - [self.editIndexPaths addObject:indexPath]; - } else { - // We insert null if we don't have the cell to keep the index valid. - [self.editIndexPaths addObject:[NSNull null]]; - } -} - -- (void)removeEditNode:(const BookmarkNode*)node - atIndexPath:(NSIndexPath*)indexPath { - if (_editNodes.find(node) == _editNodes.end()) - return; - - _editNodes.erase(node); - std::vector<const BookmarkNode*>::iterator it = - std::find(_editNodesOrdered.begin(), _editNodesOrdered.end(), node); - DCHECK(it != _editNodesOrdered.end()); - _editNodesOrdered.erase(it); - if (indexPath) { - [self.editIndexPaths removeObject:indexPath]; - } else { - // If we don't have the cell, we remove it by using its index. - const NSUInteger index = std::distance(_editNodesOrdered.begin(), it); - if (index < self.editIndexPaths.count) { - [self.editIndexPaths removeObjectAtIndex:index]; - } - } -} - -- (void)removeEditNode:(const BookmarkNode*)node - cell:(UICollectionViewCell*)cell { - [self removeEditNode:node atIndexPath:[self indexPathForCell:cell]]; - - if (_editNodes.size() == 0) - [self setEditing:NO animated:YES]; - else - [self updateEditingStateAnimated:YES]; -} - #pragma mark - UIViewController methods - (void)viewDidLoad { @@ -338,63 +235,10 @@ orientation:GetInterfaceOrientation()]; } -#pragma mark - Editing bar methods. +#pragma mark - Editing bar super methods overrides -- (void)updateEditingStateAnimated:(BOOL)animated { - if (!self.editing) { - [self hideEditingBarAnimated:animated]; - [self updateEditBarShadow]; - [self.panelView enableSideSwiping:YES]; - return; - } - - if (!self.editingBar) { - self.editingBar = - [[BookmarkEditingBar alloc] initWithFrame:self.navigationBar.frame]; - [self.editingBar setCancelTarget:self action:@selector(editingBarCancel)]; - [self.editingBar setDeleteTarget:self action:@selector(editingBarDelete)]; - [self.editingBar setMoveTarget:self action:@selector(editingBarMove)]; - [self.editingBar setEditTarget:self action:@selector(editingBarEdit)]; - - [self.view addSubview:self.editingBar]; - self.editingBar.alpha = 0; - } - - int bookmarkCount = 0; - int folderCount = 0; - for (auto* node : _editNodes) { - if (node->is_url()) - ++bookmarkCount; - else - ++folderCount; - } - [self.editingBar updateUIWithBookmarkCount:bookmarkCount - folderCount:folderCount]; - - [self showEditingBarAnimated:animated]; - [self updateEditBarShadow]; - [self.panelView enableSideSwiping:NO]; -} - -- (void)setEditing:(BOOL)editing animated:(BOOL)animated { - if (_editing == editing) - return; - - _editing = editing; - - // Only reset the editing state when leaving edit mode. This allows subclasses - // to add nodes for editing before entering edit mode. - if (!editing) - [self resetEditNodes]; - - [self updateEditingStateAnimated:animated]; - if ([[self primaryMenuItem] supportsEditing]) - [[self primaryView] setEditing:editing animated:animated]; - - if (editing) - self.bookmarkPromoController.promoState = NO; - else - [self.bookmarkPromoController updatePromoState]; +- (CGRect)editingBarFrame { + return self.navigationBar.frame; } - (void)showEditingBarAnimated:(BOOL)animated { @@ -443,31 +287,6 @@ }]; } -- (void)updateEditBarShadow { - [self.editingBar showShadow:self.editing]; -} - -#pragma mark Editing Bar Callbacks - -- (void)editingBarCancel { - [self setEditing:NO animated:YES]; -} - -- (void)editingBarMove { - [self moveNodes:_editNodes]; -} - -- (void)editingBarDelete { - [self deleteSelectedNodes]; - [self setEditing:NO animated:YES]; -} - -- (void)editingBarEdit { - DCHECK_EQ(_editNodes.size(), 1u); - const BookmarkNode* node = *(_editNodes.begin()); - [self editNode:node]; -} - #pragma mark - BookmarkMenuViewDelegate - (void)bookmarkMenuView:(BookmarkMenuView*)view @@ -599,15 +418,6 @@ [self.bookmarkPromoController hidePromoCell]; } -#pragma mark Action Sheet Callbacks - -- (void)selectFirstNode:(const BookmarkNode*)node - withCell:(UICollectionViewCell*)cell { - DCHECK(!self.editing); - [self insertEditNode:node atIndexPath:[self indexPathForCell:cell]]; - [self setEditing:YES animated:YES]; -} - #pragma mark - BookmarkCollectionViewDelegate - (void)bookmarkCollectionView:(BookmarkCollectionView*)view @@ -624,12 +434,6 @@ [self updatePrimaryMenuItem:menuItem animated:YES]; } -#pragma mark - Internal Utility Methods - -- (void)deleteSelectedNodes { - [self deleteNodes:_editNodes]; -} - #pragma mark - Navigation bar - (CGRect)navigationBarFrame { @@ -895,13 +699,6 @@ navigationToUrl:url]; } -- (NSIndexPath*)indexPathForCell:(UICollectionViewCell*)cell { - DCHECK([self primaryView].collectionView); - NSIndexPath* indexPath = - [[self primaryView].collectionView indexPathForCell:cell]; - return indexPath; -} - + (const BookmarkNode*) defaultMoveFolderFromBookmarks:(const std::set<const BookmarkNode*>&)bookmarks model:(bookmarks::BookmarkModel*)model {
diff --git a/ios/chrome/browser/ui/bookmarks/bookmark_home_handset_view_controller_unittest.mm b/ios/chrome/browser/ui/bookmarks/bookmark_home_handset_view_controller_unittest.mm index 0eb1c2d..b213cef 100644 --- a/ios/chrome/browser/ui/bookmarks/bookmark_home_handset_view_controller_unittest.mm +++ b/ios/chrome/browser/ui/bookmarks/bookmark_home_handset_view_controller_unittest.mm
@@ -5,6 +5,7 @@ #import "ios/chrome/browser/ui/bookmarks/bookmark_home_handset_view_controller.h" #include "components/bookmarks/browser/bookmark_model.h" #import "ios/chrome/browser/browser_state/test_chrome_browser_state.h" +#import "ios/chrome/browser/ui/bookmarks/bookmark_home_view_controller_protected.h" #include "ios/chrome/browser/ui/bookmarks/bookmark_ios_unittest.h" #import "ios/chrome/browser/ui/bookmarks/bookmark_promo_controller.h" #import "ios/chrome/browser/ui/bookmarks/bookmark_utils_ios.h"
diff --git a/ios/chrome/browser/ui/bookmarks/bookmark_home_tablet_ntp_controller.mm b/ios/chrome/browser/ui/bookmarks/bookmark_home_tablet_ntp_controller.mm index 533330c1..dbb3f03 100644 --- a/ios/chrome/browser/ui/bookmarks/bookmark_home_tablet_ntp_controller.mm +++ b/ios/chrome/browser/ui/bookmarks/bookmark_home_tablet_ntp_controller.mm
@@ -61,44 +61,15 @@ BookmarkModelBridgeObserver> { // Bridge to register for bookmark changes. std::unique_ptr<bookmarks::BookmarkModelBridge> _bridge; - - // The following 2 ivars both represent the set of nodes being edited. - // The set is for fast lookup. - // The vector maintains the order that edit nodes were added. - // Use the relevant instance methods to modify these two ivars in tandem. - // DO NOT modify these two ivars directly. - std::set<const BookmarkNode*> _editNodes; - std::vector<const BookmarkNode*> _editNodesOrdered; } #pragma mark - Properties and methods akin to BookmarkHomeHandsetViewController - -// Whether the view controller is in editing mode. -@property(nonatomic, assign) BOOL editing; -// The set of edited index paths. -@property(nonatomic, strong) NSMutableArray* editIndexPaths; - -// Replaces |_editNodes| and |_editNodesOrdered| with new container objects. -- (void)resetEditNodes; -// Adds |node| corresponding to a |cell| if it isn't already present. -- (void)insertEditNode:(const BookmarkNode*)node - atIndexPath:(NSIndexPath*)indexPath; -// Removes |node| corresponding to a |cell| if it's present. -- (void)removeEditNode:(const BookmarkNode*)node - atIndexPath:(NSIndexPath*)indexPath; -// This method updates the property, and resets the edit nodes. -- (void)setEditing:(BOOL)editing animated:(BOOL)animated; - -#pragma mark - Properties and methods akin to BookmarkHomeHandsetViewController // When the view is first shown on the screen, this property represents the // cached value of the y of the content offset of the primary view. This // property is set to nil after it is used. @property(nonatomic, strong) NSNumber* cachedContentPosition; // FIXME: INACTIVE -// The editing bar present when items are selected. -@property(nonatomic, strong) BookmarkEditingBar* editingBar; - // The action sheet coordinator used when trying to edit a single bookmark. @property(nonatomic, strong) ActionSheetCoordinator* actionSheetCoordinator; @@ -127,43 +98,9 @@ // Returns the frame of the primary view. - (CGRect)frameForPrimaryView; -#pragma mark Editing related methods -// This method statelessly updates the editing top bar from |_editNodes| and -// |editing|. -- (void)updateEditingStateAnimated:(BOOL)animated; -// Shows or hides the editing bar. -- (void)showEditingBarAnimated:(BOOL)animated; -- (void)hideEditingBarAnimated:(BOOL)animated; -// Instaneously updates the shadow of the edit bar. -// This method should be called anytime: -// (1)|editing| property changes. -// (2)The primary view changes. -// (3)The primary view's collection view is scrolled. -// When |editing| is NO, the shadow is never shown. -- (void)updateEditBarShadow; - -#pragma mark Editing bar callbacks -// The cancel button was tapped on the editing bar. -- (void)editingBarCancel; -// The move button was tapped on the editing bar. -- (void)editingBarMove; -// The delete button was tapped on the editing bar. -- (void)editingBarDelete; -// The edit button was tapped on the editing bar. -- (void)editingBarEdit; // The menu button is pressed on the editing bar. - (void)toggleMenuAnimated; -#pragma mark Action sheet callbacks -// Enters into edit mode by selecting the given node corresponding to the -// given cell. -- (void)selectFirstNode:(const BookmarkNode*)node - withCell:(UICollectionViewCell*)cell; - -#pragma mark private utility methods -// Deletes the nodes, and presents a toast with an undo button. -- (void)deleteSelectedNodes; - #pragma mark Navigation bar // Updates the UI of the navigation bar with the primaryMenuItem. // This method should be called anytime: @@ -182,18 +119,11 @@ // Called when the back button is pressed on the navigation bar. - (void)navigationBarBack:(id)sender; -// TODO(crbug.com/450646): This should not be needed but require refactoring of -// the BookmarkCollectionViewDelegate. -- (NSIndexPath*)indexPathForCell:(UICollectionViewCell*)cell; - @end @implementation BookmarkHomeTabletNTPController -@synthesize editing = _editing; -@synthesize editIndexPaths = _editIndexPaths; @synthesize cachedContentPosition = _cachedContentPosition; -@synthesize editingBar = _editingBar; @synthesize actionSheetCoordinator = _actionSheetCoordinator; @@ -205,7 +135,6 @@ self = [super initWithLoader:loader browserState:browserState]; if (self) { _bridge.reset(new bookmarks::BookmarkModelBridge(self, self.bookmarks)); - _editIndexPaths = [[NSMutableArray alloc] init]; } return self; } @@ -248,53 +177,6 @@ [self updateMenuViewLayout]; } -#pragma mark HomeViewController simili methods. - -- (void)resetEditNodes { - _editNodes = std::set<const BookmarkNode*>(); - _editNodesOrdered = std::vector<const BookmarkNode*>(); - [self.editIndexPaths removeAllObjects]; -} - -- (void)insertEditNode:(const BookmarkNode*)node - atIndexPath:(NSIndexPath*)indexPath { - if (_editNodes.find(node) != _editNodes.end()) - return; - _editNodes.insert(node); - _editNodesOrdered.push_back(node); - if (indexPath) { - [self.editIndexPaths addObject:indexPath]; - } else { - // Insert null to keep the index valid. - [self.editIndexPaths addObject:[NSNull null]]; - } -} - -- (void)removeEditNode:(const BookmarkNode*)node - atIndexPath:(NSIndexPath*)indexPath { - if (_editNodes.find(node) == _editNodes.end()) - return; - _editNodes.erase(node); - std::vector<const BookmarkNode*>::iterator it = - std::find(_editNodesOrdered.begin(), _editNodesOrdered.end(), node); - DCHECK(it != _editNodesOrdered.end()); - _editNodesOrdered.erase(it); - if (indexPath) { - [self.editIndexPaths removeObject:indexPath]; - } else { - // If we don't have the cell, we remove it by using its index. - const NSUInteger index = std::distance(_editNodesOrdered.begin(), it); - if (index < self.editIndexPaths.count) { - [self.editIndexPaths removeObjectAtIndex:index]; - } - } - - if (_editNodes.size() == 0) - [self setEditing:NO animated:YES]; - else - [self updateEditingStateAnimated:YES]; -} - #pragma mark - private methods - (void)loadURL:(const GURL&)url { @@ -431,66 +313,12 @@ return LayoutRectGetRect(primaryViewLayout); } -#pragma mark - Editing bar methods. +#pragma mark - Editing bar super methods overrides. - (CGRect)editingBarFrame { return CGRectInset(self.navigationBar.frame, 24.0, 0); } -- (void)updateEditingStateAnimated:(BOOL)animated { - if (!self.editing) { - [self hideEditingBarAnimated:animated]; - [self updateEditBarShadow]; - return; - } - - if (!self.editingBar) { - self.editingBar = - [[BookmarkEditingBar alloc] initWithFrame:[self editingBarFrame]]; - [self.editingBar setCancelTarget:self action:@selector(editingBarCancel)]; - [self.editingBar setDeleteTarget:self action:@selector(editingBarDelete)]; - [self.editingBar setMoveTarget:self action:@selector(editingBarMove)]; - [self.editingBar setEditTarget:self action:@selector(editingBarEdit)]; - - [self.view addSubview:self.editingBar]; - self.editingBar.hidden = YES; - } - - int bookmarkCount = 0; - int folderCount = 0; - for (auto* node : _editNodes) { - if (node->is_url()) - ++bookmarkCount; - else - ++folderCount; - } - [self.editingBar updateUIWithBookmarkCount:bookmarkCount - folderCount:folderCount]; - - [self showEditingBarAnimated:animated]; - [self updateEditBarShadow]; -} - -- (void)setEditing:(BOOL)editing animated:(BOOL)animated { - if (_editing == editing) - return; - - _editing = editing; - - if (editing) { - self.bookmarkPromoController.promoState = NO; - } else { - // Only reset the editing state when leaving edit mode. This allows - // subclasses to add nodes for editing before entering edit mode. - [self resetEditNodes]; - [self.bookmarkPromoController updatePromoState]; - } - - [self updateEditingStateAnimated:animated]; - if ([[self primaryMenuItem] supportsEditing]) - [[self primaryView] setEditing:editing animated:animated]; -} - - (void)showEditingBarAnimated:(BOOL)animated { CGRect endFrame = [self editingBarFrame]; if (self.editingBar.hidden) { @@ -503,6 +331,7 @@ delay:0 options:UIViewAnimationOptionBeginFromCurrentState animations:^{ + self.editingBar.alpha = 1; self.editingBar.frame = endFrame; } completion:^(BOOL finished) { @@ -521,6 +350,7 @@ delay:0 options:UIViewAnimationOptionBeginFromCurrentState animations:^{ + self.editingBar.alpha = 0; self.editingBar.frame = frame; } completion:^(BOOL finished) { @@ -529,42 +359,6 @@ }]; } -- (void)updateEditBarShadow { - [self.editingBar showShadow:self.editing]; -} - -#pragma mark Editing Bar Callbacks - -- (void)editingBarCancel { - [self setEditing:NO animated:YES]; -} - -- (void)editingBarMove { - [self moveNodes:_editNodes]; -} - -- (void)editingBarDelete { - [self deleteSelectedNodes]; - [self setEditing:NO animated:YES]; -} - -- (void)editingBarEdit { - DCHECK_EQ(_editNodes.size(), 1u); - const BookmarkNode* node = *(_editNodes.begin()); - [self editNode:node]; -} - -- (void)toggleMenuAnimated { - if ([self.panelView userDrivenAnimationInProgress]) - return; - - if (self.panelView.showingMenu) { - [self.panelView hideMenuAnimated:YES]; - } else { - [self.panelView showMenuAnimated:YES]; - } -} - #pragma mark - BookmarkMenuViewDelegate - (void)bookmarkMenuView:(BookmarkMenuView*)view @@ -577,6 +371,17 @@ } } +- (void)toggleMenuAnimated { + if ([self.panelView userDrivenAnimationInProgress]) + return; + + if (self.panelView.showingMenu) { + [self.panelView hideMenuAnimated:YES]; + } else { + [self.panelView showMenuAnimated:YES]; + } +} + #pragma mark - BookmarkCollectionViewDelegate // This class owns multiple views that have a delegate that conforms to // BookmarkCollectionViewDelegate, or a subprotocol of @@ -592,6 +397,10 @@ cell:(UICollectionViewCell*)cell removeNodeForEditing:(const BookmarkNode*)node { [self removeEditNode:node atIndexPath:[self indexPathForCell:cell]]; + if (_editNodes.size() == 0) + [self setEditing:NO animated:YES]; + else + [self updateEditingStateAnimated:YES]; } - (const std::set<const BookmarkNode*>&)nodesBeingEdited { @@ -698,15 +507,6 @@ [self.bookmarkPromoController hidePromoCell]; } -#pragma mark Action Sheet Callbacks - -- (void)selectFirstNode:(const BookmarkNode*)node - withCell:(UICollectionViewCell*)cell { - DCHECK(!self.editing); - [self insertEditNode:node atIndexPath:[self indexPathForCell:cell]]; - [self setEditing:YES animated:YES]; -} - #pragma mark - BookmarkCollectionViewDelegate - (void)bookmarkCollectionView:(BookmarkCollectionView*)view @@ -723,12 +523,6 @@ [self updatePrimaryMenuItem:menuItem animated:YES]; } -#pragma mark - Internal Utility Methods - -- (void)deleteSelectedNodes { - [self deleteNodes:_editNodes]; -} - - (void)moveEditingNodesToFolder:(const BookmarkNode*)folder { // The UI only supports moving nodes when there are at least one selected. DCHECK_GE(_editNodes.size(), 1u); @@ -917,11 +711,4 @@ [self setEditing:NO animated:YES]; } -- (NSIndexPath*)indexPathForCell:(UICollectionViewCell*)cell { - DCHECK([self primaryView].collectionView); - NSIndexPath* indexPath = - [[self primaryView].collectionView indexPathForCell:cell]; - return indexPath; -} - @end
diff --git a/ios/chrome/browser/ui/bookmarks/bookmark_home_view_controller.h b/ios/chrome/browser/ui/bookmarks/bookmark_home_view_controller.h index c613ae3..155f7df 100644 --- a/ios/chrome/browser/ui/bookmarks/bookmark_home_view_controller.h +++ b/ios/chrome/browser/ui/bookmarks/bookmark_home_view_controller.h
@@ -16,9 +16,22 @@ class ChromeBrowserState; } // namespace ios +namespace bookmarks { +class BookmarkNode; +} // namespace bookmarks + // Class to navigate the bookmark hierarchy, needs subclassing for tablet / // handset case. -@interface BookmarkHomeViewController : UIViewController +@interface BookmarkHomeViewController : UIViewController { + @protected + // The following 2 ivars both represent the set of nodes being edited. + // The set is for fast lookup. + // The vector maintains the order that edit nodes were added. + // Use the relevant instance methods to modify these two ivars in tandem. + // DO NOT modify these two ivars directly. + std::set<const bookmarks::BookmarkNode*> _editNodes; + std::vector<const bookmarks::BookmarkNode*> _editNodesOrdered; +} - (instancetype)initWithNibName:(NSString*)nibNameOrNil bundle:(NSBundle*)nibBundleOrNil NS_UNAVAILABLE;
diff --git a/ios/chrome/browser/ui/bookmarks/bookmark_home_view_controller.mm b/ios/chrome/browser/ui/bookmarks/bookmark_home_view_controller.mm index 2ada57ec..c25e240 100644 --- a/ios/chrome/browser/ui/bookmarks/bookmark_home_view_controller.mm +++ b/ios/chrome/browser/ui/bookmarks/bookmark_home_view_controller.mm
@@ -7,6 +7,7 @@ #include "components/bookmarks/browser/bookmark_model.h" #include "ios/chrome/browser/bookmarks/bookmark_model_factory.h" #include "ios/chrome/browser/browser_state/chrome_browser_state.h" +#import "ios/chrome/browser/ui/bookmarks/bars/bookmark_editing_bar.h" #import "ios/chrome/browser/ui/bookmarks/bars/bookmark_navigation_bar.h" #import "ios/chrome/browser/ui/bookmarks/bookmark_collection_view.h" #import "ios/chrome/browser/ui/bookmarks/bookmark_edit_view_controller.h" @@ -45,6 +46,9 @@ @synthesize bookmarkPromoController = _bookmarkPromoController; @synthesize bookmarks = _bookmarks; @synthesize browserState = _browserState; +@synthesize editing = _editing; +@synthesize editIndexPaths = _editIndexPaths; +@synthesize editingBar = _editingBar; @synthesize editViewController = _editViewController; @synthesize folderEditor = _folderEditor; @synthesize folderSelector = _folderSelector; @@ -54,8 +58,9 @@ @synthesize navigationBar = _navigationBar; @synthesize panelView = _panelView; @synthesize primaryMenuItem = _primaryMenuItem; -@synthesize waitForModelView = _waitForModelView; @synthesize scrollToTop = _scrollToTop; +@synthesize sideSwipingPossible = _sideSwipingPossible; +@synthesize waitForModelView = _waitForModelView; #if !defined(__has_feature) || !__has_feature(objc_arc) #error "This file requires ARC support." @@ -68,6 +73,9 @@ DCHECK(browserState); self = [super initWithNibName:nil bundle:nil]; if (self) { + _editIndexPaths = [[NSMutableArray alloc] init]; + [self resetEditNodes]; + _browserState = browserState->GetOriginalChromeBrowserState(); _loader = loader; @@ -170,6 +178,13 @@ #pragma mark Action sheet callbacks +- (void)selectFirstNode:(const BookmarkNode*)node + withCell:(UICollectionViewCell*)cell { + DCHECK(!self.editing); + [self insertEditNode:node atIndexPath:[self indexPathForCell:cell]]; + [self setEditing:YES animated:YES]; +} + - (void)moveNodes:(const std::set<const BookmarkNode*>&)nodes { DCHECK(!self.folderSelector); DCHECK(nodes.size() > 0); @@ -308,4 +323,161 @@ [self setEditing:NO animated:NO]; } +#pragma mark - Edit + +- (void)resetEditNodes { + _editNodes = std::set<const BookmarkNode*>(); + _editNodesOrdered = std::vector<const BookmarkNode*>(); + [self.editIndexPaths removeAllObjects]; +} + +- (void)insertEditNode:(const BookmarkNode*)node + atIndexPath:(NSIndexPath*)indexPath { + if (_editNodes.find(node) != _editNodes.end()) + return; + _editNodes.insert(node); + _editNodesOrdered.push_back(node); + if (indexPath) { + [self.editIndexPaths addObject:indexPath]; + } else { + // Insert null to keep the index valid. + [self.editIndexPaths addObject:[NSNull null]]; + } +} + +- (void)removeEditNode:(const BookmarkNode*)node + atIndexPath:(NSIndexPath*)indexPath { + if (_editNodes.find(node) == _editNodes.end()) + return; + _editNodes.erase(node); + std::vector<const BookmarkNode*>::iterator it = + std::find(_editNodesOrdered.begin(), _editNodesOrdered.end(), node); + DCHECK(it != _editNodesOrdered.end()); + _editNodesOrdered.erase(it); + if (indexPath) { + [self.editIndexPaths removeObject:indexPath]; + } else { + // If we don't have the cell, we remove it by using its index. + const NSUInteger index = std::distance(_editNodesOrdered.begin(), it); + if (index < self.editIndexPaths.count) { + [self.editIndexPaths removeObjectAtIndex:index]; + } + } +} + +- (void)showEditingBarAnimated:(BOOL)animated { + NOTREACHED() << "Must be overridden by subclass"; +} + +- (void)hideEditingBarAnimated:(BOOL)animated { + NOTREACHED() << "Must be overridden by subclass"; +} + +- (CGRect)editingBarFrame { + NOTREACHED() << "Must be implemented by subclass"; + return CGRectZero; +} + +- (void)updateEditingStateAnimated:(BOOL)animated { + if (!self.editing) { + [self hideEditingBarAnimated:animated]; + [self updateEditBarShadow]; + [self enableSideSwiping:YES]; + return; + } + + if (!self.editingBar) { + self.editingBar = + [[BookmarkEditingBar alloc] initWithFrame:[self editingBarFrame]]; + [self.editingBar setCancelTarget:self action:@selector(editingBarCancel)]; + [self.editingBar setDeleteTarget:self action:@selector(editingBarDelete)]; + [self.editingBar setMoveTarget:self action:@selector(editingBarMove)]; + [self.editingBar setEditTarget:self action:@selector(editingBarEdit)]; + + [self.view addSubview:self.editingBar]; + self.editingBar.alpha = 0; + self.editingBar.hidden = YES; + } + + int bookmarkCount = 0; + int folderCount = 0; + for (auto* node : _editNodes) { + if (node->is_url()) + ++bookmarkCount; + else + ++folderCount; + } + [self.editingBar updateUIWithBookmarkCount:bookmarkCount + folderCount:folderCount]; + + [self showEditingBarAnimated:animated]; + [self updateEditBarShadow]; + [self enableSideSwiping:NO]; +} + +- (void)setEditing:(BOOL)editing animated:(BOOL)animated { + if (_editing == editing) + return; + + _editing = editing; + + if (editing) { + self.bookmarkPromoController.promoState = NO; + } else { + // Only reset the editing state when leaving edit mode. This allows + // subclasses to add nodes for editing before entering edit mode. + [self resetEditNodes]; + [self.bookmarkPromoController updatePromoState]; + } + + [self updateEditingStateAnimated:animated]; + if ([[self primaryMenuItem] supportsEditing]) + [[self primaryView] setEditing:editing animated:animated]; +} + +- (void)updateEditBarShadow { + [self.editingBar showShadow:self.editing]; +} + +#pragma mark Editing Bar Callbacks + +- (void)editingBarCancel { + [self setEditing:NO animated:YES]; +} + +- (void)editingBarMove { + [self moveNodes:_editNodes]; +} + +- (void)editingBarDelete { + [self deleteSelectedNodes]; + [self setEditing:NO animated:YES]; +} + +- (void)editingBarEdit { + DCHECK_EQ(_editNodes.size(), 1u); + const BookmarkNode* node = *(_editNodes.begin()); + [self editNode:node]; +} + +#pragma mark - private + +- (void)enableSideSwiping:(BOOL)enable { + if (self.sideSwipingPossible) { + [self.panelView enableSideSwiping:enable]; + } +} + +// Deletes the nodes, and presents a toast with an undo button. +- (void)deleteSelectedNodes { + [self deleteNodes:_editNodes]; +} + +- (NSIndexPath*)indexPathForCell:(UICollectionViewCell*)cell { + DCHECK([self primaryView].collectionView); + NSIndexPath* indexPath = + [[self primaryView].collectionView indexPathForCell:cell]; + return indexPath; +} + @end
diff --git a/ios/chrome/browser/ui/bookmarks/bookmark_home_view_controller_protected.h b/ios/chrome/browser/ui/bookmarks/bookmark_home_view_controller_protected.h index 9ddc698..9e3cee1 100644 --- a/ios/chrome/browser/ui/bookmarks/bookmark_home_view_controller_protected.h +++ b/ios/chrome/browser/ui/bookmarks/bookmark_home_view_controller_protected.h
@@ -13,6 +13,7 @@ } // namespace bookmarks @class BookmarkCollectionView; +@class BookmarkEditingBar; @class BookmarkEditViewController; @class BookmarkFolderEditorViewController; @class BookmarkFolderViewController; @@ -67,6 +68,16 @@ // The view controller used to view and edit a single bookmark. @property(nonatomic, strong) BookmarkEditViewController* editViewController; +// Whether the view controller is in editing mode. +@property(nonatomic, assign) BOOL editing; + +// The set of selected index paths for edition. +@property(nonatomic, strong) NSMutableArray* editIndexPaths; + +// The layout code in this class relies on the assumption that the editingBar +// has the same frame as the navigationBar. +@property(nonatomic, strong) BookmarkEditingBar* editingBar; + // The view controller to present when editing the current folder. @property(nonatomic, strong) BookmarkFolderEditorViewController* folderEditor; @@ -74,6 +85,9 @@ // controller. @property(nonatomic, strong) BookmarkPromoController* bookmarkPromoController; +// Whether the panel view can be brought into view and hidden by swipe gesture. +@property(nonatomic, assign) BOOL sideSwipingPossible; + // This method should be called at most once in the life-cycle of the class. // It should be called at the soonest possible time after the view has been // loaded, and the bookmark model is loaded. @@ -93,6 +107,9 @@ // The active collection view that corresponds to primaryMenuItem. - (UIView<BookmarkHomePrimaryView>*)primaryView; +// Returns NSIndexPath for a given cell. +- (NSIndexPath*)indexPathForCell:(UICollectionViewCell*)cell; + #pragma mark - Navigation bar callbacks. // Called when the edit button is pressed on the navigation bar. @@ -100,6 +117,11 @@ #pragma mark - Action sheet callbacks +// Enters into edit mode by selecting the given node corresponding to the +// given cell. +- (void)selectFirstNode:(const bookmarks::BookmarkNode*)node + withCell:(UICollectionViewCell*)cell; + // Opens the folder move editor for the given node. - (void)moveNodes:(const std::set<const bookmarks::BookmarkNode*>&)nodes; @@ -109,6 +131,56 @@ // Opens the editor on the given node. - (void)editNode:(const bookmarks::BookmarkNode*)node; +#pragma mark - Edit + +// Replaces |_editNodes| and |_editNodesOrdered| with new container objects. +- (void)resetEditNodes; + +// Adds |node| corresponding to a |cell| if it isn't already present. +- (void)insertEditNode:(const bookmarks::BookmarkNode*)node + atIndexPath:(NSIndexPath*)indexPath; + +// Removes |node| corresponding to a |cell| if it's present. +- (void)removeEditNode:(const bookmarks::BookmarkNode*)node + atIndexPath:(NSIndexPath*)indexPath; + +// This method updates the property, and resets the edit nodes. +- (void)setEditing:(BOOL)editing animated:(BOOL)animated; + +// This method statelessly updates the editing top bar from |_editNodes| and +// |editing|. +- (void)updateEditingStateAnimated:(BOOL)animated; + +// Shows the editing bar, this method MUST be overridden by subclass to +// tailor the behaviour according to device. +- (void)showEditingBarAnimated:(BOOL)animated; + +// Hides the editing bar, this method MUST be overridden by subclass to +// tailor the behaviour according to device. +- (void)hideEditingBarAnimated:(BOOL)animated; + +// Returns the frame for editingBar, MUST be overridden by subclass. +- (CGRect)editingBarFrame; + +// Instaneously updates the shadow of the edit bar. +// This method should be called anytime: +// (1)|editing| property changes. +// (2)The primary view changes. +// (3)The primary view's collection view is scrolled. +// (2) is not necessary right now, as it is only possible to switch primary +// views when |editing| is NO. When |editing| is NO, the shadow is never shown. +- (void)updateEditBarShadow; + +#pragma mark - Editing bar callbacks +// The cancel button was tapped on the editing bar. +- (void)editingBarCancel; +// The move button was tapped on the editing bar. +- (void)editingBarMove; +// The delete button was tapped on the editing bar. +- (void)editingBarDelete; +// The edit button was tapped on the editing bar. +- (void)editingBarEdit; + @end #endif // IOS_CHROME_BROWSER_UI_BOOKMARKS_HOME_VIEW_CONTROLLER_PROTECTED_H_
diff --git a/ios/chrome/browser/ui/settings/password_details_collection_view_controller.mm b/ios/chrome/browser/ui/settings/password_details_collection_view_controller.mm index 04badcf..6f91bea3 100644 --- a/ios/chrome/browser/ui/settings/password_details_collection_view_controller.mm +++ b/ios/chrome/browser/ui/settings/password_details_collection_view_controller.mm
@@ -10,9 +10,9 @@ #include "base/metrics/histogram_macros.h" #include "base/strings/sys_string_conversions.h" #include "components/autofill/core/common/password_form.h" -#include "components/password_manager/core/browser/android_affiliation/affiliation_utils.h" #include "components/password_manager/core/browser/password_manager_metrics_util.h" #include "components/password_manager/core/browser/password_store.h" +#include "components/password_manager/core/browser/password_ui_utils.h" #include "ios/chrome/browser/browser_state/chrome_browser_state.h" #import "ios/chrome/browser/ui/collection_view/cells/MDCCollectionViewCell+Chrome.h" #import "ios/chrome/browser/ui/collection_view/cells/collection_view_item.h" @@ -126,11 +126,11 @@ base::SysUTF8ToNSString(_passwordForm.federation_origin.host()); } } - _site = base::SysUTF8ToNSString(_passwordForm.origin.spec()); + auto name_and_link = + password_manager::GetShownOriginAndLinkUrl(_passwordForm); + _site = base::SysUTF8ToNSString(name_and_link.second.spec()); self.title = [PasswordDetailsCollectionViewController - simplifyOrigin:base::SysUTF8ToNSString( - password_manager::GetHumanReadableOrigin( - _passwordForm))]; + simplifyOrigin:base::SysUTF8ToNSString(name_and_link.first)]; self.collectionViewAccessibilityIdentifier = @"PasswordDetailsCollectionViewController"; NSNotificationCenter* defaultCenter = [NSNotificationCenter defaultCenter];
diff --git a/ios/chrome/browser/ui/settings/password_details_collection_view_controller_unittest.mm b/ios/chrome/browser/ui/settings/password_details_collection_view_controller_unittest.mm index 88b95945..1d975f9 100644 --- a/ios/chrome/browser/ui/settings/password_details_collection_view_controller_unittest.mm +++ b/ios/chrome/browser/ui/settings/password_details_collection_view_controller_unittest.mm
@@ -258,7 +258,7 @@ {GURL("android://" "Qllt1FacrB0NYCeSFvmudHvssWBPFfC54EbtHTpFxukvw2wClI1rafcVB3kQOMxfJg" "xbVAkGXvC_A52kbPL1EQ==@com.parkingpanda.mobile/"), - @"com.parkingpanda.mobile"}}; + @"mobile.parkingpanda.com"}}; for (const auto& data : test_data) { origin_ = base::SysUTF8ToNSString(data.origin.spec());
diff --git a/ios/chrome/browser/ui/settings/passwords_settings_egtest.mm b/ios/chrome/browser/ui/settings/passwords_settings_egtest.mm index 6a32f16..9fb0d4a 100644 --- a/ios/chrome/browser/ui/settings/passwords_settings_egtest.mm +++ b/ios/chrome/browser/ui/settings/passwords_settings_egtest.mm
@@ -396,8 +396,7 @@ [self tapDone]; // Inspect "password details" view. - [[EarlGrey - selectElementWithMatcher:Entry(@"https://example.com, concrete username")] + [[EarlGrey selectElementWithMatcher:Entry(@"example.com, concrete username")] performAction:grey_tap()]; chrome_test_util::VerifyAccessibilityForCurrentScreen(); [[EarlGrey selectElementWithMatcher:SettingsMenuBackButton()] @@ -419,8 +418,7 @@ [self openPasswordSettings]; - [[EarlGrey - selectElementWithMatcher:Entry(@"https://example.com, concrete username")] + [[EarlGrey selectElementWithMatcher:Entry(@"example.com, concrete username")] performAction:grey_tap()]; MockReauthenticationModule* mock_reauthentication_module = @@ -473,8 +471,7 @@ [self openPasswordSettings]; - [[EarlGrey - selectElementWithMatcher:Entry(@"https://example.com, concrete username")] + [[EarlGrey selectElementWithMatcher:Entry(@"example.com, concrete username")] performAction:grey_tap()]; // Check the snackbar. @@ -508,8 +505,7 @@ [self openPasswordSettings]; - [[EarlGrey - selectElementWithMatcher:Entry(@"https://example.com, concrete username")] + [[EarlGrey selectElementWithMatcher:Entry(@"example.com, concrete username")] performAction:grey_tap()]; // Check the snackbar. @@ -544,8 +540,7 @@ [self openPasswordSettings]; - [[EarlGrey - selectElementWithMatcher:Entry(@"https://example.com, concrete username")] + [[EarlGrey selectElementWithMatcher:Entry(@"example.com, concrete username")] performAction:grey_tap()]; // Tap the Delete... button. @@ -575,8 +570,7 @@ nullptr)] assertWithMatcher:grey_notNil()]; // Also verify that the removed password is no longer in the list. - [[EarlGrey - selectElementWithMatcher:Entry(@"https://example.com, concrete username")] + [[EarlGrey selectElementWithMatcher:Entry(@"example.com, concrete username")] assertWithMatcher:grey_not(grey_sufficientlyVisible())]; [[EarlGrey selectElementWithMatcher:SettingsMenuBackButton()] @@ -594,8 +588,7 @@ [self openPasswordSettings]; - [[EarlGrey - selectElementWithMatcher:Entry(@"https://example.com, concrete username")] + [[EarlGrey selectElementWithMatcher:Entry(@"example.com, concrete username")] performAction:grey_tap()]; // Tap the Delete... button. @@ -625,8 +618,7 @@ // list. [[EarlGrey selectElementWithMatcher:SettingsMenuBackButton()] performAction:grey_tap()]; - [[EarlGrey - selectElementWithMatcher:Entry(@"https://example.com, concrete username")] + [[EarlGrey selectElementWithMatcher:Entry(@"example.com, concrete username")] assertWithMatcher:grey_sufficientlyVisible()]; [[EarlGrey selectElementWithMatcher:SettingsMenuBackButton()] @@ -647,8 +639,7 @@ [self tapEdit]; - [[EarlGrey - selectElementWithMatcher:Entry(@"https://example.com, concrete username")] + [[EarlGrey selectElementWithMatcher:Entry(@"example.com, concrete username")] performAction:grey_tap()]; // Check that the current view is not the detail view, by failing to locate @@ -674,7 +665,7 @@ [self openPasswordSettings]; - [[EarlGrey selectElementWithMatcher:Entry(@"https://example.com")] + [[EarlGrey selectElementWithMatcher:Entry(@"example.com")] performAction:grey_tap()]; // Check that the Site section is there as well as the Delete button. @@ -712,8 +703,7 @@ [self openPasswordSettings]; - [[EarlGrey - selectElementWithMatcher:Entry(@"https://example.com, concrete username")] + [[EarlGrey selectElementWithMatcher:Entry(@"example.com, concrete username")] performAction:grey_tap()]; // Tap the site cell to display the context menu. @@ -755,8 +745,7 @@ [self openPasswordSettings]; - [[EarlGrey - selectElementWithMatcher:Entry(@"https://example.com, concrete username")] + [[EarlGrey selectElementWithMatcher:Entry(@"example.com, concrete username")] performAction:grey_tap()]; // Tap the username cell to display the context menu. @@ -799,8 +788,7 @@ [self openPasswordSettings]; - [[EarlGrey - selectElementWithMatcher:Entry(@"https://example.com, concrete username")] + [[EarlGrey selectElementWithMatcher:Entry(@"example.com, concrete username")] performAction:grey_tap()]; // Tap the password cell to display the context menu. @@ -850,8 +838,7 @@ [self openPasswordSettings]; - [[EarlGrey - selectElementWithMatcher:Entry(@"https://example.com, concrete username")] + [[EarlGrey selectElementWithMatcher:Entry(@"example.com, concrete username")] performAction:grey_tap()]; // Tap the password cell to display the context menu. @@ -923,9 +910,7 @@ [self openPasswordSettings]; - [[EarlGrey - selectElementWithMatcher:Entry( - @"https://example.com, federated username")] + [[EarlGrey selectElementWithMatcher:Entry(@"example.com, federated username")] performAction:grey_tap()]; // Check that the Site, Username, Federation and Delete Saved Password
diff --git a/ios/chrome/browser/ui/settings/save_passwords_collection_view_controller.mm b/ios/chrome/browser/ui/settings/save_passwords_collection_view_controller.mm index b38d68c..424f0e8 100644 --- a/ios/chrome/browser/ui/settings/save_passwords_collection_view_controller.mm +++ b/ios/chrome/browser/ui/settings/save_passwords_collection_view_controller.mm
@@ -15,10 +15,10 @@ #include "components/autofill/core/common/password_form.h" #include "components/google/core/browser/google_util.h" #include "components/keyed_service/core/service_access_type.h" -#include "components/password_manager/core/browser/android_affiliation/affiliation_utils.h" #include "components/password_manager/core/browser/password_manager_constants.h" #include "components/password_manager/core/browser/password_store.h" #include "components/password_manager/core/browser/password_store_consumer.h" +#include "components/password_manager/core/browser/password_ui_utils.h" #include "components/password_manager/core/common/password_manager_pref_names.h" #include "components/prefs/pref_member.h" #include "components/prefs/pref_service.h" @@ -260,8 +260,8 @@ - (SavedFormContentItem*)savedFormItemWithForm:(autofill::PasswordForm*)form { SavedFormContentItem* passwordItem = [[SavedFormContentItem alloc] initWithType:ItemTypeSavedPassword]; - passwordItem.text = - base::SysUTF8ToNSString(password_manager::GetHumanReadableOrigin(*form)); + passwordItem.text = base::SysUTF8ToNSString( + password_manager::GetShownOriginAndLinkUrl(*form).first); passwordItem.detailText = base::SysUTF16ToNSString(form->username_value); if (experimental_flags::IsViewCopyPasswordsEnabled()) { passwordItem.accessibilityTraits |= UIAccessibilityTraitButton; @@ -275,8 +275,8 @@ (autofill::PasswordForm*)form { BlacklistedFormContentItem* passwordItem = [[BlacklistedFormContentItem alloc] initWithType:ItemTypeBlacklisted]; - passwordItem.text = - base::SysUTF8ToNSString(password_manager::GetHumanReadableOrigin(*form)); + passwordItem.text = base::SysUTF8ToNSString( + password_manager::GetShownOriginAndLinkUrl(*form).first); if (experimental_flags::IsViewCopyPasswordsEnabled()) { passwordItem.accessibilityTraits |= UIAccessibilityTraitButton; passwordItem.accessoryType =
diff --git a/ui/views/controls/button/md_text_button.cc b/ui/views/controls/button/md_text_button.cc index 8aa0a62..114cbe5 100644 --- a/ui/views/controls/button/md_text_button.cc +++ b/ui/views/controls/button/md_text_button.cc
@@ -220,6 +220,10 @@ label()->font_list().GetFontSize() - style::GetFont(style::CONTEXT_BUTTON_MD, style::STYLE_PRIMARY) .GetFontSize(); + // TODO(tapted): This should get |target_height| using LayoutProvider:: + // GetControlHeightForFont(). It can't because that only returns a correct + // result with --secondary-ui-md, and MdTextButtons appear in top chrome + // without that. const int kBaseHeight = 28; int target_height = std::max(kBaseHeight + size_delta * 2, label()->font_list().GetFontSize() * 2);