Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Fire pixel without measure #3
Fire pixel without measure #3
Changes from 2 commits
7df1c4b
05ec12c
8439525
7ba5778
17a4058
35a9e38
b31c0f9
7a94f13
22f2532
b03709b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
QSERVE_URL
? This isn't really a domain.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.
Question: Do we want to use a protocol relative URL instead? I mean maybe not these days, but it is common practice.
Note: If we did, it would be
QSERVE_URL = "//pixel.quantserve.com/pixel";
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.
Hmm, the advantage of this is that the URL would have the same protocol as the site it is on (versus always being https), right? I think that makes sense.
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 think we need to use secure these days for certain cookie based stuff to work in Chrome? (might be mistaken about that). I guess that's less of a concern here, although i suppose these requests will set 3rd party cookies where they can.
My vote would just be to keep this secure i think. Does that ever cause issues if done on a non-secure site?
We should use
QSERVE_URL
as Scott suggests though.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.
somewhere we need to be checking we have a valid clientId before we set fpa or call quantserve
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 add tests to cover this)
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.
So this isn't your fault and this is in suit with the current status-quo in Prebid.js, but this is a little maddening.
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.
@ssmccoy - Yeah, this could have been one common const somewhere.
Btw, Any objections to also change 86400000 to 60 * 60 * 24 * 1000? I think it is at least a little clearer.
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 just had removed 'et' and 'tzo' in the previous commit - b31c0f9
Only cause the general consensus currently is that we want to keep the number of parameters we send through prebid minimum in our first iteration unless we have strong justification for needing 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.
is coppaDataHandler always available?
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, coppaDataHandler is a Prebid utility function. It checks for Coppa configuration parameter that the publisher is expected to pass. When not present, it returns undefined.