Rename `IsAccountStorageEnabled()` to `IsAccountStorageActive()` [chromium/src : main]
Thomas Thrainer (Gerrit)
Thomas Thrainer abandoned this change![]( "Open in Gerrit")
Related details
Code-Coverage
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thomas Thrainer (Gerrit)
Thomas Thrainer added 1 comment![]( "Open in Gerrit")
Hi Ioana!
I took a stab at letting Gemini make the renaming we talked about the other day. I cleaned up the changes and split them in a bunch of CLs, and I think this first one is ready for review now.
This review isn't time critical at all, I'll be OOO over the holidays anyways. But I wanted to share it already so you're aware that I started looking into this 😊.
Cheers & happy holidays,
Thomas
Related details
- Ioana Pandele
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ioana Pandele (Gerrit)
Ioana Pandele voted and added 3 comments![]( "Open in Gerrit")
Votes added by Ioana Pandele
| Code-Review | +1 |
3 comments
bool PasswordsPrivateDelegateImpl::IsAccountStorageEnabled() {Could you please file a bug for updating the names/intention of these methods too and add it to the fixit hotlist? There are multiple cases now in which a method called IsAccountStorageEnabled calls IsAccountStorageActive.
// signed-in user. This always returns false for sync-the-feature users andnit: Should we add a disclaimer that these users sync passwords from the profile store instead? Only because in combination with the paragraph below, it might sound like passwords are synced to the Google account only if IsAccountStorageActive is true.
Related details
- Thomas Thrainer
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. |
Thomas Thrainer (Gerrit)
Thomas Thrainer added 2 comments![]( "Open in Gerrit")
bool PasswordsPrivateDelegateImpl::IsAccountStorageEnabled() {Could you please file a bug for updating the names/intention of these methods too and add it to the fixit hotlist? There are multiple cases now in which a method called IsAccountStorageEnabled calls IsAccountStorageActive.
There's actually a whole list of follow-up CLs that I'm going to send your way after this one (https://crrev.com/c/7278259, https://crrev.com/c/7277269, https://crrev.com/c/7280345 and https://crrev.com/c/7280346). They cover this (and other) methods. I saw crbug.com/469954506 as the umbrella bug for all of those changes...
// signed-in user. This always returns false for sync-the-feature users andnit: Should we add a disclaimer that these users sync passwords from the profile store instead? Only because in combination with the paragraph below, it might sound like passwords are synced to the Google account only if IsAccountStorageActive is true.
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
gwsq (Gerrit)
Message from gwsq![]( "Open in Gerrit")
Reviewer source(s):
ankus...@google.com is from context(googleclient/chrome/chromium_gwsq/components/sync/config.gwsq)
Related details
- Alexis Hétu
- Ankush Singh
- Hiroshi Ichikawa
- Martin Šrámek
- Mohamed Amir Yosef
- Rafał Godlewski
- Sylvain Defresne
- Vasilii Sukhanov
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mohamed Amir Yosef (Gerrit)
Mohamed Amir Yosef voted and added 1 comment![]( "Open in Gerrit")
Votes added by Mohamed Amir Yosef
| Code-Review | +1 |
1 comment
Related details
- Alexis Hétu
- Ankush Singh
- Hiroshi Ichikawa
- Martin Šrámek
- Rafał Godlewski
- Sylvain Defresne
- Thomas Thrainer
- Vasilii Sukhanov
Alexis Hétu (Gerrit)
Alexis Hétu voted Code-Review+1![]( "Open in Gerrit")
Ankush Singh (Gerrit)
Ankush Singh added 2 comments![]( "Open in Gerrit")
Thanks!
// enabled and there are no sync errors preventing sync from working. Thus,Not true tbh. First, we're [not really waiting for sync to be active](https://source.chromium.org/chromium/chromium/src/+/main:components/password_manager/core/browser/features/password_manager_features_util.cc;drc=441c9d764ccc435d421116b52d1de9cb17b3fa03;l=60) to say that there are no sync errors. Second, even if we did, passwords data type could still have faced some error and be not active, in which case we should use [GetActiveDataTypes](https://source.chromium.org/chromium/chromium/src/+/main:components/sync/service/sync_service.h;drc=c100f7a290abb5edfa2e071e480926a15c80f549;l=433), which we're not doing here.
Based on the current implementation, `IsAccountStorageSelected` or tbh the original name feels better to me. WDYT?
Related details
- Hiroshi Ichikawa
- Martin Šrámek
- Rafał Godlewski
- Sylvain Defresne
- Thomas Thrainer
- Vasilii Sukhanov
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
Sylvain Defresne (Gerrit)
Sylvain Defresne voted and added 1 comment![]( "Open in Gerrit")
Votes added by Sylvain Defresne
| Code-Review | +1 |
1 comment
- Hiroshi Ichikawa
- Martin Šrámek
- Rafał Godlewski
- Thomas Thrainer
- Vasilii Sukhanov
Thomas Thrainer (Gerrit)
Thomas Thrainer added 1 comment![]( "Open in Gerrit")
// enabled and there are no sync errors preventing sync from working. Thus,Not true tbh. First, we're [not really waiting for sync to be active](https://source.chromium.org/chromium/chromium/src/+/main:components/password_manager/core/browser/features/password_manager_features_util.cc;drc=441c9d764ccc435d421116b52d1de9cb17b3fa03;l=60) to say that there are no sync errors. Second, even if we did, passwords data type could still have faced some error and be not active, in which case we should use [GetActiveDataTypes](https://source.chromium.org/chromium/chromium/src/+/main:components/sync/service/sync_service.h;drc=c100f7a290abb5edfa2e071e480926a15c80f549;l=433), which we're not doing here.
Based on the current implementation, `IsAccountStorageSelected` or tbh the original name feels better to me. WDYT?
Thanks for your feedback!
I chatted about this with treib@ before the holidays and summarise the rationale for the change in https://docs.google.com/document/d/1nUFA1uIzfiXLm0FxuNwgW8Y7MVkfjOmwgaippB7YoUo/edit. In particular, sync service has clear distinction between "enabled" and "active", see https://source.chromium.org/chromium/chromium/src/+/main:components/sync/service/sync_service.h;l=113;drc=c100f7a290abb5edfa2e071e480926a15c80f549.
This method first uses `sync_service->GetUserSettings()->GetSelectedTypes().Has(syncer::UserSelectableType::kPasswords)` to check whether password syncing is enabled. It also uses `IsUserEligibleForAccountStorage(...)`, which performs some checks on whether sync actually is in a syncing state and whether there are encryption related errors that prevent password syncing. I think those additional checks are confusing, because they clash quite clearly with sync's definition of "enabled" (which was also the main motivation of the renaming, see the doc above).
I agree that the method doesn't match sync's definition of "active" either, esp. while waiting for sync to become active. But IMO the behaviour is closer to "active" (i.e. checking for runtime state & encryption errors) than it is to "enabled". I would even argue that we should re-consider its implementation to see if it can be moved to `GetActiveDataTypes`.
I'd like to avoid keeping the current name, because for a feature that I want to implement I want to create a `IsAccountStorageEnabled` function that actually follows sync's definition of "enabled".
`IsAccountStorageSelected` IMO doesn't convey the fact that runtime state affects the result of the function. Also, it introduces "selected" as new state that readers have to wrap their head around, whereas "active" already mostly matches sync's definition (with a notable exception when starting). And I think that it's worthwhile to check if we could refactor this method to use `GetActiveDataTypes` eventually.
I've updated the comment to reflect the differences between "active" here and "active" in the sync sense, WDYT?
Related details
- Ankush Singh
- Hiroshi Ichikawa
- Martin Šrámek
- Rafał Godlewski
- Vasilii Sukhanov
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Martin Šrámek (Gerrit)
Martin Šrámek added 1 comment![]( "Open in Gerrit")
// TODO(crbug.com/470332074): Verify whether this should check for
// "enabled" instead of "active".I think this part should really be "enabled".
There are pieces of the browsing data code that care about sync being active, e.g. the part where we hold off on deleting google.com cookies to allow deletions to propagate via sync first.
But I don't think that's the case here. This code is just trying to delete data from all the relevant places. The `if()` statement is just making sure the storage exists before we make a deletion call on it.
```
if (storage) {
storage->DeleteData().
}
```
This seems to match the semantics of "enabled".
Related details
- Ankush Singh
- Hiroshi Ichikawa
- Rafał Godlewski
- Thomas Thrainer
- Vasilii Sukhanov
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
Ankush Singh (Gerrit)
Ankush Singh voted and added 2 comments![]( "Open in Gerrit")
Votes added by Ankush Singh
| Code-Review | +1 |
Thanks!
// enabled and there are no sync errors preventing sync from working. Thus,Thomas ThrainerNot true tbh. First, we're [not really waiting for sync to be active](https://source.chromium.org/chromium/chromium/src/+/main:components/password_manager/core/browser/features/password_manager_features_util.cc;drc=441c9d764ccc435d421116b52d1de9cb17b3fa03;l=60) to say that there are no sync errors. Second, even if we did, passwords data type could still have faced some error and be not active, in which case we should use [GetActiveDataTypes](https://source.chromium.org/chromium/chromium/src/+/main:components/sync/service/sync_service.h;drc=c100f7a290abb5edfa2e071e480926a15c80f549;l=433), which we're not doing here.
Based on the current implementation, `IsAccountStorageSelected` or tbh the original name feels better to me. WDYT?
Thanks for your feedback!
I chatted about this with treib@ before the holidays and summarise the rationale for the change in https://docs.google.com/document/d/1nUFA1uIzfiXLm0FxuNwgW8Y7MVkfjOmwgaippB7YoUo/edit. In particular, sync service has clear distinction between "enabled" and "active", see https://source.chromium.org/chromium/chromium/src/+/main:components/sync/service/sync_service.h;l=113;drc=c100f7a290abb5edfa2e071e480926a15c80f549.
This method first uses `sync_service->GetUserSettings()->GetSelectedTypes().Has(syncer::UserSelectableType::kPasswords)` to check whether password syncing is enabled. It also uses `IsUserEligibleForAccountStorage(...)`, which performs some checks on whether sync actually is in a syncing state and whether there are encryption related errors that prevent password syncing. I think those additional checks are confusing, because they clash quite clearly with sync's definition of "enabled" (which was also the main motivation of the renaming, see the doc above).
I agree that the method doesn't match sync's definition of "active" either, esp. while waiting for sync to become active. But IMO the behaviour is closer to "active" (i.e. checking for runtime state & encryption errors) than it is to "enabled". I would even argue that we should re-consider its implementation to see if it can be moved to `GetActiveDataTypes`.
I'd like to avoid keeping the current name, because for a feature that I want to implement I want to create a `IsAccountStorageEnabled` function that actually follows sync's definition of "enabled".
`IsAccountStorageSelected` IMO doesn't convey the fact that runtime state affects the result of the function. Also, it introduces "selected" as new state that readers have to wrap their head around, whereas "active" already mostly matches sync's definition (with a notable exception when starting). And I think that it's worthwhile to check if we could refactor this method to use `GetActiveDataTypes` eventually.
I've updated the comment to reflect the differences between "active" here and "active" in the sync sense, WDYT?
Ack, since this has been discussed with @tr...@chromium.org
I would even argue that we should re-consider its implementation to see if it can be moved to GetActiveDataTypes.
This might require an audit of the current usages. A lot or probably most of the usages might be like "does the user want to sync these passwords" instead of "are we syncing these passwords right now".
Another good name might have been `ShouldUseAccountStorage`, which I think fits well with the current usages? But no strong preference.
Related details
Thomas Thrainer (Gerrit)
Thomas Thrainer added 1 comment![]( "Open in Gerrit")
// TODO(crbug.com/470332074): Verify whether this should check for
// "enabled" instead of "active".I think this part should really be "enabled".
There are pieces of the browsing data code that care about sync being active, e.g. the part where we hold off on deleting google.com cookies to allow deletions to propagate via sync first.
But I don't think that's the case here. This code is just trying to delete data from all the relevant places. The `if()` statement is just making sure the storage exists before we make a deletion call on it.
```
if (storage) {
storage->DeleteData().
}
```This seems to match the semantics of "enabled".
Thanks for you comment! I added it to crbug.com/470332074 so we can follow up on that one eventually.
I don't want to change any behaviour in this CL (this is just a pure renaming), I hope that's OK with you.
Related details
- Hiroshi Ichikawa
- Martin Šrámek
- Rafał Godlewski
- Vasilii Sukhanov
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
Martin Šrámek (Gerrit)
Martin Šrámek voted and added 2 comments![]( "Open in Gerrit")
Votes added by Martin Šrámek
| Code-Review | +1 |
2 comments
browsing_data/ LGTM
// TODO(crbug.com/470332074): Verify whether this should check for
// "enabled" instead of "active".Thomas ThrainerI think this part should really be "enabled".
There are pieces of the browsing data code that care about sync being active, e.g. the part where we hold off on deleting google.com cookies to allow deletions to propagate via sync first.
But I don't think that's the case here. This code is just trying to delete data from all the relevant places. The `if()` statement is just making sure the storage exists before we make a deletion call on it.
```
if (storage) {
storage->DeleteData().
}
```This seems to match the semantics of "enabled".
Thanks for you comment! I added it to crbug.com/470332074 so we can follow up on that one eventually.
I don't want to change any behaviour in this CL (this is just a pure renaming), I hope that's OK with you.
In that case, please update the comment to "This should ... because ..." instead of "Verify if this should..." :)
Related details
- Hiroshi Ichikawa
- Rafał Godlewski
- Thomas Thrainer
- Vasilii Sukhanov
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
Rafał Godlewski (Gerrit)
Rafał Godlewski voted Code-Review+1![]( "Open in Gerrit")
- Hiroshi Ichikawa
- Thomas Thrainer
- Vasilii Sukhanov
Vasilii Sukhanov (Gerrit)
Vasilii Sukhanov voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Hiroshi Ichikawa
- Thomas Thrainer
Code-Coverage
Code-Owners