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.

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

Support priority for Brave Ads #5663

Merged
merged 1 commit into from Jun 3, 2020
Merged

Support priority for Brave Ads #5663

merged 1 commit into from Jun 3, 2020

Conversation

@tmancey
Copy link
Collaborator

tmancey commented May 26, 2020

Resolves brave/brave-browser#5232

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.
@tmancey tmancey added the feature/ads label May 26, 2020
@tmancey tmancey requested review from moritzhaller, emerick and NejcZdovc May 26, 2020
@tmancey tmancey self-assigned this May 26, 2020
Copy link
Contributor

emerick left a comment

LGTM

@tmancey tmancey force-pushed the issues/5232 branch from f60b880 to 064ebee May 27, 2020
Copy link
Collaborator

moritzhaller left a comment

LGTM!

FailedToServeAdNotification("Pacing ad delivery");
return;
}

ShowAdNotification(ad);

This comment has been minimized.

Copy link
@moritzhaller

moritzhaller May 27, 2020

Collaborator

not sure if I read this correctly but generally speaking - since this is applied after picking the eligible ad we basically reduce the chance of an ad opportunity being filled proportional to the priorities in the catalog. Do we have a sense of how much this will reduce ad impressions overall?

This comment has been minimized.

Copy link
@tmancey

tmancey Jun 2, 2020

Author Collaborator

Discussed in DM and good to go

@tmancey tmancey force-pushed the issues/5232 branch 11 times, most recently from 7b79405 to 58318a2 May 27, 2020
@tmancey tmancey force-pushed the issues/5232 branch from 58318a2 to e147835 Jun 2, 2020
@tmancey tmancey merged commit 09e9d35 into master Jun 3, 2020
3 checks passed
3 checks passed
SonarCloud Code Analysis Quality Gate passed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@tmancey tmancey deleted the issues/5232 branch Jun 3, 2020
@tmancey tmancey added this to the 1.11.x - Nightly milestone Jun 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants
You can’t perform that action at this time.