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

Add a performance entry type for visibility state changes #8206

Merged
merged 1 commit into from Apr 27, 2023

Conversation

noamr
Copy link
Contributor

@noamr noamr commented Aug 18, 2022

The entry is added when the visiblity changes for any reason, and an initial entry is added when the document becomes active.

See w3c/performance-timeline#105
See also w3c/page-visibility#29

This is the solution that was proposed at WebPerfWG (TODO add link to minutes)
See Design review at CTAG

(See WHATWG Working Mode: Changes for more details.)


/browsing-the-web.html ( diff )
/index.html ( diff )
/infrastructure.html ( diff )
/interaction.html ( diff )
/references.html ( diff )

source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
Copy link
Contributor Author

@noamr noamr left a comment

Choose a reason for hiding this comment

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

I removed set the visibility state and instead queued an entry directly from the place where we set an active document, with timestamp 0. I prefer if when adding this we don't change the implementation in a way that adds more visibilitychange events and changes to other specs.

source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

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

Just formatting issues, otherwise looks good.

source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

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

LGTM. Needs the PR template filled out, and then we can merge.

Somewhat favorable Mozilla position

I don't see any Mozillan positions in that thread?

@noamr
Copy link
Contributor Author

noamr commented Aug 24, 2022

LGTM. Needs the PR template filled out, and then we can merge.

Thanks! I'm still working on the WPTs :)

Somewhat favorable Mozilla position

I don't see any Mozillan positions in that thread?

You're right, I saw @dbaron's name (it's from the time when he was still at Mozilla) but it's W3CTag. @dbaron do you remember who from Mozilla was involved in this? /cc @emilio

@emilio
Copy link
Contributor

emilio commented Aug 24, 2022

@sefeng211 does the performance team have any concern with this?

@domenic
Copy link
Member

domenic commented Apr 6, 2023

Ping @sefeng211 for any opinions from Mozilla?

@sefeng211
Copy link
Contributor

This looks good from our side.

@domenic
Copy link
Member

domenic commented Apr 7, 2023

Thanks! @noamr I'll wait to merge this until you give the sign that you're ready to ship this in Chromium, and file appropriate Gecko/WebKit bugs. Also if you could fix the merge conflicts that would be lovely.

@noamr
Copy link
Contributor Author

noamr commented Apr 7, 2023

Thanks! @noamr I'll wait to merge this until you give the sign that you're ready to ship this in Chromium, and file appropriate Gecko/WebKit bugs. Also if you could fix the merge conflicts that would be lovely.

On it. AFAICT the chromium implementation has been ready for a while, will double check and start the I2S thing etc. thanks!

source Outdated
@@ -94920,6 +94993,11 @@ location.href = '#foo';</code></pre>
<span>node navigable</span>'s <span data-x="nav-traversable">traversable navigable</span>'s
<span>system visibility state</span>.</p></li>

<li><p><span data-x="queue a performance entry">Queue</span> a new
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure I get this.. This is queueing an entry when the document becomes active, but AFAICT that doesn't necessarily map to all the places where the visibility state gets updated. (e.g. I don't think this is called when the document is hidden)

Copy link
Contributor Author

@noamr noamr Apr 25, 2023

Choose a reason for hiding this comment

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

Oh maybe something important got lost in the rebase. Let me check

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

@yoavweiss yoavweiss left a comment

Choose a reason for hiding this comment

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

Non-authoritative LGTM

@domenic domenic merged commit b0e00b9 into whatwg:main Apr 27, 2023
2 checks passed
@noamr noamr deleted the visibility-entry branch April 27, 2023 07:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants