Fix brave ads check includes #25828

bridiver · 2024-10-05T23:15:49Z

Resolves brave/brave-browser#24163

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:

The associated issue milestone is set to the smallest version that the
changes has landed on
All relevant documentation has been updated, for instance:

Test Plan:

tmancey

LGTM

simonhong

browser/widevine ++

aseren · 2024-10-06T04:49:44Z

components/brave_ads/browser/ads_service_impl.cc

@@ -1530,17 +1482,13 @@ void AdsServiceImpl::CanShowNotificationAds(
 void AdsServiceImpl::CanShowNotificationAdsWhileBrowserIsBackgrounded(
    CanShowNotificationAdsWhileBrowserIsBackgroundedCallback callback) {
  std::move(callback).Run(
-      NotificationHelper::GetInstance()
-          ->CanShowSystemNotificationsWhileBrowserIsBackgrounded());
+      delegate_->CanShowSystemNotificationsWhileBrowserIsBackgrounded());
 }

 void AdsServiceImpl::ShowNotificationAd(base::Value::Dict dict) {


Maybe we can move this function to AdsServiceDelegate and pass ShouldShowCustomNotificationAds() value as a parameter? Declaration can look something like:

virtual void ShowNotificationAd( bool should_show_custom_notification_ads, const std::string& id, const std::u16string& title, const std::u16string& body);

If so, we will get rid of ui/message_center dependency in header because the new function will replace following functions in AdsService::Delegate:

virtual void Display(const message_center::Notification& notification) = 0; virtual void ShowNotificationAd(const std::string& id, const std::u16string& title, const std::u16string& body);

I think there's still quite a bit of refactoring that needs to be done here, but for this first pass my goal is just to remove check_includes = false

aseren · 2024-10-06T04:52:10Z

components/brave_ads/browser/ads_service_impl.cc

@@ -1584,28 +1531,23 @@ void AdsServiceImpl::ShowNotificationAd(base::Value::Dict dict) {
    notification->set_never_timeout(true);
 #endif

-    display_service_->Display(NotificationHandler::Type::BRAVE_ADS,
-                              *notification, /*metadata=*/nullptr);
+    delegate_->Display(*notification);
  }

  StartNotificationAdTimeOutTimer(ad.placement_id);
 }

 void AdsServiceImpl::CloseNotificationAd(const std::string& placement_id) {


Maybe we can move this function to AdsServiceDelegate and pass ShouldShowCustomNotificationAds() value as a parameter? Declaration can look something like:

virtual void CloseNotificationAd( bool should_show_custom_notification_ads, const std::string& placement_id);

If so, the new function will replace following functions in AdsService::Delegate:

virtual void Close(const std::string& notification_id) = 0; virtual void CloseNotificationAd(const std::string& id) = 0;

aseren

Great change! left few comments, thanks!

components/brave_ads/browser/BUILD.gn

github-actions · 2024-10-06T18:03:14Z

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

Description

This PR introduces a new AdsServiceDelegate class and refactors the AdsService and AdsServiceImpl classes to use this delegate. The main motivation appears to be improving the separation of concerns and reducing direct dependencies on platform-specific implementations within the ads service.

Changes

Changes

browser/brave_ads/ads_service_delegate.cc and ads_service_delegate.h:
- New files introducing the AdsServiceDelegate class.
- This class acts as an intermediary between the AdsService and platform-specific implementations.
- Implements methods for handling notifications, opening new tabs, and other platform-specific operations.
browser/brave_ads/ads_service_factory.cc:
- Updated to use the new AdsServiceDelegate.
- Creates an instance of AdsServiceDelegate and passes it to AdsServiceImpl.
components/brave_ads/browser/ads_service.h:
- Added a Delegate nested class to AdsService.
- Modified the constructor to take a Delegate* parameter.
components/brave_ads/browser/ads_service_impl.cc and ads_service_impl.h:
- Refactored to use the new AdsServiceDelegate.
- Removed direct dependencies on Profile, NotificationDisplayService, and other platform-specific classes.
- Updated method implementations to use the delegate for platform-specific operations.
components/brave_ads/browser/ads_service_mock.cc and ads_service_mock.h:
- Updated to take a Delegate* in the constructor.
components/brave_ads/browser/application_state/background_helper.h:
- Moved from browser/brave_ads/application_state/background_helper/background_helper.h to components/brave_ads/browser/application_state/background_helper.h.
Various test files:
- Updated to use the new AdsServiceMock constructor with a null delegate.

Possible Issues

The PR introduces a new layer of abstraction, which might slightly increase the complexity of the codebase.
Some platform-specific code still remains in AdsServiceImpl, which might be further refactored in the future.

Security Hotspots

None identified. The changes primarily focus on architectural improvements and don't introduce new security risks.

bridiver · 2024-10-06T20:23:52Z

unrelated test failure on macos

brave-builds · 2024-10-07T00:03:36Z

Released in v1.72.65

boocmp · 2024-10-07T10:50:57Z

components/brave_ads/browser/ads_service.h

+    virtual void Close(const std::string& notification_id) = 0;
+    virtual void ShowNotificationAd(const std::string& id,
+                                    const std::u16string& title,
+                                    const std::u16string& body);


Missed = 0? I've got 'undefined symbol' referenced by 'const brave_ads::AdsService::Delegate::`vftable'

oops, I wonder why it wasn't caught in CI

@cdesouza-chromium says he is creating a PR for it

@boocmp what build were you running when you saw that?

I've already merged a fix. I should have linked to it #25847

I believe it may have been debug builds only. At least, that's what it seemed like when I encountered it.

Yes, it only fails when optimization is turned off

cdesouza-chromium · 2024-10-07T14:14:40Z

This PR has broken the ubsan build for me:

ld.lld: error: undefined symbol: typeinfo for brave_ads::AdsService::Delegate
>>> referenced by ads_service_impl.cc
>>>               browser/ads_service_impl.o:(.data+0x18) in archive obj/brave/components/brave_ads/browser/libbrowser.a
>>> referenced by ads_service_impl.cc
>>>               browser/ads_service_impl.o:(.data+0x18) in archive obj/brave/components/brave_ads/browser/libbrowser.a
>>> referenced by ads_service_impl.cc
>>>               browser/ads_service_impl.o:(.data+0x18) in archive obj/brave/components/brave_ads/browser/libbrowser.a
>>> referenced 13 more times

bridiver requested review from simonhong and a team as code owners October 5, 2024 23:15

github-actions bot assigned bridiver Oct 5, 2024

tmancey approved these changes Oct 5, 2024

View reviewed changes

Base automatically changed from fix-extensions-check-includes to master October 5, 2024 23:37

bridiver requested review from a team and iefremov as code owners October 5, 2024 23:37

bridiver force-pushed the fix-brave-ads-check-includes branch from 2ed4e48 to 906a60f Compare October 5, 2024 23:39

github-actions bot added the puLL-Merge label Oct 5, 2024

bridiver changed the base branch from master to rewards-page-gn-check-fix October 6, 2024 00:33

bridiver changed the base branch from rewards-page-gn-check-fix to master October 6, 2024 00:35

simonhong approved these changes Oct 6, 2024

View reviewed changes

emerick approved these changes Oct 6, 2024

View reviewed changes

aseren reviewed Oct 6, 2024

View reviewed changes

components/brave_ads/browser/BUILD.gn Show resolved Hide resolved

bridiver force-pushed the fix-brave-ads-check-includes branch 2 times, most recently from ca85ed1 to a75c474 Compare October 6, 2024 18:02

bridiver force-pushed the fix-brave-ads-check-includes branch from a75c474 to 17760b8 Compare October 6, 2024 18:15

remove //brave/components/brave_ads/browser check_includes = false

892cdad

bridiver force-pushed the fix-brave-ads-check-includes branch from 17760b8 to 892cdad Compare October 6, 2024 18:46

bridiver merged commit 1ac1d51 into master Oct 6, 2024
18 of 19 checks passed

bridiver deleted the fix-brave-ads-check-includes branch October 6, 2024 20:24

github-actions bot added this to the 1.72.x - Nightly milestone Oct 6, 2024

boocmp reviewed Oct 7, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix brave ads check includes #25828

Fix brave ads check includes #25828

bridiver commented Oct 5, 2024 •

edited

Loading

tmancey left a comment

simonhong left a comment

aseren Oct 6, 2024

bridiver Oct 6, 2024

aseren Oct 6, 2024

aseren left a comment

github-actions bot commented Oct 6, 2024

Changes

bridiver commented Oct 6, 2024

brave-builds commented Oct 7, 2024

boocmp Oct 7, 2024

bridiver Oct 7, 2024

bridiver Oct 7, 2024

bridiver Oct 7, 2024

cdesouza-chromium Oct 7, 2024

emerick Oct 7, 2024

boocmp Oct 8, 2024

cdesouza-chromium commented Oct 7, 2024

Fix brave ads check includes #25828

Fix brave ads check includes #25828

Conversation

bridiver commented Oct 5, 2024 • edited Loading

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

tmancey left a comment

Choose a reason for hiding this comment

simonhong left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aseren left a comment

Choose a reason for hiding this comment

github-actions bot commented Oct 6, 2024

Description

Changes

Possible Issues

Security Hotspots

bridiver commented Oct 6, 2024

brave-builds commented Oct 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdesouza-chromium commented Oct 7, 2024

bridiver commented Oct 5, 2024 •

edited

Loading