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

Jump to bottom

feat: add WebFrameMain.visibilityState #28706

Merged
merged 5 commits into from Apr 22, 2021
Merged

feat: add WebFrameMain.visibilityState #28706

merged 5 commits into from Apr 22, 2021

Conversation

samuelmaddock
Copy link
Member

@samuelmaddock samuelmaddock commented Apr 16, 2021 •
edited

Description of Change

Adds WebFrameMain.visibilityState instance property which aligns with document.visibilityState in the renderer.

This can be useful for telling whether a frame is visible without needing to execute any JS in the renderer.

Checklist

Release Notes

Notes: Added WebFrameMain.visibilityState instance property.

@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Apr 16, 2021
@samuelmaddock samuelmaddock force-pushed the feat/webframemain-visibilitystate branch from ea5d842 to fde0e6d Compare April 16, 2021 23:16
@samuelmaddock samuelmaddock added the semver/minor backwards-compatible functionality label Apr 16, 2021
jkleinsc
jkleinsc requested changes Apr 20, 2021
View reviewed changes
Copy link
Contributor

@jkleinsc jkleinsc left a comment

Choose a reason for hiding this comment

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

Looks like newly added test is failing on macOS: https://app.circleci.com/pipelines/github/electron/electron/38127/workflows/4af17c41-7d17-47c2-b050-a1c088106dec/jobs/840026

@samuelmaddock
Copy link
Member Author

Looks like newly added test is failing on macOS: https://app.circleci.com/pipelines/github/electron/electron/38127/workflows/4af17c41-7d17-47c2-b050-a1c088106dec/jobs/840026

Thanks, I'll investigate. All of the Goma builds were failing previously.

@nornagon
Copy link
Member

API LGTM

@samuelmaddock
Copy link
Member Author

I've added a delay in the test to fix the issue on macOS. ec9fe90

I'm not happy with this, but I don't think there's any other reliable way to test it without binding another event. Let me know whether this is okay.

jkleinsc
jkleinsc requested changes Apr 21, 2021
View reviewed changes
Copy link
Contributor

@jkleinsc jkleinsc left a comment

Choose a reason for hiding this comment

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

Relying on a timeout will produce flaky results but we can use something like I've suggested here to effectively wait for the change.

spec-main/api-web-frame-main-spec.ts Outdated Show resolved Hide resolved
@samuelmaddock samuelmaddock force-pushed the feat/webframemain-visibilitystate branch from 8715960 to 271a5cc Compare April 22, 2021 03:10
@jkleinsc
Copy link
Contributor

API LGTM

@jkleinsc jkleinsc merged commit 43d27cc into electron:master Apr 22, 2021
@release-clerk
Copy link

release-clerk bot commented Apr 22, 2021

Release Notes Persisted

Added WebFrameMain.visibilityState instance property.

@jkleinsc jkleinsc removed the new-pr 🌱 PR opened in the last 24 hours label Apr 22, 2021
@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Apr 22, 2021
@samuelmaddock samuelmaddock deleted the feat/webframemain-visibilitystate branch April 22, 2021 16:04
@Mario-Kart-Felix Mario-Kart-Felix mentioned this pull request May 17, 2021
Open
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nornagon nornagon nornagon approved these changes

@jkleinsc jkleinsc jkleinsc approved these changes

@miniak miniak miniak approved these changes

Assignees
No one assigned
Labels
api-review/requested 🗳 new-pr 🌱 PR opened in the last 24 hours semver/minor backwards-compatible functionality
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@samuelmaddock @nornagon @jkleinsc @miniak