Instruct Resources manager to remeasure/layout/unload mutating elemen…

…ts and their siblings.

extensions/amp-access/0.1/amp-access.js

@@ -29,6 +29,7 @@ import {log} from '../../../src/log';
 import {onDocumentReady} from '../../../src/document-state';
 import {openLoginDialog} from './login-dialog';
 import {parseQueryString} from '../../../src/url';
+import {resourcesFor} from '../../../src/resources';
 import {templatesFor} from '../../../src/template';
 import {timer} from '../../../src/timer';
 import {urlReplacementsFor} from '../../../src/url-replacements';
@@ -130,6 +131,9 @@ export class AccessService {
     /** @private @const {!Templates} */
     this.templates_ = templatesFor(this.win);
 
+    /** @private @const {!Resources} */
+    this.resources_ = resourcesFor(this.win);
+
     /** @private @const {function(string):Promise<string>} */
     this.openLoginDialog_ = openLoginDialog.bind(null, this.win);
 
@@ -331,7 +335,11 @@ export class AccessService {
    * @private
    */
   applyAuthorizationAttrs_(element, on) {
-    return this.vsync_.mutatePromise(() => {
+    const wasOn = !element.hasAttribute('amp-access-hide');
+    if (on == wasOn) {
+      return Promise.resolve();
+    }
+    return this.resources_.mutateElement(element, () => {
       if (on) {
         element.removeAttribute('amp-access-hide');
       } else {

extensions/amp-access/0.1/test/test-amp-access.js

@@ -229,6 +229,10 @@ describe('AccessService authorization', () => {
     service = new AccessService(window);
     service.isExperimentOn_ = true;
 
+    sandbox.stub(service.resources_, 'mutateElement',
+        (unusedElement, mutator) => {
+          mutator();
+        });
     service.vsync_ = {
       mutate: callback => {
         callback();
@@ -340,6 +344,7 @@ describe('AccessService applyAuthorizationToElement_', () => {
   let sandbox;
   let configElement, elementOn, elementOff;
   let templatesMock;
+  let mutateElementStub;
 
   beforeEach(() => {
     sandbox = sinon.sandbox.create();
@@ -368,6 +373,10 @@ describe('AccessService applyAuthorizationToElement_', () => {
     service = new AccessService(window);
     service.isExperimentOn_ = true;
 
+    mutateElementStub = sandbox.stub(service.resources_, 'mutateElement',
+        (unusedElement, mutator) => {
+          mutator();
+        });
     service.vsync_ = {
       mutatePromise: callback => {
         callback();
@@ -405,11 +414,16 @@ describe('AccessService applyAuthorizationToElement_', () => {
     service.applyAuthorizationToElement_(elementOff, {access: true});
     expect(elementOn).not.to.have.attribute('amp-access-hide');
     expect(elementOff).to.have.attribute('amp-access-hide');
+    expect(mutateElementStub.callCount).to.equal(1);
+    expect(mutateElementStub.getCall(0).args[0]).to.equal(elementOff);
 
     service.applyAuthorizationToElement_(elementOn, {access: false});
     service.applyAuthorizationToElement_(elementOff, {access: false});
     expect(elementOn).to.have.attribute('amp-access-hide');
     expect(elementOff).not.to.have.attribute('amp-access-hide');
+    expect(mutateElementStub.callCount).to.equal(3);
+    expect(mutateElementStub.getCall(1).args[0]).to.equal(elementOn);
+    expect(mutateElementStub.getCall(2).args[0]).to.equal(elementOff);
   });
 
   it('should render and re-render templates when access is on', () => {

src/service/resources-impl.js

@@ -43,6 +43,7 @@ const PRIORITY_PENALTY_TIME_ = 1000;
 const POST_TASK_PASS_DELAY_ = 1000;
 const MUTATE_DEFER_DELAY_ = 500;
 const FOCUS_HISTORY_TIMEOUT_ = 1000 * 60;  // 1min
+const FOUR_FRAME_DELAY_ = 70;
 
 
 /**
@@ -434,6 +435,60 @@ export class Resources {
     this.schedulePassVsync();
   }
 
+  /**
+   * Runs the specified mutation on the element and ensures that measures
+   * and layouts performed for the affected elements.
+   *
+   * This method should be called whenever a significant mutations are done
+   * on the DOM that could affect layout of elements inside this subtree or
+   * its siblings. The top-most affected element should be specified as the
+   * first argument to this method and all the mutation work should be done
+   * in the mutator callback which is called in the "mutation" vsync phase.
+   *
+   * @param {!Element} element
+   * @param {function()} mutator
+   * @return {!Promise}
+   */
+  mutateElement(element, mutator) {
+    const calcRelayoutTop = () => {
+      const box = this.viewport_.getLayoutRect(element);
+      if (box.width != 0 && box.height != 0) {
+        return box.top;
+      }
+      return -1;
+    };
+    let relayoutTop = -1;
+    return this.vsync_.runPromise({
+      measure: () => {
+        relayoutTop = calcRelayoutTop();
+      },
+      mutate: () => {
+        mutator();
+
+        // Mark children for re-measurement.
+        const ampElements = element.getElementsByClassName('-amp-element');
+        for (let i = 0; i < ampElements.length; i++) {
+          const r = this.getResourceForElement(ampElements[i]);
+          r.requestMeasure();
+        }
+        if (relayoutTop != -1) {
+          this.setRelayoutTop_(relayoutTop);
+        }
+        this.schedulePass(FOUR_FRAME_DELAY_);
+
+        // Need to measure again in case the element has become visible or
+        // shifted.
+        this.vsync_.measure(() => {
+          const updatedRelayoutTop = calcRelayoutTop();
+          if (updatedRelayoutTop != -1 && updatedRelayoutTop != relayoutTop) {
+            this.setRelayoutTop_(updatedRelayoutTop);
+            this.schedulePass(FOUR_FRAME_DELAY_);
+          }
+        });
+      }
+    });
+  }
+
   /**
    * Schedules the work pass at the latest with the specified delay.
    * @param {number=} opt_delay
@@ -627,7 +682,7 @@ export class Resources {
       }
 
       if (minTop != -1) {
-        this.relayoutTop_ = minTop;
+        this.setRelayoutTop_(minTop);
       }
 
       // Execute scroll-adjusting resize requests, if any.
@@ -645,7 +700,7 @@ export class Resources {
               request.resource./*OK*/changeHeight(request.newHeight);
             });
             if (minTop != -1) {
-              this.relayoutTop_ = minTop;
+              this.setRelayoutTop_(minTop);
             }
             // Sync is necessary here to avoid UI jump in the next frame.
             const newScrollHeight = this.viewport_./*OK*/getScrollHeight();
@@ -659,6 +714,18 @@ export class Resources {
     }
   }
 
+  /**
+   * @param {number} relayoutTop
+   * @private
+   */
+  setRelayoutTop_(relayoutTop) {
+    if (this.relayoutTop_ == -1) {
+      this.relayoutTop_ = relayoutTop;
+    } else {
+      this.relayoutTop_ = Math.min(relayoutTop, this.relayoutTop_);
+    }
+  }
+
   /**
    * Reschedules change height request when an overflown element is activated.
    * @param {!Element} element
@@ -704,6 +771,7 @@ export class Resources {
 
     // Phase 1: Build and relayout as needed. All mutations happen here.
     let relayoutCount = 0;
+    let remeasureCount = 0;
     for (let i = 0; i < this.resources_.length; i++) {
       const r = this.resources_[i];
       if (r.getState() == ResourceState_.NOT_BUILT) {
@@ -713,24 +781,41 @@ export class Resources {
         r.applySizesAndMediaQuery();
         relayoutCount++;
       }
+      if (r.isMeasureRequested()) {
+        remeasureCount++;
+      }
     }
 
     // Phase 2: Remeasure if there were any relayouts. Unfortunately, currently
     // there's no way to optimize this. All reads happen here.
-    if (relayoutCount > 0 || relayoutAll || relayoutTop != -1) {
+    const toUnload = [];
+    if (relayoutCount > 0 || remeasureCount > 0 ||
+            relayoutAll || relayoutTop != -1) {
       for (let i = 0; i < this.resources_.length; i++) {
         const r = this.resources_[i];
         if (r.getState() == ResourceState_.NOT_BUILT || r.hasOwner()) {
           continue;
         }
         if (relayoutAll ||
                 r.getState() == ResourceState_.NOT_LAID_OUT ||
+                r.isMeasureRequested() ||
                 relayoutTop != -1 && r.getLayoutBox().bottom >= relayoutTop) {
+          const wasDisplayed = r.isDisplayed();
           r.measure();
+          if (wasDisplayed && !r.isDisplayed()) {
+            toUnload.push(r);
+          }
         }
       }
     }
 
+    // Unload all in one cycle.
+    if (toUnload.length > 0) {
+      this.vsync_.mutate(() => {
+        toUnload.forEach(r => r.unload());
+      });
+    }
+
     const viewportRect = this.viewport_.getRect();
     // Load viewport = viewport + 3x up/down when document is visible or
     // depending on prerenderSize in pre-render mode.
@@ -1222,6 +1307,9 @@ export class Resource {
     /** @private {!LayoutRect} */
     this.layoutBox_ = layoutRectLtwh(-10000, -10000, 0, 0);
 
+    /** @private {boolean} */
+    this.isMeasureRequested_ = false;
+
     /** @private {boolean} */
     this.isInViewport_ = false;
 
@@ -1369,6 +1457,7 @@ export class Resource {
   measure() {
     assert(this.element.isUpgraded(), 'Must be upgraded to measure: %s',
         this.debugid);
+    this.isMeasureRequested_ = false;
     if (this.state_ == ResourceState_.NOT_BUILT) {
       // Can't measure unbuilt element.
       return;
@@ -1388,6 +1477,24 @@ export class Resource {
     this.element.updateLayoutBox(box);
   }
 
+  /**
+   * @return {boolean}
+   */
+  isMeasureRequested() {
+    return this.isMeasureRequested_;
+  }
+
+  /**
+   * Requests the element to be remeasured on the next pass.
+   */
+  requestMeasure() {
+    if (this.state_ == ResourceState_.NOT_BUILT) {
+      // Can't measure unbuilt element.
+      return;
+    }
+    this.isMeasureRequested_ = true;
+  }
+
   /**
    * Returns a previously measured layout box.
    * @return {!LayoutRect}
@@ -1557,6 +1664,14 @@ export class Resource {
     }
   }
 
+  /**
+   * Called when a previously visible element has become invisible.
+   */
+  unload() {
+    // TODO(dvoytenko): Likely warrants its own callback and re-layout rules.
+    this.documentBecameInactive();
+  }
+
   /**
    * Only allowed in dev mode when runtime is turned off. Performs all steps
    * necessary to render an element.

src/service/vsync-impl.js

@@ -110,6 +110,31 @@ export class Vsync {
     this.schedule_();
   }
 
+  /**
+   * Runs vsync task: measure followed by mutate. Returns the promise that
+   * will be resolved as soon as the task has been completed.
+   *
+   * If state is not provided, the value passed to the measure and mutate
+   * will be undefined.
+   *
+   * @param {!VsyncTaskSpecDef} task
+   * @param {!VsyncStateDef=} opt_state
+   * @return {!Promise}
+   */
+  runPromise(task, opt_state) {
+    return new Promise(resolve => {
+      this.run({
+        measure: state => {
+          task.measure(state);
+        },
+        mutate: state => {
+          task.mutate(state);
+          resolve();
+        }
+      }, opt_state);
+    });
+  }
+
   /**
    * Creates a function that will call {@link run} method.
    * @param {!VsyncTaskSpecDef} task