Yubico ads conversion script #46
Conversation
|
Check for enabled ads is missing |
saberistic
changed the title
Greaselion.json
Outdated
| }, | ||
| { | ||
| "urls": [ | ||
| "https://*yubico.com*/checkout/confirmation*" |
There was a problem hiding this comment.
The stars in the host portion of this url seem problematic. Wouldn't that end up running this script on potentially unrelated sites?
There was a problem hiding this comment.
pushed a change to target domain and subdomains explicitly
There was a problem hiding this comment.
Is the .com* intentional? It doesn't seem like you need a * there, but could be I just don't understand the goal.
There was a problem hiding this comment.
"https://*.yubico.com/*/checkout/confirmation*",
"https://yubico.com/*/checkout/confirmation*"
moved the asterisk to after domain separation from address
| const link = document.createElement('meta') | ||
| link.setAttribute('name', 'ad-conversion-id') | ||
| link.content = orderId | ||
| document.getElementsByTagName('head')[0].appendChild(link) |
There was a problem hiding this comment.
Probably would be good to do some minimal null checks here.
There was a problem hiding this comment.
what line could possibly go null?
There was a problem hiding this comment.
Is getElementsByTagName('head') guaranteed to have an element? I think that's the only one I was concerned about, but maybe it's fine.
| "scripts/brave_rewards/ads/conversions/yubico.bundle.js" | ||
| ], | ||
| "preconditions": { | ||
| "ads-enabled": true |
There was a problem hiding this comment.
@emerick did we ever add the feature to disable greaselion scripts in incognito by default?
There was a problem hiding this comment.
It hasn't been implemented yet (brave/brave-browser#11409), I'll bring it up at our triage meeting this week!
There was a problem hiding this comment.
@emerick will a script with ads-enabled true get injected into incognito/tor windows?
There was a problem hiding this comment.
I believe so, as it seems that supporting incognito mode is the default for extensions.
There was a problem hiding this comment.
Actually, I didn't realize that we've recently made the following change to the Greaselion service to disallow incognito injection: brave/brave-core@6bbdfb1. So I think we should be good on that front.
Greaselion.json
Outdated
| { | ||
| "urls": [ | ||
| "https://*.yubico.com*/checkout/confirmation*", | ||
| "https://yubico.com*/checkout/confirmation*" |
There was a problem hiding this comment.
what's the code that parses these match patterns? would these match https://yubico.com.example.com for instance?
There was a problem hiding this comment.
also mentioned by @emerick in above comments, moved asterisk to after /
| const el = document.getElementsByClassName('order-info') | ||
| let orderId = '' | ||
| if (el && el[0] && el[0].firstChild && el[0].firstChild.firstChild) { | ||
| orderId = el[0].firstChild.firstChild.textContent!.replace('Order Number: ', '') |
There was a problem hiding this comment.
will this work for non-English versions of the page? (assuming they have localization)
| @@ -28,6 +28,7 @@ module.exports = (env, argv) => { | |||
| ['scripts/brave_rewards/publisher/vimeo/vimeoAutoContribution']: './scripts/brave_rewards/publisher/vimeo/vimeoAutoContribution', | |||
| ['scripts/brave_rewards/publisher/youtube/youtubeBase']: './scripts/brave_rewards/publisher/youtube/youtubeBase', | |||
| ['scripts/brave_rewards/publisher/youtube/youtubeAutoContribution']: './scripts/brave_rewards/publisher/youtube/youtubeAutoContribution', | |||
| ['scripts/brave_rewards/ads/conversions/yubico']: './scripts/brave_rewards/ads/conversions/yubico', | |||
There was a problem hiding this comment.
should have a wiki page to document why this script is injected. we already have users complaining that the greaselion injected scripts are not transparent enough and can't be disabled.
There was a problem hiding this comment.
I have a wiki update page written down in the spec, awaiting for the final approval and copy edits on it
|
Yubico has agreed to work on this from their end: https://bravesoftware.slack.com/archives/CJMAD25L7/p1616449958005200?thread_ts=1616446792.001900&cid=CJMAD25L7. Can we close this out for now? |
|
Closing this in favor of better approach |
No description provided.