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.

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

Improve Facebook link unwrapping #2033

Closed
wants to merge 3 commits into from

Conversation

bcyphers
Copy link
Contributor

@bcyphers bcyphers commented May 23, 2018

This addresses a few issues that have come up since we merged the feature. Some of the issues are pulled from https://github.com/mgziminsky/FacebookTrackingRemoval.

  • StopPropagation calls are replaced by StopImmediatePropagation
  • Only nodes of type ELEMENT_NODE are "cleaned"
  • The unwrapping script now runs on Facebook's .onion domain as well

Closes #2012, closes #2014, closes #2018.

@bcyphers
Copy link
Contributor Author

This has some conflicts with #2016 -- @ghostwords should I just merge these changes into that PR?

@ghostwords
Copy link
Member

ghostwords commented May 23, 2018

Yeah, if that makes things easier.

By the way, suggest making several small(er) commits instead of monolithic "bug fixes" commits. I mean it's a balance, but it makes sense if it makes things clearer (can review one change at a time in isolation) and adds flexibility (can back out individual pieces without needing to rewrite the whole). Always easier to squash unnecessarily-detailed commits than to break up a confusing commit.

@bcyphers
Copy link
Contributor Author

bcyphers commented Jun 5, 2018

I incorporated the changes here into #2016, so this branch is closed.

@bcyphers bcyphers closed this Jun 5, 2018
@ghostwords ghostwords deleted the facebook-stop-immediate-propagation branch June 6, 2018 15:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants