This repository has been archived by the owner on May 10, 2024. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
[ads] Trigger search result ad viewed event #8758
[ads] Trigger search result ad viewed event #8758
Changes from all commits
32141b6
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be great to change this comment like the above comment, i.e.,
// This script will ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if
tab?.loadRequest(modifiedRequest)
is called on line 328. I know thatBraveSearchResultAdManager
returnsnil
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if
tab?.loadRequest(modifiedRequest)
is called, then we will run this code again but withX-Brave-Ads-Enabled
header.tab?.braveSearchResultAdManager
will be nil because rewards are enabled. Seems like this works as expected.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit:
triggerSearchResultAdViewedEvent
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.clicked
event is missingThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
renamed to triggerSearchResultAdViewedEvent. Clicked event support will be added as a follow-up
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes because we will not enable the feature via Griffin until click support is added. Perfect!
Check failure on line 23 in Sources/Brave/Frontend/Browser/Search/BraveSearchResultAdManager.swift
Check failure on line 23 in Sources/Brave/Frontend/Browser/Search/BraveSearchResultAdManager.swift
Check failure on line 25 in Sources/Brave/Frontend/Browser/Search/BraveSearchResultAdManager.swift
Check failure on line 25 in Sources/Brave/Frontend/Browser/Search/BraveSearchResultAdManager.swift
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe
Failed to pars search result ads response
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ad
notads
as this failure appears to be on a single adThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe
Failed to process search result ads JSON-LD
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit:
var conversion: BraveAds.ConversionInfo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe define
24 * 60 * 60
as a constantThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added
private static secondsInDay = 24 * 60 * 60;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit:
let searchResultAd...