Measure document height when necessary and send to viewer #7327

muxin · 2017-02-03T07:48:37Z

Implements #6530

Measure document height when necessary
- deferMutate
- mutateElement
- resize
Manual tested in three types of viewport.

jridgewell · 2017-02-03T20:06:50Z

src/service/resources-impl.js

@@ -859,6 +871,19 @@ export class Resources {
    this.visibilityStateMachine_.setState(
      this.viewer_.getVisibilityState()
    );
+
+    this.vsync_.measure(() => {


This might be expensive to do every time?

Yeah, I should move the if statement outside.

muxin · 2017-02-03T22:34:47Z

Tests coming, but PTAL @dvoytenko

muxin · 2017-02-13T23:50:54Z

@dvoytenko I marked the maybeChangeHeight_ inside mutateWork_ and checked it in the doPass_, is this fine?

dvoytenko · 2017-02-14T21:27:22Z

src/service/resources-impl.js

+          this.scrollHeight_ = measuredScrollHeight;
+          dev().fine(TAG_, 'document height changed: ' + this.scrollHeight_);
+        }
+        this.maybeChangeHeight_ = false;


Set it to false before vsync.measure(). In other words as a first line inside the if. This will avoid running several measures unnecessary.

dvoytenko · 2017-02-14T21:28:40Z

src/service/resources-impl.js

@@ -859,6 +871,19 @@ export class Resources {
    this.visibilityStateMachine_.setState(
      this.viewer_.getVisibilityState()
    );
+
+    if (this.maybeChangeHeight_) {


Can you add unit-tests for this?

I'm thinking about assigning true to maybeChangeHeight_ and call doPass_ directly in the test, is it fine?

muxin · 2017-02-16T21:30:57Z

src/service/resources-impl.js

+        if (measuredScrollHeight != this.scrollHeight_) {
+          this.viewer_.sendMessage('documentHeight',
+              {height: measuredScrollHeight}, /* cancelUnsent */true);
+          this.scrollHeight_ = measuredScrollHeight;


In the test, the code inside vsync_.meanure is happening after the expect(resources.scrollHeight_).to.equal... which makes the test to fail. But later resources.scrollHeight is changed. How could I run expect after vsync_.measure? @dvoytenko

You need to make #measure execute immediately.

jridgewell · 2017-02-16T22:35:10Z

src/service/resources-impl.js

+
+    if (this.maybeChangeHeight_) {
+      this.maybeChangeHeight_ = false;
+      this.vsync_.measure(() => {


Nit: We can still queue up multiple measures here. We can add a second boolean hasMeasuredDocHeight, setting to false when we queue, and setting to true when it's measured. Then, when we see that maybeChangeHeight_ && !hasMeasuredDocHeight, we know we can skip since it's already queued.

Actually, I think doPass_ happens less frequently than vsync.measure, maybe there's no need to dedupe?

@jridgewell I would rather not do this optimization now, if in the future this become a performance problem we can recheck.

jridgewell · 2017-02-16T22:36:24Z

src/service/resources-impl.js

+        if (measuredScrollHeight != this.scrollHeight_) {
+          this.viewer_.sendMessage('documentHeight',
+              {height: measuredScrollHeight}, /* cancelUnsent */true);
+          this.scrollHeight_ = measuredScrollHeight;


You need to make #measure execute immediately.

muxin · 2017-02-17T18:37:41Z

Tests updated, PTAL

dvoytenko

LGTM on my side.

…#7327)

jridgewell reviewed Feb 3, 2017

View reviewed changes

muxin requested a review from dvoytenko February 3, 2017 22:14

muxin assigned dvoytenko Feb 3, 2017

muxin force-pushed the document-height branch 2 times, most recently from 0003cf1 to db457b3 Compare February 3, 2017 22:33

muxin force-pushed the document-height branch from db457b3 to 6a8c58c Compare February 6, 2017 01:26

muxin mentioned this pull request Feb 6, 2017

Weekly Status 2017-02-06 #7370

Closed

muxin force-pushed the document-height branch 3 times, most recently from 696b5eb to 2fbf4ec Compare February 7, 2017 01:08

dvoytenko suggested changes Feb 14, 2017

View reviewed changes

muxin force-pushed the document-height branch 2 times, most recently from 611447e to 961dd56 Compare February 16, 2017 21:22

muxin commented Feb 16, 2017

View reviewed changes

jridgewell reviewed Feb 16, 2017

View reviewed changes

muxin force-pushed the document-height branch from 961dd56 to b209d7f Compare February 16, 2017 23:21

dvoytenko approved these changes Feb 17, 2017

View reviewed changes

muxin added 4 commits February 17, 2017 13:36

Measure document height when necessary and send to viewer

f487ba2

Measure contentHeight instead for doc with little content

ac741f6

Revert to measure scrollHeight

1e8699b

Add tests

ec4131e

muxin force-pushed the document-height branch from b209d7f to ec4131e Compare February 17, 2017 21:36

jridgewell approved these changes Feb 17, 2017

View reviewed changes

muxin merged commit 7071ff1 into ampproject:master Feb 17, 2017

muxin deleted the document-height branch February 17, 2017 22:17

muxin mentioned this pull request Feb 21, 2017

Provide document height to viewer #6530

Closed

mrjoro pushed a commit to mrjoro/amphtml that referenced this pull request Apr 28, 2017

Measure document height when necessary and send to viewer (ampproject…

9b16db0

…#7327)

This was referenced Jul 16, 2020

🚨 Error: startLayout called but not LAYOUT_SCHEDULED currently: 2 #29304

Closed

🚨 Error: startLayout called but not LAYOUT_SCHEDULED currently: 2 #29341

Closed

ampprojectbot mentioned this pull request Oct 11, 2020

🚨 Error: startLayout called but not LAYOUT_SCHEDULED currently: 2 #30616

Closed

This was referenced Mar 4, 2021

🚨 Error: Assertion failed ampproject/error-reporting#62

Open

🚨 Error: Assertion failed ampproject/error-reporting#65

Open

ampprojectbot mentioned this pull request Apr 13, 2021

🚨 Error: viewport has zero dimensions #33792

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Measure document height when necessary and send to viewer #7327

Measure document height when necessary and send to viewer #7327

muxin commented Feb 3, 2017 •

edited

Loading

jridgewell Feb 3, 2017

muxin Feb 3, 2017

muxin commented Feb 3, 2017

muxin commented Feb 13, 2017

dvoytenko Feb 14, 2017

dvoytenko Feb 14, 2017

muxin Feb 14, 2017

dvoytenko Feb 16, 2017

muxin Feb 16, 2017

jridgewell Feb 16, 2017

jridgewell Feb 16, 2017

muxin Feb 16, 2017

muxin Feb 17, 2017

jridgewell Feb 16, 2017

muxin commented Feb 17, 2017

dvoytenko left a comment

Measure document height when necessary and send to viewer #7327

Measure document height when necessary and send to viewer #7327

Conversation

muxin commented Feb 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

muxin commented Feb 3, 2017

muxin commented Feb 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

muxin commented Feb 17, 2017

dvoytenko left a comment

Choose a reason for hiding this comment

muxin commented Feb 3, 2017 •

edited

Loading