-
Notifications
You must be signed in to change notification settings - Fork 10
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
Proposes alternative tipping logic #102
Conversation
f75fb2b
to
a66921a
Compare
@jonathansampson Seems like there are still some build errors based on the CI above - sorry if I reviewed before it was ready! Thanks for undertaking this, BTW. |
Addresses brave/brave-browser#32615 |
a66921a
to
9f73f12
Compare
@emerick Yeah, apologies. I just pushed to address some/all of those build issues. |
9f73f12
to
550976d
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.
This is awesome - thanks @jonathansampson!
@@ -7,3 +7,29 @@ export const mediaDomain = 'twitter.com' | |||
|
|||
export const sendHeadersUrls = ['https://api.twitter.com/1.1/*'] | |||
export const sendHeadersExtra = ['requestHeaders', 'extraHeaders'] | |||
|
|||
export type TweetDetails = { | |||
user: { |
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.
nit: 2-spaces indentation.
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.
On a related note, how up-to-date are our linting rules in the project itself? Do we enforce single quotes, two-space tabs, and more? I wasn't having much success with --fix
.
Addresses #100
550976d
to
ec65d60
Compare
@zenparsing Do you know if these scripts are executing in the main world, so as to have access to the React state to begin with? |
I think they run in an isolated world actually, because they're just dynamically-generated content scripts and I don't see them setting the We would probably have to test this out to be 100% sure, though. BTW, I use the following aliases to build and "deploy" Greaselion locally when testing. I thought you might find them helpful (they're geared towards Windows directories at the moment):
|
@jonathansampson @emerick We should definitely test to make sure we can get to the React state from the script, and that everything works as expected. I have some other commitments at the moment, but I'll try to do some testing over the next couple of days. |
Hi @jonathansampson, I tested this out today locally. One thing we should probably do in this PR is revert the following commit which temporarily disabled Twitter-based inline tipping: f0079ae I did that locally but it seems like the React store initialization is failing. I saw the following messages:
Just for testing purposes, I added the following line to the Greaselion service but it didn't appear to have any effect so I'm not sure if there's more to running a content script in the main world:
If we do find a way to run this content script in the main world, we'd have to run that approach by our security folks of course. I'm not sure if this would be a non-starter for them, but it might make sense to start having that conversation now just in case. If there's something you'd like me to inspect while running in this mode, let me know! |
I had a productive call with @emerick this morning to take a closer look at the extension. From our cursory review, it seems clear that the extension logic works. That said, we identified a couple obstacles. MV3 RouteFirst, we currently register grease lion scripts via manifest v2. In order to easily execute the updated script in the main world, we would need to register it with v3 (which adds support for a There are a couple other migration-related changes that would need to be made (e.g., around detection of an incognito window context) as well. As such, before this work can proceed we need to do the following:
MV2 RouteIt is also possible to proceed with mv2, but we would need to create a background script for this extension, and register the content script(s) via This approach would also require a change to our manifest-creation process, specifying a
|
Please open a security review for this. |
No longer relevant as we've removed the inline Tip buttons feature entirely. |
Addresses #100
The goal here is to retrieve from the front-end data that was previously retrieved via the API.
It is important to note that this script must be ran in the main world, and not in an isolated world. If it is ran in an isolated world it will not have access to React properties.
Here is an example of the input/output: