Expire ads respectively to the seatbid.bid.exp - #945
Conversation
YuriyVelichkoPI
left a comment
There was a problem hiding this comment.
Review
Implementation covers the mechanical parts of prebid-mobile-ios#850 well (bid.exp parsing, timer scheduling/cancellation across Banner/Interstitial/Rewarded, isLoaded()/show() gating, native cache expiration wiring, listener additions as non-breaking default methods, RewardedAdUnit.destroy() listener cleanup alignment) and has thorough test coverage (destroy-before-expiry, reload-cancels-old-timer, no-exp-means-no-timer, single-callback guarantee, null-listener safety, etc.).
A few things I'd like addressed before merging:
1. Expiration timer starts too late relative to bid.exp semantics
The issue spec says: "Once the bid is received and the model is created... create an async call after the bid.exp time." In BaseInterstitialAdUnit, bidResponse is assigned in onFetchCompleted (bid-receipt time), but scheduleExpirationIfNeeded() isn't invoked until onInterstitialReadyForDisplay() / onAdServerWin() — i.e. after the creative has finished downloading/rendering. Since bid.exp represents "seconds between auction and impression," anchoring the countdown to "ready to display" instead of "bid received" silently extends the effective window by the creative load time (potentially significant for VAST/video or heavy HTML creatives), undermining the billing-window enforcement this feature exists for. Please start the timer at bid-receipt time (or confirm against the iOS reference implementation which anchor point is intended, and document the choice).
2. Dead/unused plumbing in CreativeModel / CreativeModelMakerBids
expirationTimeSeconds is added to CreativeModel and populated in parseAcj (not for video/VAST models), but it is never read anywhere in production code — actual scheduling in BannerView/BaseInterstitialAdUnit reads BidResponse.getExpirationTimeSeconds() directly instead. The only consumers are round-trip getter/setter unit tests. Please either wire this up to something real or remove it to avoid confusing future maintainers.
3. Redundant condition in BaseInterstitialAdUnit.show()
if (expired || !isAuctionWinnerReadyToDisplay()) — isAuctionWinnerReadyToDisplay() was itself updated to include !expired, so the expired || is redundant. Please simplify to just if (!isAuctionWinnerReadyToDisplay()).
4. Banner "do nothing if not refreshable" still tears down the view
expireAd() in BannerView unconditionally calls displayView.destroy() + removeAllViews() regardless of autoRefreshDelay. The issue's "do nothing if ad unit is not refreshable" reads as "leave the currently displayed creative alone," but current behavior always blanks the view even when not refreshable. Please confirm this is the intended UX and adjust so the non-refreshable path truly does nothing to the displayed view (only fires the onAdExpired callback), or update the issue/PR description to clarify the deliberate deviation.
Requesting changes on all four points above — they all seem worth resolving before merge.
Follows the final behavior of prebid/prebid-mobile-ios#1267 after the Rendering API requirements were relaxed: expiration only notifies the app. - Interstitial/rewarded: expired ads stay loaded and showable; loadAd() is allowed after expiration and releases the expired ad first. - Expiration is scheduled only for Prebid-rendered ads, never on GAM win. - Banner: cancel the timer on impression and when the ad view is replaced, so a refreshed bid without exp does not inherit the previous timer. - Banner: remove the expired view before notifying, and reload only when auto-refresh is enabled and not stopped via stopRefresh(). - Native: never-registered ads expire as on iOS; drop the test helper that faked a released view through reflection. - BidInfo: native cache honors bid.exp for fetchDemand(OnFetchDemandResult) and PrebidAdUnit. - Listener Javadoc matches iOS wording; the test app gets a dedicated onAdExpired indicator. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Per review on prebid/prebid-mobile-ios#1267: when the banner is not refreshable (no auto-refresh, or refresh stopped), expiration only notifies the listener and leaves the displayed creative untouched. The view is torn down only when a reload follows. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Addresses review on #945: - bid.exp counts from the auction, so the expiration deadline now starts when the bid response arrives instead of when the ad is ready. If the bid expires while the creative is still loading, onAdExpired is delivered right after onAdLoaded; ad server wins and failed loads never report expiration. - Remove CreativeModel.expirationTimeSeconds: it was set for HTML models only and never read in production code. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
#850
bid.expparsing and expiration handling for Native and Rendering API flows.BidResponse.RewardedAdUnit.destroy()listener cleanup with interstitial behavior.