Migrate brave ads from rapidjson to base::Value #14176
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cdesouza-chromium
force-pushed
the
brave-ads-base-value-refactor
branch
3 times, most recently
from
73056cb to
2638c82
Compare
cdesouza-chromium
force-pushed
the
brave-ads-base-value-refactor
branch
5 times, most recently
from
91ef2bb to
1485d1d
Compare
AlexeyBarabash
approved these changes
Jul 20, 2022
tmancey
reviewed
Jul 20, 2022
vendor/bat-native-ledger/src/bat/ledger/internal/legacy/bat_helper.cc
Outdated
Show resolved
Hide resolved
vendor/bat-native-ledger/src/bat/ledger/internal/legacy/bat_helper.cc
Outdated
Show resolved
Hide resolved
vendor/bat-native-ledger/src/bat/ledger/internal/legacy/bat_helper.cc
Outdated
Show resolved
Hide resolved
tmancey
requested changes
Jul 20, 2022
...r/bat-native-ads/src/bat/ads/internal/deprecated/confirmations/confirmation_state_manager.cc
Show resolved
Hide resolved
vendor/bat-native-ads/src/bat/ads/internal/deprecated/client/client_info.cc
Show resolved
Hide resolved
vendor/bat-native-ledger/src/bat/ledger/internal/legacy/bat_helper.cc
Outdated
Show resolved
Hide resolved
vendor/bat-native-ledger/src/bat/ledger/internal/legacy/bat_helper.cc
Outdated
Show resolved
Hide resolved
vendor/bat-native-ledger/src/bat/ledger/internal/legacy/client_properties.cc
Outdated
Show resolved
Hide resolved
tmancey
reviewed
Jul 20, 2022
tmancey
reviewed
Jul 20, 2022
tmancey
reviewed
Jul 20, 2022
tmancey
reviewed
Jul 20, 2022
tmancey
reviewed
Jul 20, 2022
tmancey
reviewed
Jul 20, 2022
...bat/ads/internal/resources/behavioral/purchase_intent/purchase_intent_signal_history_info.cc
Outdated
Show resolved
Hide resolved
vendor/bat-native-ledger/src/bat/ledger/internal/legacy/client_properties.cc
Outdated
Show resolved
Hide resolved
vendor/bat-native-ledger/src/bat/ledger/internal/legacy/client_properties.cc
Show resolved
Hide resolved
25 tasks
szilardszaloki
approved these changes
Jul 21, 2022
tmancey
reviewed
Jul 21, 2022
tmancey
reviewed
Jul 21, 2022
cdesouza-chromium
force-pushed
the
brave-ads-base-value-refactor
branch
from
0b48805 to
32eb339
Compare
This change removes the use of rapidjson in the types used by brave ads for serialisation/deserialisation. The overall model of how these types worked has been kept, with ToValue()/FromValue() pairs being introduced to replace the pervasive use of FromJson()/ToJson(), which was the common approach with rapidjson.
This change migrates the existing rapidjson code to use base::Value.
Replacing rapidjson with base:Value has caused a few tests to fail to recognise that the contents of the JSON object in question is still the same.
This has been triggered by the CI for some reason, so it is being submitted in a separate change for correction.
This change removes the use of deprecated base::Value methods in QrCodeData.
This change removes the use of deprecated classes/methods from base::Value in NotificationAdManager. It also replaces the use of pointers alongside boundaries where a reference for the specific type was more appropriate.
This change simplifies the implementation for StatementInfo, as well as removes the use of deprecated interfaces from base::Value.
This changes removes the use of base::ListValue, and base::DictionaryValue, as well as of the deprecated base::Value interface for ConfirmationStateManager, and all its associated dependencies, in favour of statically-typed interfaces.
This change removes the use of any deprecated methods/interfaces from base::Value in EpsilonGreedyBanditArmMap.
This change removes the use of rapidjson, and of all depreacted classes, and methods from base::Value, occuring in ClientProperty, and WalletInfoProperty. With the removal of rapidjson, it is not necessary anymore to have the parsing process decoupled from the actual data types.
The contents of these two classes have been moved to be part of the actual corresponding data structure they were used to parse. This also removes the use of rapidjson for serialisation/deserialisation. Additionally, base::Value modernisations have been applied to remove the use of any deprecated methods/interfaces.
For the types used by brave ads and ledger which we have to serialise, it is possible to have failure, although we don't expect to see it. This change adds a check to the result, so we may catch any cases in which it might be taking place.
cdesouza-chromium
force-pushed
the
brave-ads-base-value-refactor
branch
from
32eb339 to
ee39677
Compare
|
@cdesouza-chromium is there an uplift to |
|
@mkarolin I haven't put one in. Would it be necessary? |
|
@cdesouza-chromium cr104 will be uplifted to 1.42.x, I am seeing errors with base::DictionaryValue in AdsServiceImpl::OnGetInlineContentAd. Perhaps I can work around that particular one. |
|
@cdesouza-chromium got around it, no need to uplift. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This change removes the use of rapidjson in the types used by brave ads
for serialisation/deserialisation. The overall model of how these types
worked has been kept, with ToValue()/FromValue() pairs being introduced
to replace the pervasive use of FromJson()/ToJson(), which was the
common approach with rapidjson.
Resolves brave/brave-browser#21759
Submitter Checklist:
QA/YesorQA/No;release-notes/includeorrelease-notes/exclude;OS/...) to the associated issuenpm run test -- brave_browser_tests,npm run test -- brave_unit_tests,npm run lint,npm run gn_check,npm run tslintgit rebase master(if needed)Reviewer Checklist:
gnAfter-merge Checklist:
changes has landed on
Test Plan: