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

DuckPlayer updates and rewrites #3455

Open
wants to merge 34 commits into
base: main
Choose a base branch
from
Open

Conversation

afterxleep
Copy link
Collaborator

@afterxleep afterxleep commented Oct 18, 2024

Task/Issue URL: https://app.asana.com/0/0/1208563377656003/f
Tech Design URL:
CC:

Description:

DuckPlayer Updates

  • Updates and streamlines DuckPlayer logic to be based on URL changes exclusively, which removes all decidePolicy logic and more stuff from TabViewController 🎉
  • Adds New Tests to cover for more navigation scenarios
  • Reviews and fixes DuckPlayer Pixel logic
  • Removes DuckPlayer Launch Experiment

Open In New tab Updates

  • Adds Open in New Tab Settings and pixel placeholders

Steps to test this PR:

Unit tests cover base DuckPlayer navigation scenarios, but you can:

  1. Set DuckPlayer to Always Ask
  2. Smoke Test links from SERP and YouTube Website
  3. Set DuckPlayer to 'Always'
  4. Smoke Test links from SERP and YouTube Website

Note: No new tab functionality is present in this PR.

Copy link

github-actions bot commented Oct 18, 2024

Warnings
⚠️ PR has more than 500 lines of code changing. Consider splitting into smaller PRs if possible.
Messages
📖

You seem to be updating localized strings. Make sure that you request translations and include translated strings before you ship your change. See Localization Guidelines for more information.

Generated by 🚫 dangerJS against 419a253

@Bunn
Copy link
Contributor

Bunn commented Oct 21, 2024

Code LGTM, but there's an issue I can reproduce on device, details here

# Conflicts:
#	DuckDuckGo.xcodeproj/project.pbxproj
#	DuckDuckGo.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved
#	DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandler.swift
#	DuckDuckGo/DuckPlayer/DuckPlayerNavigationHandling.swift
#	DuckDuckGo/TabViewController.swift
#	DuckDuckGoTests/YoutublePlayerNavigationHandlerTests.swift
// This is to be used in DecidePolicy For to prevent the webView
// from opening the Youtube app on user-triggered links
@MainActor
func shouldCancelNavigation(navigationAction: WKNavigationAction, webView: WKWebView) -> Bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add a test case for this?

Copy link
Contributor

@Bunn Bunn left a comment

Choose a reason for hiding this comment

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

Changes fixed the issue I was having

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants