Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Refactor GetGrantCaptcha to use callback #2940

Merged
merged 2 commits into from
Jul 25, 2019
Merged

Refactor GetGrantCaptcha to use callback #2940

merged 2 commits into from
Jul 25, 2019

Conversation

jdkuki
Copy link
Contributor

@jdkuki jdkuki commented Jul 17, 2019

Resolves brave/brave-browser#5140

Submitter Checklist:

Test Plan:

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.

@jdkuki jdkuki self-assigned this Jul 17, 2019
@jdkuki jdkuki requested a review from NejcZdovc July 17, 2019 03:22
@jdkuki jdkuki force-pushed the callback-refactor-get-grant-captcha branch from 8e1a775 to 307dc13 Compare July 17, 2019 05:36
kylehickinson
kylehickinson requested changes Jul 17, 2019
View reviewed changes
vendor/brave-ios/Ledger/BATBraveLedger.mm
}

- (void)onGrantCaptcha:(const std::string &)image hint:(const std::string &)hint
{
if (self.grantCaptchaBlock) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can delete the grantCaptchaBlock @Property definition now that its no longer used.

Also can you just add a comment here saying this is unused, better than fully empty I guess 😛

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

vendor/brave-ios/Ledger/BATBraveLedger.mm Outdated
ledger->GetGrantCaptcha(headers);
ledger->GetGrantCaptcha(headers,
^(const std::string &image, const std::string &hint) {
completion([NSString stringWithUTF8String:image.c_str()],
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is grant captcha doing an asynchronous network call? I can't recall, but if it is, can we wrap this completion block in a dispatch_async to main thread as seen in other callbacks

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@jdkuki jdkuki force-pushed the callback-refactor-get-grant-captcha branch 2 times, most recently from 13be142 to 8584882 Compare July 17, 2019 17:27
emerick
emerick reviewed Jul 22, 2019
View reviewed changes
Copy link
Contributor

@emerick emerick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just a few comments.

components/services/bat_ledger/bat_ledger_impl.cc Outdated
// static
void BatLedgerImpl::OnGetGrantCaptcha(
CallbackHolder<GetGrantCaptchaCallback>* holder,
std::string image,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we make image and hint const reference parameters, e.g., const std::string& image?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

components/services/bat_ledger/bat_ledger_impl.cc Show resolved Hide resolved
components/services/bat_ledger/public/interfaces/bat_ledger.mojom Outdated
@@ -63,7 +63,7 @@ interface BatLedger {
int32 month, int32 year, uint32 data);

FetchGrants(string lang, string payment_id);
GetGrantCaptcha(array<string> headers);
GetGrantCaptcha(array<string> headers) =>(string image, string hint);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Should have a space after the arrow.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

vendor/bat-native-ledger/include/bat/ledger/ledger.h Outdated
@@ -50,6 +50,8 @@ using GetAddressesCallback =
using HasSufficientBalanceToReconcileCallback = std::function<void(bool)>;
using FetchBalanceCallback = std::function<void(ledger::Result,
ledger::BalancePtr)>;
using GetGrantCaptchaCallback = std::function<void(std::string,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment as above: let's use const std::string& for these parameters if possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

NejcZdovc
NejcZdovc reviewed Jul 22, 2019
View reviewed changes
vendor/bat-native-ledger/src/bat/ledger/internal/ledger_impl.cc Outdated
bat_grants_->GetGrantCaptcha(headers);
const std::vector<std::string>& headers,
ledger::GetGrantCaptchaCallback callback) const {
bat_grants_->GetGrantCaptcha(headers, std::move(callback));
}

void LedgerImpl::OnGrantCaptcha(const std::string& image,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please remove it as it's not needed anymore

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

@jdkuki jdkuki force-pushed the callback-refactor-get-grant-captcha branch from 8584882 to 3523a01 Compare July 22, 2019 21:33
emerick
emerick previously approved these changes Jul 23, 2019
View reviewed changes
Copy link
Contributor

@emerick emerick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

kylehickinson
kylehickinson previously approved these changes Jul 23, 2019
View reviewed changes
NejcZdovc
NejcZdovc reviewed Jul 23, 2019
View reviewed changes
vendor/bat-native-ledger/src/bat/ledger/internal/ledger_impl.cc

void LedgerImpl::OnGrantCaptcha(const std::string& image,
const std::string& hint) {
ledger_client_->OnGrantCaptcha(image, hint);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see this removed from ledger_client.h and then all other places. Do we use it somewhere else?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OnGrantCaptcha is the callback I am passing down from rewards_service to ledger

NejcZdovc
NejcZdovc suggested changes Jul 23, 2019
View reviewed changes
Copy link
Contributor

@NejcZdovc NejcZdovc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ledger_client.h check

@NejcZdovc NejcZdovc dismissed stale reviews from kylehickinson and emerick via d602fc4 July 23, 2019 14:54
@NejcZdovc NejcZdovc force-pushed the callback-refactor-get-grant-captcha branch from 3523a01 to d602fc4 Compare July 23, 2019 14:54
jdkuki
jdkuki commented Jul 23, 2019
View reviewed changes
components/brave_rewards/browser/rewards_service_impl.cc
@@ -1402,7 +1402,8 @@ void RewardsServiceImpl::GetGrantCaptcha(
headers.push_back("brave-product:brave-core");
headers.push_back("promotion-id:" + promotion_id);
headers.push_back("promotion-type:" + promotion_type);
bat_ledger_->GetGrantCaptcha(headers);
bat_ledger_->GetGrantCaptcha(headers,
base::BindOnce(&RewardsServiceImpl::OnGrantCaptcha, AsWeakPtr()));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OnGrantCaptcha used here

@jdkuki jdkuki force-pushed the callback-refactor-get-grant-captcha branch 3 times, most recently from 93b04ff to bf52714 Compare July 23, 2019 20:47
kylehickinson
kylehickinson previously approved these changes Jul 23, 2019
View reviewed changes
Copy link
Collaborator

@kylehickinson kylehickinson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ios++

@jdkuki jdkuki force-pushed the callback-refactor-get-grant-captcha branch 3 times, most recently from 884d028 to f251b28 Compare July 25, 2019 17:14
@jdkuki jdkuki force-pushed the callback-refactor-get-grant-captcha branch from f251b28 to 6393375 Compare July 25, 2019 17:33
kylehickinson
kylehickinson previously approved these changes Jul 25, 2019
View reviewed changes
NejcZdovc
NejcZdovc previously approved these changes Jul 25, 2019
View reviewed changes
@kylehickinson kylehickinson dismissed stale reviews from NejcZdovc and themself via 243bcde July 25, 2019 20:12
@jdkuki jdkuki merged commit 8ec66a6 into master Jul 25, 2019
@jdkuki jdkuki deleted the callback-refactor-get-grant-captcha branch July 25, 2019 20:13
@NejcZdovc NejcZdovc added this to the 0.70.x - Nightly milestone Jul 25, 2019
@kylehickinson kylehickinson mentioned this pull request Jul 30, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@NejcZdovc NejcZdovc NejcZdovc approved these changes

@kylehickinson kylehickinson kylehickinson approved these changes

@emerick emerick emerick left review comments

Assignees

@jdkuki jdkuki

Labels
feature/rewards
Projects
None yet
Milestone
0.69.x - Release
Development

Successfully merging this pull request may close these issues.

Add callback to GetGrantCaptcha
4 participants
@jdkuki @kylehickinson @emerick @NejcZdovc