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.

Sign up for GitHub

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

Jump to bottom

Remove viewportCallback from the rest of non-ads related components. #30802

Merged
merged 15 commits into from
Oct 30, 2020
Merged

Remove viewportCallback from the rest of non-ads related components. #30802

merged 15 commits into from
Oct 30, 2020

Conversation

samouri
Copy link
Member

@samouri samouri commented Oct 21, 2020 •
edited

summary
Partial for #30620

Included components:

  1. 3d-gltf
  2. amp-anim
  3. apester-media
  4. base-carousel 0.1: base-carousel, base-slides, slidescroll, scrollable-carousel.
  5. delight-player
  6. img-slider
  7. playbuzz
  8. timeago

Notes:

  • For apester-media, I removed all custom placeholder logic. I couldn't figure out why it existed.

@CLAassistant
Copy link

CLAassistant commented Oct 22, 2020 •
edited

CLA assistant check
All committers have signed the CLA.

@samouri samouri marked this pull request as ready for review October 23, 2020 20:55
@amp-owners-bot
Copy link

Hey @alanorozco! These files were changed:

extensions/amp-delight-player/0.1/amp-delight-player.js

@samouri samouri requested review from dvoytenko and removed request for kristoferbaxter October 23, 2020 20:55
@samouri samouri self-assigned this Oct 23, 2020
dvoytenko
dvoytenko suggested changes Oct 23, 2020
View reviewed changes
extensions/amp-anim/0.1/amp-anim.js
}

/** @override */
unlayoutCallback() {
unobserveWithSharedInOb(this.element);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's one more nuance I completely missed. In the old world, before unlayoutCallback is executed, the resources system would also call viewportCallback(false). See this code. I don't think it matters most of the time, but it's important here. When it matters, I think this can simply be done in the unlayoutCallback like this:

unlayoutCallback() {
  unobserveWithSharedInOb(this.element);
  this.viewportCallback_(false);
}

Please check other cases.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch! I didn't know that happened. Indeed this case is to hide the animating image so it stops taking up CPU/GPU?
I've updated this case and checked the other 7, only one of which seemed like it could be mildly important (3d-gltf).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I just found that pauseCallback() should also trigger viewportCallback(false)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, the only one that also has a pauseCallback is gltf, and it doesn't seem super necessary in this case.

src/viewport-observer.js Outdated Show resolved Hide resolved
@samouri samouri mentioned this pull request Oct 26, 2020
Merged
@samouri samouri mentioned this pull request Oct 27, 2020
Closed
@samouri
Copy link
Member Author

samouri commented Oct 28, 2020

E2E testing found a legitimate bug in the amp-carousel change I had made. The bug was that I had observe/unobserve within
base-carousel.layoutCallback and didn't also ensure it was properly setup in subclasses. That means any subclasses with overrides for layout/unlayout that didnt include a call to super.(un)layoutCallback were missing the InOb.

Should be fixed with the latest push.

@samouri
Copy link
Member Author

samouri commented Oct 28, 2020

Found another (small) issue. Some unit tests call layoutCallback twice in a row without an unlayout in between. Is this actually valid? Since the observe() was receiving a referentially different callback the devAssert was throwing. This occured in a4a examples too.

Could either change the test or make the code more resilient, opted for introducing bound viewportCallbacks

@dvoytenko
Copy link
Contributor

Found another (small) issue. Some unit tests call layoutCallback twice in a row without an unlayout in between. Is this actually valid? Since the observe() was receiving a referentially different callback the devAssert was throwing. This occured in a4a examples too.

Could either change the test or make the code more resilient, opted for introducing bound viewportCallbacks

I don't believe two adjacent layoutCallback calls without unlayoutCallback is a valid runtime case. IMHO we should just fix tests.

@rcebulko rcebulko removed their request for review October 28, 2020 20:30
@samouri
Copy link
Member Author

samouri commented Oct 30, 2020

Just sanity tested each of the components via local server examples, and all LGTM. Merging now

@samouri samouri merged commit 4591a44 into ampproject:master Oct 30, 2020
@samouri samouri deleted the low-stakes-vp branch October 30, 2020 20:33
ed-bird pushed a commit to ed-bird/amphtml that referenced this pull request Dec 10, 2020
@samouri @ed-bird
Remove viewportCallback from the rest of non-ads related components. (a…
54981d3 
…mpproject#30802)

* viewportCallback: remove all but ads

* remove rest of this.isInViewport calls()

* rebase

* typefixin

* unlayout --> vpcb(false)

* fix timeago test

* fix test, and missed two carousel cases.

* private --> protected

* more test fix

* e2e test found bugs. woot, testing works.

* add TODO

* ugh. lint

* fix potentially wrong test that happens to call layoutCallback twice in a row without unlayout

* Revert "fix potentially wrong test that happens to call layoutCallback twice in a row without unlayout"

This reverts commit 213f4f4.

* fix the tests
@samouri samouri mentioned this pull request Oct 5, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dvoytenko dvoytenko dvoytenko approved these changes

Assignees

@samouri samouri

Labels
PR use: In Beta / Experimental
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@samouri @CLAassistant @dvoytenko @ampprojectbot