Skip to content
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.

Sign up for GitHub

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

Enhance location detection within utils #2167

Merged
merged 23 commits into from
Mar 26, 2018
+173 −14
Merged

Enhance location detection within utils #2167

Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
c0c28d7
fix sovrn dealid
Feb 8, 2018
d7df108
Merge branch 'master' of https://github.com/prebid/Prebid.js
aprakash-sovrn Feb 9, 2018
0700e91
Merge branch 'master' of https://github.com/prebid/Prebid.js
aprakash-sovrn Feb 14, 2018
cf0ca21
send 'iv' param if present
aprakash-sovrn Feb 14, 2018
f89135c
`page` field in `site` object sends full url
aprakash-sovrn Feb 14, 2018
be091fd
Merge pull request #1 from sovrn/HS-303_Send_iv_param
aprakash-sovrn Feb 14, 2018
af86376
Merge remote-tracking branch 'upstream/master'
Feb 16, 2018
c2f2350
Enhance location detection within util function.
Feb 16, 2018
4867c53
Merge pull request #3 from sovrn/enhanced-location-detection
Feb 20, 2018
28f3e0e
fix func call
Feb 20, 2018
497723b
fix tagid var type
aprakash-sovrn Feb 22, 2018
f90b2e0
add test for converting tagid to string
aprakash-sovrn Feb 23, 2018
2e9e413
Merge branch 'master' of https://github.com/prebid/Prebid.js
aprakash-sovrn Feb 23, 2018
74ed7f9
merge upstream master
aprakash-sovrn Feb 26, 2018
1269a33
CR
Mar 1, 2018
55284d5
Merge pull request #4 from sovrn/enhanced-location-detection
Mar 1, 2018
3c2f8bc
Reorg gettopwindowlocation util & add tests.
Mar 8, 2018
fee1789
Merge branch 'master' into enhanced-location-detection
Mar 12, 2018
cd78968
Merge pull request #5 from sovrn/enhanced-location-detection
Mar 12, 2018
68a8afa
lint fixes
Mar 12, 2018
35b3f4c
Merge remote-tracking branch 'upstream/master'
Mar 12, 2018
b2bc2a4
search param is string in window location objects
Mar 12, 2018
7da5620
Merge branch 'master' into master
Mar 26, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions modules/sovrnBidAdapter.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,23 +22,24 @@ export const spec = {
* @return object of parameters for Prebid AJAX request
*/
buildRequests: function(bidReqs) {
const loc = utils.getTopWindowLocation();
let sovrnImps = [];
let iv;
utils._each(bidReqs, function (bid) {
iv = iv || utils.getBidIdParameter('iv', bid.params);
sovrnImps.push({
id: bid.bidId,
banner: { w: 1, h: 1 },
tagid: utils.getBidIdParameter('tagid', bid.params),
tagid: String(utils.getBidIdParameter('tagid', bid.params)),
bidfloor: utils.getBidIdParameter('bidfloor', bid.params)
});
});
const sovrnBidReq = {
id: utils.getUniqueIdentifierStr(),
imp: sovrnImps,
site: {
domain: window.location.host,
page: window.location.host + window.location.pathname + location.search + location.hash
domain: loc.host,
page: loc.host + loc.pathname + loc.search + loc.hash
}
};
if (iv) sovrnBidReq.iv = iv;
Expand Down
1 change: 1 addition & 0 deletions src/url.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ export function parse(url, options) {
parsed.href = decodeURIComponent(url);
}
return {
href: parsed.href,
protocol: (parsed.protocol || '').replace(/:$/, ''),
hostname: parsed.hostname,
port: +parsed.port,
Expand Down
46 changes: 39 additions & 7 deletions src/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ import { config } from './config';
import clone from 'just-clone';
import find from 'core-js/library/fn/array/find';
import includes from 'core-js/library/fn/array/includes';
import { parse } from './url';

var CONSTANTS = require('./constants');

var _loggingChecked = false;
Expand Down Expand Up @@ -157,17 +159,47 @@ export function parseGPTSingleSizeArray(singleSize) {
}
};

exports.getTopWindowLocation = function () {
let location;
export function getTopWindowLocation() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Several modules rely on this function, changing it may introduce subtle differences. Any reason to not keep the existing getTopWindowLocation as is, and create a new function for this case?

Copy link
Contributor Author

@rachelrj rachelrj Mar 1, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

The old function getTopWindowLocation either returns window.top.location or returns window.location.

The new getTopWindowLocation returns window.location, window.top.location, window.document.referrer, window.document.location.ancestorOrigins[0], or an object that recreates the values within window.location

The new version attempts to get the same info, just from additional objects.

I checked the modules, and I think this new version should fit all the current use cases.

I am not opposed to creating a new function if you’re worried about it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok, thanks for the explanation. Changes to core require a second review, assigning that now

if (exports.inIframe()) {
return getIframeParentLoc();
} else {
return window.location;
}
}

const getIframeParentLoc = function() {
let loc = window.location;
try {
// force an exception in x-domain enviornments. #1509
window.top.location.toString();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Given the comment, not sure removing this is the correct thing to do, unless there's an equivalent fix in the new code i'm not seeing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I didn't realize that the exception wasn't thrown when trying to access window.top until I read that 1509 PR. Interesting. Thanks for pointing out. I added back in.

location = window.top.location;
if (window.$sf) {
loc = window.document.referrer;
} else {
if (window.document.location && window.document.location.ancestorOrigins &&
window.document.location.ancestorOrigins.length >= 1) {
loc = window.document.location.ancestorOrigins[window.document.location.ancestorOrigins.length - 1];
} else if (window.document.location) {
// force an exception in x-domain environments. #1509
window.top.location.toString();
loc = getNonWebKitIframeParentLoc();
}
}
loc = parse(loc);
} catch (e) {
location = window.location;
this.logMessage('getTopParentLoc failure', e);
}
return loc;
};

return location;
const getNonWebKitIframeParentLoc = function() {
let referrerLoc = '';
let currentWindow;
do {
currentWindow = currentWindow ? currentWindow.parent : window;
if (currentWindow.document && currentWindow.document.referrer) {
referrerLoc = currentWindow.document.referrer;
Copy link
Member

Choose a reason for hiding this comment

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

won't this return a string when an object is expected?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Returns a string to getIframeParentLoc which then gets sent to parse which turns the string into the expected object.

}
}
while (currentWindow !== window.top);
return referrerLoc;
};

exports.getTopWindowUrl = function () {
Expand Down
20 changes: 20 additions & 0 deletions test/spec/modules/sovrnBidAdapter_spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,26 @@ describe('sovrnBidAdapter', function() {

expect(request.data).to.contain('"iv":"vet"')
})

it('converts tagid to string', () => {
const ivBidRequests = [{
'bidder': 'sovrn',
'params': {
'tagid': 403370,
'iv': 'vet'
},
'adUnitCode': 'adunit-code',
'sizes': [
[300, 250]
],
'bidId': '30b31c1838de1e',
'bidderRequestId': '22edbae2733bf6',
'auctionId': '1d1a030790a475'
}];
const request = spec.buildRequests(ivBidRequests);

expect(request.data).to.contain('"tagid":"403370"')
})
});

describe('interpretResponse', () => {
Expand Down