-
Notifications
You must be signed in to change notification settings - Fork 868
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
Fixes Already sustaining ad notification interaction
is shown for conversion of the second ad within the same creative set
#4906
Conversation
e14748b
to
7b49f89
Compare
Passed on macOS, Windows, Android, iOS. Failed on Linux, restarting Linux |
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.
lgtm
@@ -1784,12 +1784,17 @@ void AdsImpl::SustainAdNotificationInteractionIfNeeded() { | |||
BLOG(INFO) << "Failed to sustain ad notification interaction, domain for " | |||
"the focused tab does not match the last shown ad notification for " | |||
<< last_shown_ad_notification_.target_url; | |||
|
|||
last_sustained_ad_notification_url_ = ""; |
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.
could this be moved at the top of SustainAdNotificationInteractionIfNeeded
, so that we don't need to do it twice? As far as I can see all paths set this
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, was put here originally as I was resetting last_shown_ad_notification_
which was needed here. Changing now
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.
Fixed
…onversion of the second ad within the same creative set
if (!IsStillViewingAdNotification()) { | ||
BLOG(INFO) << "Failed to sustain ad notification interaction, domain for " | ||
"the focused tab does not match the last shown ad notification for " | ||
<< last_shown_ad_notification_.target_url; | ||
|
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: extra line
Unrelated CI lint error |
Resolves brave/brave-browser#8634
Submitter Checklist:
npm run lint
)git rebase master
(if needed).git rebase -i
to squash commits (if needed).Test Plan:
See brave/brave-browser#8634 however if the same ad already exists landed confirmations will not occur and is covered by brave/brave-browser#5949
Reviewer Checklist:
After-merge Checklist:
changes has landed on.