Add Prebid Plugin Renderer #1021
Conversation
…bid-mobile-ios into feature/custom_render
Co-authored-by: Paul NICOLAS <89924913+github-paul-nicolas@users.noreply.github.com>
…data type, removed unused
Co-authored-by: Paul NICOLAS <89924913+github-paul-nicolas@users.noreply.github.com>
|
@github-richard-depierre is this feature-complete or do you have a list of what still needs to be done? |
|
@jsligh This feature is complete and I have implemented it on the internal sample app as an example. |
There was a problem hiding this comment.
Looks good to me but I'd also like @YuriyVelichkoPI to review before I merge.
|
Hi everyone! Sorry, I really want to help, but very busy this week. If you do not hurry with the release - I'll take a look mid-next week or earlier. |
|
This refers to Issue #697 |
There was a problem hiding this comment.
Sorry for the long silence.
In general functionality looks good, but we still need to improve / clean up some places.
Also it's very important to add unit tests for new classes.
A new review will be needed once comments are addressed. I promise to do it as soon as fixes are provided.
|
|
||
| queue.async(flags: .barrier) { [weak self] in | ||
| guard self?.plugins[rendererName] == nil else { | ||
| Log.debug("New plugin renderer with name \(rendererName) will replace the previous one with same name") |
There was a problem hiding this comment.
This message doesn't seem correct because nothing will be replaced according to implementation.
| } | ||
| } | ||
|
|
||
| private func get(for key: String) -> PrebidMobilePluginRenderer? { |
There was a problem hiding this comment.
Please make the function name more verbose. getPluginRenderer I suppose
| /// If no preferred renderer is found, it returns PrebidRenderer to perform default behavior | ||
| /// Once bid is win we want to resolve the best PluginRenderer candidate to render the ad | ||
| @objc public func getPluginForPreferredRenderer(bid: Bid) -> PrebidMobilePluginRenderer { | ||
| guard let preferredRendererName = bid.getPreferredPluginRendererName(), |
There was a problem hiding this comment.
It looks like if let suits better in this case. It allows us to follow positive logic in the condition.
| let rendererName = renderer.name | ||
|
|
||
| queue.async(flags: .barrier) { [weak self] in | ||
| guard self?.plugins[rendererName] == nil else { |
There was a problem hiding this comment.
It's hard to follow the condition of this expression. Can we use if self?.plugins[rendererName] == nil { ?
| @objc public static let shared = PrebidMobilePluginRegister() | ||
|
|
||
| private let queue = DispatchQueue(label: "PrebidMobilePluginRegisterQueue", attributes: .concurrent) | ||
| private var plugins = [String: PrebidMobilePluginRenderer]() |
There was a problem hiding this comment.
We need to add unit tests for all functions in this class, especially those that perform sync and async operations on this map.
|
|
||
| /// Creates and returns an implementation of PrebidMobileInterstitialControllerInterface for a given bid response | ||
| /// Returns nil in the case of an internal error | ||
| @objc optional func createInterstitialController( bid: Bid, adConfiguration: AdUnitConfig, connection: PrebidServerConnectionProtocol, adViewManagerDelegate: InterstitialController?, videoControlsConfig: VideoControlsConfiguration?) |
There was a problem hiding this comment.
Please format the functions' declaration so the each parameter is placed on a separate line.
Please do it for all functions introduced in this PR
| @@ -64,40 +65,12 @@ public class InterstitialController: NSObject, PBMAdViewManagerDelegate { | |||
| } | |||
|
|
|||
| @objc public func loadAd() { | |||
There was a problem hiding this comment.
Please put a comment for this function
// TODO: provide a more relevant name for this function.
It's out of the scope of this PR, but I want to return back later and solve the inconvenience of naming the function. Because, in fact, this function doesn't load anything, it initializes the internal class respectively to the received bid and other properties.
| PBMWinNotifier.notifyThroughConnection(PrebidServerConnection.shared, | ||
| winning: bid) { [weak self] adMarkup in | ||
|
|
||
| self?.transactionFactory?.load(withAdMarkup: adMarkup!) |
There was a problem hiding this comment.
Don't use force unwrap
| if let adMarkup = adMarkup { | ||
| self?.transactionFactory?.load(withAdMarkup: adMarkup) | ||
| } else { | ||
| //TODO: inform failure |
There was a problem hiding this comment.
Log a respective message instead of a comment
| #import "CustomRendererDisplayBannerViewController.h" | ||
| #import "PrebidDemoMacros.h" | ||
|
|
||
| NSString * const storedImpDisplayBannerCustomRenderer = @"prebid-demo-banner-320-50"; |
There was a problem hiding this comment.
What is the purpose of these files?
From the GitHub UI I don't see changes in the project.
And as I understand, the integration case for third-party rendering should use prebid-ita-banner-320-50-meta-custom-renderer for proper work.
It also should be an integration case for the Swift demo app.
| @@ -40,6 +47,13 @@ - (nonnull PBMJsonDictionary *)toJsonDictionary { | |||
|
|
|||
| PBMMutableJsonDictionary * const storedRequest = [PBMMutableJsonDictionary new]; | |||
| ret[@"storedrequest"] = storedRequest; | |||
| NSArray *renderers = [[PrebidMobilePluginRegister shared] getAllPlugins]; | |||
| // TODO How to get the isOriginalAPI value here | |||
| // if (renderers.count != 0 || self.adConfiguration.isOriginalAPI) { | |||
There was a problem hiding this comment.
PBMParameterBuilderService contains adConfiguration.
The most straightforward way is to pass adConfiguration to [PBMORTBParameterBuilder buildOpenRTBFor:bidRequest]; and further through the call chain.
There was a problem hiding this comment.
@github-richard-depierre, just want to remind you that isOriginalAPI is not utilized in the logic yet. Please, see my above comment about how to get this value.
There was a problem hiding this comment.
Hi @github-richard-depierre ! Sorry, but it looks like I misdirected you.
The current class PBMORTBBidRequestExtPrebid serves only as a data structure. It shouldn't fill itself.
If you need to add some data to ext.prebid, you should do it in PBMBasicParameterBuilder.buildBidRequest
in this class the adConfiguration is available so you can easily determine the kind of API.
So, please try to move this code into PBMBasicParameterBuilder.
| @@ -40,6 +47,13 @@ - (nonnull PBMJsonDictionary *)toJsonDictionary { | |||
|
|
|||
| PBMMutableJsonDictionary * const storedRequest = [PBMMutableJsonDictionary new]; | |||
| ret[@"storedrequest"] = storedRequest; | |||
| NSArray *renderers = [[PrebidMobilePluginRegister shared] getAllPlugins]; | |||
| // TODO How to get the isOriginalAPI value here | |||
| // if (renderers.count != 0 || self.adConfiguration.isOriginalAPI) { | |||
There was a problem hiding this comment.
@github-richard-depierre, just want to remind you that isOriginalAPI is not utilized in the logic yet. Please, see my above comment about how to get this value.
| @objc var version: String { get } | ||
| @objc var data: [AnyHashable: Any]? { get } | ||
|
|
||
| var transactionFactory: PBMTransactionFactory? { get set } |
There was a problem hiding this comment.
@github-richard-depierre, we should still remove PBMTransactionFactory from the public protocol.
| PBMMutableJsonDictionary * const sdk = [PBMMutableJsonDictionary new]; | ||
| NSMutableArray *renderersArray = [NSMutableArray array]; | ||
| NSArray *renderers = [[PrebidMobilePluginRegister shared] getAllPlugins]; | ||
| for (id<PrebidMobilePluginRenderer> renderer in renderers) { |
There was a problem hiding this comment.
SDK should send only custom renderers in the request.
I see two renderers in the request :
"renderers": [{
"name": "SampleCustomRenderer",
"version": "1.0.0"
}, {
"name": "PrebidRenderer",
"version": "2.2.3"
}]
|
LGTM |
|
@github-richard-depierre is there a docs PR for iOS documentation for this? |
Hello Prebid team, this pull request adds the Prebid Plugin Renderer on the Prebid iOS SDK repo.
Just for recap, we had a meeting couple of months ago where we planned together that we would be implementing that for iOS to sync with how Android project is currently.
This feature is already in production for Android, you can find the Prebid docs explaining it here
I'm also adding diagrams below that describe at a high level how the feature is supposed to behave.
Prebid Plugin Renderer diagrams
We appreciate if you could carefully review it since Android and iOS project are not quite the same in some aspects. Thanks.