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.

Sign up for GitHub

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

Jump to bottom

fix: treat resume as start if sessions lost, trigger resume if needed #927

Merged
merged 17 commits into from
Feb 25, 2025
Merged

fix: treat resume as start if sessions lost, trigger resume if needed #927

merged 17 commits into from
Feb 25, 2025

Conversation

sidvishnoi
Copy link
Member

@sidvishnoi sidvishnoi commented Feb 19, 2025 •
edited
Loading

Context

Closes #841
Closes #871
More context: #871 (comment)

Changes proposed in this pull request

  • Make ResumeMonetizationPayload to be same as StartMonetizationPayload
  • When resume is called and background doesn't have sessions, use that payload to trigger a "start monetization". This addresses back-forward navigation sessions cleared issue.
  • To address resume on sleep-clearing-all-sessions-in-background (here we don't have a payload we can trigger start from), pass REQUEST_RESUME_MONETIZATION message to content script to call resume in content, which will trigger the start.
  • Trigger this resume on in background.start().

Testing

See #871 (comment) for how to reproduce. The bf-cache one is covered by E2E tests, but sleep one requires manual testing. I've tested following scenarios (with computer sent to sleep around 60-70 times 😆):

  • Press Win+X U S to put computer to sleep (triggers window focus lose typically), Click start menu -> power -> sleep (doesn't trigger window focus lose typically)
  • Keep monetized open when sending computer to sleep; keep window minimized when sending to sleep
  • Keep a non-monetized tab active (window in view/minimized both cases)

@github-actions github-actions bot added area: content Improvements or additions to extension content script area: background Improvements or additions to extension background script area: scripts Improvements or additions to custom scripts area: shared Changes to shared libraries and utilities labels Feb 19, 2025
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Feb 19, 2025 •
edited
Loading

Extension builds preview

Name Link
Latest commit 76c09d7
Latest job logs Run #13495863162
BadgeDownload
BadgeDownload

@github-actions github-actions bot removed the area: scripts Improvements or additions to custom scripts label Feb 19, 2025
@github-actions github-actions bot added the area: tests Improvements or additions to tests label Feb 20, 2025
sidvishnoi
sidvishnoi commented Feb 20, 2025
View reviewed changes
Copy link
Member Author

@sidvishnoi sidvishnoi left a comment

Choose a reason for hiding this comment

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

test-e2e

@sidvishnoi sidvishnoi marked this pull request as ready for review February 20, 2025 07:47
@sidvishnoi sidvishnoi mentioned this pull request Feb 20, 2025
Open
@sidvishnoi sidvishnoi changed the title fix(bg/monetization): treat resume as start if sessions lost fix: treat resume as start if sessions lost, trigger resume if needed Feb 20, 2025
sidvishnoi
sidvishnoi commented Feb 20, 2025
View reviewed changes
Copy link
Member Author

@sidvishnoi sidvishnoi left a comment

Choose a reason for hiding this comment

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

test-e2e

@DarianM
Copy link
Member

DarianM commented Feb 24, 2025

fix was tested on Windows for Firefox/Chrome/Edge:

  • Win X U S with active tab monetized on minimized
  • keeping monetized browser tab open when sending computer to sleep
  • keeping monetized browser tab minimized when sending to sleep
  • have a non-monetized tab focused, send to sleep, then focus monetized tab back (+ case lose focus with Win X U S)
  • have a non-monetized tab minimized, then focus monetized tab back (+ case lose focus with Win X U S)

also this is a fix for #841 on Firefox/Chrome/Edge

DarianM
DarianM approved these changes Feb 24, 2025 •
edited
Loading
View reviewed changes
@sidvishnoi sidvishnoi merged commit c531e86 into main Feb 25, 2025
9 checks passed
@sidvishnoi sidvishnoi deleted the start-on-resume-if-cleared branch February 25, 2025 06:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DarianM DarianM DarianM approved these changes

@raducristianpopa raducristianpopa Awaiting requested review from raducristianpopa

Assignees
No one assigned
Labels
area: background Improvements or additions to extension background script area: content Improvements or additions to extension content script area: shared Changes to shared libraries and utilities area: tests Improvements or additions to tests
Projects
None yet
Milestone
No milestone
2 participants
@sidvishnoi @DarianM