Remove resize logs and enable activation for resize decisions (#24860)

ampproject · Oct 4, 2019 · e10276b · e10276b
1 parent c057203
commit e10276b
Show file tree

Hide file tree

Showing 3 changed files with 54 additions and 42 deletions.
diff --git a/extensions/amp-ad/0.1/amp-ad-ui.js b/extensions/amp-ad/0.1/amp-ad-ui.js
@@ -16,10 +16,6 @@
 
 import {ancestorElementsByTag} from '../../../src/dom';
 import {getAdContainer} from '../../../src/ad-helper';
-import {isProxyOrigin} from '../../../src/url';
-import {user} from '../../../src/log';
-
-const TAG = 'amp-ad';
 
 export class AmpAdUIHandler {
   /**
@@ -192,32 +188,11 @@ export class AmpAdUIHandler {
       resizeInfo.success = false;
       return Promise.resolve(resizeInfo);
     }
-    // TODO(#23926): cleanup once user activation for resize is
-    // implemented.
-    const isProxy = isProxyOrigin(this.baseInstance_.win.location);
-    if (isProxy) {
-      user().expectedError(TAG, 'RESIZE_REQUEST');
-    }
     return this.baseInstance_
       .attemptChangeSize(newHeight, newWidth, event)
       .then(
+        () => resizeInfo,
         () => {
-          return resizeInfo;
-        },
-        () => {
-          if (isProxy) {
-            // TODO(#23926): cleanup once user activation for resize is
-            // implemented.
-            user().expectedError(TAG, 'RESIZE_REJECT');
-            const activated =
-              event &&
-              event.userActivation &&
-              event.userActivation.hasBeenActive;
-            if (activated) {
-              // Report false negatives.
-              user().expectedError(TAG, 'RESIZE_REJECT_ACTIVE');
-            }
-          }
           resizeInfo.success = false;
           return resizeInfo;
         }

diff --git a/src/service/resources-impl.js b/src/service/resources-impl.js
@@ -26,9 +26,9 @@ import {VisibilityState} from '../visibility-state';
 import {areMarginsChanged, expandLayoutRect} from '../layout-rect';
 import {closest, hasNextNodeInDocumentOrder} from '../dom';
 import {computedStyle} from '../style';
-import {dev, devAssert, user} from '../log';
+import {dev, devAssert} from '../log';
 import {dict} from '../utils/object';
-import {getSourceUrl, isProxyOrigin} from '../url';
+import {getSourceUrl} from '../url';
 import {checkAndFix as ieMediaCheckAndFix} from './ie-media-bug';
 import {isBlockedByConsent, reportError} from '../error';
 import {isExperimentOn} from '../experiments';
@@ -969,23 +969,13 @@ export class ResourcesImpl {
         } else if (request.force || !this.visible_) {
           // 2. An immediate execution requested or the document is hidden.
           resize = true;
-        } else if (this.activeHistory_.hasDescendantsOf(resource.element)) {
+        } else if (
+          this.activeHistory_.hasDescendantsOf(resource.element) ||
+          (event && event.userActivation && event.userActivation.hasBeenActive)
+        ) {
           // 3. Active elements are immediately resized. The assumption is that
           // the resize is triggered by the user action or soon after.
           resize = true;
-          if (
-            isProxyOrigin(this.win.location) &&
-            event &&
-            event.userActivation
-          ) {
-            // Report false positives.
-            // TODO(#23926): cleanup once user activation for resize is
-            // implemented.
-            user().expectedError(TAG_, 'RESIZE_APPROVE');
-            if (!event.userActivation.hasBeenActive) {
-              user().expectedError(TAG_, 'RESIZE_APPROVE_NOT_ACTIVE');
-            }
-          }
         } else if (
           topUnchangedBoundary >= viewportRect.bottom - bottomOffset ||
           (topMarginDiff == 0 &&

diff --git a/test/unit/test-resources.js b/test/unit/test-resources.js
@@ -1797,6 +1797,53 @@ describe('Resources changeSize', () => {
       expect(overflowCallbackSpy.firstCall.args[0]).to.equal(false);
     });
 
+    it('should NOT change size via activation if has not been active', () => {
+      viewportMock
+        .expects('getContentHeight')
+        .returns(10000)
+        .atLeast(0);
+      const event = {
+        userActivation: {
+          hasBeenActive: false,
+        },
+      };
+      resources.scheduleChangeSize_(
+        resource1,
+        111,
+        222,
+        undefined,
+        event,
+        false
+      );
+      resources.mutateWork_();
+      expect(resource1.changeSize).to.not.be.called;
+      expect(overflowCallbackSpy).to.be.calledOnce.calledWith(true);
+    });
+
+    it('should change size via activation if has been active', () => {
+      viewportMock
+        .expects('getContentHeight')
+        .returns(10000)
+        .atLeast(0);
+      const event = {
+        userActivation: {
+          hasBeenActive: true,
+        },
+      };
+      resources.scheduleChangeSize_(
+        resource1,
+        111,
+        222,
+        undefined,
+        event,
+        false
+      );
+      resources.mutateWork_();
+      expect(resources.requestsChangeSize_).to.be.empty;
+      expect(resource1.changeSize).to.be.calledOnce;
+      expect(overflowCallbackSpy).to.be.calledOnce.calledWith(false);
+    });
+
     it('should change size when below the viewport', () => {
       resource1.layoutBox_ = {
         top: 10,