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: pageVisibility state when backgroundThrottling disabled #39223

Merged
merged 1 commit into from
Jul 28, 2023
Merged

fix: pageVisibility state when backgroundThrottling disabled #39223

merged 1 commit into from
Jul 28, 2023

Conversation

codebytere
Copy link
Member

Description of Change

Closes #39165.
Refs CL:2082213

Fixes an issue where the pageVisibility API returned incorrect values in some situations when backgroundThrottling was disabled. This happened when page lifecycle state began being managed on the browser side - our existing patch no longer covered all paths that changed the page visibility state. This should address that by ensuring that the page visibility is always visible when backgroundThrottling is disabled, and that the visibilitychange event is no longer emitted.

Also allows us to re-enable the associated test.

Checklist

Release Notes

Notes: Fixed an issue where the pageVisibility API returned incorrect values in some situations when backgroundThrottling was disabled.

@codebytere codebytere added semver/patch backwards-compatible bug fixes target/24-x-y PR should also be added to the "24-x-y" branch. target/25-x-y PR should also be added to the "25-x-y" branch. target/26-x-y PR should also be added to the "26-x-y" branch. labels Jul 25, 2023
@codebytere codebytere requested a review from a team as a code owner July 25, 2023 09:13
@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Jul 25, 2023
@codebytere codebytere changed the title fix: pageVisibility state when backgroundThrottling disabled fix: pageVisibility state when backgroundThrottling disabled Jul 25, 2023
@codebytere codebytere force-pushed the page-visibility-fix branch 2 times, most recently from 452e4b2 to e0b922c Compare July 25, 2023 13:21
xiaofeihan1
xiaofeihan1 reviewed Jul 25, 2023
View reviewed changes
patches/chromium/allow_disabling_blink_scheduler_throttling_per_renderview.patch Show resolved Hide resolved
patches/chromium/allow_disabling_blink_scheduler_throttling_per_renderview.patch
- observer.OnPageVisibilityChanged(visibility_state);
+
+ // If backgroundThrottling is disabled, the page is always visible.
+ if (!scheduler_throttling_allowed_) {
Copy link

Choose a reason for hiding this comment

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

The following style would reduce the number of lines of change and perhaps make the code clearer as well.

 if (!scheduler_throttling_allowed_) {
       GetPage()->SetVisibilityState(mojom::blink::PageVisibilityState::kVisible, is_initial_state);
       GetPage()->GetPageScheduler()->SetPageVisible(true);
       return;
 }

@codebytere
Copy link
Member Author

Looks like this test surfaced a DCHECK that was present prior to this PR in render_widget_host_view_aura.cc - I'll think about a best approach here though we may need to just disable it.

@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Jul 26, 2023
@codebytere codebytere requested a review from zcbenz July 27, 2023 10:15
@codebytere codebytere merged commit 6df3921 into main Jul 28, 2023
17 checks passed
@codebytere codebytere deleted the page-visibility-fix branch July 28, 2023 08:48
@release-clerk
Copy link

release-clerk bot commented Jul 28, 2023

Release Notes Persisted

Fixed an issue where the pageVisibility API returned incorrect values in some situations when backgroundThrottling was disabled.

@trop
Copy link
Contributor

trop bot commented Jul 28, 2023

I was unable to backport this PR to "24-x-y" cleanly;
you will need to perform this backport manually.

@trop trop bot removed the target/24-x-y PR should also be added to the "24-x-y" branch. label Jul 28, 2023
@trop
Copy link
Contributor

trop bot commented Jul 28, 2023

I was unable to backport this PR to "26-x-y" cleanly;
you will need to perform this backport manually.

@trop
Copy link
Contributor

trop bot commented Jul 28, 2023

I was unable to backport this PR to "25-x-y" cleanly;
you will need to perform this backport manually.

@trop trop bot added needs-manual-bp/26-x-y needs-manual-bp/25-x-y and removed target/26-x-y PR should also be added to the "26-x-y" branch. target/25-x-y PR should also be added to the "25-x-y" branch. labels Jul 28, 2023
@codebytere codebytere mentioned this pull request Jul 31, 2023
Merged
@trop
Copy link
Contributor

trop bot commented Jul 31, 2023

@codebytere has manually backported this PR to "26-x-y", please check out #39298

@codebytere codebytere mentioned this pull request Jul 31, 2023
Merged
@trop
Copy link
Contributor

trop bot commented Jul 31, 2023

@codebytere has manually backported this PR to "25-x-y", please check out #39299

@trop trop bot added in-flight/25-x-y merged/26-x-y PR was merged to the "26-x-y" branch. merged/25-x-y PR was merged to the "25-x-y" branch. and removed needs-manual-bp/25-x-y in-flight/26-x-y in-flight/25-x-y labels Jul 31, 2023
win32ss pushed a commit to win32ss/supermium-electron that referenced this pull request Sep 24, 2023
@codebytere
fix: pageVisibility state when backgroundThrottling disabled (ele…
2bcb25e 
…ctron#39223)

fix: pageVisibility state when backgroundThrottling disabled
@commit-lint commit-lint bot mentioned this pull request Oct 6, 2023
Open
@samu samu mentioned this pull request Oct 27, 2023
Closed
3 tasks
MrHuangJser pushed a commit to MrHuangJser/electron that referenced this pull request Dec 11, 2023
@codebytere
fix: pageVisibility state when backgroundThrottling disabled (ele…
353cf58 
…ctron#39223)

fix: pageVisibility state when backgroundThrottling disabled
@beyondkmp beyondkmp mentioned this pull request Feb 3, 2024
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xiaofeihan1 xiaofeihan1 xiaofeihan1 left review comments

@zcbenz zcbenz zcbenz approved these changes

@deepak1556 deepak1556 Awaiting requested review from deepak1556

Assignees
No one assigned
Labels
merged/25-x-y PR was merged to the "25-x-y" branch. merged/26-x-y PR was merged to the "26-x-y" branch. semver/patch backwards-compatible bug fixes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Bug]: visibility state can be changed to hidden when backgroundThrottling is disabled
3 participants
@codebytere @zcbenz @xiaofeihan1