[ads] Trigger search result ad viewed event #8758
Conversation
| domainForMainFrame.isShieldExpected(.AdblockAndTp, considerAllShieldsOption: true) | ||
| domainForMainFrame.isShieldExpected(.AdblockAndTp, considerAllShieldsOption: true), | ||
|
|
||
| // Add Brave search result ads processing script. |
There was a problem hiding this comment.
Would be great to change this comment like the above comment, i.e., // This script will ...
| self.rewards = rewards | ||
| } | ||
|
|
||
| func triggerSearchResultAdEvent(_ searchResultAdInfo: BraveAds.SearchResultAdInfo) { |
There was a problem hiding this comment.
nit: triggerSearchResultAdViewedEvent
There was a problem hiding this comment.
renamed to triggerSearchResultAdViewedEvent. Clicked event support will be added as a follow-up
There was a problem hiding this comment.
Ah yes because we will not enable the feature via Griffin until click support is added. Perfect!
Sources/Brave/Frontend/Browser/BrowserViewController/BVC+WKNavigationDelegate.swift
Show resolved
Hide resolved
| @@ -324,6 +329,8 @@ extension BrowserViewController: WKNavigationDelegate { | |||
| return (.cancel, preferences) | |||
| } | |||
|
|
|||
| tab?.braveSearchResultAdManager = BraveSearchResultAdManager(url: requestURL, rewards: rewards, isPrivateBrowsing: isPrivateBrowsing) | |||
There was a problem hiding this comment.
What if tab?.loadRequest(modifiedRequest) is called on line 328. I know that BraveSearchResultAdManager returns nil for Brave Rewards users, however it may not be clear to others in the future if/when these events are sent when ads are enabled.
There was a problem hiding this comment.
if tab?.loadRequest(modifiedRequest) is called, then we will run this code again but with X-Brave-Ads-Enabled header. tab?.braveSearchResultAdManager will be nil because rewards are enabled. Seems like this works as expected.
| var conversionInfo: BraveAds.ConversionInfo? | ||
| if let conversionUrlPatternValue = ad.conversionUrlPatternValue, | ||
| let conversionObservationWindowValue = ad.conversionObservationWindowValue { | ||
| let timeInterval = TimeInterval(conversionObservationWindowValue * 24 * 60 * 60) |
There was a problem hiding this comment.
Maybe define 24 * 60 * 60 as a constant
There was a problem hiding this comment.
added private static secondsInDay = 24 * 60 * 60;
| return | ||
| } | ||
|
|
||
| var conversionInfo: BraveAds.ConversionInfo? |
There was a problem hiding this comment.
nit: var conversion: BraveAds.ConversionInfo
| for ad in searchResultAds.creatives { | ||
| guard let rewardsValue = Double(ad.rewardsValue) | ||
| else { | ||
| Logger.module.error("Failed to process Brave search result ads") |
There was a problem hiding this comment.
ad not ads as this failure appears to be on a single ad
There was a problem hiding this comment.
Maybe Failed to process search result ads JSON-LD
| let messageData = try? JSONSerialization.data(withJSONObject: message.body, options: []), | ||
| let searchResultAds = try? JSONDecoder().decode(SearchResultAdResponse.self, from: messageData) | ||
| else { | ||
| Logger.module.error("Failed to process Brave search result ads") |
There was a problem hiding this comment.
Maybe Failed to pars search result ads response
| ) | ||
| } | ||
|
|
||
| let searchResultAdInfo: BraveAds.SearchResultAdInfo = .init( |
There was a problem hiding this comment.
nit: let searchResultAd...
|
PR has been moved to brave/brave-core#21974 |
Summary of Changes
Corresponding brave-core change: brave/brave-core#21974
This pull request fixes brave/brave-browser#33469
Submitter Checklist:
NSLocalizableString()Test Plan:
[ads] Served search result ad with placement idlog message[ads] Viewed search result ad with placement idlog messageScreenshots:
Reviewer Checklist:
QA/(Yes|No)bug/enhancement