Skip to content

Commit

Permalink
Remove SyncService::{Is,Set}SyncAllowedByPlatform()
Browse files Browse the repository at this point in the history
No behavior is changed: the setter is no longer called, so the value
of the getter remains always true. Delete the APIs.
Since the "disallowed" state no longer happens, also delete the
corresponding:
- DISABLE_REASON_PLATFORM_OVERRIDE.
- Android UI exposing the error.
- Sync.ResetEngineReason metric bucket.

Bug: 1107904
Change-Id: I862f38c939497dd81439b44ac7a7449faf44c06a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3418396
Reviewed-by: Jan Krčál <jkrcal@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Commit-Queue: Victor Vianna <victorvianna@google.com>
Auto-Submit: Victor Vianna <victorvianna@google.com>
Cr-Commit-Position: refs/heads/main@{#964540}
  • Loading branch information
Victor Hugo Vianna Silva authored and Chromium LUCI CQ committed Jan 28, 2022
1 parent fff5e5a commit d18700c
Show file tree
Hide file tree
Showing 24 changed files with 28 additions and 189 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -173,10 +173,6 @@ private boolean shouldShowBookmarkSigninPromo() {
return SyncPromoState.NO_PROMO;
}

if (!mSyncService.isSyncAllowedByPlatform()) {
return SyncPromoState.NO_PROMO;
}

if (!mSignInManager.getIdentityManager().hasPrimaryAccount(ConsentLevel.SYNC)) {
if (!shouldShowBookmarkSigninPromo()) {
return SyncPromoState.NO_PROMO;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,6 @@
package org.chromium.chrome.browser.signin;

import android.content.Context;
import android.content.Intent;
import android.provider.Settings;
import android.util.AttributeSet;
import android.view.LayoutInflater;
import android.view.View;
Expand All @@ -15,7 +13,6 @@
import android.widget.LinearLayout;
import android.widget.TextView;

import org.chromium.base.IntentUtils;
import org.chromium.chrome.R;
import org.chromium.chrome.browser.settings.SettingsLauncherImpl;
import org.chromium.chrome.browser.sync.SyncService;
Expand Down Expand Up @@ -95,9 +92,7 @@ public void init(@AccessPoint int accessPoint) {

private void update() {
ViewState viewState;
if (!SyncService.get().isSyncAllowedByPlatform()) {
viewState = getStateForEnableAndroidSync();
} else if (!SyncService.get().isSyncRequested()) {
if (!SyncService.get().isSyncRequested()) {
viewState = getStateForEnableChromeSync();
} else {
viewState = getStateForStartUsing();
Expand Down Expand Up @@ -159,19 +154,6 @@ public void apply(Button button) {
}
}

private ViewState getStateForEnableAndroidSync() {
assert mAccessPoint == SigninAccessPoint.RECENT_TABS
: "Enable Android Sync should not be showing from bookmarks";

int descId = R.string.recent_tabs_sync_promo_enable_android_sync;

ButtonState positiveButton = new ButtonPresent(R.string.open_settings_button, view -> {
IntentUtils.safeStartActivity(getContext(), new Intent(Settings.ACTION_SYNC_SETTINGS));
});

return new ViewState(descId, positiveButton);
}

private ViewState getStateForEnableChromeSync() {
int descId = mAccessPoint == SigninAccessPoint.BOOKMARK_MANAGER
? R.string.bookmarks_sync_promo_enable_sync
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@
import android.content.Intent;
import android.net.Uri;
import android.os.Bundle;
import android.provider.Settings;
import android.text.SpannableString;
import android.text.style.ForegroundColorSpan;
import android.view.LayoutInflater;
Expand Down Expand Up @@ -670,10 +669,6 @@ public void onSyncErrorCardPrimaryButtonClicked() {
assert primaryAccountInfo != null;

switch (syncError) {
case SyncError.ANDROID_SYNC_DISABLED:
IntentUtils.safeStartActivity(
getActivity(), new Intent(Settings.ACTION_SYNC_SETTINGS));
return;
case SyncError.AUTH_ERROR:
AccountManagerFacadeProvider.getInstance().updateCredentials(
CoreAccountInfo.getAndroidAccountFrom(primaryAccountInfo), getActivity(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,24 +50,23 @@ public class SyncSettingsUtils {
private static final String MY_ACCOUNT_URL = "https://myaccount.google.com/smartlink/home";
private static final String TAG = "SyncSettingsUtils";

@IntDef({SyncError.NO_ERROR, SyncError.ANDROID_SYNC_DISABLED, SyncError.AUTH_ERROR,
SyncError.PASSPHRASE_REQUIRED, SyncError.TRUSTED_VAULT_KEY_REQUIRED_FOR_EVERYTHING,
@IntDef({SyncError.NO_ERROR, SyncError.AUTH_ERROR, SyncError.PASSPHRASE_REQUIRED,
SyncError.TRUSTED_VAULT_KEY_REQUIRED_FOR_EVERYTHING,
SyncError.TRUSTED_VAULT_KEY_REQUIRED_FOR_PASSWORDS,
SyncError.TRUSTED_VAULT_RECOVERABILITY_DEGRADED_FOR_EVERYTHING,
SyncError.TRUSTED_VAULT_RECOVERABILITY_DEGRADED_FOR_PASSWORDS,
SyncError.CLIENT_OUT_OF_DATE, SyncError.SYNC_SETUP_INCOMPLETE, SyncError.OTHER_ERRORS})
@Retention(RetentionPolicy.SOURCE)
public @interface SyncError {
int NO_ERROR = -1;
int ANDROID_SYNC_DISABLED = 0;
int AUTH_ERROR = 1;
int PASSPHRASE_REQUIRED = 2;
int TRUSTED_VAULT_KEY_REQUIRED_FOR_EVERYTHING = 3;
int TRUSTED_VAULT_KEY_REQUIRED_FOR_PASSWORDS = 4;
int TRUSTED_VAULT_RECOVERABILITY_DEGRADED_FOR_EVERYTHING = 5;
int TRUSTED_VAULT_RECOVERABILITY_DEGRADED_FOR_PASSWORDS = 6;
int CLIENT_OUT_OF_DATE = 7;
int SYNC_SETUP_INCOMPLETE = 8;
int AUTH_ERROR = 0;
int PASSPHRASE_REQUIRED = 1;
int TRUSTED_VAULT_KEY_REQUIRED_FOR_EVERYTHING = 2;
int TRUSTED_VAULT_KEY_REQUIRED_FOR_PASSWORDS = 3;
int TRUSTED_VAULT_RECOVERABILITY_DEGRADED_FOR_EVERYTHING = 4;
int TRUSTED_VAULT_RECOVERABILITY_DEGRADED_FOR_PASSWORDS = 5;
int CLIENT_OUT_OF_DATE = 6;
int SYNC_SETUP_INCOMPLETE = 7;
int OTHER_ERRORS = 128;
}

Expand All @@ -81,10 +80,6 @@ public static int getSyncError() {
return SyncError.NO_ERROR;
}

if (!syncService.isSyncAllowedByPlatform()) {
return SyncError.ANDROID_SYNC_DISABLED;
}

if (!syncService.isSyncRequested()) {
return SyncError.NO_ERROR;
}
Expand Down Expand Up @@ -135,8 +130,6 @@ public static int getSyncError() {
*/
public static String getSyncErrorHint(Context context, @SyncError int error) {
switch (error) {
case SyncError.ANDROID_SYNC_DISABLED:
return context.getString(R.string.hint_android_sync_disabled);
case SyncError.AUTH_ERROR:
return context.getString(R.string.hint_sync_auth_error);
case SyncError.CLIENT_OUT_OF_DATE:
Expand Down Expand Up @@ -169,7 +162,6 @@ public static String getSyncErrorHint(Context context, @SyncError int error) {
*/
public static String getSyncErrorCardTitle(Context context, @SyncError int error) {
switch (error) {
case SyncError.ANDROID_SYNC_DISABLED:
case SyncError.AUTH_ERROR:
case SyncError.CLIENT_OUT_OF_DATE:
case SyncError.OTHER_ERRORS:
Expand All @@ -191,8 +183,6 @@ public static String getSyncErrorCardTitle(Context context, @SyncError int error
public static @Nullable String getSyncErrorCardButtonLabel(
Context context, @SyncError int error) {
switch (error) {
case SyncError.ANDROID_SYNC_DISABLED:
return context.getString(R.string.android_sync_disabled_error_card_button);
case SyncError.AUTH_ERROR:
case SyncError.OTHER_ERRORS:
// Both these errors should be resolved by signing the user again.
Expand Down Expand Up @@ -231,10 +221,6 @@ public static String getSyncStatusSummary(Context context) {
return context.getString(R.string.sync_off);
}

if (!syncService.isSyncAllowedByPlatform()) {
return context.getString(R.string.sync_android_system_sync_disabled);
}

if (syncService.isSyncDisabledByEnterprisePolicy()) {
return context.getString(R.string.sync_is_disabled_by_administrator);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -174,8 +174,7 @@ public void setUp() {
mActivityTestRule.getActivity().getActivityTab().getWebContents()));
mBookmarkBridge = mActivityTestRule.getActivity().getBookmarkBridgeForTesting();

// Stub SyncService state to make sure promos aren't suppressed.
when(mSyncService.isSyncAllowedByPlatform()).thenReturn(true);
// Emulate sync disabled so promos are shown.
when(mSyncService.isSyncRequested()).thenReturn(false);
SyncService.overrideForTests(mSyncService);
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -98,25 +98,6 @@ public static void tearDownAfterActivityDestroyed() {
ChromeNightModeTestUtils.tearDownNightModeAfterChromeActivityDestroyed();
}

@Test
@LargeTest
@Feature("RenderTest")
@ParameterAnnotations.UseMethodParameter(NightModeTestUtils.NightModeParams.class)
public void testSyncErrorCardForAndroidSyncDisabled(boolean nightModeEnabled) throws Exception {
mAccountManagerTestRule.addTestAccountThenSigninAndEnableSync(mFakeSyncServiceImpl);
TestThreadUtils.runOnUiThreadBlocking(() -> {
mFakeSyncServiceImpl.setSyncAllowedByPlatform(false);

Assert.assertEquals("ANDROID_SYNC_DISABLED SyncError should be set",
SyncSettingsUtils.SyncError.ANDROID_SYNC_DISABLED,
SyncSettingsUtils.getSyncError());
});

mSettingsActivityTestRule.startSettingsActivity();
mRenderTestRule.render(
getPersonalizedSyncPromoView(), "sync_error_card_android_sync_disabled");
}

@Test
@LargeTest
@Feature("RenderTest")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -214,10 +214,6 @@ public interface SyncSetupInProgressHandle {

public abstract void removeSyncStateChangedListener(SyncStateChangedListener listener);

public abstract boolean isSyncAllowedByPlatform();

public abstract void setSyncAllowedByPlatform(boolean allowed);

/**
* Returns the actual passphrase type being used for encryption. The sync engine must be
* running (isEngineInitialized() returns true) before calling this function.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -233,16 +233,6 @@ protected void syncStateChanged() {
}
}

@Override
public boolean isSyncAllowedByPlatform() {
return SyncServiceImplJni.get().isSyncAllowedByPlatform(mSyncServiceAndroidBridge);
}

@Override
public void setSyncAllowedByPlatform(boolean allowed) {
SyncServiceImplJni.get().setSyncAllowedByPlatform(mSyncServiceAndroidBridge, allowed);
}

@Override
public @PassphraseType int getPassphraseType() {
assert isEngineInitialized();
Expand Down Expand Up @@ -404,8 +394,6 @@ interface Natives {
boolean isSyncRequested(long nativeSyncServiceAndroidBridge);
void setSyncRequested(long nativeSyncServiceAndroidBridge, boolean requested);
boolean canSyncFeatureStart(long nativeSyncServiceAndroidBridge);
boolean isSyncAllowedByPlatform(long nativeSyncServiceAndroidBridge);
void setSyncAllowedByPlatform(long nativeSyncServiceAndroidBridge, boolean allowed);
boolean isSyncFeatureEnabled(long nativeSyncServiceAndroidBridge);
boolean isSyncFeatureActive(long nativeSyncServiceAndroidBridge);
boolean isSyncDisabledByEnterprisePolicy(long nativeSyncServiceAndroidBridge);
Expand Down
12 changes: 0 additions & 12 deletions chrome/browser/sync/android/sync_service_android_bridge.cc
Original file line number Diff line number Diff line change
Expand Up @@ -111,18 +111,6 @@ jboolean SyncServiceAndroidBridge::CanSyncFeatureStart(JNIEnv* env) {
return native_sync_service_->CanSyncFeatureStart();
}

jboolean SyncServiceAndroidBridge::IsSyncAllowedByPlatform(JNIEnv* env) {
DCHECK_CURRENTLY_ON(BrowserThread::UI);
return !native_sync_service_->HasDisableReason(
syncer::SyncService::DISABLE_REASON_PLATFORM_OVERRIDE);
}

void SyncServiceAndroidBridge::SetSyncAllowedByPlatform(JNIEnv* env,
jboolean allowed) {
DCHECK_CURRENTLY_ON(BrowserThread::UI);
native_sync_service_->SetSyncAllowedByPlatform(allowed);
}

jboolean SyncServiceAndroidBridge::IsSyncFeatureEnabled(JNIEnv* env) {
DCHECK_CURRENTLY_ON(BrowserThread::UI);
return native_sync_service_->IsSyncFeatureEnabled();
Expand Down
2 changes: 0 additions & 2 deletions chrome/browser/sync/android/sync_service_android_bridge.h
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,6 @@ class SyncServiceAndroidBridge : public syncer::SyncServiceObserver {
jboolean IsSyncRequested(JNIEnv* env);
void SetSyncRequested(JNIEnv* env, jboolean requested);
jboolean CanSyncFeatureStart(JNIEnv* env);
jboolean IsSyncAllowedByPlatform(JNIEnv* env);
void SetSyncAllowedByPlatform(JNIEnv* env, jboolean allowed);
jboolean IsSyncFeatureEnabled(JNIEnv* env);
jboolean IsSyncFeatureActive(JNIEnv* env);
jboolean IsSyncDisabledByEnterprisePolicy(JNIEnv* env);
Expand Down
12 changes: 0 additions & 12 deletions chrome/browser/ui/android/strings/android_chrome_strings.grd
Original file line number Diff line number Diff line change
Expand Up @@ -1701,9 +1701,6 @@ Your Google account may have other forms of browsing history like searches and a
</message>

<!-- Sync strings in account management dialog -->
<message name="IDS_SYNC_ANDROID_SYSTEM_SYNC_DISABLED" desc="Message to show when Android system-level sync flag is disabled">
Android system sync disabled
</message>
<message name="IDS_SYNC_ON" desc="The text is displayed as a gray subtitle of a settings item titled 'Sync' and indicates that sync is on.">
On
</message>
Expand Down Expand Up @@ -1806,9 +1803,6 @@ Your Google account may have other forms of browsing history like searches and a
<message name="IDS_RECENT_TABS_SYNC_PROMO_ENABLE_CHROME_SYNC" desc="Text that displayed in a textview encouraging the user to enable sync.">
To get your tabs from your other devices, turn on sync.
</message>
<message name="IDS_RECENT_TABS_SYNC_PROMO_ENABLE_ANDROID_SYNC" desc="Text that displayed in a textview encouraging the user to enable sync in the android settings.">
To get your tabs from your other devices, turn on “Auto-sync data” in Android account settings.
</message>
<message name="IDS_NTP_RECENT_TABS_SYNC_PROMO_INSTRUCTIONS" desc="Information about sync displayed on the NTP when the user has signed in on mobile but not on desktop">
Tabs that you've opened in Chrome on your other devices will appear here.
</message>
Expand Down Expand Up @@ -1931,9 +1925,6 @@ To change this setting, <ph name="BEGIN_LINK">&lt;resetlink&gt;</ph>reset sync<p
<message name="IDS_SYNC_NEEDS_VERIFICATION_TITLE" desc="Title of the error message shown when sync needs to verify the user.">
Sync needs to verify it's you
</message>
<message name="IDS_HINT_ANDROID_SYNC_DISABLED" desc="Hint message to resolve Android system sync is disabled error.">
Open Android settings and re-enable Android system sync to start Chrome sync
</message>
<message name="IDS_HINT_SYNC_AUTH_ERROR" desc="Hint message to resolve sync auth error.">
Sign in again to start sync
</message>
Expand Down Expand Up @@ -1963,9 +1954,6 @@ To change this setting, <ph name="BEGIN_LINK">&lt;resetlink&gt;</ph>reset sync<p
</message>

<!-- Sync error card button strings -->
<message name="IDS_ANDROID_SYNC_DISABLED_ERROR_CARD_BUTTON" desc="Button text for android sync disabled error in sync error cards.">
Enable Android system sync
</message>
<message name="IDS_CLIENT_OUT_OF_DATE_ERROR_CARD_BUTTON" desc="Button text for client out of date error in sync error cards.">
Update <ph name="PRODUCT_NAME">%1$s<ex>Chrome</ex></ph>
</message>
Expand Down

This file was deleted.

This file was deleted.

2 changes: 0 additions & 2 deletions components/sync/driver/fake_sync_service.cc
Original file line number Diff line number Diff line change
Expand Up @@ -65,8 +65,6 @@ bool FakeSyncService::HasObserver(const SyncServiceObserver* observer) const {

void FakeSyncService::StopAndClear() {}

void FakeSyncService::SetSyncAllowedByPlatform(bool allowed) {}

void FakeSyncService::OnDataTypeRequestsSyncStartup(ModelType type) {}

ModelTypeSet FakeSyncService::GetPreferredDataTypes() const {
Expand Down
1 change: 0 additions & 1 deletion components/sync/driver/fake_sync_service.h
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,6 @@ class FakeSyncService : public SyncService {
bool HasObserver(const SyncServiceObserver* observer) const override;
void OnDataTypeRequestsSyncStartup(ModelType type) override;
void StopAndClear() override;
void SetSyncAllowedByPlatform(bool allowed) override;
ModelTypeSet GetPreferredDataTypes() const override;
std::unique_ptr<SyncSetupInProgressHandle> GetSetupInProgressHandle()
override;
Expand Down
1 change: 0 additions & 1 deletion components/sync/driver/mock_sync_service.h
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,6 @@ class MockSyncService : public SyncService {
MOCK_METHOD(ModelTypeSet, GetPreferredDataTypes, (), (const override));
MOCK_METHOD(ModelTypeSet, GetActiveDataTypes, (), (const override));
MOCK_METHOD(void, StopAndClear, (), (override));
MOCK_METHOD(void, SetSyncAllowedByPlatform, (bool), (override));
MOCK_METHOD(void,
OnDataTypeRequestsSyncStartup,
(ModelType type),
Expand Down
2 changes: 0 additions & 2 deletions components/sync/driver/sync_internals_util.cc
Original file line number Diff line number Diff line change
Expand Up @@ -201,8 +201,6 @@ std::string GetDisableReasonsString(
return "None";
}
std::vector<std::string> reason_strings;
if (disable_reasons.Has(SyncService::DISABLE_REASON_PLATFORM_OVERRIDE))
reason_strings.push_back("Platform override");
if (disable_reasons.Has(SyncService::DISABLE_REASON_ENTERPRISE_POLICY))
reason_strings.push_back("Enterprise policy");
if (disable_reasons.Has(SyncService::DISABLE_REASON_NOT_SIGNED_IN))
Expand Down
21 changes: 8 additions & 13 deletions components/sync/driver/sync_service.h
Original file line number Diff line number Diff line change
Expand Up @@ -116,20 +116,19 @@ class SyncService : public KeyedService {
// Sync-the-transport might still start up even in the presence of (some)
// disable reasons. Meant to be used as a enum set.
enum DisableReason {
// Sync is disabled via platform-level override (e.g. Android's "MasterSync"
// toggle).
DISABLE_REASON_PLATFORM_OVERRIDE,
DISABLE_REASON_FIRST = DISABLE_REASON_PLATFORM_OVERRIDE,
// Sync is disabled by enterprise policy, either browser policy (through
// prefs) or account policy received from the Sync server.
DISABLE_REASON_ENTERPRISE_POLICY,
DISABLE_REASON_FIRST = DISABLE_REASON_ENTERPRISE_POLICY,
// Sync can't start because there is no authenticated user.
DISABLE_REASON_NOT_SIGNED_IN,
// Sync is suppressed by user choice, either via the feature toggle in
// Chrome settings (which exists on Android and iOS), a platform-level
// toggle (e.g. Android's "ChromeSync" toggle), or a “Reset Sync” operation
// from the dashboard. This is also set if there's simply no signed-in user
// (in addition to DISABLE_REASON_NOT_SIGNED_IN).
// Sync is suppressed by user choice, either by disabling all the data
// type toggles (*), or a “Reset Sync” operation from the dashboard. This is
// also set if there's simply no signed-in user (in addition to
// DISABLE_REASON_NOT_SIGNED_IN).
//
// (*) As of 01/2022, this is only true on mobile, where the logic was
// introduced as part of a migration (see crbug.com/1291946).
DISABLE_REASON_USER_CHOICE,
// Sync has encountered an unrecoverable error. It won't attempt to start
// again until either the browser is restarted, or the user fully signs out
Expand Down Expand Up @@ -322,10 +321,6 @@ class SyncService : public KeyedService {
// Sync-the-transport may remain active after calling this.
virtual void StopAndClear() = 0;

// Controls whether sync is allowed at the platform level. If set to false
// sync will be disabled with DISABLE_REASON_PLATFORM_OVERRIDE.
virtual void SetSyncAllowedByPlatform(bool allowed) = 0;

// Called when a datatype (SyncableService) has a need for sync to start
// ASAP, presumably because a local change event has occurred but we're
// still in deferred start mode, meaning the SyncableService hasn't been
Expand Down
Loading

0 comments on commit d18700c

Please sign in to comment.