Refactor loadFromJson from RapidJSON to base::JSONReader

[tmancey]

Fixes brave/brave-browser#6351

Submitter Checklist:

• Submitted a ticket for my issue if one did not already exist.
• Used Github auto-closing keywords in the commit message.
• Added/updated tests for this change (for new code or code which already has tests).
• Verified that these changes build without errors on
  • Android
  • iOS
  • Linux
  • macOS
  • Windows
• Verified that these changes pass automated tests (unit, browser, security tests) on
  • iOS
  • Linux
  • macOS
  • Windows
• Verified that all lint errors/warnings are resolved (npm run lint)
• Ran git rebase master (if needed).
• Ran git rebase -i to squash commits (if needed).
• Tagged reviewers and labelled the pull request as needed.
• Request a security/privacy review as needed.
• Add appropriate QA labels (QA/Yes or QA/No) to include the closed issue in milestone
• Public documentation has been updated as necessary. For instance:
  • https://github.com/brave/brave-browser/wiki/Deviations-from-Chromium-(features-we-disable-or-remove)
  • https://github.com/brave/brave-browser/wiki/Proxy-redirected-URLs
  • https://github.com/brave/brave-browser/wiki/Fingerprinting-Protection-Mode
  • https://github.com/brave/brave-browser/wiki/Brave%E2%80%99s-Use-of-Referral-Codes
  • https://github.com/brave/brave-browser/wiki/Custom-Headers
  • https://github.com/brave/brave-browser/wiki/Web-compatibility-issues-with-tracking-protection
  • https://github.com/brave/brave-browser/wiki/QA-Guide

Test Plan:

• Confirm ledger state is persisted for upgrade paths
• Confirm ledger state is persisted for fresh installs

Reviewer Checklist:

• New files have MPL-2.0 license header.
• Request a security/privacy review as needed.
• Adequate test coverage exists to prevent regressions
• Verify test plan is specified in PR before merging to source

After-merge Checklist:

• The associated issue milestone is set to the smallest version that the
  changes has landed on.
• All relevant documentation has been updated.

[tmancey]

c465ab7 is legacy code just refactored to use new code/properties. Legacy code should not be considered

[masparrow]

👍

[NejcZdovc]

I would suggest that we don't add append Properties to every struck. It sounds redundant. We could just do CurrentReconcileProperties -> CurrentReconcile or if we are worried about collision put all properties in a separate namespace. Some names are real long because of that and require a lot of typing 😃 wdyt @tmancey ?

[tmancey]

I would suggest that we don't add append Properties to every struck. It sounds redundant. We could just do CurrentReconcileProperties -> CurrentReconcile or if we are worried about collision put all properties in a separate namespace. Some names are real long because of that and require a lot of typing 😃 wdyt @tmancey ?

@NejcZdovc Following style guide at https://google.github.io/styleguide/cppguide.html#Type_Names and also within the same BUILD.gn sources we cannot have the same class name so by having wallet_properties.cc and WalletProperties this resolves the issue. Also makes code clear what is a data structure and what is an object.

[NejcZdovc]

@tmancey that may be, but one thing to consider is that we will put everything that will be left after anonize removal into mojo, where we don't have *Properties. This will mean another big renaming refactor, which could be avoided in my opinion.

Also I think we need to have this two the same ledger::Transactions vs ledger::TransactionProperties. So adding Properties to the vector. Now it looks like they are different structs

[tmancey]

CI passed for all builds other than Windows. Rebuilding Windows

[gdregalo]

It looks like there is nothing special in this copy-constructor. Having it declared explicitly prevents generating of move operations.

[tmancey]

Complex class/struct needs an explicit out-of-line copy constructor as per Chromium style guide

[gdregalo]

I don't agree.
The document explains the style checked by Chromium Style Checker tool.
(https://www.chromium.org/developers/coding-style/chromium-style-checker-errors)
For classes/structs encapsulating other complex types the style requires to have explicit out-of-line constructor/copy-constructor/destructor only if it does non-trival initialization or copying. For other cases, including the one I mentioned above, we should stick with defaulted implementation.

Prefer to use =default:
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++-dos-and-donts.md#prefer-to-use

Also, initialize members in the declaration where possible:
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++-dos-and-donts.md#initialize-members-in-the-declaration-where-possible

[gdregalo]

Approving.
There are many cases when move operations are not generated for types like the one I mentioned in my comments.
When refactoring, please don't forget to default all trival constructors/copy-constructors/destructors and add move operations when necessary.