[Sync] Add user actionable error for bookmarks limit exceeded [chromium/src : main]
Michael Tatarski (Gerrit)
Michael Tatarski added 1 comment![]( "Open in Gerrit")
Hi Marc, it's just a WIP but could you glimpse over these files to tell me if I'm on the right track:
components/sync/service/data_type_manager_impl.cc
components/sync/service/sync_service_impl.cc
components/sync/service/sync_error.cc
components/sync/service/sync_service_impl.cc
Related details
- Marc Treib
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| 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")
Hi Marc, it's just a WIP but could you glimpse over these files to tell me if I'm on the right track:
components/sync/service/data_type_manager_impl.cc
components/sync/service/sync_service_impl.cc
components/sync/service/sync_error.cc
components/sync/service/sync_service_impl.cc
Yeah, overall approach seems okay. Some thoughts below.
</message>These strings are identical between the Chromium and GoogleChrome versions, right? Then there's a common settings_strings.grdp file where they should go.
// `MODEL_ERROR`.Hm, maybe in that case we should make the ctor private and introduce static factory functions instead? Like `CreateModelError`, `CreateConfigurationError`, etc. Then each can take exactly the params it needs.
(This is a detail though, not a high-level concern.)
}I'm not thrilled about putting so much data-type-specific logic into SyncService - ideally we'd find a way to put this logic closer to the bookmarks code.
Related details
- Michael Tatarski
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Tatarski (Gerrit)
Michael Tatarski voted and added 2 comments![]( "Open in Gerrit")
Votes added by Michael Tatarski
| Commit-Queue | +1 |
2 comments
These strings are identical between the Chromium and GoogleChrome versions, right? Then there's a common settings_strings.grdp file where they should go.
I'm not thrilled about putting so much data-type-specific logic into SyncService - ideally we'd find a way to put this logic closer to the bookmarks code.
True. I created a free function that encapsulates the logic for the bookmark related model errors. WDYT about it?
Related details
- Ankush Singh
- Marc Treib
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ankush Singh (Gerrit)
Ankush Singh added 7 comments![]( "Open in Gerrit")
Thanks! If possible, please split the current CL into atleast 3:
1. Model error to user actionable error part (maybe you can even skip introducing a new user actionable error in this CL since that'd require updating the switch statements).
2. Using the user actionable error on the UI side.
3. ios code.
WDYT?
<!-- Sync Page / Account Settings Page -->
<message name="IDS_SETTINGS_ERROR_BOOKMARKS_LIMIT_EXCEEDED_USER_ERROR_DESCRIPTION" desc="Error message to resolve the error that the bookmarks limit has been exceeded.">
To continue syncing, reduce the number of bookmarks
</message>
<message name="IDS_SETTINGS_BOOKMARKS_LIMIT_EXCEEDED_ERROR_BUTTON" desc="Label for a button that opens a help article about the bookmarks limit.">
Learn more
</message>I'll recommend we use some pre-existing strings for now, instead of introducing temporary strings.
return IDS_SETTINGS_BOOKMARKS_LIMIT_EXCEEDED_ERROR_BUTTON;Can you a TODO to update this later?
SyncStatusMessageType::kSyncError,
IDS_SETTINGS_ERROR_BOOKMARKS_LIMIT_EXCEEDED_USER_ERROR_DESCRIPTION,
IDS_SETTINGS_BOOKMARKS_LIMIT_EXCEEDED_ERROR_BUTTON,
IDS_SETTINGS_SIGN_OUT, SyncStatusActionType::kNoAction};Can you a TODO to update this later?
case syncer::SyncService::UserActionableError::kBookmarksLimitExceeded:Can you a TODO to update this later and other similar places in other files?
chrome::ShowSettingsSubPage(&browser(), chrome::kSyncSetupSubPage);Can you add a TODO to fix this to instead point to an HC article?
// User needs to manager their bookmarks.
kManageBookmarks,Can we add the ios logic in a separate CL?
Related details
- Michael Tatarski
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Marc Treib (Gerrit)
Marc Treib added 13 comments![]( "Open in Gerrit")
</message>Are the strings final? If not, it's best practice to add `translateable="false"` to avoid unnecessary translation churn.
// Returns the button text for a given sync error.nit: I think this comment doesn't really add anything; it just repeats the function name in more words
chrome::ShowSettingsSubPage(&browser(), chrome::kSyncSetupSubPage);Is this the right action? What can the user do on that page?
// `MODEL_ERROR`.Hm, maybe in that case we should make the ctor private and introduce static factory functions instead? Like `CreateModelError`, `CreateConfigurationError`, etc. Then each can take exactly the params it needs.
(This is a detail though, not a high-level concern.)
(still open)
// type specific errors.I guess we could introduce something like a `map<ModelError::Type, UserActionableError>` but that would only make sense once there's maybe 3 such errors. I'm not sure it's worth adding a TODO for this already, if there are no plans to address it in the foreseeable future. (One way to look at this: You can't close the bug until the TODO is addressed!)
Michael TatarskiI'm not thrilled about putting so much data-type-specific logic into SyncService - ideally we'd find a way to put this logic closer to the bookmarks code.
True. I created a free function that encapsulates the logic for the bookmark related model errors. WDYT about it?
SG for now. Ideally this can at some point move out of components/sync, when GetUserActionableError() moves to its own service, but until then this seems like more or less the best we can do.
MOCK_METHOD(void, ResetDataTypeErrors, (), (override));Does this work? ResetDataTypeErrors is private
You have reached the limit for the number of bookmarks. To add more, you’ll need to delete some.1) Please unify "chromium" and "google_chrome" strings since they're identical.
2) These are different from the desktop strings - is that intentional?
case syncer::SyncService::UserActionableError::kBookmarksLimitExceeded:I don't think this is really NOTREACHED?!
// User needs to manager their bookmarks.`manage`
case syncer::SyncService::UserActionableError::kNeedsClientUpgrade:
case syncer::SyncService::UserActionableError::kBookmarksLimitExceeded:
// UI not implemented for this case.Should it be? (Which UI does this correspond to?)
Similar for the instances below.
Michael Tatarski (Gerrit)
Michael Tatarski voted and added 14 comments![]( "Open in Gerrit")
Votes added by Michael Tatarski
| Commit-Queue | +1 |
Thanks! If possible, please split the current CL into atleast 3:
1. Model error to user actionable error part (maybe you can even skip introducing a new user actionable error in this CL since that'd require updating the switch statements).
2. Using the user actionable error on the UI side.
3. ios code.
WDYT?
Yes, it definitely makes sense.
<!-- Sync Page / Account Settings Page -->
<message name="IDS_SETTINGS_ERROR_BOOKMARKS_LIMIT_EXCEEDED_USER_ERROR_DESCRIPTION" desc="Error message to resolve the error that the bookmarks limit has been exceeded.">
To continue syncing, reduce the number of bookmarks
</message>
<message name="IDS_SETTINGS_BOOKMARKS_LIMIT_EXCEEDED_ERROR_BUTTON" desc="Label for a button that opens a help article about the bookmarks limit.">
Learn more
</message>I'll recommend we use some pre-existing strings for now, instead of introducing temporary strings.
Done
</message>Michael TatarskiAre the strings final? If not, it's best practice to add `translateable="false"` to avoid unnecessary translation churn.
Thanks
return IDS_SETTINGS_BOOKMARKS_LIMIT_EXCEEDED_ERROR_BUTTON;Can you a TODO to update this later?
Done
SyncStatusMessageType::kSyncError,
IDS_SETTINGS_ERROR_BOOKMARKS_LIMIT_EXCEEDED_USER_ERROR_DESCRIPTION,
IDS_SETTINGS_BOOKMARKS_LIMIT_EXCEEDED_ERROR_BUTTON,
IDS_SETTINGS_SIGN_OUT, SyncStatusActionType::kNoAction};Can you a TODO to update this later?
Done
case syncer::SyncService::UserActionableError::kBookmarksLimitExceeded:Can you a TODO to update this later and other similar places in other files?
Done
nit: I think this comment doesn't really add anything; it just repeats the function name in more words
Done
chrome::ShowSettingsSubPage(&browser(), chrome::kSyncSetupSubPage);Can you add a TODO to fix this to instead point to an HC article?
Done
chrome::ShowSettingsSubPage(&browser(), chrome::kSyncSetupSubPage);Is this the right action? What can the user do on that page?
No, it was just a placeholder, I added a TODO instead
// Copyright 2024 The Chromium AuthorsMichael Tatarski2025
Done
// Copyright 2024 The Chromium AuthorsMichael Tatarski2025
Done
// `MODEL_ERROR`.Marc TreibHm, maybe in that case we should make the ctor private and introduce static factory functions instead? Like `CreateModelError`, `CreateConfigurationError`, etc. Then each can take exactly the params it needs.
(This is a detail though, not a high-level concern.)
(still open)
Moved the discussion to the precursor CL. Therefore, I will resolve the suggestion.
I guess we could introduce something like a `map<ModelError::Type, UserActionableError>` but that would only make sense once there's maybe 3 such errors. I'm not sure it's worth adding a TODO for this already, if there are no plans to address it in the foreseeable future. (One way to look at this: You can't close the bug until the TODO is addressed!)
Acknowledged
Does this work? ResetDataTypeErrors is private
- Ankush Singh
- Marc Treib
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Marc Treib (Gerrit)
Marc Treib added 7 comments![]( "Open in Gerrit")
<message name="IDS_SETTINGS_ERROR_BOOKMARKS_LIMIT_EXCEEDED_USER_ERROR_DESCRIPTION" desc="When Chrome shows a toast notification after the user turned off the Enhanced Protection setting, this is the text on the toast letting the user know that the Enhanced Protection setting is off." translateable="false">Please update the description
<message name="IDS_SETTINGS_BOOKMARKS_LIMIT_EXCEEDED_ERROR_BUTTON" desc="When Chrome shows a toast notification after the user turned off the Enhanced Protection setting, this is the text on the toast letting the user know that the Enhanced Protection setting is off." translateable="false">Also here
// TODO(crbug.com/452968646): Update this when the string is finalized.Update what?
// TODO(crbug.com/452968646): Update this when the string is finalized.Update what?
IDS_SETTINGS_BOOKMARKS_LIMIT_EXCEEDED_ERROR_BUTTON,`button_string_id`?
// Only shown for "Sync-the-feature".This comment doesn't seem correct here? Certainly not for the new error code, but I think also already not for kUnrecoverableError. I think it refers to kNeedsSettingsConfirmation only?
// TODO(crbug.com/452968646): Update this when action is finalized.If you're planning to land an incomplete implementation, it should be behind a feature flag, so that we don't accidentally roll out a partial implementation.
Related details
- Ankush Singh
- Michael Tatarski
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Tatarski (Gerrit)
Michael Tatarski voted and added 7 comments![]( "Open in Gerrit")
Votes added by Michael Tatarski
| Commit-Queue | +1 |
7 comments
<message name="IDS_SETTINGS_ERROR_BOOKMARKS_LIMIT_EXCEEDED_USER_ERROR_DESCRIPTION" desc="When Chrome shows a toast notification after the user turned off the Enhanced Protection setting, this is the text on the toast letting the user know that the Enhanced Protection setting is off." translateable="false">Please update the description
Ankush suggested we should use some existing strings. We are still waiting for the actual strings that should be surfaced.
<message name="IDS_SETTINGS_BOOKMARKS_LIMIT_EXCEEDED_ERROR_BUTTON" desc="When Chrome shows a toast notification after the user turned off the Enhanced Protection setting, this is the text on the toast letting the user know that the Enhanced Protection setting is off." translateable="false">Also here
Same.
// TODO(crbug.com/452968646): Update this when the string is finalized.Michael TatarskiUpdate what?
Done
// TODO(crbug.com/452968646): Update this when the string is finalized.Michael TatarskiUpdate what?
Done
IDS_SETTINGS_BOOKMARKS_LIMIT_EXCEEDED_ERROR_BUTTON,Michael Tatarski`button_string_id`?
Done
// Only shown for "Sync-the-feature".This comment doesn't seem correct here? Certainly not for the new error code, but I think also already not for kUnrecoverableError. I think it refers to kNeedsSettingsConfirmation only?
Done
// TODO(crbug.com/452968646): Update this when action is finalized.If you're planning to land an incomplete implementation, it should be behind a feature flag, so that we don't accidentally roll out a partial implementation.
I'm not planning to land it. I just want to get the patch reviewed :) But thanks for the information, might be useful in the future.
Related details
- Ankush Singh
- Marc Treib
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| 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")
<message name="IDS_SETTINGS_ERROR_BOOKMARKS_LIMIT_EXCEEDED_USER_ERROR_DESCRIPTION" desc="When Chrome shows a toast notification after the user turned off the Enhanced Protection setting, this is the text on the toast letting the user know that the Enhanced Protection setting is off." translateable="false">Michael TatarskiPlease update the description
Ankush suggested we should use some existing strings. We are still waiting for the actual strings that should be surfaced.
use some existing strings
But that's not what you did..?
Introducing a new string, but with a mismatched description, definitely doesn't make sense.
@ankus...@google.com Why did you suggest not introducing new strings? If it's to avoid translation churn, that's IMO better achieved by "translateable=false".
Related details
- Ankush Singh
- Michael Tatarski
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ankush Singh (Gerrit)
Ankush Singh added 4 comments![]( "Open in Gerrit")
Thanks!
ModelError::Type::kModelLoadManagerDataTypeInFailedState));Do we plan to update https://source.chromium.org/chromium/chromium/src/+/main:components/sync/service/model_load_manager.cc;drc=c88d056b850747d4cae5e9e24222491efd85faf4;l=226 in a follow-up CL?
const std::map<DataType, SyncError> data_type_errors =
data_type_manager_->GetDataTypeErrors();
auto it = data_type_errors.find(BOOKMARKS);
if (it != data_type_errors.end() &&
IsBookmarksLimitExceededError(it->second)) {
return UserActionableError::kBookmarksLimitExceeded;
}+1 to Marc's comment about this being behind a feature flag. And second, we also want to show the error only if the user hasn't already "resolved" it: https://docs.google.com/document/d/1ykLyqhMV1aPNOHtAhGlT-nMrkJqwX23RfsjkeQIBhjg/edit?tab=t.0#bookmark=id.gx1z1nndmj5e (sorry, Googlers-only).
const std::map<DataType, SyncError> data_type_errors =
data_type_manager_->GetDataTypeErrors();
auto it = data_type_errors.find(BOOKMARKS);
if (it != data_type_errors.end() &&
IsBookmarksLimitExceededError(it->second)) {
return UserActionableError::kBookmarksLimitExceeded;
}Please add tests for this.
Related details
- Michael Tatarski
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ankush Singh (Gerrit)
Ankush Singh added 1 comment![]( "Open in Gerrit")
<message name="IDS_SETTINGS_ERROR_BOOKMARKS_LIMIT_EXCEEDED_USER_ERROR_DESCRIPTION" desc="When Chrome shows a toast notification after the user turned off the Enhanced Protection setting, this is the text on the toast letting the user know that the Enhanced Protection setting is off." translateable="false">Michael TatarskiPlease update the description
Marc TreibAnkush suggested we should use some existing strings. We are still waiting for the actual strings that should be surfaced.
use some existing strings
But that's not what you did..?
Introducing a new string, but with a mismatched description, definitely doesn't make sense.@ankus...@google.com Why did you suggest not introducing new strings? If it's to avoid translation churn, that's IMO better achieved by "translateable=false".
TBH I didn't know about translateable=false. If that's the recommended approach, then np!
Related details
- Michael Tatarski
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ankush Singh (Gerrit)
Ankush Singh added 1 comment![]( "Open in Gerrit")
Bug: 452968646Please use crbug.com/452813642 as the tracking bug, the other one linked is not in Chromium bug tracker. Same for the other CLs you're sending out. Thanks!
Michael Tatarski (Gerrit)
Michael Tatarski voted and added 9 comments![]( "Open in Gerrit")
Votes added by Michael Tatarski
| Commit-Queue | +1 |
9 comments
Bug: 452968646Please use crbug.com/452813642 as the tracking bug, the other one linked is not in Chromium bug tracker. Same for the other CLs you're sending out. Thanks!
Thank you, I used crbug.com/452968646 instead since it is more descriptive and closed the one you mentioned as a duplicate.
<message name="IDS_SETTINGS_ERROR_BOOKMARKS_LIMIT_EXCEEDED_USER_ERROR_DESCRIPTION" desc="When Chrome shows a toast notification after the user turned off the Enhanced Protection setting, this is the text on the toast letting the user know that the Enhanced Protection setting is off." translateable="false">Michael TatarskiPlease update the description
Marc TreibAnkush suggested we should use some existing strings. We are still waiting for the actual strings that should be surfaced.
Ankush Singhuse some existing strings
But that's not what you did..?
Introducing a new string, but with a mismatched description, definitely doesn't make sense.@ankus...@google.com Why did you suggest not introducing new strings? If it's to avoid translation churn, that's IMO better achieved by "translateable=false".
TBH I didn't know about translateable=false. If that's the recommended approach, then np!
Done
<message name="IDS_SETTINGS_BOOKMARKS_LIMIT_EXCEEDED_ERROR_BUTTON" desc="When Chrome shows a toast notification after the user turned off the Enhanced Protection setting, this is the text on the toast letting the user know that the Enhanced Protection setting is off." translateable="false">Michael TatarskiAlso here
Same.
Done
ModelError::Type::kModelLoadManagerDataTypeInFailedState));Do we plan to update https://source.chromium.org/chromium/chromium/src/+/main:components/sync/service/model_load_manager.cc;drc=c88d056b850747d4cae5e9e24222491efd85faf4;l=226 in a follow-up CL?
It was adjusted in the previous CL such that the actual model error is propagated. :)
const std::map<DataType, SyncError> data_type_errors =
data_type_manager_->GetDataTypeErrors();
auto it = data_type_errors.find(BOOKMARKS);
if (it != data_type_errors.end() &&
IsBookmarksLimitExceededError(it->second)) {
return UserActionableError::kBookmarksLimitExceeded;
}Please add tests for this.
Done
const std::map<DataType, SyncError> data_type_errors =
data_type_manager_->GetDataTypeErrors();
auto it = data_type_errors.find(BOOKMARKS);
if (it != data_type_errors.end() &&
IsBookmarksLimitExceededError(it->second)) {
return UserActionableError::kBookmarksLimitExceeded;
}+1 to Marc's comment about this being behind a feature flag. And second, we also want to show the error only if the user hasn't already "resolved" it: https://docs.google.com/document/d/1ykLyqhMV1aPNOHtAhGlT-nMrkJqwX23RfsjkeQIBhjg/edit?tab=t.0#bookmark=id.gx1z1nndmj5e (sorry, Googlers-only).
Done
case syncer::SyncService::UserActionableError::kBookmarksLimitExceeded:Michael TatarskiI don't think this is really NOTREACHED?!
Done
// User needs to manager their bookmarks.Michael Tatarski`manage`
Done
// User needs to manager their bookmarks.
kManageBookmarks,Michael TatarskiCan we add the ios logic in a separate CL?
Done
Related details
- Ankush Singh
- Marc Treib
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Marc Treib (Gerrit)
Marc Treib added 8 comments![]( "Open in Gerrit")
NOTREACHED();Out of curiosity: Why add this NOTREACHED? I think we usually don't add it after a `switch` with `return`s - e.g. `GetAvatarSyncErrorLabelsForSettings` below doesn't have it.
base::UTF8ToUTF16(user_email));
case syncer::SyncService::UserActionableError::kNeedsSettingsConfirmation:
case syncer::SyncService::UserActionableError::kUnrecoverableError:
case syncer::SyncService::UserActionableError::kBookmarksLimitExceeded:This is changing the string for these existing errors
case syncer::SyncService::UserActionableError::kUnrecoverableError:
case syncer::SyncService::UserActionableError::kBookmarksLimitExceeded:
command_id = IDC_SHOW_SYNC_SETTINGS;Is this the correct action for the new error? If not, please add a separate case for it and add a TODO.
// TODO(crbug.com/452968646): Update this to the correct help center link.`CHECK(base::FeatureList::IsEnabled(kSyncShowBookmarksLimitExceededError));`?
// TODO(crbug.com/452968646): Update this when the string is finalized.This is just a debug string, I don't think it needs to be updated.
void ClearBookmarksLimitExceededErrorAcknowledged();Do we actually need this? AFAIK the PRD doesn't contain anything about acknowledging/silencing the error.
virtual void AcknowledgeBookmarksLimitExceededError() = 0;If we need this API at all (see other comment), it should probably be on `SyncUserSettings` rather than on the service directly.
// GetUserActionableError() will not return kBookmarksLimitExceeded until the
// next browser restart.If it's just until the next restart, why persist it in prefs?
Related details
- Ankush Singh
- Michael Tatarski
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Tatarski (Gerrit)
Michael Tatarski voted and added 9 comments![]( "Open in Gerrit")
Votes added by Michael Tatarski
| Commit-Queue | +1 |
9 comments
Out of curiosity: Why add this NOTREACHED? I think we usually don't add it after a `switch` with `return`s - e.g. `GetAvatarSyncErrorLabelsForSettings` below doesn't have it.
Done
base::UTF8ToUTF16(user_email));
case syncer::SyncService::UserActionableError::kNeedsSettingsConfirmation:
case syncer::SyncService::UserActionableError::kUnrecoverableError:
case syncer::SyncService::UserActionableError::kBookmarksLimitExceeded:This is changing the string for these existing errors
Done
case syncer::SyncService::UserActionableError::kUnrecoverableError:
case syncer::SyncService::UserActionableError::kBookmarksLimitExceeded:
command_id = IDC_SHOW_SYNC_SETTINGS;Is this the correct action for the new error? If not, please add a separate case for it and add a TODO.
Done
// TODO(crbug.com/452968646): Update this to the correct help center link.Michael Tatarski`CHECK(base::FeatureList::IsEnabled(kSyncShowBookmarksLimitExceededError));`?
Done
// TODO(crbug.com/452968646): Update this when the string is finalized.This is just a debug string, I don't think it needs to be updated.
Done
Do we actually need this? AFAIK the PRD doesn't contain anything about acknowledging/silencing the error.
As discussed via chat, there is a specific section mentioning that the error can be silenced:
virtual void AcknowledgeBookmarksLimitExceededError() = 0;If we need this API at all (see other comment), it should probably be on `SyncUserSettings` rather than on the service directly.
Done
// GetUserActionableError() will not return kBookmarksLimitExceeded until the
// next browser restart.If it's just until the next restart, why persist it in prefs?
Yeah, true. I now saved in memory within a boolean flag. This should be fine, correct?
Related details
- Ankush Singh
- Marc Treib
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Marc Treib (Gerrit)
Marc Treib added 2 comments![]( "Open in Gerrit")
command_id = IDC_HELP_PAGE_VIA_MENU;Is this the right ID? It's already used elsewhere in this class for a different purpose.
prefs_->AcknowledgeBookmarksLimitExceededError();Can we store the flag directly in SyncUserSettings? I don't see a reason to go to SyncPrefs now (and it seems somewhat misleading to store in-memory-only state in SyncPrefs).
Related details
- Ankush Singh
- Michael Tatarski
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ankush Singh (Gerrit)
Ankush Singh added 6 comments![]( "Open in Gerrit")
Thanks!
command_id = IDC_HELP_PAGE_VIA_MENU;
icon = &vector_icons::kErrorOutlineIcon;Let's implement this in a separate CL?
CHECK(base::FeatureList::IsEnabled(
syncer::kSyncShowBookmarksLimitExceededError));
syncer::SyncService* const service =
SyncServiceFactory::GetForProfile(browser_->profile());
if (service && service->GetUserActionableError() ==
syncer::SyncService::UserActionableError::
kBookmarksLimitExceeded) {
service->GetUserSettings()->AcknowledgeBookmarksLimitExceededError();
}Let's implement in a separate CL?
std::map<DataType, SyncError> GetDataTypeErrors() const override;very optional nit: use `DataTypeStatusTable::TypeErrorMap` instead.
bool bookmarks_limit_exceeded_error_acknowledged_ = false;I don't think this has anything to do with SyncPrefs? Same for the above methods.
One idea can be that we introduce a class like BookmarkLimitErrorHandler - or maybe we can thinking about giving it a more generic name (which is ideally better tbh)
WDYT?
virtual void AcknowledgeBookmarksLimitExceededError() = 0;Same as commented for SyncPrefs, this doesn't really have anything to do with the sync user settings right?
Related details
- Michael Tatarski
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ankush Singh (Gerrit)
Ankush Singh added 1 comment![]( "Open in Gerrit")
Let's implement the actions part in follow-up CLs. Thanks!
Marc Treib (Gerrit)
Marc Treib added 1 comment![]( "Open in Gerrit")
bool bookmarks_limit_exceeded_error_acknowledged_ = false;I don't think this has anything to do with SyncPrefs? Same for the above methods.
One idea can be that we introduce a class like BookmarkLimitErrorHandler - or maybe we can thinking about giving it a more generic name (which is ideally better tbh)
WDYT?
Certainly not related to prefs; I commented something similar.
Would you introduce a new class basically just to store this one bool? I guess it could at least then also contain the "is bookmarks limit error" logic...
Who would own it?
Ankush Singh (Gerrit)
Ankush Singh added 2 comments![]( "Open in Gerrit")
Thanks!
bool bookmarks_limit_exceeded_error_acknowledged_ = false;Marc TreibI don't think this has anything to do with SyncPrefs? Same for the above methods.
One idea can be that we introduce a class like BookmarkLimitErrorHandler - or maybe we can thinking about giving it a more generic name (which is ideally better tbh)
WDYT?
Certainly not related to prefs; I commented something similar.
Would you introduce a new class basically just to store this one bool? I guess it could at least then also contain the "is bookmarks limit error" logic...
Who would own it?
SyncService should own that.
Would you introduce a new class basically just to store this one bool? I guess it could at least then also contain the "is bookmarks limit error" logic...
Yes, that will encapsulate the different logic we have spread out: maintaining this bool, clearing it, the logic in bookmark_sync_error_util.h. It might be even better if we can name it DataTypeErrorHandler to make it more generic but that needs more thought.