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 ActivityInfoFilter #2875

Merged
merged 1 commit into from
Jul 25, 2019
Merged

Refactor ActivityInfoFilter #2875

merged 1 commit into from
Jul 25, 2019

Conversation

jdkuki
Copy link
Contributor

@jdkuki jdkuki commented Jul 5, 2019 •
edited
Loading

Submitter Checklist:

Resolves brave/brave-browser#5129

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 added CI/skip-android Do not run CI builds for Android CI/skip-linux CI/skip-macos-x64 Do not run CI builds for macOS x64 labels Jul 6, 2019
@jdkuki jdkuki force-pushed the activity-info-filter-refactor branch from 1418f24 to 9960120 Compare July 6, 2019 20:06
@jdkuki jdkuki changed the title Refactor ActivityInfoFilter WIP Refactor ActivityInfoFilter Jul 6, 2019
@jdkuki jdkuki marked this pull request as ready for review July 6, 2019 20:14
@jdkuki jdkuki self-assigned this Jul 6, 2019
@jdkuki jdkuki force-pushed the activity-info-filter-refactor branch 2 times, most recently from 93c94b9 to 0a9fa3d Compare July 8, 2019 20:08
@jdkuki jdkuki added the CI/skip Do not run CI builds (except noplatform) label Jul 9, 2019
@jdkuki jdkuki force-pushed the activity-info-filter-refactor branch 8 times, most recently from e569a80 to 93f5ecc Compare July 9, 2019 22:46
@jdkuki jdkuki removed CI/skip Do not run CI builds (except noplatform) CI/skip-android Do not run CI builds for Android CI/skip-linux CI/skip-macos-x64 Do not run CI builds for macOS x64 labels Jul 9, 2019
@jdkuki jdkuki force-pushed the activity-info-filter-refactor branch from 93f5ecc to 9be4e17 Compare July 9, 2019 22:47
@jdkuki jdkuki changed the title WIP Refactor ActivityInfoFilter Refactor ActivityInfoFilter Jul 9, 2019
@jdkuki jdkuki assigned NejcZdovc and unassigned NejcZdovc Jul 9, 2019
kylehickinson
kylehickinson reviewed Jul 10, 2019
View reviewed changes
vendor/brave-ios/tests/ledger_database_test.mm Outdated
@@ -656,24 +656,24 @@ - (void)testPublishersWithFiltersExcluded
const auto filter = [[BATActivityInfoFilter alloc] init];

// Filter all - skip exclude filtering
filter.excluded = BATExcludeFilterFilterAll;
filter.excluded = BATEXCLUDE_FILTERFilterAll;
Copy link
Collaborator

@kylehickinson kylehickinson Jul 10, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

This naming is unfortunate 😅 I will have to fix the formatting for Obj-C generated enum names, or we can stop using all caps snake case for enums names in mojo? It seems like it's a bit inconsistent atm: https://github.com/brave/brave-core/pull/2889/files#diff-99c488f19578bfd75399396011e2db1bR99

@NejcZdovc You have any preference?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@kylehickinson I have spoken with @NejcZdovc and for now this is ok and we will raise a ticket for consistency

Copy link
Collaborator

Choose a reason for hiding this comment

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

Gotcha, I will push a mojo gen fix to this branch that refactors the all caps snake case then

@jdkuki jdkuki force-pushed the activity-info-filter-refactor branch from 3cc6fa6 to 155f0e8 Compare July 11, 2019 04:32
tmancey
tmancey requested changes Jul 22, 2019
View reviewed changes
vendor/bat-native-ledger/src/bat/ledger/internal/bat_publishers.cc Outdated Show resolved Hide resolved
components/brave_rewards/browser/rewards_service_impl.cc Outdated Show resolved Hide resolved
vendor/bat-native-ledger/src/bat/ledger/internal/bat_publishers.cc Outdated Show resolved Hide resolved
vendor/bat-native-ledger/src/bat/ledger/internal/bat_publishers.cc Outdated Show resolved Hide resolved
vendor/brave-ios/Ledger/BATBraveLedger.mm Outdated Show resolved Hide resolved
components/brave_rewards/browser/publisher_info_database_unittest.cc Outdated Show resolved Hide resolved
components/brave_rewards/browser/publisher_info_database_unittest.cc Outdated Show resolved Hide resolved
components/brave_rewards/browser/publisher_info_database_unittest.cc Outdated Show resolved Hide resolved
components/brave_rewards/browser/publisher_info_database_unittest.cc Outdated Show resolved Hide resolved
components/brave_rewards/browser/publisher_info_database_unittest.cc Outdated Show resolved Hide resolved
@jdkuki jdkuki force-pushed the activity-info-filter-refactor branch 4 times, most recently from ebc1249 to 28505c6 Compare July 23, 2019 03:52
@tmancey tmancey self-requested a review July 23, 2019 06:29
tmancey
tmancey previously approved these changes Jul 23, 2019
View reviewed changes
Copy link
Collaborator

@tmancey tmancey left a comment

Choose a reason for hiding this comment

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

LGTM++

@jdkuki jdkuki force-pushed the activity-info-filter-refactor branch 3 times, most recently from 1879f20 to 4dcfa1b Compare July 23, 2019 21:30
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 activity-info-filter-refactor branch from 4dcfa1b to 1cb67ab Compare July 24, 2019 18:22
@NejcZdovc NejcZdovc added this to the 0.70.x - Nightly milestone Jul 25, 2019
@jdkuki jdkuki force-pushed the activity-info-filter-refactor branch from 1cb67ab to 84bb6c7 Compare July 25, 2019 20:38
kylehickinson
kylehickinson approved these changes Jul 25, 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 merged commit 7e18e29 into master Jul 25, 2019
@NejcZdovc NejcZdovc deleted the activity-info-filter-refactor branch July 26, 2019 06:44
@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

@kylehickinson kylehickinson kylehickinson approved these changes

@tmancey tmancey tmancey left review comments

@NejcZdovc NejcZdovc Awaiting requested review from NejcZdovc

Assignees

@jdkuki jdkuki

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

Successfully merging this pull request may close these issues.

Move ActivityInfoFilter to mojo interface
4 participants
@jdkuki @kylehickinson @tmancey @NejcZdovc