An experiment to substitute onMeasureChanged with ResizeObserver #31554
Conversation
|
This pull request introduces 1 alert when merging a7d8741 into 35d3569 - view on LGTM.com new alerts:
|
| processEntries_(entries) { | ||
| for (let i = entries.length - 1; i >= 0; i--) { | ||
| const {target, clientRect} = entries[i]; | ||
| let seenTarget = false; |
There was a problem hiding this comment.
seenTarget is to ensure it only gives the latest size? are you confident about the ordering of the array? (newest first)
There was a problem hiding this comment.
The ordering is per spect. But the newest are last. That's why I'm going through array in the reverse order.
| const size = {width, height}; | ||
| this.lastSizes_.set(target, size); | ||
| for (let k = 0; k < callbacks.length; k++) { | ||
| callCallbackNoInline(callbacks[k], size); |
There was a problem hiding this comment.
nit: this is cleaner imo if you pass in the whole thunk. E.g.
callNoInline(() => callbacks[k](size))There was a problem hiding this comment.
I was trying to reduce number of function allocations.
| if (!pushIfNotExist(callbacks, callback)) { | ||
| return; | ||
| } | ||
| const lastSize = this.lastSizes_.get(element); |
There was a problem hiding this comment.
Can we ensure the lastSize logic only occurs in the case of an element with a non-empty callbacks array?
I want to protect from a very stale value being used and then quickly corrected after observation.
E.g.:
- Observe el1. After some time, unobserve el1
- While not observed, size changes happen to el1
- Observe el1 again. The stale
lastSizeis used initially.
There was a problem hiding this comment.
I clean out the lastSize after the unobserve call, thus there shouldn't be stale values here.
| * Pools resize observers. Eventually the hope is that the call sites can | ||
| * instantiate their own resize observers. But currently there are some | ||
| * performance issues and pooling is desired. For more info, see | ||
| * https://bugs.chromium.org/p/chromium/issues/detail?id=1157544 |
There was a problem hiding this comment.
Thanks for linking this bug! Very interesting results with a significant impact on how to structure our ResOb code.
|
Shouldn't we merge the ResizeObserver polyfill before making the changes in |
I added the build flag just for this reason. Polyfill itself is pretty easy and we can do it as early as we want. But the polyfill suffers from the similar issues that we've previously fixed for |
|
Closed in favor of #31899 |
Partial for #31540
TODO: