Skip to content
This repository has been archived by the owner on Apr 21, 2023. It is now read-only.

Prefer Paint Timing API over chrome.loadTimes() #1802

Closed
wants to merge 14 commits into from
Closed

Prefer Paint Timing API over chrome.loadTimes() #1802

wants to merge 14 commits into from

Conversation

injust
Copy link
Contributor

@injust injust commented Oct 14, 2018

Copy link
Contributor

@jmarantz jmarantz left a comment

Choose a reason for hiding this comment

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

Were you able to rebuild mod_pagespeed with this change and try it out? DId it fix the problem for you with the deprecation warnings?

net/instaweb/rewriter/add_instrumentation.js Outdated Show resolved Hide resolved
net/instaweb/rewriter/add_instrumentation.js Outdated Show resolved Hide resolved
net/instaweb/rewriter/add_instrumentation.js Outdated Show resolved Hide resolved
@oschaaf
Copy link
Member

oschaaf commented Oct 16, 2018

This looks good to me; did you get a chance to test this?
Is this ready for merging?

Copy link
Contributor

@jmarantz jmarantz left a comment

Choose a reason for hiding this comment

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

Thanks for doing this -- this is great stuff so far.

To clarify my previous question -- when I asked whether this compiles -- I meant in the mod_pagepseed build process.

What happens in the build process is that this file gets built by a javascript compiler called 'closure compiler' into a faster/smaller version of itself. The resultant JS file is then compiled into a C++ source file that represents the code as C strings, so it can be served directly from the module without having to find it on disk.

This whole flow is automated by the mod_pagespeed build system. But it looks to me like some of the JS you have here might not make the Closure Compiler happy (missing var declarations mainly).

I also don't know how you could test this fully without doing that. But if you actually have done that -- and I'm wrong about the missing var declarations causing the compile to fail -- then apologies :)

net/instaweb/rewriter/add_instrumentation.js Outdated Show resolved Hide resolved
net/instaweb/rewriter/add_instrumentation.js Outdated Show resolved Hide resolved
net/instaweb/rewriter/add_instrumentation.js Outdated Show resolved Hide resolved
@injust
Copy link
Contributor Author

injust commented Dec 10, 2018

Just tested with the mod_pagespeed build process. Indeed, Closure Compiler doesn't like the omission of var declarations.

Right now, getEntriesByType('paint') always returns an empty array. The entries only populate after the load event for window. Is it possible to defer running the entire instrumentation block?

I noticed that the build process uses pre-optimized files in net/instaweb/genfiles/rewriter, which will need to be updated. How were those initially generated?

@oschaaf
Copy link
Member

oschaaf commented Jul 28, 2020

@injust is this good to go as far as you are concerned?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add_instrumentation filter uses "chrome.loadTimes" instead of the API Paint Timing
3 participants