Check for win.performance before using it #6316
Conversation
| @@ -38,6 +38,8 @@ export const DEFAULT_THRESHOLD = | |||
| [0, 0.05, 0.1, 0.15, 0.2, 0.25, 0.3, 0.35, 0.4, | |||
| 0.45, 0.5, 0.55, 0.6, 0.65, 0.7, 0.75, 0.8, 0.85, 0.9, 0.95, 1]; | |||
|
|
|||
| const INIT_TIME = Date.now(); | |||
There was a problem hiding this comment.
This won't pass presubmit as the Date.now() call can't be DCEd.
There was a problem hiding this comment.
presubmit is passing locally for me.
Well if I can't do this where should I get the init time? I can create one in IntersectionObserverPolyfill, but what about the export function #getIntersectionChangeEntry?
There was a problem hiding this comment.
Maybe it's lint? Something will fail.
but what about the export function #getIntersectionChangeEntry
First call to calculateChangeEntry sets?
There was a problem hiding this comment.
Nothing fails. Is it something with our test? Or do I still need to worry about this?
There was a problem hiding this comment.
Oh, it's because I'm only checking exports. While this won't be DCEd, it's fine as is.
| @@ -411,7 +413,9 @@ function calculateChangeEntry( | |||
| } | |||
|
|
|||
| return /** @type {!IntersectionObserverEntry} */ ({ | |||
| time: performance.now(), | |||
| time: (typeof performance !== 'undefined' && | |||
| typeof performance.now !== 'undefined') ? performance.now() | |||
There was a problem hiding this comment.
(performance && performance.now) ? performance.now() : .... The performance.now check is probably unnecessary anyways.
There was a problem hiding this comment.
As @aghassemi pointed out #6312 (comment)
for performance.now check. yes it's probably unnecessary. However I see we are doing this check in other places. I thought it would be safer to include the extra check. I am ok with removing it.
There was a problem hiding this comment.
Oh, yah, it's an unbound global variable if it's not defined. Man that's annoying.
If you keep the performance.now check, no need to typeof it.
|
LOL~ I don't think anyone is relying on that value. However that is what native IntersectionObserver returns. I think as long as we don't return Date.now() here, it's ok to be not accurate. |
|
I'd vote for the simplified code (and performance win, ironically). |
* git diff HEAD~1 * check performance * also check for performance.now * nit
* git diff HEAD~1 * check performance * also check for performance.now * nit
fix #6312
@jridgewell, @aghassemi and I talked about it, and there are other files that use
win.performancefunction. they basically check the existence ofwin.performance. With this I don't want to create awin.performancepolyfill just forperformance.now()because it will break their checks.I decide not to polyfill it here. If we go with polyfilling, I think we should polyfill everything for
win.performance