Refactor metadata clearing in ClientTagBasedDataTypeProcessor [chromium/src : main]
Marc Treib (Gerrit)
Marc Treib added 1 comment![]( "Open in Gerrit")
pending_clear_metadata_ = false;This is a little bit awkward: `ShouldClearPersistedMetadata()` sounds like it shouldn't have side effects, but it does.
I think it's still slightly better overall, but we could also try to e.g. extract the "clear due to pending clear request" code path into a separate method from "clear due to invalid metadata". WDYT?
Related details
- Chromium Sync Reviews
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| 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):
mmosk...@google.com is from context(googleclient/chrome/chromium_gwsq/components/sync/config.gwsq)
Related details
- Maksim Moskvitin
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Maksim Moskvitin (Gerrit)
Maksim Moskvitin added 1 comment![]( "Open in Gerrit")
pending_clear_metadata_ = false;This is a little bit awkward: `ShouldClearPersistedMetadata()` sounds like it shouldn't have side effects, but it does.
I think it's still slightly better overall, but we could also try to e.g. extract the "clear due to pending clear request" code path into a separate method from "clear due to invalid metadata". WDYT?
Can we just clear it on the calling side? (regardless of the previous state / returned value)
Related details
- Marc Treib
Code-Coverage
Code-Owners
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
| Commit-Queue | +1 |
2 comments
Maksim MoskvitinThis is a little bit awkward: `ShouldClearPersistedMetadata()` sounds like it shouldn't have side effects, but it does.
I think it's still slightly better overall, but we could also try to e.g. extract the "clear due to pending clear request" code path into a separate method from "clear due to invalid metadata". WDYT?
Can we just clear it on the calling side? (regardless of the previous state / returned value)
Yes, that works. It's a bit awkward that the checking and the resetting are at different places then, but maybe it's better than what I had here. Done.
Related details
- Maksim Moskvitin
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |