Prebid Core: add documentResolver callback and allow the user to supply a different document object to render by olafbuitelaar · Pull Request #8262 · prebid/Prebid.js

olafbuitelaar · 2022-04-05T15:50:58Z

Type of change

Bugfix
Feature
New bidder adapter
Code style update (formatting, local variables)
Refactoring (no functional changes, no api changes)
Build related changes
CI related changes
Does this change affect user-facing APIs or examples documented on http://prebid.org?
Other

Description of change

This PR, allows the user to supply a different document object to render the ad/outstream video in. This can be usefull if the prebid instance is loaded in a different document/iframe, while the ad/video should be rendered in another iframe/document.
This implements this optional change also in the Appnexus bid adapter, and allows the Appnexus bid adapter to fail silently when certain elements don't exist.
The feature add's an optional contextResolver callback method to the config of the Renderer, which receives the current Bid, the document where prebid is loaded, and the document object which is passed to the renderAd method. And should return the document object in which it should render the ad

Configuration can be on the adunit or mediaType level.
For example;

mediaTypes: {
    video: {
        context: 'outstream',
        playerSize: [640, 480],
        mimes: ['video/mp4'],
        protocols: [1, 2, 3, 4, 5, 6, 7, 8],
        playbackmethod: [2],
        skip: 1,
        playback_method: ['auto_play_sound_off'],
        renderer:{                                        
            options:{                                            
                contextResolver:function(bid, ownDoc, renderDoc){                                                
                    return renderDoc;                                                
                }
            }                                        
        }
    }
}

* cache url's per resolvable context * pass options as config to Renderer * pass renderDocument to render * utility function to resolve the containing window object of a document

* support user-provided renderer context * handle when there is no div[id^='google_ads'] element

dgirardi · 2022-04-06T17:41:45Z

src/utils.js

+  if (!doc) {
+    return null;
+  }
+  return doc.defaultView || doc.parentWindow;


where does parentWindow come from? I don't see it specified on MDN: https://developer.mozilla.org/en-US/docs/Web/API/Document

parentWindow is used on older IE browsers

We do not support older IE browsers - in fact we don't support IE at all - many other things would not work. IMO it can be removed.

I agree. We shouldnt be adding support back in for old IE versions. Can the contents of this function also just be one ternary line saying return (doc) ? doc.defaultView : null

dgirardi · 2022-04-06T17:42:58Z

src/Renderer.js

+    context = renderer.config.contextResolver(bid, document, doc);// a user provided callback, which should return a Document, and expect the parameters; bid, sourceDocument, renderDocument
+  }
+  if (!context) {
+    context = document;


IMO doc would be a better default; what is the reasoning for using document instead?

document is the current default, changing it to doc would introduce a possible breaking change in the current behaviour. But generally i agree, in fact one of the main reasons this PR exists.

You're right - I misread and thought it would default to the document passed to renderAd. Nevermind.

dgirardi · 2022-04-06T17:43:37Z

modules/appnexusBidAdapter.js

 }

 function newRenderer(adUnitCode, rtbBid, rendererOptions = {}) {
+  debugger;


dgirardi · 2022-04-06T17:56:28Z

src/adloader.js

+ * @param {function} [callback] callback function to be called after the script is loaded
 */
-export function loadExternalScript(url, moduleCode, callback) {
+export function loadExternalScript(url, moduleCode, callback, context) {


Why does this need to be aware of the context? (also, is context a good name for what is always a document?)
Would it work if instead of keeping track of context, we add an option to always run the callback, and let the caller decide if they need to run their logic again?

the renderer can load scripts, when the config contains a url property, like;

renderer: { url: 'https://cdn.adnxs.com/renderer/video/ANOutstreamVideo.js' }

in which case the script should be loaded in the correct/desired document context.

I'm not sure if calling the callback always would be the best solution, since other modules also make use of this method.
Also this would push the (repeating) logic of caching or not, always to the callback.
However there is 1 advantage to that, since once a script is loaded, it could reference/re initiate the already loaded code in the new context. But this could also be implemented else if desired, like in a pre-render hook. Which would save a url load, if the script needs to be loaded in different document contexts.
Also one could say, you could simply rely on the caching behaviour of the browser.

I missed that you are using context.createScript - you are right. I have two suggestions:

change the name from context to something that's clearly a document (like doc)

use WeakMap to separate caches by document, e.g. const caches = new WeakMap(); const _requestCache = caches.get(doc) || caches.set(doc, {}).get(doc). It's a smaller change, easier to follow, and marginally more efficient.

dgirardi · 2022-04-06T18:08:06Z

modules/appnexusBidAdapter.js

 }

-function outstreamRender(bid) {
+function outstreamRender(bid, context) {


Since this new context option needs support from individual adapters anyway,, it looks to me like you could reduce the scope of this change if you look for the document / context in here, without requiring it to be passed around as an argument. By that I mean look at this.options.context in here to see what document / window to use, leaving almost everything in core unchanged.

I considered that, but only the fields url, renderder and config/options are passed allong, so adding another field would mean to make sure the new context field is always passed in all circumstances. having it a prop of config, ensures it's always passed.
While it's true Adapter should implement this to support it fully, and the ones i've checked it seems implementations vary a lot surrounding this subject.
The core part, for loading the url, when there is not an adapter specific renderer defined, requires this change to live in the core part.

I think I agree after giving it some more thought, if the idea is that the same renderer instance could be used on different documents. The only pushback I'll give on is the naming again - context sound a lot like object, while the arument is very well defined as a document. Same for contextResolver.

…bid.js into render_context

* use WeakMap for caching of url's and documents * flipped key order of url cache * added unit test

olafbuitelaar · 2022-04-08T15:54:20Z

i've renamed all context's to doc's, implemented the WeakMap for the caching store, and added a unit test.

dgirardi · 2022-04-08T16:40:41Z

src/adloader.js

-  function requestResource(tagSrc, callback) {
-    var jptScript = document.createElement('script');
+  function requestResource(tagSrc, callback, doc) {
+    if (!doc) {


here you check that doc and cacheObject are defined before using them. It doesn't hurt, except maybe for adding a couple bytes, but they should always be defined here - conceptually, if you're going to check for that, it would make more sense to throw an error when they are undefined (but I wouldn't do that either).

It's not that consequential so I don't mind if it stays this way, just noting it as an odd choice IMO.

It depends, doc being not set defaults to the loaded document, which in theory does also happen down the line in insertElement and some other methods. But now it's specific from here on. But i do agree it could also be an Argument Exception if there is a need to be specific about this.
The cacheObject could theoeratically be gone for what ever reason it's resolved there. This could actually be an error, since the tag property won't be set, but the callback would flag the object as loaded, which is sort of inconsistent. But not failing there and just loading the script, without failing there is i think not that troublesome.

dgirardi · 2022-04-08T16:48:56Z

test/spec/renderer_spec.js

      expect(loadExternalScript.called).to.be.true;
    });
+
+    it('call\'s documentResolver when configured', function () {


Can you add a case to adloater_spec to check that when the same script, but different document is passed to loadExternalScript, it does not hit the cache?

mmoschovas · 2022-04-12T11:14:53Z

modules/appnexusBidAdapter.js

-  if (el[0]) {
-    el[0].style.setProperty('display', 'none');
+  try {
+    var el = document.getElementById(elementId).querySelectorAll("div[id^='google_ads']");


Since you are making changes here anyway, can this be updated to use a const instead of a var?

mmoschovas · 2022-04-12T11:19:12Z

src/utils.js

+  if (!doc) {
+    return null;
+  }
+  return doc.defaultView || doc.parentWindow;


I agree. We shouldnt be adding support back in for old IE versions. Can the contents of this function also just be one ternary line saying return (doc) ? doc.defaultView : null

* drop IE support

olafbuitelaar · 2022-04-12T11:35:01Z

i've made the requested amendments.

olafbuitelaar added 3 commits April 5, 2022 17:31

* allow Render to have a use resolvable document context

4f6d88b

* cache url's per resolvable context * pass options as config to Renderer * pass renderDocument to render * utility function to resolve the containing window object of a document

* prefer mediaTypes.video.renderer.options over renderer.options

bbc95b8

* support user-provided renderer context * handle when there is no div[id^='google_ads'] element

confics

0108444

ChrisHuie requested a review from dgirardi April 6, 2022 16:37

ChrisHuie assigned dgirardi Apr 6, 2022

ChrisHuie added the feature label Apr 6, 2022

ChrisHuie requested review from mmoschovas and patmmccann and removed request for patmmccann April 6, 2022 16:37

ChrisHuie assigned mmoschovas Apr 6, 2022

ChrisHuie changed the title ~~Render context~~ Prebid Core: add contextResolver callback and allow thte user to supply a different document object to render Apr 6, 2022

kick off circleci tests

0a7d686

ChrisHuie requested a review from robertrmartinez April 6, 2022 16:44

ChrisHuie assigned robertrmartinez Apr 6, 2022

ChrisHuie removed the request for review from robertrmartinez April 6, 2022 16:45

ChrisHuie unassigned robertrmartinez Apr 6, 2022

dgirardi reviewed Apr 6, 2022

View reviewed changes

olafbuitelaar added 2 commits April 7, 2022 10:31

remove debugger

a5dd667

Merge branch 'render_context' of https://github.com/olafbuitelaar/Pre…

2d17a66

…bid.js into render_context

dgirardi added the needs unit tests label Apr 7, 2022

* changed context to doc

a3aa31b

* use WeakMap for caching of url's and documents * flipped key order of url cache * added unit test

olafbuitelaar changed the title ~~Prebid Core: add contextResolver callback and allow thte user to supply a different document object to render~~ Prebid Core: add contextResolver callback and allow the user to supply a different document object to render Apr 8, 2022

olafbuitelaar changed the title ~~Prebid Core: add contextResolver callback and allow the user to supply a different document object to render~~ Prebid Core: add documentResolver callback and allow the user to supply a different document object to render Apr 8, 2022

dgirardi reviewed Apr 8, 2022

View reviewed changes

olafbuitelaar added 2 commits April 8, 2022 20:00

* added adLoader double document cache test

91bef73

* ensure call's are uneven

1b10e89

dgirardi approved these changes Apr 8, 2022

View reviewed changes

marked doc parameter optional

58b7af1

mmoschovas requested changes Apr 12, 2022

View reviewed changes

* change var to const

6b14f5e

* drop IE support

mmoschovas approved these changes Apr 12, 2022

View reviewed changes

dgirardi removed the needs unit tests label Apr 12, 2022

dgirardi merged commit 2d14b7d into prebid:master Apr 12, 2022

dgirardi mentioned this pull request Apr 28, 2022

Merge master into prebid-7 #8336

Merged

saxsir mentioned this pull request Oct 5, 2022

update: bid-strap use prebid v6.29.3 voyagegroup/Prebid.js#29

Closed

10 tasks

Conversation

olafbuitelaar commented Apr 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Type of change

Description of change

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dgirardi Apr 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

olafbuitelaar commented Apr 8, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

olafbuitelaar commented Apr 12, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Comments

olafbuitelaar commented Apr 5, 2022 •

edited

Loading

dgirardi Apr 7, 2022 •

edited

Loading