Layers: Scoring Algorithm #12958
Layers: Scoring Algorithm #12958
Conversation
More information is necessary from the caller to determine whether it should be forced.
| * @return {boolean} The boolean value. | ||
| */ | ||
| assertBoolean(shouldBeBoolean, opt_message) { | ||
| this.assert(!!shouldBeBoolean === shouldBeBoolean, |
src/service/layers-impl.js
Outdated
| } | ||
| from.remeasure(); |
There was a problem hiding this comment.
Feels like this should be inverted name-wise.
if (force)
remeasure()
else
requestRemeasure()
There was a problem hiding this comment.
It's not in an else. The #requestRemeasure ensure that the layout thinks in needs to be measured (dirties), while the #remeasure does the actual measuring.
There was a problem hiding this comment.
Ok, how about changing requestMeasure to setDirty or setNeedsRemeasure? 🚲 🏚
src/service/layers-impl.js
Outdated
| * | ||
| * @return {boolean} | ||
| */ | ||
| isActive() { |
There was a problem hiding this comment.
Because it throws?
There was a problem hiding this comment.
Because you shouldn't call it unless you really know what you're doing. Similar to "unsafe" suffixes in resources-impl.js.
| const win = /** @type {!Window } */ (dev().assert( | ||
| element.ownerDocument.defaultView)); | ||
| // If it remains fixed, it will still be a layer. | ||
| if (computedStyle(win, element).position === 'fixed') { |
There was a problem hiding this comment.
How can we ensure this is called in a measure cycle (without interleaved mutates)?
src/service/layers-impl.js
Outdated
| } | ||
|
|
||
| /** | ||
| * Gets the minimal horizontal distance of this element from it's parent's |
There was a problem hiding this comment.
I know the difference.... 😳
This is literally (as in literally, not figuratively) the one contraction I always screw up.
src/service/resource.js
Outdated
| @@ -682,6 +693,8 @@ export class Resource { | |||
| let scrollPenalty = 1; | |||
| let distance = 0; | |||
|
|
|||
| // TODO | |||
src/service/resources-impl.js
Outdated
| this.schedulePass(); | ||
| }); | ||
|
|
||
| /** @const */ |
src/service/resources-impl.js
Outdated
| } | ||
|
|
||
| /** | ||
| * Calculates the ancestry score... todo description trololololol. |
Please fix.
src/service/layers-impl.js
Outdated
| * @return {T} | ||
| * @template T | ||
| */ | ||
| ancestry(element, iterator, state) { |
There was a problem hiding this comment.
This name is really too vague. iterateAncestry?
src/service/layers-impl.js
Outdated
| layer = layer.getParentLayer(); | ||
| } | ||
|
|
||
| let value; |
There was a problem hiding this comment.
accumulator since this is reduce-like, and explicitly initialize to undefined.
src/service/layers-impl.js
Outdated
| @@ -74,6 +79,19 @@ function positionLt(left, top) { | |||
| }; | |||
| } | |||
|
|
|||
| /** | |||
| * Stores an LayoutElement's parent layer ancestry (temporarily). | |||
There was a problem hiding this comment.
Mention that this an optimization is to avoid array creation in a tight inner loop for interateAncestry.
src/service/layers-impl.js
Outdated
| // Gather, and update whether the layers are descendants of the active | ||
| // layer. | ||
| let isActive = activeLayer === this || activeLayer.contains(this); | ||
|
|
There was a problem hiding this comment.
Maybe dev().assert that cache is empty before use.
src/service/layers-impl.js
Outdated
| @@ -413,6 +482,9 @@ export class LayoutElement { | |||
| */ | |||
| this.scrollTop_ = 0; | |||
|
|
|||
| /** | |||
| * | |||
| @@ -46,6 +46,11 @@ export let SizeDef; | |||
| */ | |||
| export let PositionDef; | |||
|
|
|||
| /** | |||
| * @typedef {Object<string, *>} | |||
There was a problem hiding this comment.
"Anonymous state/cache passed from ancestry iterator."
When I first read this I thought it was "state of ancestry".
jridgewell
force-pushed
the
amp-layers-3
branch
from
55e3937
to
b6e82df
Compare
* Add comments for future todos * Ensure size/position is always re-measured * scoring * Fix fixed-position undeclare layer * Ensure a remeasure happens * Fix cache and score * Fix width/height calc * Add force remeasurement More information is necessary from the caller to determine whether it should be forced. * Lint * Review comments * Review comments * Fix renames
* Add comments for future todos * Ensure size/position is always re-measured * scoring * Fix fixed-position undeclare layer * Ensure a remeasure happens * Fix cache and score * Fix width/height calc * Add force remeasurement More information is necessary from the caller to determine whether it should be forced. * Lint * Review comments * Review comments * Fix renames
This implements the scoring algorithm for Layers. It does not change how viewport distance is calculated yet, which prevents them from being scored far-out of viewport.
/cc @dvoytenko on the
#ancestryiteration. I ended up waffling, and allow direct access to theLayoutElementduring iteration, instead of forcing the caller to do the calculations (with a bunch of parameters to the iterator).Fixes #12650.