Skip to content

GitLab

T
tangled
Commit bbb682d5 authored by Mihai Sardarescu's avatar Mihai Sardarescu Committed by Commit Bot
Browse files
Options

Remove pre-UnifiedConsent code from sync confirmation dialog. 

This CL is the first CL to remove the per-UnifiedConsent code from
the sync confirmation dialog. The pre-UnifiedConsent sync confirmation
HTML and JS are still for the case when sync is disabled by policy.

Follow-up CLs will attempt to do the clean-up for these cases.

Bug: 971183

Change-Id: I9b24b5b23fad9a0658ebea0ab0c80f3157e530ae
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1645358Reviewed-by: default avatarThomas Tangl <tangltom@chromium.org>
Reviewed-by: default avatarDan Beam <dbeam@chromium.org>
Commit-Queue: Dan Beam <dbeam@chromium.org>
Auto-Submit: Mihai Sardarescu <msarda@chromium.org>
Cr-Commit-Position: refs/heads/master@{#667253}
parent c04c61a0
Expand all Hide whitespace changes
Inline Side-by-side
Showing with 20 additions and 546 deletions
chrome/browser/resources/signin/sync_confirmation/sync_confirmation.js
View file @ bbb682d5
......@@ -42,24 +42,10 @@ cr.define('sync.confirmation', function() {
chrome.send('undo');
}
function onGoToSettings(e) {
chrome.send(
'goToSettings',
[getConsentDescription(), getConsentConfirmation(e.path)]);
}
function initialize() {
document.addEventListener('keydown', onKeyDown);
$('confirmButton').addEventListener('click', onConfirm);
$('undoButton').addEventListener('click', onUndo);
if (loadTimeData.getBoolean('isSyncAllowed')) {
$('settingsLink').addEventListener('click', onGoToSettings);
$('profile-picture').addEventListener('load', onPictureLoaded);
$('syncDisabledDetails').hidden = true;
} else {
$('syncConfirmationDetails').hidden = true;
}
// Prefer using |document.body.offsetHeight| instead of
// |document.body.scrollHeight| as it returns the correct height of the
// even when the page zoom in Chrome is different than 100%.
......@@ -70,18 +56,6 @@ cr.define('sync.confirmation', function() {
document.activeElement.blur();
}
function setUserImageURL(url) {
if (loadTimeData.getBoolean('isSyncAllowed')) {
$('profile-picture').src = url;
}
}
function onPictureLoaded(e) {
if (loadTimeData.getBoolean('isSyncAllowed')) {
$('picture-container').classList.add('loaded');
}
}
function onKeyDown(e) {
// If the currently focused element isn't something that performs an action
// on "enter" being pressed and the user hits "enter", perform the default
......@@ -93,15 +67,11 @@ cr.define('sync.confirmation', function() {
}
}
// TODO(tangltom): clearFocus and setUserImageURL are called directly by the
// C++ handler. C++ handlers should not be calling JS functions by name
// anymore. They should be firing events with FireWebuiListener and have the
// page itself decide whether to listen or not listen to the event.
return {
clearFocus: clearFocus,
initialize: initialize,
setUserImageURL: setUserImageURL
};
// TODO(tangltom): clearFocus is called directly by the C++ handler.
// C++ handlers should not be calling JS functions by name anymore. They
// should be firing events with FireWebuiListener and have the page itself
// decide whether to listen or not listen to the event.
return {clearFocus: clearFocus, initialize: initialize};
});
document.addEventListener('DOMContentLoaded', sync.confirmation.initialize);
chrome/browser/ui/webui/signin/sync_confirmation_handler.cc
View file @ bbb682d5
......@@ -11,6 +11,7 @@
#include "base/metrics/user_metrics.h"
#include "base/threading/thread_task_runner_handle.h"
#include "chrome/browser/consent_auditor/consent_auditor_factory.h"
#include "chrome/browser/profiles/profile.h"
#include "chrome/browser/profiles/profile_avatar_icon_util.h"
#include "chrome/browser/signin/identity_manager_factory.h"
#include "chrome/browser/ui/browser_list.h"
......@@ -90,6 +91,7 @@ void SyncConfirmationHandler::HandleConfirm(const base::ListValue* args) {
}
void SyncConfirmationHandler::HandleGoToSettings(const base::ListValue* args) {
DCHECK(profile_->IsSyncAllowed());
did_user_explicitly_interact = true;
RecordConsent(args);
CloseModalSigninWindow(LoginUIService::CONFIGURE_SYNC_FIRST);
......@@ -102,6 +104,7 @@ void SyncConfirmationHandler::HandleUndo(const base::ListValue* args) {
void SyncConfirmationHandler::HandleAccountImageRequest(
const base::ListValue* args) {
DCHECK(profile_->IsSyncAllowed());
base::Optional<AccountInfo> primary_account_info =
identity_manager_->FindExtendedAccountInfoForAccount(
identity_manager_->GetPrimaryAccountInfo());
......@@ -165,6 +168,12 @@ void SyncConfirmationHandler::RecordConsent(const base::ListValue* args) {
}
void SyncConfirmationHandler::SetUserImageURL(const std::string& picture_url) {
if (!profile_->IsSyncAllowed()) {
// The sync disabled confirmation handler does not present the user image.
// Avoid updating the image URL in this case.
return;
}
std::string picture_url_to_load;
GURL picture_gurl(picture_url);
if (picture_gurl.is_valid()) {
......@@ -179,12 +188,7 @@ void SyncConfirmationHandler::SetUserImageURL(const std::string& picture_url) {
base::Value picture_url_value(picture_url_to_load);
AllowJavascript();
if (unified_consent::IsUnifiedConsentFeatureEnabled()) {
FireWebUIListener("account-image-changed", picture_url_value);
} else {
CallJavascriptFunction("sync.confirmation.setUserImageURL",
picture_url_value);
}
FireWebUIListener("account-image-changed", picture_url_value);
}
void SyncConfirmationHandler::OnExtendedAccountInfoUpdated(
......
chrome/browser/ui/webui/signin/sync_confirmation_handler_unittest.cc
View file @ bbb682d5
......@@ -210,62 +210,12 @@ class SyncConfirmationHandlerTest : public BrowserWithTestWindowTest,
DISALLOW_COPY_AND_ASSIGN(SyncConfirmationHandlerTest);
};
class SyncConfirmationHandlerTest_UnifiedConsentDisabled
: public SyncConfirmationHandlerTest {
public:
SyncConfirmationHandlerTest_UnifiedConsentDisabled()
: scoped_unified_consent_(
unified_consent::UnifiedConsentFeatureState::kDisabled) {}
private:
unified_consent::ScopedUnifiedConsent scoped_unified_consent_;
DISALLOW_COPY_AND_ASSIGN(SyncConfirmationHandlerTest_UnifiedConsentDisabled);
};
const char SyncConfirmationHandlerTest::kConsentText1[] = "consentText1";
const char SyncConfirmationHandlerTest::kConsentText2[] = "consentText2";
const char SyncConfirmationHandlerTest::kConsentText3[] = "consentText3";
const char SyncConfirmationHandlerTest::kConsentText4[] = "consentText4";
const char SyncConfirmationHandlerTest::kConsentText5[] = "consentText5";
TEST_F(SyncConfirmationHandlerTest_UnifiedConsentDisabled,
TestSetImageIfPrimaryAccountReady) {
identity_test_env()->SimulateSuccessfulFetchOfAccountInfo(
account_info_.account_id, account_info_.email, account_info_.gaia, "",
"full_name", "given_name", "locale",
"http://picture.example.com/picture.jpg");
base::ListValue args;
args.Set(0, std::make_unique<base::Value>(kDefaultDialogHeight));
handler()->HandleInitializedWithSize(&args);
EXPECT_EQ(2U, web_ui()->call_data().size());
// When the primary account is ready, setUserImageURL happens before
// clearFocus since the image URL is known before showing the dialog.
EXPECT_EQ("sync.confirmation.setUserImageURL",
web_ui()->call_data()[0]->function_name());
EXPECT_TRUE(web_ui()->call_data()[0]->arg1()->is_string());
std::string passed_picture_url;
EXPECT_TRUE(
web_ui()->call_data()[0]->arg1()->GetAsString(&passed_picture_url));
EXPECT_EQ("sync.confirmation.clearFocus",
web_ui()->call_data()[1]->function_name());
identity::IdentityManager* identity_manager =
IdentityManagerFactory::GetForProfile(profile());
base::Optional<AccountInfo> primary_account_info =
identity_manager->FindExtendedAccountInfoForAccount(
identity_manager->GetPrimaryAccountInfo());
std::string original_picture_url =
primary_account_info ? primary_account_info->picture_url : std::string();
GURL picture_url_with_size = signin::GetAvatarImageURLWithOptions(
GURL(original_picture_url), kExpectedProfileImageSize,
false /* no_silhouette */);
EXPECT_EQ(picture_url_with_size.spec(), passed_picture_url);
}
TEST_F(SyncConfirmationHandlerTest, TestSetImageIfPrimaryAccountReady) {
identity_test_env()->SimulateSuccessfulFetchOfAccountInfo(
account_info_.account_id, account_info_.email, account_info_.gaia, "",
......@@ -281,55 +231,6 @@ TEST_F(SyncConfirmationHandlerTest, TestSetImageIfPrimaryAccountReady) {
web_ui()->call_data()[1]->function_name());
}
TEST_F(SyncConfirmationHandlerTest_UnifiedConsentDisabled,
TestSetImageIfPrimaryAccountReadyLater) {
base::ListValue args;
args.Set(0, std::make_unique<base::Value>(kDefaultDialogHeight));
handler()->HandleInitializedWithSize(&args);
EXPECT_EQ(2U, web_ui()->call_data().size());
identity_test_env()->SimulateSuccessfulFetchOfAccountInfo(
account_info_.account_id, account_info_.email, account_info_.gaia, "",
"full_name", "given_name", "locale",
"http://picture.example.com/picture.jpg");
EXPECT_EQ(3U, web_ui()->call_data().size());
// When the primary account isn't yet ready when the dialog is shown,
// setUserImageURL is called with the default placeholder image.
EXPECT_EQ("sync.confirmation.setUserImageURL",
web_ui()->call_data()[0]->function_name());
EXPECT_TRUE(web_ui()->call_data()[0]->arg1()->is_string());
std::string passed_picture_url;
EXPECT_TRUE(
web_ui()->call_data()[0]->arg1()->GetAsString(&passed_picture_url));
EXPECT_EQ(profiles::GetPlaceholderAvatarIconUrl(), passed_picture_url);
// When the primary account isn't yet ready when the dialog is shown,
// clearFocus is called before the second call to setUserImageURL.
EXPECT_EQ("sync.confirmation.clearFocus",
web_ui()->call_data()[1]->function_name());
EXPECT_EQ("sync.confirmation.setUserImageURL",
web_ui()->call_data()[2]->function_name());
EXPECT_TRUE(web_ui()->call_data()[2]->arg1()->is_string());
EXPECT_TRUE(
web_ui()->call_data()[2]->arg1()->GetAsString(&passed_picture_url));
identity::IdentityManager* identity_manager =
IdentityManagerFactory::GetForProfile(profile());
base::Optional<AccountInfo> primary_account_info =
identity_manager->FindExtendedAccountInfoForAccount(
identity_manager->GetPrimaryAccountInfo());
std::string original_picture_url =
primary_account_info ? primary_account_info->picture_url : std::string();
GURL picture_url_with_size = signin::GetAvatarImageURLWithOptions(
GURL(original_picture_url), kExpectedProfileImageSize,
false /* no_silhouette */);
EXPECT_EQ(picture_url_with_size.spec(), passed_picture_url);
}
TEST_F(SyncConfirmationHandlerTest, TestSetImageIfPrimaryAccountReadyLater) {
base::ListValue args;
args.Set(0, std::make_unique<base::Value>(kDefaultDialogHeight));
......@@ -349,36 +250,6 @@ TEST_F(SyncConfirmationHandlerTest, TestSetImageIfPrimaryAccountReadyLater) {
ExpectAccountImageChanged(*web_ui()->call_data()[2]);
}
TEST_F(SyncConfirmationHandlerTest_UnifiedConsentDisabled,
TestSetImageIgnoredIfSecondaryAccountUpdated) {
base::ListValue args;
args.Set(0, std::make_unique<base::Value>(kDefaultDialogHeight));
handler()->HandleInitializedWithSize(&args);
EXPECT_EQ(2U, web_ui()->call_data().size());
AccountInfo account_info =
identity_test_env()->MakeAccountAvailable("bar@example.com");
identity_test_env()->SimulateSuccessfulFetchOfAccountInfo(
account_info.account_id, account_info.email, account_info.gaia, "",
"bar_full_name", "bar_given_name", "bar_locale",
"http://picture.example.com/bar_picture.jpg");
// Updating the account info of a secondary account should not update the
// image of the sync confirmation dialog.
EXPECT_EQ(2U, web_ui()->call_data().size());
identity_test_env()->SimulateSuccessfulFetchOfAccountInfo(
account_info_.account_id, account_info_.email, account_info_.gaia, "",
"full_name", "given_name", "locale",
"http://picture.example.com/picture.jpg");
// Updating the account info of the primary account should update the
// image of the sync confirmation dialog.
EXPECT_EQ(3U, web_ui()->call_data().size());
EXPECT_EQ("sync.confirmation.setUserImageURL",
web_ui()->call_data()[2]->function_name());
}
TEST_F(SyncConfirmationHandlerTest,
TestSetImageIgnoredIfSecondaryAccountUpdated) {
base::ListValue args;
......
chrome/browser/ui/webui/signin/sync_confirmation_ui.cc
View file @ bbb682d5
......@@ -29,10 +29,9 @@
SyncConfirmationUI::SyncConfirmationUI(content::WebUI* web_ui)
: SigninWebDialogUI(web_ui),
consent_feature_(consent_auditor::Feature::CHROME_SYNC) {
DCHECK(unified_consent::IsUnifiedConsentFeatureEnabled());
Profile* profile = Profile::FromWebUI(web_ui);
bool is_sync_allowed = profile->IsSyncAllowed();
bool is_unified_consent_enabled =
unified_consent::IsUnifiedConsentFeatureEnabled();
content::WebUIDataSource* source =
content::WebUIDataSource::Create(chrome::kChromeUISyncConfirmationHost);
......@@ -42,7 +41,7 @@ SyncConfirmationUI::SyncConfirmationUI(content::WebUI* web_ui)
int title_ids = -1;
int confirm_button_ids = -1;
int undo_button_ids = -1;
if (is_unified_consent_enabled && is_sync_allowed) {
if (is_sync_allowed) {
source->SetDefaultResource(IDR_DICE_SYNC_CONFIRMATION_HTML);
source->AddResourcePath("sync_confirmation_browser_proxy.html",
IDR_DICE_SYNC_CONFIRMATION_BROWSER_PROXY_HTML);
......@@ -90,33 +89,13 @@ SyncConfirmationUI::SyncConfirmationUI(content::WebUI* web_ui)
} else {
source->SetDefaultResource(IDR_SYNC_CONFIRMATION_HTML);
source->AddResourcePath("sync_confirmation.js", IDR_SYNC_CONFIRMATION_JS);
source->AddBoolean("isSyncAllowed", is_sync_allowed);
AddStringResource(source, "syncConfirmationChromeSyncTitle",
IDS_SYNC_CONFIRMATION_CHROME_SYNC_TITLE);
AddStringResource(source, "syncConfirmationChromeSyncBody",
IDS_SYNC_CONFIRMATION_CHROME_SYNC_MESSAGE);
AddStringResource(source, "syncConfirmationPersonalizeServicesTitle",
IDS_SYNC_CONFIRMATION_PERSONALIZE_SERVICES_TITLE);
AddStringResource(source, "syncConfirmationPersonalizeServicesBody",
IDS_SYNC_CONFIRMATION_PERSONALIZE_SERVICES_BODY);
AddStringResource(source, "syncConfirmationSyncSettingsLinkBody",
IDS_SYNC_CONFIRMATION_SYNC_SETTINGS_LINK_BODY);
AddStringResource(source, "syncDisabledConfirmationDetails",
IDS_SYNC_DISABLED_CONFIRMATION_DETAILS);
title_ids = AccountConsistencyModeManager::IsDiceEnabledForProfile(profile)
? IDS_SYNC_CONFIRMATION_DICE_TITLE
: IDS_SYNC_CONFIRMATION_TITLE;
confirm_button_ids = IDS_SETTINGS_TURN_ON;
undo_button_ids = IDS_CANCEL;
title_ids = IDS_SYNC_DISABLED_CONFIRMATION_CHROME_SYNC_TITLE;
confirm_button_ids = IDS_SYNC_DISABLED_CONFIRMATION_CONFIRM_BUTTON_LABEL;
undo_button_ids = IDS_SYNC_DISABLED_CONFIRMATION_UNDO_BUTTON_LABEL;
consent_feature_ = consent_auditor::Feature::CHROME_SYNC;
if (!is_sync_allowed) {
title_ids = IDS_SYNC_DISABLED_CONFIRMATION_CHROME_SYNC_TITLE;
confirm_button_ids = IDS_SYNC_DISABLED_CONFIRMATION_CONFIRM_BUTTON_LABEL;
undo_button_ids = IDS_SYNC_DISABLED_CONFIRMATION_UNDO_BUTTON_LABEL;
}
}
DCHECK_GE(title_ids, 0);
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment