Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove measuring APIs from the runtime #31540

Open
4 of 10 tasks
dvoytenko opened this issue Dec 10, 2020 · 3 comments
Open
4 of 10 tasks

Remove measuring APIs from the runtime #31540

dvoytenko opened this issue Dec 10, 2020 · 3 comments

Comments

@dvoytenko
Copy link
Contributor

dvoytenko commented Dec 10, 2020

Proposed plan:

  1. Only supply one measurement per load/layout operation.
  2. Provide this measurement in the appropriate callbacks to avoid misuse. E.g. layoutCallback(size).
  3. The elements that need to react to size changes should take care of it themselves via ResizeObserver.
  4. getLayoutBox and related APIs should be phased out. Many uses are currently incorrect or imprecise. We could instead add one getSize-style API, but we should call it clearly to explain that this is the size at the last load/layout operation and has no guarantees on freshness.

TODO:

  • Create resize-observer utilities.
  • Supply layout size to the layoutCallback as a transaction argument value.
  • Phase out BaseElement.onMeasureChanged (the clearest indication of resize tracking functionality).
  • Phase out getPageLayoutBox API.
  • Phase out getLayoutBox API.
  • Phase out getIntersectionChangeEntry API.
  • Phase out getIntersectionElementLayoutBox API.
  • Phase out getLayoutWidth API.
  • Reimplement ini-load via intersection observer
  • Reimplement resizer via intersection observer
@samouri
Copy link
Member

samouri commented Dec 11, 2020

  • onMeasureChanged only has 5 usages and should be easy to swap out with the new observer service.
  • Why provide size to layoutCallback? Is that intended to replace onLayoutMeasure which has ~20 usages

@dvoytenko
Copy link
Contributor Author

  • onMeasureChanged only has 5 usages and should be easy to swap out with the new observer service.

Correct. The only reason I'm setting up the experiment is to allow the ResizeObserver polyfill to be worked on independently. Also, onMeasureChanged are merely clear usages of resize monitoring. The wide-used onLayoutMeasure callback has a lot of uses cases for resize monitoring that are harder to distinguish from one-time measures.

  • Why provide size to layoutCallback? Is that intended to replace onLayoutMeasure which has ~20 usages

Two reasons:

  1. The onLayoutMeasure is currently used for two purposes: one-off pre-layout size and resize monitoring. I hope to remove this callback eventually by requiring resize monitoring to use the ResizeObserver (as in An experiment to substitute onMeasureChanged with ResizeObserver #31554) and by providing size to the layoutCallback.
  2. The getLayoutBox() is misused a lot. It's been intended as a size-used-for-load. However, it's now used as the "current" size. I'd like first separate the size-used-for-load cases and I think easiest to do so is via passing this size directly to the layoutCallback.

@samouri
Copy link
Member

samouri commented Dec 17, 2020

I just did a survey of the various usages. By my count there are ~18 instances that an be replaced via ResizeObserver, 7 that need the size exactly when layoutCallback is called (so the InOb rect would be fine), ~6 that need sizes in a more one-off way (not in layoutCallback), 3 that can be replaced via IntersectionObserver, and then 4 "other" category that didn't fit in with the rest.

Some thoughts:

  • I do like the idea of reusing resources inob rects for layoutCallback but am worried we'd end up regretting that as well. It could be safest to always use async measurements (either via vsync or the observe/unobserve trick) instead.
  • Some of the measuring usages seem like they are recreating IntersectionObserver before it existed, and we can hopefully swap them out with the real thing.
  • For all the current sync calls, should we migrate them to async?
Full notes

ResizeObserver cases.

  • extensions/amp-3d-gltf/0.1/amp-3d-gltf.js
  • extensions/amp-ad-exit/0.1/amp-ad-exit.js (and /filters/click-location)
  • extensions/amp-carousel/0.1/slidescroll.js: maybe use one-off instead on page resize events.
  • extensions/amp-connatix-player/0.1/amp-connatix-player.js
  • extensions/amp-ima-video/0.1/amp-ima-video.js
  • extensions/amp-image-viewer/0.1/amp-image-viewer.js
  • extensions/amp-pan-zoom/0.1/amp-pan-zoom.js
  • extensions/amp-sidebar/0.2/amp-sidebar.js
  • extensions/amp-story-dev-tools/0.1/amp-story-dev-tools-tab-preview.js: (can this be done via CSS instead?)
  • extensions/amp-story/1.0/amp-story-page.js
  • extensions/amp-viz-vega/0.1/amp-viz-vega.js:

A4A

  • extensions/amp-a4a/0.1/amp-a4a.js
  • extensions/amp-ad-network-doubleclick-impl/0.1/amp-ad-network-doubleclick-impl.js:
  • extensions/amp-ad-network-doubleclick-impl/0.1/safeframe-host.js:
  • extensions/amp-ad-network-doubleclick-impl/0.1/sra-utils.js:

AmpAd

  • extensions/amp-ad/0.1/amp-ad-3p-impl.js:
  • extensions/amp-ad/0.1/amp-ad-xorigin-iframe-handler.js:
  • extensions/amp-ad/0.1/legacy-ad-intersection-observer-host.js:

layoutCallback InOb Rect

  • builtins/amp-img.js
  • extensions/amp-auto-lightbox/0.1/amp-auto-lightbox.js: complication that it needs other element's initial layout rect.
  • extensions/amp-fx-flying-carpet/0.1/amp-fx-flying-carpet.js
  • extensions/amp-iframe/0.1/amp-iframe.js: as long as we can stop overriding intersectionElementLayoutBox
  • src/iframe-attributes.js
  • src/iframe-helper.js
  • src/ini-load.js

One-offs (needs boundingClientRect + not in layoutCallback)

  • ads/google/a4a/utils.js
  • extensions/amp-auto-ads/0.1/utils.js
  • extensions/amp-playbuzz/0.1/amp-playbuzz.js
  • extensions/amp-story/1.0/page-advancement.js
  • extensions/amp-video-docking/0.1/amp-video-docking.js (and math.js)
  • src/friendly-iframe-embed.js

IntersectionObserver

  • extensions/amp-next-page/1.0/visibility-observer.js: ideally replace most of the logic via an IntersectionObserver.
  • extensions/amp-video-iframe/0.1/amp-video-iframe.js: move to IntersectionObserver
  • src/utils/intersection-observer-3p-host.js: not sure, ideally can use real InOb.

Other

  • extensions/amp-a4a/0.1/amp-ad-network-base.js: tbd what to do. Uses onLayoutMeasure, but just to initiate ad request which is handled by subclasses (which might measure). Can we move this?

  • extensions/amp-script/0.1/amp-script.js: remove this need by enabling a flag to always build/layout specific amp elements (e.g. amp-state, amp-script, etc.)

  • src/service/video-manager-impl.js: idk

  • extensions/amp-analytics/0.1/visibility-manager.js: idk

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

No branches or pull requests

2 participants