-
-
Notifications
You must be signed in to change notification settings - Fork 386
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
Surrogates (fixes #400) #935
Conversation
Chrome currently produces the following warning in dev console: Synchronous XMLHttpRequest on the main thread is deprecated because of its detrimental effects to the end user's experience. For more help, check https://xhr.spec.whatwg.org/.
require.scopes.surrogatedb = (function() { | ||
|
||
const hostnames = { | ||
'www.google-analytics.com': [ |
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 data structure should be better commented.
], | ||
}; | ||
|
||
const surrogates = { |
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.
More documentation needed here as well.
function getSurrogateURI(script_url, script_hostname) { | ||
// do we have an entry for the script hostname? | ||
if (db.hostnames.hasOwnProperty(script_hostname)) { | ||
const hosts = db.hostnames[script_hostname]; |
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.
needs more doc
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.
rename to tokens
// do any of the pattern tokens for that hostname match the script URL? | ||
for (let i = 0; i < hosts.length; i++) { | ||
const token = hosts[i]; | ||
if (script_url.endsWith(token)) { |
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.
Handle query strings e.g.
ga.com/ga.js?foo=bar
|
||
test("surrogate script URL lookups", function() { | ||
// URLs that should have a surrogate | ||
ok(!!getSurrogateURI('http://www.google-analytics.com/ga.js', 'www.google-analytics.com')); |
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.
Test for no surrogate?
Test other surrogates...
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.
test that query strings are handled in a sane way
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.
selenium integration test
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 address the issues I commented on.
3b8cd35
to
a88a76a
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.
Looks great pending finishing the selenium test.
Still needs to be moved to dedicated testing page: #928.
So that it can get auto collected.
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.
Replace avianca with locally hosted page.
Merge master.
Conflicts: src/webrequest.js
Conflicts: src/webrequest.js
Conflicts: src/webrequest.js
More controlled, more reliable, faster. Related to #928.
@cooperq OK, should be good to merge now. Take a look please.
|
@gunesacar Let me know if you have any suggestions to improve the script surrogates integration test. |
Conflicts: src/background.js src/domainExceptions.js src/webrequest.js tests/index.html
Looks good to me! |
It looks pretty good to me. Maybe you could consider adding a test for outbound link tracking: |
WIP pull request for #400
Besides review, what's left is more surrogates and more testing. We should run some queries against the "report broken site" db to get some recent high-profile breakages we can surrogate.