Skip to content
This repository has been archived by the owner on May 10, 2024. It is now read-only.

Fix #511: Youtube ads #2655

Merged
merged 4 commits into from
Jul 3, 2020
Merged

Fix #511: Youtube ads #2655

merged 4 commits into from
Jul 3, 2020

Conversation

Brandon-T
Copy link
Collaborator

@Brandon-T Brandon-T commented Jun 22, 2020

Summary of Changes

Added Javascript injection into the MainFrame only (same as Desktop).. to block youtube ads. The JS is injected into EVERY page and not just the youtube domain which can come in multiple forms: youtube.com, youtu.be, etc..

If we want it only on youtube domain, we can add it whenever. Just let me know.

This pull request fixes #511

Submitter Checklist:

  • Unit Tests are updated to cover new or changed functionality
  • User-facing strings use NSLocalizableString()

Test Plan:

Screenshots:

Reviewer Checklist:

  • Issues include necessary QA labels:
    • QA/(Yes|No)
    • release-notes/(include|exclude)
    • bug / enhancement
  • Necessary security reviews have taken place.
  • Adequate unit test coverage exists to prevent regressions.
  • Adequate test plan exists for QA to validate (if applicable).
  • Issue is assigned to a milestone (should happen at merge time).

Copy link
Contributor

@iccub iccub left a comment

Choose a reason for hiding this comment

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

Looks good, left small comments.

let isPrivateBrowsing = PrivateBrowsingManager.shared.isPrivateBrowsing

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a comment referencing the ticket we are fixing?

I think we should use this old ticket for it
#511

let isPrivateBrowsing = PrivateBrowsingManager.shared.isPrivateBrowsing

if url.baseDomain == "youtube.com" {
Copy link
Contributor

Choose a reason for hiding this comment

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

the WKNavDelegate getting bigger and bigger :(

Nothing to do now, but if we add another site specific hack for adblocking we should abstract it somewhere, maybe in Tab or in userScriptManager itself

@iccub iccub changed the title Added adblock JS Fix #511: Youtube ads Jun 22, 2020
@iccub iccub added blocked If a ticket is blocked for some reason, if not using a sub-block label, please provide info in issue and removed blocked If a ticket is blocked for some reason, if not using a sub-block label, please provide info in issue labels Jun 24, 2020
@iccub
Copy link
Contributor

iccub commented Jun 26, 2020

Unblocking this, turns out the script works well, no more adjustments are needed.
We may consider changing architecture to put 'if-youtube' code directly into the script

@iccub iccub merged commit 3a61a6d into development Jul 3, 2020
@iccub iccub deleted the feature/AdblockJS branch July 3, 2020 15:22
iccub pushed a commit that referenced this pull request Jul 3, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Youtube pre-roll ads are not blocked
2 participants