-
Notifications
You must be signed in to change notification settings - Fork 93
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
Prebid plugin render changes #1068
base: master
Are you sure you want to change the base?
Conversation
3dd14b6
to
d3b3e83
Compare
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.
@OlenaPostindustria Please take a look into the comments
import UIKit | ||
|
||
@objcMembers | ||
public class InterstitialController: |
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.
@OlenaTeqBlaze this class needs to be open so that other renderers can create the child class and return it in PrebidMobileInterstitialPluginRenderer
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.
This class is the SDK internal one. Could you please clarify the reason for needing to create the child classes? The current implementation of the plugin renderer indicates that you need to create your own class and conform it to PrebidMobilePluginRenderer
protocol. You can use SampleRenderer as reference.
guard transactionFactory == nil else { | ||
return | ||
} | ||
|
||
adConfiguration.adConfiguration.winningBidAdFormat = bid.adFormat | ||
videoControlsConfig.initialize(with: bid.videoAdConfiguration) | ||
|
||
// This part is dedicating to test server-side ad configurations. | ||
// Need to be removed when ext.prebid.passthrough will be available. | ||
#if DEBUG | ||
adConfiguration.adConfiguration.videoControlsConfig.initialize(with: bid.testVideoAdConfiguration) | ||
#endif | ||
|
||
transactionFactory = PBMTransactionFactory( |
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 I inherit from InterstitialController, I need to override the loadAd method to receive the callback required for my renderer to load an ad. However, I cannot call super.loadAd because that would initiate the creation of the transactionFactory and start loading the prebid render ad. If i don't call it then bid.adFormat will not able to set.
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.
Please, take a look at the comment above
PrebidMobile/PrebidMobileRendering/PluginRenderer/PrebidMobilePluginRegister.swift
Outdated
Show resolved
Hide resolved
PrebidMobile/PrebidMobileRendering/PluginRenderer/PrebidMobilePluginRenderer.swift
Outdated
Show resolved
Hide resolved
PrebidMobile/PrebidMobileRendering/Prebid/Integrations/GAM/AdLoading/PBMBannerAdLoader.m
Show resolved
Hide resolved
PrebidMobile/PrebidMobileRendering/Prebid/Integrations/GAM/AdLoading/PBMBannerAdLoader.m
Outdated
Show resolved
Hide resolved
...bid/PBMCacheRenderers/InterstitialController/InterstitialControllerInteractionDelegate.swift
Outdated
Show resolved
Hide resolved
...e/PrebidMobileRendering/Prebid/PBMCacheRenderers/DisplayView/PrebidDisplayViewRenderer.swift
Outdated
Show resolved
Hide resolved
Hi @jangirtony2009 ! Thanks for the review! We'll address your notes soon. Please ignore the removed question if you saw it :) |
f5f4fc1
to
e868f09
Compare
Hi, @jangirtony2009! Thank you for taking the time to review this pull request! |
7c7ba8e
to
3f2d806
Compare
|
||
PBMLogWarn(@"SDK couldn't retrieve an implementation of PrebidMobileDisplayViewManagerProtocol. SDK will use the default one."); | ||
|
||
PrebidRenderer *defaultRenderer = [PrebidRenderer new]; |
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.
why are you creating the new object only for PrebidRenderer? Why not for the customRenderer?
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.
The third-party renderer has the option to return nil when creating a banner or interstitial. In such cases, we must fall back to the default renderer, even if the "custom" renderer was originally selected.
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.
Since Prebid will maintain the same object, developers won’t be able to manage local variables within the class. Therefore, it’s better to make the createBanner and createInterstitial methods static. This will provide more clarity to developers.
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.
Hi @jangirtony2009 ! We'll make a small change in order not to create a separate instance of PrebidRenderer.
However, it doesn't look meaningful to make methods like createBanner static. The reason is that third-party renderers will still be passed to the SDK as objects.
Please review the following changes and let us know if they add clarity to the solution and workflow design.
} | ||
|
||
return preferredPlugin | ||
} | ||
|
||
public func getAllPlugins() -> [PrebidMobilePluginRenderer] { |
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.
In prebid-android sdk is adding the default renderer in the adRequest and sending it to the server.
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.
I'm not certain if we should do that, perhaps @YuriyVelichkoPI can help here.
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.
Yes, we should register the default renderer in the list. It will be a good design approach. However, I'm not sure if SDK sends any signals indicating the presence of a default renderer. We'll double-check it.
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.
@jangirtony2009, we checked the Android behavior and @ValentinPostindustria says that the SDK doesn't send any info about the default rendered in the request. So could you clarify this part of your comment
... and sending it to the server
Thanks!
02736b0
to
4b4e8d4
Compare
…stitialControllerProtocol
4b4e8d4
to
1ac4fd5
Compare
#697, #1067