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

[ads] Decouple AdsService functionality from iOS BraveAds implementation #26451

Merged
merged 4 commits into from
Nov 8, 2024

Conversation

aseren
Copy link
Collaborator

@aseren aseren commented Nov 8, 2024

This is a prerequisite for the Ads-Internals page task brave/brave-browser#40952, as we now need to access Ads Service from various parts of the Brave iOS codebase.

This is also the first step in Unification of Service Layer for iOS: brave/brave-browser#33868.
During next steps the current class AdsServiceImplIOS will be replaced by AdsService and AdsServiceImpl which will be directly re-used for iOS.

Resolves brave/brave-browser#42161

Submitter Checklist:

  • I confirm that no security/privacy review is needed and no other type of reviews are needed, or that I have requested them
  • There is a ticket for my issue
  • Used Github auto-closing keywords in the PR description above
  • Wrote a good PR/commit description
  • Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
  • Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
  • Checked the PR locally:
    • npm run test -- brave_browser_tests, npm run test -- brave_unit_tests wiki
    • npm run presubmit wiki, npm run gn_check, npm run tslint
  • Ran git rebase master (if needed)

Reviewer Checklist:

  • A security review is not needed, or a link to one is included in the PR description
  • New files have MPL-2.0 license header
  • Adequate test coverage exists to prevent regressions
  • Major classes, functions and non-trivial code blocks are well-commented
  • Changes in component dependencies are properly reflected in gn
  • Code follows the style guide
  • Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

Test Case 1

  • Fresh install
  • Trigger search result ad for a staging server with the giraffe request
  • Click search result ad
  • Convert search result ad by visiting https://www.brave.com/vertical-tabs/
  • Toggle Settings -> Shields & Privacy -> CLEAR PRIVATE DATA -> Brave Ads Data
  • Tap Clear Data Now button
  • Make sure that Brave Ads service is restarted
  • Confirm that following tables in ads/Ads.sqlite are empty:
    • creative_set_conversions

Test Case 2

  • Join Brave Rewards
  • Confirm that the following ad types can be served, viewed and clicked:
    • NTT ads
    • Brave News Ads
    • Push Notification ads
  • Confirm that view and click confirmations are sent

@aseren aseren requested a review from a team as a code owner November 8, 2024 01:53
@aseren aseren requested a review from tmancey November 8, 2024 01:53
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++

Copy link
Contributor

github-actions bot commented Nov 8, 2024

[puLL-Merge] - brave/brave-core@26451

Description

This PR introduces significant changes to the Brave Ads implementation on iOS. The main change is the introduction of a new AdsServiceImplIOS class, which encapsulates much of the functionality previously handled directly in the BraveAds class. This change aims to improve the structure and maintainability of the Brave Ads codebase on iOS.

Changes

Changes

  1. ios/browser/api/ads/BUILD.gn:

    • Added dependency on "//brave/ios/browser/brave_ads"
    • Removed dependency on "//sql:sql"
  2. ios/browser/api/ads/brave_ads.mm:

    • Removed direct database handling
    • Introduced AdsServiceImplIOS to handle most ad-related operations
    • Updated method implementations to use AdsServiceImplIOS
    • Removed ads and adsDatabase members, replaced with adsService
    • Updated initialization and shutdown processes
  3. ios/browser/brave_ads/BUILD.gn (new file):

    • Added new build configuration for the Brave Ads service
  4. ios/browser/brave_ads/ads_service_factory_ios.h (new file):

    • Introduced AdsServiceFactoryIOS class for creating AdsServiceImplIOS instances
  5. ios/browser/brave_ads/ads_service_factory_ios.mm (new file):

    • Implemented AdsServiceFactoryIOS
  6. ios/browser/brave_ads/ads_service_impl_ios.h (new file):

    • Defined AdsServiceImplIOS class, which now handles most ad-related operations
  7. ios/browser/brave_ads/ads_service_impl_ios.mm (new file):

    • Implemented AdsServiceImplIOS
  8. ios/browser/profile/model/BUILD.gn:

    • Added dependency on "//brave/ios/browser/brave_ads"
  9. ios/browser/profile/model/brave_keyed_service_factories.mm:

    • Added initialization of AdsServiceFactoryIOS

Possible Issues

  1. The transition to using AdsServiceImplIOS might introduce temporary instability or bugs in the Brave Ads functionality on iOS.
  2. Some functionality might have been missed during the refactoring process, potentially leading to reduced features or unexpected behavior.

Security Hotspots

No significant security hotspots were identified in this change. The refactoring primarily focuses on restructuring the code rather than introducing new security-sensitive operations.

Comment on lines +255 to +257
ProfileIOS* profile = [self getLastUsedProfile];
adsService = brave_ads::AdsServiceFactoryIOS::GetForBrowserState(profile);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just fyi, this will always be the regular profile (not private/OTR), which matches the current behaviour anyways, but just figured I'd point it out

Copy link
Collaborator

@tmancey tmancey Nov 8, 2024

Choose a reason for hiding this comment

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

@aseren could we please add a comment explaining this, along the lines of this will always be the regular profile (not private/OTR)? Thanks

Copy link
Collaborator

Choose a reason for hiding this comment

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

Or maybe we add a CHECK instead of a comment?

Copy link
Collaborator

Choose a reason for hiding this comment

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

There's actually a CHECK under this line already :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added an explanatory comment

@aseren aseren enabled auto-merge November 8, 2024 17:33
@aseren aseren merged commit eea7c32 into master Nov 8, 2024
19 checks passed
@aseren aseren deleted the issues/42161 branch November 8, 2024 18:38
@github-actions github-actions bot added this to the 1.75.x - Nightly milestone Nov 8, 2024
@brave-builds
Copy link
Collaborator

Released in v1.75.8

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[ads] Decouple AdsService functionality from iOS BraveAds implementation
4 participants