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

Only fire Duck Player navigational pixel for main frame navigations #3599

Merged
merged 1 commit into from
Nov 26, 2024

Conversation

ayoy
Copy link
Collaborator

@ayoy ayoy commented Nov 26, 2024

Task/Issue URL: https://app.asana.com/0/1203249713006009/1208836976436118/f

Description:
This change limits Duck Player navigational pixels to be sent only on main frame navigations

Steps to test this PR:
See this Asana comment for more context of the breakage.

  1. Run the app from Xcode
  2. Filter logs by "PixelKit" category
  3. Set DuckPlayer to "always ask" mode
  4. Go to youtube.com, search for video, open video page
  5. Verify that refreshing doesn't trigger navigation_back or navigation_outside_youtube pixels

Definition of Done:


Internal references:

Pull Request Review Checklist
Software Engineering Expectations
Technical Design Template
Pull Request Documentation

@ayoy ayoy requested a review from Bunn November 26, 2024 14:55
Copy link
Collaborator

@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.

LGTM, btw, the refresh button gets disabled in duck player, so I just selected the address bar and pressed enter to force refresh

@ayoy
Copy link
Collaborator Author

ayoy commented Nov 26, 2024

@Bunn sorry, I was referring to refreshing the YouTube video page, not Duck Player.

@Bunn
Copy link
Collaborator

Bunn commented Nov 26, 2024

@Bunn sorry, I was referring to refreshing the YouTube video page, not Duck Player.

All good, I tested this case as well.
Refreshing youtube triggers:
m_mac_duck-player_overlay_youtube_impressions
m_mac_duckplayer_youtube_overlay_navigation_refresh

but not the ones mentioned in the description

@ayoy ayoy merged commit 12e5529 into release/1.116.0 Nov 26, 2024
41 checks passed
@ayoy ayoy deleted the dominik/duck-player-pixels-main-frame branch November 26, 2024 19:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants