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

Support publisher branded site banners #939

Merged
merged 1 commit into from
Dec 4, 2018
Merged

Support publisher branded site banners #939

merged 1 commit into from
Dec 4, 2018

Conversation

emerick
Copy link
Contributor

@emerick emerick commented Nov 21, 2018 •
edited
Loading

Fixes brave/brave-browser#1923
Requires brave-intl/bat-native-ledger#184

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
    • Windows
    • macOS
    • Linux
  • Verified that these changes pass automated tests (npm test brave_unit_tests && npm test brave_browser_tests) on
    • Windows
    • macOS
    • Linux
  • 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

Test Plan:

  • Delete the current profile, to begin with a clean slate
  • Launch Brave
  • Enable Rewards
  • Visit http://yachtcaptain23.github.io/
  • Send a tip
  • Ensure that the site banner background and logo are set

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

@emerick emerick self-assigned this Nov 21, 2018
@emerick emerick mentioned this pull request Nov 21, 2018
Merged
@emerick emerick force-pushed the publisher-branded-site-banners branch 2 times, most recently from 65267a7 to bbd37f1 Compare November 22, 2018 12:14
@NejcZdovc NejcZdovc requested a review from bbondy December 3, 2018 18:23
@NejcZdovc NejcZdovc force-pushed the publisher-branded-site-banners branch from bbd37f1 to bf3a138 Compare December 4, 2018 05:25
@NejcZdovc NejcZdovc merged commit b0c4a7b into master Dec 4, 2018
@NejcZdovc NejcZdovc deleted the publisher-branded-site-banners branch December 4, 2018 05:28
@NejcZdovc
Copy link
Contributor

NejcZdovc commented Dec 4, 2018 •
edited
Loading

master (0.60) b0c4a7b
0.59.x 5eb086e
0.58.x 0c14bd2

NejcZdovc added a commit that referenced this pull request Dec 4, 2018
@NejcZdovc
Merge pull request #939 from brave/publisher-branded-site-banners
5eb086e 
Support publisher branded site banners
@NejcZdovc
Copy link
Contributor

@kjozwiak @rebron can we uplift this one to 58. Thank you

@kjozwiak
Copy link
Member

kjozwiak commented Dec 4, 2018

@NejcZdovc approved uplift to 0.58.x after deliberating with @rebron 👍Please update all the required labels and ensure that the appropriate issue(s) are moved to the correct milestones.

@NejcZdovc
Copy link
Contributor

@kjozwiak will do thx

NejcZdovc added a commit that referenced this pull request Dec 4, 2018
@NejcZdovc
Merge pull request #939 from brave/publisher-branded-site-banners
0c14bd2 
Support publisher branded site banners
@bbondy bbondy added this to the 0.58.x - Release milestone Jan 14, 2019
bridiver
bridiver reviewed Dec 6, 2023
View reviewed changes
browser/ui/webui/brave_rewards_source.cc
} // namespace

BraveRewardsSource::BraveRewardsSource(Profile* profile)
: profile_(profile->GetOriginalProfile()) {}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I know this is really old, but it's still in the code and we shouldn't be doing this. There is a method to "redirect" webuis to open in regular profiles which is used by settings, etc...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This isn't a WebUI though, it's a URLDataSource. Our code is based off of upstream's favicon data source implementation, which does a similar thing (though maybe that's also problematic): https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/webui/favicon_source.cc;l=71

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bridiver bridiver bridiver left review comments

@NejcZdovc NejcZdovc NejcZdovc approved these changes

@bbondy bbondy Awaiting requested review from bbondy

Assignees

@emerick emerick

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

Successfully merging this pull request may close these issues.

Support publisher branded banner
5 participants
@emerick @NejcZdovc @kjozwiak @bridiver @bbondy