Move away from using base::{Bind,Callback,Closure}() #8811
Merged
Conversation
This file contains 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
mariospr
changed the title
zenparsing
approved these changes
May 14, 2021
| virtual void GetPublisherMinVisitTime( | ||
| const GetPublisherMinVisitTimeCallback& callback) = 0; | ||
| GetPublisherMinVisitTimeCallback callback) = 0; |
There was a problem hiding this comment.
Thanks for these fixes!
yrliou
approved these changes
May 14, 2021
darkdh
approved these changes
May 14, 2021
simonhong
reviewed
May 15, 2021
simonhong
reviewed
May 18, 2021
| base::Bind( | ||
| &BraveRewardsNativeWorker::OnBalance, | ||
| weak_factory_.GetWeakPtr())); | ||
| rewards_service->FetchBalance(base::BindRepeating( |
There was a problem hiding this comment.
@mariospr can you check this file also? I found that all methods from RewardsService receives once callback type?
There was a problem hiding this comment.
Oops! Another instance of this, sorry. Fixed now too (+ reviewed again the PR and couldn't find more problems)
simonhong
approved these changes
May 18, 2021
mariospr
added a commit
that referenced
this pull request
May 18, 2021
This patch migrates from base::Bind() to base::BindRepeating(), and
from base::{Callback,Closure} to base::Repeating{Callback,Closure}
just to unblock the Cr92 rebase while the right fix is not yet, merged
to master (see #8811).
Chromium changes:
https://chromium.googlesource.com/chromium/src/+/6cc94b53393c1275843e43d3bbd6eda1f40fdd78
commit 6cc94b53393c1275843e43d3bbd6eda1f40fdd78
Author: Colin Blundell <blundell@chromium.org>
Date: Wed May 5 11:13:38 2021 +0000
[base::Bind conversion] Remove deprecated APIs
This CL removes the following deprecated APIs:
- base::Bind()
- base::Callback
- base::Closure
- base::CancelableCallback
- base::CancelableClosure
It also converts the remaining simple usages of these APIs that the
removal shook out.
The behavior that these APIs provided is still available the *Repeating*
variants. However, consider strongly whether using these variants is
actually necessary in your case or whether the *Once* variants will
suffice: unless your callback *objects* (note: not variables!) need to
be called multiple times, they most likely can and should be the Once
variants.
Bug: 714018
|
browser/net lgtm |
iefremov
reviewed
May 19, 2021
| @@ -636,6 +642,10 @@ void BraveRewardsAttestPromotionFunction::OnAttestPromotion( | |||
| Respond(TwoArguments(base::Value(static_cast<int>(result)), std::move(data))); | |||
| } | |||
|
|
|||
| BraveRewardsGetPendingContributionsTotalFunction:: | |||
| BraveRewardsGetPendingContributionsTotalFunction() | |||
| : weak_factory_(this) {} | |||
There was a problem hiding this comment.
functions are refcounted, so I gather we don't need weak pointers for them
There was a problem hiding this comment.
Good point. I'll remove this and also update the other pre-existing cases in that file.
iefremov
reviewed
May 20, 2021
| rewards_service->GetAnonWalletStatus(base::Bind( | ||
| &BraveRewardsGetAnonWalletStatusFunction::OnGetAnonWalletStatus, | ||
| this)); | ||
| rewards_service->GetAnonWalletStatus(base::BindOnce( |
There was a problem hiding this comment.
@mariospr this is not valid as is, just pass this as it was before. Bind will accept a refcounted pointer
There was a problem hiding this comment.
Oops! true... fixed now, sorry for the second overlook
base::{Bind,Callback,Closure}() have been deprecatedfor a while now,
are finally gone on Chromium 92, so it would be good to migrate all
current instances in the code (and stop introducing new ones) to the
Once/Repeating variants instead to easy further rebases.
This patch changes ALL instances of those old definitions and move
to using the Once variants whenever possible (as they are preferred)
and Repeating variants in every other case.
Last, this patch also makes some changes in brave_rewards_api.{h,cc}
to use weak pointers instead of base::Unretained() when binding
callbacks, for extra safety (and consistency with existing code).
[1] https://chromium-review.googlesource.com/c/chromium/src/+/2867526
Resolves brave/brave-browser#15855
bridiver
approved these changes
May 20, 2021
|
Thanks for the reviews everyone. Android and Mac bots failed with unrelated failures so merging |
23 tasks
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.
base::{Bind,Callback,Closure}() have been deprecatedfor a while now,
are finally gone on Chromium 92, so it would be good to migrate all
current instances in the code (and stop introducing new ones) to the
Once/Repeating variants instead to easy further rebases.
This patch changes ALL instances of those old definitions and move
to using the Once variants whenever possible (as they are preferred)
and Repeating variants in every other case.
Last, this patch also makes some changes in brave_rewards_api.{h,cc}
to use weak pointers instead of base::Unretained() when binding
callbacks, for extra safety (and consistency with existing code).
[1] https://chromium-review.googlesource.com/c/chromium/src/+/2867526
Resolves brave/brave-browser#15855
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