[sync] Enable bookmarks by default on desktop depending on flags [chromium/src : main]
Mikel Astiz (Gerrit)
Mikel Astiz voted and added 3 comments![]( "Open in Gerrit")
Votes added by Mikel Astiz
| Commit-Queue | +1 |
3 comments
David Rogerdroger@: is this work on your radar? Is there an existing bug I can link the patch and TODO against?
Given than SiginPrefs was chosen to implement opt ins on desktop, we will now need to carefully migrate them all to SyncPrefs as done here for bookmarks.
(this patch started with a test-only change following your comment in a previous review, but I ran into this missing functionality)
Mikel AstizThank you for looking into this.
You can use https://crbug.com/424124636
I have a question (see my other comment).
Acknowledged
Thanks! (bots will confirm if this is fine, but tests pass locally at least)
.GetBookmarksExplicitBrowserSignin(gaia_id);Mikel AstizI think we do not generally force the preference to ON when signing in through a promo. We instead just reset it to the default value (which is ON).
I asked Amelie why it was done that way, and she said it was done to make rollback simpler.
Maybe we should have the same behavior in both cases? If so, which one is the right behavior?
This topic was indeed discussed with treib@ in https://chromium-review.googlesource.com/c/chromium/src/+/6308817/comment/8ea81371_360470b2/ and reportedly via vc too, so there must be good reasons for it.
A relevant requirement here could be that, for B4Bookmarks (and before kReplaceSyncPromosWithSignInPromos), the desire is that the opt-in (prefs::kExplicitBrowserSignin) gets cleared upon signout.
With the implementation I proposed originally here, I believe this requirement could break if kReplaceSyncPromosWithSignInPromos rolls out and then rolls back again. This is because [SyncPrefs::GetSelectedTypesForAccount() ignores](https://source.chromium.org/chromium/chromium/src/+/main:components/sync/service/sync_prefs.cc;l=290;drc=612cb630bd07b9c11393dc910998d9e113ad321d) prefs::kExplicitBrowserSignin if the value is explicitly populated (instead of cleared).
Changing this also means we would be modifying the behavior on mobile, but the risk is low IMO and it seems better than having platform-specific quirks.
This is what I did in patchset 9, together with a comment capturing the rationale above.
HOWEVER: I took a look at the PRD for desktop and noticed my assumption was wrong: the desired behavior is actually to apply the defaults regardless of prior state. From that PoV, the discussion in this thread is moot. I now updated the patch to disable all migration logic on desktop and enable bookmarks for all signed-in users.
Related details
- David Roger
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Roger (Gerrit)
David Roger voted and added 4 comments![]( "Open in Gerrit")
Votes added by David Roger
| Code-Review | +1 |
4 comments
kMigratedPart1ButNot2);Do this whole migration and steps make sense on desktop? Should we keep them, or bypass the entire migration on desktop?
.GetBookmarksExplicitBrowserSignin(gaia_id);Mikel AstizI think we do not generally force the preference to ON when signing in through a promo. We instead just reset it to the default value (which is ON).
I asked Amelie why it was done that way, and she said it was done to make rollback simpler.
Maybe we should have the same behavior in both cases? If so, which one is the right behavior?
This topic was indeed discussed with treib@ in https://chromium-review.googlesource.com/c/chromium/src/+/6308817/comment/8ea81371_360470b2/ and reportedly via vc too, so there must be good reasons for it.
A relevant requirement here could be that, for B4Bookmarks (and before kReplaceSyncPromosWithSignInPromos), the desire is that the opt-in (prefs::kExplicitBrowserSignin) gets cleared upon signout.
With the implementation I proposed originally here, I believe this requirement could break if kReplaceSyncPromosWithSignInPromos rolls out and then rolls back again. This is because [SyncPrefs::GetSelectedTypesForAccount() ignores](https://source.chromium.org/chromium/chromium/src/+/main:components/sync/service/sync_prefs.cc;l=290;drc=612cb630bd07b9c11393dc910998d9e113ad321d) prefs::kExplicitBrowserSignin if the value is explicitly populated (instead of cleared).
Changing this also means we would be modifying the behavior on mobile, but the risk is low IMO and it seems better than having platform-specific quirks.
This is what I did in patchset 9, together with a comment capturing the rationale above.
HOWEVER: I took a look at the PRD for desktop and noticed my assumption was wrong: the desired behavior is actually to apply the defaults regardless of prior state. From that PoV, the discussion in this thread is moot. I now updated the patch to disable all migration logic on desktop and enable bookmarks for all signed-in users.
Indeed, the plan on desktop is to enable all features to all signed in users, and show an IPH as notification. This is maybe different from what was done on mobile.
Given this context, the CL now does what I was expecting: only change the default value. Then we can just delete the SigninPref value once kReplaceSyncPromosWithSignInPromos is launched.
If you don't have any concerns with this approach this LGTM.
// On desktop, preferences should've been turned off in the account-scopedThis probably needs to be updated. Example:
// On desktop, preferences are not changed.
// Om desktop, after the migration, the types should be disabled.same here
Related details
- Mikel Astiz
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mikel Astiz (Gerrit)
Mikel Astiz added 4 comments![]( "Open in Gerrit")
Thanks! treib@ too for another pair of eyes.
kMigratedPart1ButNot2);Do this whole migration and steps make sense on desktop? Should we keep them, or bypass the entire migration on desktop?
I think we will be lucky if there's literally nothing to migrate.
Adding @tr...@chromium.org for overall review too and this topic specifically.
// On desktop, preferences should've been turned off in the account-scopedThis probably needs to be updated. Example:
// On desktop, preferences are not changed.
Done
// Om desktop, after the migration, the types should be disabled.Mikel Astizsame here
Done
Related details
- David Roger
- Marc Treib
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Marc Treib (Gerrit)
Marc Treib added 3 comments![]( "Open in Gerrit")
Initial response, haven't looked at the tests yet
base::FeatureList::IsEnabled(kReplaceSyncPromosWithSignInPromos) ||I've lost the overview of how all the various prefs and flags should work together on desktop...
Should we also check `prefs::kExplicitBrowserSignin` here?
Or does `kReplaceSyncPromosWithSignInPromos` basically deprecate `prefs::kExplicitBrowserSignin`? And in that case, should the pref checks for passwords and extensions also be ORed with a `kReplaceSyncPromosWithSignInPromos` flag check?
kMigratedPart1ButNot2);Mikel AstizDo this whole migration and steps make sense on desktop? Should we keep them, or bypass the entire migration on desktop?
I think we will be lucky if there's literally nothing to migrate.
Adding @tr...@chromium.org for overall review too and this topic specifically.
I don't entirely know how the whole user/settings migration will work on desktop, so I can't say whether there'll be anything to migrate. But for simplicity, I'd suggest keeping the method for now, even if it's all ifdef'd out. (IIUC, "Part2" is still needed on desktop, and it'd be more hassle/complication than it's worth to pull that out into a separate thing.)
Related details
- Mikel Astiz
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mikel Astiz (Gerrit)
Mikel Astiz added 3 comments![]( "Open in Gerrit")
Thanks!
base::FeatureList::IsEnabled(kReplaceSyncPromosWithSignInPromos) ||I've lost the overview of how all the various prefs and flags should work together on desktop...
Should we also check `prefs::kExplicitBrowserSignin` here?
Or does `kReplaceSyncPromosWithSignInPromos` basically deprecate `prefs::kExplicitBrowserSignin`? And in that case, should the pref checks for passwords and extensions also be ORed with a `kReplaceSyncPromosWithSignInPromos` flag check?
This is a good question!
I think you are right that we shouldn't assume kExplicitBrowserSignin is irrelevant, just in case. I will see if the code, as written here, would enable the types for implicit cases too and write a browser test for it, then fix if needed.
kMigratedPart1ButNot2);Mikel AstizDo this whole migration and steps make sense on desktop? Should we keep them, or bypass the entire migration on desktop?
Marc TreibI think we will be lucky if there's literally nothing to migrate.
Adding @tr...@chromium.org for overall review too and this topic specifically.
I don't entirely know how the whole user/settings migration will work on desktop, so I can't say whether there'll be anything to migrate. But for simplicity, I'd suggest keeping the method for now, even if it's all ifdef'd out. (IIUC, "Part2" is still needed on desktop, and it'd be more hassle/complication than it's worth to pull that out into a separate thing.)
I believe the main question is whether prefs::internal::kSyncToSigninMigrationState should be updated or not.
Related details
- Marc Treib
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Roger (Gerrit)
David Roger voted and added 1 comment![]( "Open in Gerrit")
Votes added by David Roger
| Code-Review | +1 |
1 comment
base::FeatureList::IsEnabled(kReplaceSyncPromosWithSignInPromos) ||I've lost the overview of how all the various prefs and flags should work together on desktop...
Should we also check `prefs::kExplicitBrowserSignin` here?
Or does `kReplaceSyncPromosWithSignInPromos` basically deprecate `prefs::kExplicitBrowserSignin`? And in that case, should the pref checks for passwords and extensions also be ORed with a `kReplaceSyncPromosWithSignInPromos` flag check?
Good catch, you're right that checking for kExplicitBrowserSignin would be more correct. So that would be something like (pseudo code):
(kExplicitBrowserSignin && kReplaceSyncPromosWithSignInPromos) ||
GetBookmarksExplicitBrowserSignin() ||
kEnableBookmarksSelectedTypeOnSigninForTesting
Maybe kExplicitBrowserSignin will be gone by the time we enable UNO Phase 2 (that requires migration of Dice users to UNO), but it's not guaranteed in any way.
Related details
- Marc Treib
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Marc Treib (Gerrit)
Marc Treib added 4 comments![]( "Open in Gerrit")
// `syncer::kReplaceSyncPromosWithSignInPromos.nit: Missing closing `
// `syncer::kReplaceSyncPromosWithSignInPromos.Same here
// got activated.
ASSERT_FALSE(AllowedTypesInStandaloneTransportMode().Has(
kDataTypeExcludedInTransportMode));nit: This is already CHECKed above, no need to assert it again
kMigratedPart1ButNot2);Mikel AstizDo this whole migration and steps make sense on desktop? Should we keep them, or bypass the entire migration on desktop?
Marc TreibI think we will be lucky if there's literally nothing to migrate.
Adding @tr...@chromium.org for overall review too and this topic specifically.
Mikel AstizI don't entirely know how the whole user/settings migration will work on desktop, so I can't say whether there'll be anything to migrate. But for simplicity, I'd suggest keeping the method for now, even if it's all ifdef'd out. (IIUC, "Part2" is still needed on desktop, and it'd be more hassle/complication than it's worth to pull that out into a separate thing.)
I believe the main question is whether prefs::internal::kSyncToSigninMigrationState should be updated or not.
If we keep the shape of the migration logic (with the two parts), then the pref does need to be updated, otherwise part 2 wouldn't run.
Related details
- Mikel Astiz
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mikel Astiz (Gerrit)
Mikel Astiz voted and added 6 comments![]( "Open in Gerrit")
Votes added by Mikel Astiz
| Commit-Queue | +1 |
6 comments
Sorry for the delay; this uncovered a couple of rabbit holes and I had to improve tests first before proceeding. PTAL!
// `syncer::kReplaceSyncPromosWithSignInPromos.Mikel Astiznit: Missing closing `
Obsolete.
// `syncer::kReplaceSyncPromosWithSignInPromos.Mikel AstizSame here
Done
// got activated.
ASSERT_FALSE(AllowedTypesInStandaloneTransportMode().Has(
kDataTypeExcludedInTransportMode));nit: This is already CHECKed above, no need to assert it again
Done
base::FeatureList::IsEnabled(kReplaceSyncPromosWithSignInPromos) ||David RogerI've lost the overview of how all the various prefs and flags should work together on desktop...
Should we also check `prefs::kExplicitBrowserSignin` here?
Or does `kReplaceSyncPromosWithSignInPromos` basically deprecate `prefs::kExplicitBrowserSignin`? And in that case, should the pref checks for passwords and extensions also be ORed with a `kReplaceSyncPromosWithSignInPromos` flag check?
Good catch, you're right that checking for kExplicitBrowserSignin would be more correct. So that would be something like (pseudo code):
(kExplicitBrowserSignin && kReplaceSyncPromosWithSignInPromos) ||
GetBookmarksExplicitBrowserSignin() ||
kEnableBookmarksSelectedTypeOnSigninForTestingMaybe kExplicitBrowserSignin will be gone by the time we enable UNO Phase 2 (that requires migration of Dice users to UNO), but it's not guaranteed in any way.
I've landed some related browser tests in https://crrev.com/c/6650987 and updated the logic here to consider the explicitness of signin.
kMigratedPart1ButNot2);Mikel AstizDo this whole migration and steps make sense on desktop? Should we keep them, or bypass the entire migration on desktop?
Marc TreibI think we will be lucky if there's literally nothing to migrate.
Adding @tr...@chromium.org for overall review too and this topic specifically.
Mikel AstizI don't entirely know how the whole user/settings migration will work on desktop, so I can't say whether there'll be anything to migrate. But for simplicity, I'd suggest keeping the method for now, even if it's all ifdef'd out. (IIUC, "Part2" is still needed on desktop, and it'd be more hassle/complication than it's worth to pull that out into a separate thing.)
Marc TreibI believe the main question is whether prefs::internal::kSyncToSigninMigrationState should be updated or not.
If we keep the shape of the migration logic (with the two parts), then the pref does need to be updated, otherwise part 2 wouldn't run.
- David Roger
- Marc Treib
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Roger (Gerrit)
David Roger added 1 comment![]( "Open in Gerrit")
syncer::kEnableBookmarksSelectedTypeOnSigninForTesting)},We need this flag until end of July, even on Canary. It was communicated to extension developpers to test the bookmarks API changes.
If we want to remove the base::Feature associated with it, could we somehow plug this flag on the new feature?
Related details
- Marc Treib
- Mikel Astiz
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mikel Astiz (Gerrit)
Mikel Astiz added 2 comments![]( "Open in Gerrit")
syncer::kEnableBookmarksSelectedTypeOnSigninForTesting)},We need this flag until end of July, even on Canary. It was communicated to extension developpers to test the bookmarks API changes.
If we want to remove the base::Feature associated with it, could we somehow plug this flag on the new feature?
- David Roger
- Marc Treib
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Roger (Gerrit)
David Roger voted and added 1 comment![]( "Open in Gerrit")
Votes added by David Roger
| Code-Review | +1 |
1 comment
syncer::kEnableBookmarksSelectedTypeOnSigninForTesting)},Mikel AstizWe need this flag until end of July, even on Canary. It was communicated to extension developpers to test the bookmarks API changes.
If we want to remove the base::Feature associated with it, could we somehow plug this flag on the new feature?
Good point, I wasn't aware of it. Reverted.
If we want to simplify the code further, we could connect this chrome flag internally to the kReplaceSyncPromosWithSignInPromos feature. That can be done later if needed.
Related details
- Marc Treib
- Mikel Astiz
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mikel Astiz (Gerrit)
Mikel Astiz added 1 comment![]( "Open in Gerrit")
syncer::kEnableBookmarksSelectedTypeOnSigninForTesting)},Mikel AstizWe need this flag until end of July, even on Canary. It was communicated to extension developpers to test the bookmarks API changes.
If we want to remove the base::Feature associated with it, could we somehow plug this flag on the new feature?
David RogerGood point, I wasn't aware of it. Reverted.
If we want to simplify the code further, we could connect this chrome flag internally to the kReplaceSyncPromosWithSignInPromos feature. That can be done later if needed.
Ack.
Related details
- Marc Treib
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Marc Treib (Gerrit)
Marc Treib added 1 comment![]( "Open in Gerrit")
kSyncEnableContactInfoDataTypeForCustomPassphraseUsers,
switches::kSyncEnableBookmarksInTransportMode,
#if !BUILDFLAG(IS_ANDROID)
syncer::
kReadingListEnableSyncTransportModeUponSignIn,
#endif // !BUILDFLAG(IS_ANDROID)
syncer::kReplaceSyncPromosWithSignInPromos},
/*disabled_features=*/{});
} else {
// TODO(crbug.com/424698545): The special-casing below for CromeOS may not
// be needed once the underlying bug is solved. Before that, enabling
// those flags on ChromeOS influences the behavior upon dashboard reset.
override_features_.InitWithFeatures(
#if BUILDFLAG(IS_ANDROID) || BUILDFLAG(IS_CHROMEOS)
/*enabled_features=*/{},
#else // BUILDFLAG(IS_ANDROID) || BUILDFLAG(IS_CHROMEOS)
/*enabled_features=*/
{switches::kSyncEnableBookmarksInTransportMode,
syncer::kReadingListEnableSyncTransportModeUponSignIn},
#endif // BUILDFLAG(IS_ANDROID) || BUILDFLAG(IS_CHROMEOS)I find all this pretty confusing. Why do these features need to be enabled/disabled based on the param ("UNOp2"), and the platform? Why is the bookmarks flag treated differently from the ReadingList one?
Related details
- Mikel Astiz
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mikel Astiz (Gerrit)
Mikel Astiz added 2 comments![]( "Open in Gerrit")
Thanks!
kSyncEnableContactInfoDataTypeForCustomPassphraseUsers,
switches::kSyncEnableBookmarksInTransportMode,
#if !BUILDFLAG(IS_ANDROID)
syncer::
kReadingListEnableSyncTransportModeUponSignIn,
#endif // !BUILDFLAG(IS_ANDROID)
syncer::kReplaceSyncPromosWithSignInPromos},
/*disabled_features=*/{});
} else {
// TODO(crbug.com/424698545): The special-casing below for CromeOS may not
// be needed once the underlying bug is solved. Before that, enabling
// those flags on ChromeOS influences the behavior upon dashboard reset.
override_features_.InitWithFeatures(
#if BUILDFLAG(IS_ANDROID) || BUILDFLAG(IS_CHROMEOS)
/*enabled_features=*/{},
#else // BUILDFLAG(IS_ANDROID) || BUILDFLAG(IS_CHROMEOS)
/*enabled_features=*/
{switches::kSyncEnableBookmarksInTransportMode,
syncer::kReadingListEnableSyncTransportModeUponSignIn},
#endif // BUILDFLAG(IS_ANDROID) || BUILDFLAG(IS_CHROMEOS)I find all this pretty confusing. Why do these features need to be enabled/disabled based on the param ("UNOp2"), and the platform? Why is the bookmarks flag treated differently from the ReadingList one?
I find all this pretty confusing. Why do these features need to be enabled/disabled based on the param ("UNOp2"),
The general idea is that UNOp2 is expected to roll out only after all UNOp1 are fully enabled. From that PoV is seems appropriate to explicitly enable P1 flags if GetParam()==true, which is done here.
The GetParam()==false (i.e. before phase 2) is a little unclear: we could either force disable all flags or lean on defaults. Here I have decided to use the most relevant scenario today, which is that phase 1 flags are enabled (but see next point below).
and the platform?
The main quirk is ChromeOS, which already has a TODO. Given that we don't immediately intend to roll out these UNOp1 flags on ChromeOS, it seems better to stick to the defaults.
Why is the bookmarks flag treated differently from the ReadingList one?
The reading list feature flag is ifdef-ed out on Android (i.e. always enabled), so it wouldn't build otherwise. See https://source.chromium.org/chromium/chromium/src/+/main:components/sync/base/features.cc;l=91;drc=429377981b2c1f9d30d1a04ad1abe6ca562d4b48
Related details
- Marc Treib
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Marc Treib (Gerrit)
Marc Treib added 1 comment![]( "Open in Gerrit")
return pref_service_->GetBoolean(::prefs::kExplicitBrowserSignin);Pre-existing, but for Sync-the-feature users, `IsExplicitBrowserSignin()` should probably also return true?
Related details
- Mikel Astiz
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mikel Astiz (Gerrit)
Mikel Astiz added 2 comments![]( "Open in Gerrit")
@tr...@chromium.org gentle ping, thanks.
return pref_service_->GetBoolean(::prefs::kExplicitBrowserSignin);Pre-existing, but for Sync-the-feature users, `IsExplicitBrowserSignin()` should probably also return true?
I believe this is already the case, e.g. https://source.chromium.org/chromium/chromium/src/+/main:components/signin/internal/identity_manager/primary_account_manager.cc;l=281;drc=b49c254c612709bbf6efee73667c827613f1876a
@dro...@chromium.org can you confirm?
Related details
- David Roger
- Marc Treib
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Roger (Gerrit)
David Roger added 1 comment![]( "Open in Gerrit")
return pref_service_->GetBoolean(::prefs::kExplicitBrowserSignin);Mikel AstizPre-existing, but for Sync-the-feature users, `IsExplicitBrowserSignin()` should probably also return true?
I believe this is already the case, e.g. https://source.chromium.org/chromium/chromium/src/+/main:components/signin/internal/identity_manager/primary_account_manager.cc;l=281;drc=b49c254c612709bbf6efee73667c827613f1876a
@dro...@chromium.org can you confirm?
- Marc Treib
- Mikel Astiz
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Marc Treib (Gerrit)
Marc Treib voted and added 2 comments![]( "Open in Gerrit")
Votes added by Marc Treib
| Code-Review | +1 |
2 comments
LGTM with an optional suggestion
Maybe part of what makes this hard to follow, is that it's IMO not really clear what the `GetParam()==false` state is supposed to represent - right now some (arbitrary?) phase 1 flags are enabled, but others are not (extensions, prefs, etc).
One idea to maybe simplify this a bit: Android doesn't even run the "phase 1" branch. So maybe we can do something like
```
#if BUILDFLAG(IS_ANDROID)
NOTREACHED();
#else
...
```
?
I might also help to create a wrapper around `GetParam()` with a better name.
Related details
- Mikel Astiz
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mikel Astiz (Gerrit)
Mikel Astiz voted and added 1 comment![]( "Open in Gerrit")
Votes added by Mikel Astiz
| Commit-Queue | +2 |
1 comment
kSyncEnableContactInfoDataTypeForCustomPassphraseUsers,
switches::kSyncEnableBookmarksInTransportMode,
#if !BUILDFLAG(IS_ANDROID)
syncer::
kReadingListEnableSyncTransportModeUponSignIn,
#endif // !BUILDFLAG(IS_ANDROID)
syncer::kReplaceSyncPromosWithSignInPromos},
/*disabled_features=*/{});
} else {
// TODO(crbug.com/424698545): The special-casing below for CromeOS may not
// be needed once the underlying bug is solved. Before that, enabling
// those flags on ChromeOS influences the behavior upon dashboard reset.
override_features_.InitWithFeatures(
#if BUILDFLAG(IS_ANDROID) || BUILDFLAG(IS_CHROMEOS)
/*enabled_features=*/{},
#else // BUILDFLAG(IS_ANDROID) || BUILDFLAG(IS_CHROMEOS)
/*enabled_features=*/
{switches::kSyncEnableBookmarksInTransportMode,
syncer::kReadingListEnableSyncTransportModeUponSignIn},
#endif // BUILDFLAG(IS_ANDROID) || BUILDFLAG(IS_CHROMEOS)Mikel AstizI find all this pretty confusing. Why do these features need to be enabled/disabled based on the param ("UNOp2"), and the platform? Why is the bookmarks flag treated differently from the ReadingList one?
Mikel AstizI find all this pretty confusing. Why do these features need to be enabled/disabled based on the param ("UNOp2"),
The general idea is that UNOp2 is expected to roll out only after all UNOp1 are fully enabled. From that PoV is seems appropriate to explicitly enable P1 flags if GetParam()==true, which is done here.
The GetParam()==false (i.e. before phase 2) is a little unclear: we could either force disable all flags or lean on defaults. Here I have decided to use the most relevant scenario today, which is that phase 1 flags are enabled (but see next point below).
and the platform?
The main quirk is ChromeOS, which already has a TODO. Given that we don't immediately intend to roll out these UNOp1 flags on ChromeOS, it seems better to stick to the defaults.
Why is the bookmarks flag treated differently from the ReadingList one?
The reading list feature flag is ifdef-ed out on Android (i.e. always enabled), so it wouldn't build otherwise. See https://source.chromium.org/chromium/chromium/src/+/main:components/sync/base/features.cc;l=91;drc=429377981b2c1f9d30d1a04ad1abe6ca562d4b48
Marc Treib@tr...@chromium.org gentle ping, thanks.
Maybe part of what makes this hard to follow, is that it's IMO not really clear what the `GetParam()==false` state is supposed to represent - right now some (arbitrary?) phase 1 flags are enabled, but others are not (extensions, prefs, etc).
One idea to maybe simplify this a bit: Android doesn't even run the "phase 1" branch. So maybe we can do something like
```
#if BUILDFLAG(IS_ANDROID)
NOTREACHED();
#else
...
```
?I might also help to create a wrapper around `GetParam()` with a better name.
If you don't mind, let me revisit this in the next patch, because some more changes are needed.
Related details
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
[sync] Enable bookmarks by default on desktop depending on flags
Namely, if `kReplaceSyncPromosWithSignInPromos` is enabled (it isn't
currently).
- M chrome/browser/sync/test/integration/single_client_standalone_transport_sync_test.cc
- M components/sync/service/sync_prefs.cc
- M components/sync/service/sync_prefs_unittest.cc
- M components/sync/service/sync_user_settings_impl_unittest.cc
Code-Review: +1 by David Roger, +1 by Marc Treib
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |