Skip to content

AOL adapter - dropping pixels when creatives in safe frames.#2816

Closed
vzhukovsky wants to merge 1 commit intoprebid:masterfrom
aol:contrib/aol-pixels-in-safeframes
Closed

AOL adapter - dropping pixels when creatives in safe frames.#2816
vzhukovsky wants to merge 1 commit intoprebid:masterfrom
aol:contrib/aol-pixels-in-safeframes

Conversation

@vzhukovsky
Copy link
Contributor

@vzhukovsky vzhukovsky commented Jul 4, 2018

Type of change

  • Bugfix

Description of change

Fixed dropping pixels when creatives are served into safe frames.

expect(formattedPixels).to.equal(
'<script>var w=window,prebid;' +
'for(var i=0;i<10;i++){w = w.parent;prebid=w.$$PREBID_GLOBAL$$;' +
'try{for(var i=0;i<10;i++){w = w.parent;prebid=w.$$PREBID_GLOBAL$$;' +
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1.0 compliant adapters are not permitted to reference $$PREBID_GLOBAL$$ and not permitted to drop their own pixels. Looks like this code has been here awhile but is not it is not compliant with the 1.0 adapter rules

Copy link
Contributor Author

@vzhukovsky vzhukovsky Jul 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mike-chowla
Hi. Thank you for reviewing.

It has been there since we updated our adapter for 1.0. In migration PR we provided more details about this feature (#1693) and it was accepted.

Actually, we don't drop own pixels. It is exactly the same pixels that are dropped via getUserSyncs but attached to the ad content. This feature allows to avoid making extra user syncs request and as a result, reduce the load on the server.

We would be happy if there was something similar in Prebid official.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually rejected the initial pull-request partially because of the custom pixels and it doesn't look like it was resolved before being merged. I still feel the same way I did then but I'm fine with being overruled; I'll assign @mkendall07 to review and see with how he feels (if this needs to be fixed now or not).

@snapwich snapwich requested a review from mkendall07 July 19, 2018 20:45
Copy link
Contributor

@mkendall07 mkendall07 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd reject this because no adapter should be touching the $$PREBID_GLOBAL$$ variable. The pixel dropping inside the creative is allowed since we haven't explicitly restricted it.

@stale
Copy link

stale bot commented Aug 8, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Aug 8, 2018
@vzhukovsky
Copy link
Contributor Author

@mkendall07
We support feature that allows dropping the pixels only once (Container has the same for official getUserSyncs functionality). Unfortunately, using globals is the only one way to do.

I'd say let's keep as is with using globals here until the container explicitly restricts dropping pixels inside the creatives.

@stale stale bot removed the stale label Aug 9, 2018
@mkendall07
Copy link
Contributor

You still need to remove references to $$PREBID_GLOBAL$$

@stale
Copy link

stale bot commented Aug 23, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Aug 23, 2018
@stale stale bot closed this Aug 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

Comments