Resolve navTiming variable earlier if possible#23580
Resolve navTiming variable earlier if possible#23580zhouyx merged 4 commits intoampproject:masterfrom
Conversation
|
I'm a bit concerned about the 0.16KB size increase. I don't think that I can avoid it because now that I have to hardcode the navTiming events to v0. Worth running an experiment with activeView ping to see if the change is beneficial. |
|
@jeffkaufman Please also let me know how do you want to run the experiment, and if I need to add check for meta tag. Thanks |
|
Talked to @jeffkaufman offline, we don't need metaTag for experiment. |
|
|
||
| const NAV_TIMING_WAITFOR_EVENTS = { | ||
| // ready on viewer first visible | ||
| 'navigationStart': WAITFOR_EVENTS.VIEWER_FIRST_VISIBLE, |
There was a problem hiding this comment.
Aren't these all fine to read at DOCUMENT_COMPLETE?
src/service/variable-source.js
Outdated
|
|
||
| let waitForEvent; | ||
| if (!endWaitForEvent) { | ||
| waitForEvent = startWaitForEvent; |
There was a problem hiding this comment.
This can just be moved to the null above.
src/service/variable-source.js
Outdated
| if (!endWaitForEvent) { | ||
| waitForEvent = startWaitForEvent; | ||
| } else { | ||
| waitForEvent = Math.max(startWaitForEvent, endWaitForEvent); |
There was a problem hiding this comment.
Shouldn't end always be larger than start?
There was a problem hiding this comment.
Not always, The value could come from publishers. For example ${navTiming(end, start)}.
I don't expect anyone to actually use that, but want to keep the Math.max to make it always correct.
src/service/variable-source.js
Outdated
| readyPromise = readyPromise.then(() => timer.promise(1)); | ||
| readyPromise = loadPromise(win).then(() => timer.promise(1)); | ||
| } else { | ||
| dev().error('NAV-TIMING', 'waitForEvent not found %s', waitForEvent); |
There was a problem hiding this comment.
Is this user controlled at all? If so, can we make this a dev().assert?
There was a problem hiding this comment.
Good point. switch to devAssert.
| 'connectEnd': WAITFOR_EVENTS.VIEWER_FIRST_VISIBLE, | ||
| 'requestStart': WAITFOR_EVENTS.VIEWER_FIRST_VISIBLE, | ||
| 'responseStart': WAITFOR_EVENTS.VIEWER_FIRST_VISIBLE, | ||
| 'responseEnd': WAITFOR_EVENTS.VIEWER_FIRST_VISIBLE, |
There was a problem hiding this comment.
This size increase is coming from this struct. Can we make VIEWER_FIRST_VISIBLE the default, so that it doesn't need to be specified?
There was a problem hiding this comment.
True. I thought about that as well because there are many events that are waiting for VIEWER_FIRST_VISIBLE.
However I don't know if there will be other performanceTiming events added in the future, or ones that we fail to include here. Because of that I wanted to make the default WAITFOR_EVENTS to be the WAITFOR_EVENTS.LOAD instead of WAITFOR_EVENTS.VIEWER_FIRST_VISIBLE as that's the safest fallback.
I'm still open to this size optimization if you think that our current list of performanceTiming signals are complete, and there won't be frequent change to it in the future. Please let me know thanks.
|
The ads team is waiting on this change to run an experiment. @jridgewell @lannka PTAL Thank you |
Co-Authored-By: Justin Ridgewell <justin@ridgewell.name>
|
@jridgewell Could you please approve the bundle size change again. I only commit your suggested change. Thanks. |
…cript-img-with-http-protocol * 'master' of github.com:ampproject/amphtml: (1326 commits) Fix and enable e2e tests for AMPHTML ads FIE rendering mode (ampproject#23995) 🏗 Update WorkerDOM to 0.17.0 (ampproject#24024) Make DocInfo.pageViewId64 async (ampproject#23998) 🐛 Updates amp-sidebar in amp-story (ampproject#23956) Revert "Revert "📖Update documentation for carousel 0.2 (ampproject#23840)" (ampproject#23967)" (ampproject#24016) 🔥 Revert "📈 Initial StorySpec Implementation (ampproject#23030)" (ampproject#24013) Extension skeleton code for payment widgets (ampproject#23045) 🏗🐛 Don't call `travisBuildNumber()` in the global scope (ampproject#24021) Remove suppressTypes from amp-mustache. (ampproject#23993) 🐛 Move `terser` from `dependencies` to `devDependencies` (ampproject#24018) Revert "Revert "Set the new loaders experiment to 1% of traffic. (ampproject#23780)" (ampproject#23963)" (ampproject#24014) SwG release 0.1.22.63 (ampproject#23997) Resolve navTiming variable earlier if possible (ampproject#23580) 🏗 Don't run all the runtime tests for validator-only changes (ampproject#24010) Collect document ready signal (ampproject#23981) Validator rollup (ampproject#24000) Remove flaky story branching test. (ampproject#23994) Include amp-base-carousel in amp-carousel's build. (ampproject#23984) Partial validator rollup (ampproject#23996) amp-bind: Rate-limit history operations (ampproject#23938) ...
Closes #23534
cc @jonkeller @jeffkaufman