AMP Ad Refresh #9535

glevitzky · 2017-05-24T22:27:00Z

Implements amp-ad refresh feature.

What's not included:

Logic around amp-sticky-ad. I'd prefer to leave this for a future PR.

taymonbeal · 2017-05-24T23:07:27Z

extensions/amp-a4a/0.1/amp-a4a.js

+    this.refreshManager_ = getRefreshManagerFor(this.win);
+
+    this.refreshManager_.registerElement(this.element, refresher => {
+      console.log('Eureka!');


I assume all console.log statements aren't supposed to be here.

Correct. I'll be sure to remove them once I make more progress.

taymonbeal · 2017-05-24T23:07:40Z

extensions/amp-a4a/0.1/amp-a4a.js

@@ -345,6 +347,45 @@ export class AmpA4A extends AMP.BaseElement {

    /** @private {string} */
    this.safeframeVersion_ = DEFAULT_SAFEFRAME_VERSION;
+
+    /** @private @const {RefreshManager}  */


!RefreshManager

taymonbeal · 2017-05-24T23:09:05Z

extensions/amp-a4a/0.1/refresh-manager.js

+     * share the same minOnScreenPixelRatioThreshold will be monitored by the
+     * same IO.
+     *
+     * @private {!Object<string, (IntersectionObserver|!IntersectionObserverPolyfill)>}


!IntersectionObserver

taymonbeal · 2017-05-24T23:09:18Z

extensions/amp-a4a/0.1/refresh-manager.js

+     * monitored, indexed by a unique ID stored as data-amp-a4a-refresh-id on
+     * the element.
+     *
+     * @private {!Object<string, RegisteredElementWrapper>}


!RegisteredElementWrapper

taymonbeal · 2017-05-24T23:10:53Z

extensions/amp-a4a/0.1/refresh-manager.js

@@ -0,0 +1,470 @@
+/**
+ * Copyright 2016 The AMP HTML Authors. All Rights Reserved.


taymonbeal · 2017-05-24T23:41:02Z

extensions/amp-a4a/0.1/refresh-manager.js

+
+  /**
+   * @param {!Element} element The element to be wrapped.
+   * @param {!function()} callback The function to be invoked when the element


function()

taymonbeal · 2017-05-24T23:41:34Z

extensions/amp-a4a/0.1/refresh-manager.js

+    /**
+     * The function that will be invoked when the refreshInterval has expired.
+     *
+     * @private @const {!function()}


function()

taymonbeal · 2017-05-24T23:41:48Z

extensions/amp-a4a/0.1/refresh-manager.js

+  /**
+   * Invokes the callback function.
+   *
+   * @param {RefreshManager} refreshManager The calling refreshManager, passed


!RefreshManager

taymonbeal · 2017-05-24T23:42:18Z

extensions/amp-a4a/0.1/test/test-refresh-manager.js

@@ -0,0 +1,117 @@
+/**
+ * Copyright 2015 The AMP HTML Authors. All Rights Reserved.


taymonbeal · 2017-05-24T23:43:05Z

extensions/amp-a4a/0.1/test/test-refresh-manager.js

+      });
+      refreshManager.registerElement(
+          testElement, () => {
+            expect(false).to.be.true;


You can use fail here. Also, how can resolver ever run? What's the path of execution through this test?

I wouldn't put too much stock in the tests I have at the moment, as they're most likely broken at this point. This particular one, I don't think, was ever finished, but I'll keep your point in mind.

taymonbeal · 2017-05-24T23:44:55Z

Also, please add Markdown docs for page authors.

keithwrightbos

Note that doubleclick requires at least one new parameter on refresh ad requests: rc which is the number of refresh requests.

keithwrightbos · 2017-05-25T15:13:54Z

extensions/amp-a4a/0.1/refresh-manager.js

+   * @param {(string|number)} threshold
+   * @return {(!IntersectionObserver|!IntersectionObserverPolyfill)}
+   */
+  getIntersectionObserverWithThreshold_(threshold) {


Implementations for the thresholds needed (percentage within viewport for given duration) already exist within AMP analytics so let's re-use it (https://github.com/ampproject/amphtml/blob/master/ads/google/a4a/performance.js#L278).

keithwrightbos · 2017-05-25T15:16:25Z

extensions/amp-a4a/0.1/amp-a4a.js

+    this.refreshManager_.registerElement(this.element, refresher => {
+      console.log('Eureka!');
+      this.isRefreshing_ = true;
+      this.unlayoutCallback();


Agreed and doesn't unlayoutCallback remove the frame causing the existing creative to be destroyed? The idea is to leave the existing creative in place until we have retrieved the new creative so instead of calling unlayoutCallback, you should just create a new instance of the adPromise to "restart" ad retrieval and then on response you can clear or instead have the creative render code detect its in the refresh state and clear the frame just prior to creating it.

keithwrightbos · 2017-05-25T15:18:58Z

extensions/amp-a4a/0.1/amp-a4a.js

+      this.unlayoutCallback();
+      this.onLayoutMeasure();
+      this.adPromise_.then(() => {
+        if (!this.isRefreshing_) {


When would this occur? Presumably only if unlayoutCallback was executed? If so, we should expect the promise chain to have been cancelled, right?

When would what occur, specifically? If you mean this.isRefreshing_ being set to false, then this could happen if we attempt to collapse the slot in onLayoutMeasure.

keithwrightbos · 2017-05-25T15:21:28Z

extensions/amp-a4a/0.1/amp-a4a.js

@@ -345,6 +347,44 @@ export class AmpA4A extends AMP.BaseElement {

    /** @private {string} */
    this.safeframeVersion_ = DEFAULT_SAFEFRAME_VERSION;
+
+    /** @private @const {!./refresh-manager.RefreshManager}  */
+    this.refreshManager_ = refreshManagerFor(this.win);


Negative here is that networks that do not require refresh will include its code in their binary unnecessarily. We should consider how to avoid this long term but given that only Doubleclick wants to support this currently, perhaps we should consider how to make it directly a feature in its impl.

keithwrightbos · 2017-05-25T15:24:55Z

extensions/amp-a4a/0.1/amp-a4a.js

+    /** @private @const {!./refresh-manager.RefreshManager}  */
+    this.refreshManager_ = refreshManagerFor(this.win);
+
+    this.refreshManager_.registerElement(this.element, refresher => {


Item missing here is building the config that indicates if/how element is configured for refresh. Will this be page level? block level? both?

It will be both. As mentioned in the PR description, this isn't implemented yet.

keithwrightbos · 2017-06-01T15:18:14Z

extensions/amp-ad-network-doubleclick-impl/0.1/amp-ad-network-doubleclick-impl.js

@@ -152,6 +156,39 @@ export class AmpAdNetworkDoubleclickImpl extends AmpA4A {
    ]);
  }

+  initializeRefreshIfEligible() {


Doc and any reason this can't be private?

keithwrightbos · 2017-06-01T15:20:53Z

extensions/amp-ad-network-doubleclick-impl/0.1/amp-ad-network-doubleclick-impl.js

+    }
+    new RefreshManager(this.win, this.element, refresher => {
+      this.isRefreshing = true;
+      this.tearDownSlot();


Seems like this logic should be within AmpA4A. I realize it means we'll bloat for networks that do not support refresh but it seems to me to be a cleaner way to ensure proper integration (no need to expose getAdPromise for instance). Assume that refreshmanager will call a function on the A4A instance (e.g. refresh())). Advantage is that at least refresh manager is not within AmpA4A and so it won't be compiled into networks that don't need it. Also, make getRefreshConfiguration part of RefreshManager.

glevitzky · 2017-06-01T23:47:31Z

extensions/amp-a4a/0.1/amp-a4a.js

+      if (!this.isRefreshing_) {
+        // If this refresh cycle was canceled, such as in a no-content
+        // response case, keep showing the old creative.
+        refresher.initiateRefreshCycle();


Will fix this in next pass.

keithwrightbos

Do CSI pings appropriately distinguish between refresh and non-refresh requests? Probably good to have.

keithwrightbos · 2017-06-02T14:16:38Z

ads/_a4a-config.js

+/** @type {!../extensions/amp-a4a/0.1/refresh-manager.RefreshConfig} */
+export const DEFAULT_REFRESH_CONFIG = {
+  visiblePercentageMin: 50,
+  totalTimeMin: 0,


What is totalTimeMin for? I don't see you reference it anywhere.

Total time the ad must be on screen to be considered "seen". This is the same parameter that amp-analytics' visibility takes.

keithwrightbos · 2017-06-02T14:17:10Z

extensions/amp-a4a/0.1/refresh-manager.js

+
+/**
+ * @typedef {{
+ *   visiblePercentageMin: number,


Can you add documentation explaining what these mean? Also would be good if time values indicated milliseconds

Done. All times are in ms.

keithwrightbos · 2017-06-02T14:18:02Z

extensions/amp-a4a/0.1/amp-a4a.js

@@ -741,6 +762,34 @@ export class AmpA4A extends AMP.BaseElement {
  }

  /**
+   * Refreshes ad slot by fetching new creative and rendering it.


Mention that it leaves the remaining creative in place until the new ad promise chain resolves

keithwrightbos · 2017-06-02T14:22:02Z

extensions/amp-a4a/0.1/amp-a4a.js

+   */
+  refresh() {
+    this.isRefreshing_ = true;
+    this.tearDownSlot();


You need to handle multi-size and no-fill collapse cases:
tearDown will attempt to revert the slot size to its original which could fail if the slot is within the viewport. If its collapsed it just means the ad request will never get sent (as there is code that detects if the slot has no height/width). But if multi-size I assume it would fail because the sizes given could be greater than the slot? We can handle these cases in one of two ways:

Delay the refresh attempt until after the slot could be resized (would mean re-attempting resize once we know it could be successful when the slot is not in the viewport)

For multi-size case, we could just remove sizes that are no longer eligible.

Option 2 is what we initially planned on, so that's what I'm going to implement.

keithwrightbos · 2017-06-02T14:22:39Z

extensions/amp-a4a/0.1/amp-a4a.js

+    this.tearDownSlot();
+    this.initiateAdRequest();
+    this.adPromise_.then(() => {
+      if (!this.isRefreshing_) {


Check promiseId indicating cancellation?

keithwrightbos · 2017-06-02T14:25:46Z

extensions/amp-a4a/0.1/amp-a4a.js

+   *   this.isRefreshing_ is true.
+   * @protected
+   */
+  destroyFrame(force = false) {


No need to set default value as false as undefined is essentially equivalent here.

keithwrightbos · 2017-06-02T14:26:33Z

extensions/amp-a4a/0.1/amp-a4a.js

-      this.iframe.parentElement.removeChild(this.iframe);
-      this.iframe = null;
-    }
+    this.destroyFrame();


shouldn't this be this.destroyFrame(true) to ensure the frame is destroyed on unlayoutcallback?

Depends on what we want to do in the event of unlayoutCallback firing while the slot is being refreshed. As long as !isRefreshing_, then the frame will be destroyed, but if unlayoutCallback executes while isRefreshing_, then the frame will remain.

Yeah this is somewhat complex due to the fact that we only want to destroy the frame if its not AMP (as we do not want to allow JS to execute on pages no longer visible to the user). So for AMP creatives when unlayoutCallback occurs, we should leave the frame in place and cancel any pending refresh promises (which I believe would happen anyway given we increment the promiseId though you need to add the check as I mentioned in a previous comment). However, we want to ensure that if the user swipes back to the page, the refresh interval either continues or restarts depending on production definition.

We took deliberate steps to avoid keeping a reference to the RefreshManager in AmpA4A, so it may be difficult to cancel/restart refresh on demand.

Regarding the user swiping back, the refresh cycle will stop on its own until the user sees the new creative anyway. So the worst case is that we refresh a number of slots on a page, which the user will not see unless he swipes back to the original page, at most once per refresh-enabled slot.

keithwrightbos · 2017-06-02T14:28:26Z

extensions/amp-ad-network-doubleclick-impl/0.1/amp-ad-network-doubleclick-impl.js

+
+    // Will initiate the refresh lifecycle iff the slot has been enabled to do
+    // so through an appropriate data attribute, or a page-level meta tag.
+    new RefreshManager(this).initiateRefreshCycle();


Should this wait until after layout otherwise its possible refresh interval would include time to fetch and render first ad slot?

keithwrightbos · 2017-06-02T14:29:09Z

extensions/amp-a4a/0.1/refresh-manager.js

+    }
+    analyticsForDoc(this.element_, true).then(analytics => {
+      analytics.getAnalyticsRoot(this.element_).getVisibilityManager()
+          .listenElement(this.element_, this.config_, null, null, () => {


Will this fire multiple times allowing for continued refresh? Or do you need to re-initialize after every successful render?

This passes a callback to a4a.refresh() that when invoked restarts the refresh cycle.

keithwrightbos · 2017-06-02T14:30:00Z

extensions/amp-a4a/0.1/refresh-manager.js

+      return false;
+    }
+    let refreshEnabled =
+        this.element_.getAttribute('data-enable-refresh') == 'true';


Let's assume that the value will be a number indicating interval and you take the Math.max(value, 30sec) so this function can return the interval. Something like the following which gives preference to attribute and falls back to meta tag if present.

if (!this.config) {
return null;
}
let metaTag;
return Math.max(30, Number(this.element_.getAttribute('data-enable-refresh')) || ((metaTag = this.win_.document.getElementsByName(amp-ad-enable-refresh:${this.adType_}) ? 0 : Number(metaTag.getAttribute('content')));

I've changed the spec so that enabling a slot is done by setting data-enable-refresh=true. But I see your point in that this is ugly, so I tried doing something similar to your suggestion.

glevitzky · 2017-06-08T16:35:32Z

extensions/amp-a4a/amp-ad-refresh.md

@@ -0,0 +1,65 @@
+<!---


Not sure whether this is the appropriate location for this information; will move upon reviewer's suggestion.

Let's move this to amp-ad-network-doubleclick-impl md. We should probably also indicate that this is still in beta, meaning we don't yet verify its correctness.

keithwrightbos · 2017-06-08T19:50:56Z

ads/_a4a-config.js

@@ -84,3 +84,21 @@ export const signingServerURLs = {
  'cloudflare': 'https://amp.cloudflare.com/amp-ad-verifying-keyset.json',
  'cloudflare-dev': 'https://amp.cloudflare.com/amp-ad-verifying-keyset-dev.json',
 };
+
+/** @type {!../extensions/amp-a4a/0.1/refresh-manager.RefreshConfig} */
+const DEFAULT_REFRESH_CONFIG = {


Why do we need this here? Why don't we just pass the config into RefreshManager when its created within amp-ad-network-doubleclick-impl?

RefreshManager pulls in the config on its own. The DEFAULT_REFRESH_CONFIG object is there for convenience, in case other networks want to use the same sort of configuration DoubleClick is using. I can take it or leave it, though.

Let's remove it, I don't see much value in having it here and this file is included in amp-ad

keithwrightbos · 2017-06-08T19:51:16Z

ads/google/a4a/utils.js

@@ -54,9 +54,10 @@ const AmpAdImplementation = {

 /** @const {!Object} */
 export const ValidAdContainerTypes = {
-  'AMP-STICKY-AD': 'sa',
+  'AMP-CAROUSEL': 'ac',


Did you intend for these and the multi-size changes to be included in this PR?

At this point, they're kind of entangled, and the changes are isolated such that it should (hopefully) not be too burdensome on the reviewer. If you disagree strongly, I can spend some time splitting those off into a separate PR, though.

keithwrightbos · 2017-06-08T19:55:18Z

extensions/amp-a4a/0.1/amp-a4a.js

+      // show the loader for a quarter of a second before switching to
+      // the new creative.
+      timerFor(this.win).delay(() => {
+        this.attemptToRenderCreative().then(() => {


I believe this will need to be wrapped in a vsync mutate given you're modifying outside of layoutCallback. This brings me to another point which is that we shouldn't actually render if the slot is outside of the renderOutsideViewport value otherwise we'll allow JS to execute unnecessarily and hurt the pub's active view rate. This makes me think that we should re-evaluate if we could trigger the lifecycle to re-initate layoutCallback and destroy the previous frame/load the new one at that time.

glevitzky · 2017-06-09T01:35:00Z

These test failures are really fun. I get a different set of failing tests depending on whether I run the files (1) locally and independently, (2) locally and all together, and (3) in Travis.

lannka · 2017-06-14T00:01:49Z

extensions/amp-a4a/0.1/amp-a4a.js

+   * @param {function()} refreshEndCallback When called, this function will
+   *   restart the refresh cycle.
+   */
+  refresh(refreshEndCallback) {


this file was 1.6k lines, and this PR adds 100+ more. I was about to propose a refactoring of this file.

Can we start from this PR? separate new logic out as a class or so?

Keith and I already went back and forth on this quite a bit. I think this function, at minimum, needs to be in this file, or else we'll be forced to expose various private fields that we'd prefer not to.

keithwrightbos · 2017-06-14T18:53:14Z

ads/_a4a-config.js

@@ -84,3 +84,21 @@ export const signingServerURLs = {
  'cloudflare': 'https://amp.cloudflare.com/amp-ad-verifying-keyset.json',
  'cloudflare-dev': 'https://amp.cloudflare.com/amp-ad-verifying-keyset-dev.json',
 };
+
+/** @type {!../extensions/amp-a4a/0.1/refresh-manager.RefreshConfig} */
+const DEFAULT_REFRESH_CONFIG = {


Let's remove it, I don't see much value in having it here and this file is included in amp-ad

keithwrightbos · 2017-06-14T18:53:28Z

ads/_a4a-config.js

+ *
+ * @type {!Object<string, !../extensions/amp-a4a/0.1/refresh-manager.RefreshConfig>}
+ */
+export const refreshConfigs = {


Ditto, this should be removed

Maybe this is irrelevant given that there's only one network that supports refresh at the moment, but where would other networks put their refresh configurations (which determine viewability criteria)?

keithwrightbos · 2017-06-14T18:59:09Z

ads/google/utils.js

+      if (strict) {
+        return null;
+      }
+      continue;
    }

    // Check that if multi-size-validation is on, that the secondary sizes


FWIW without multiSizeValidation enabled, we don't verify that the size if > 0. Perhaps GPT has special values that could be <= 0?

keithwrightbos · 2017-06-14T19:04:40Z

ads/google/utils.js

-  if (widthCond(width) && heightCond(height)) {
+  const isWidthIllegal = widthCond(width);
+  const isHeightIllegal = heightCond(height);
+  if (isWidthIllegal && isHeightIllegal) {


This seems overally verbose. I would instead allow badParams to be an array and errorBuilder should handle it as such:

if (widthCond(width)) { badParams.push({...}); }
if (heightCond(height)) { badParams.push({...}); }
user().assert(!reportParam || !badParams.length, errorBuilder(badParams));
return !badParams.length;

Great suggestion, thanks! Done.

keithwrightbos · 2017-06-14T19:05:38Z

extensions/amp-a4a/0.1/amp-a4a.js

@@ -36,9 +36,11 @@ import {isArray, isObject, isEnumValue} from '../../../src/types';
 import {some} from '../../../src/utils/promise';
 import {utf8Decode} from '../../../src/utils/bytes';
 import {viewerForDoc} from '../../../src/services';
+import {timerFor} from '../../../src/services';


We can combine all of these into a single import. Something like:

import {cryptoFo, resourcesForDoc, timerFor, viewerFor} from....

Done. Unless you actually want all/most of the imports inline.

keithwrightbos · 2017-06-14T19:06:23Z

extensions/amp-a4a/0.1/amp-a4a.js

+
+    /**
+     * Indicates whether the ad is currently in the process of being refreshed.
+     *


Nit: I wouldn't put an extra line here and in other fields

keithwrightbos · 2017-06-14T19:11:06Z

extensions/amp-a4a/0.1/amp-a4a.js

+   *   restart the refresh cycle.
+   */
+  refresh(refreshEndCallback) {
+    this.isRefreshing = true;


Add an assert before this: dev().assert(!this.isRefreshing)

keithwrightbos · 2017-06-14T19:12:05Z

extensions/amp-a4a/0.1/amp-a4a.js

+      this.togglePlaceholder(true);
+      this.destroyFrame(true);
+      // We don't want the next creative to appear too suddenly, so we
+      // show the loader for a quarter of a second before switching to


I still think a quarter of a second is short... most people can't really perceive that other than a flash. I suggest this be 1sec

keithwrightbos · 2017-06-14T19:17:35Z

extensions/amp-a4a/0.1/amp-a4a.js

+        refreshEndCallback();
+        return;
+      }
+      this.togglePlaceholder(true);


These should be within layoutCallback to ensure you are not mutating the DOM outside of a vsync. This includes everything within the timer delay (just make sure layoutCallback returns the results of the timerFor promise. This way you shouldn't need refresh ready promise as its all contained within layoutCallback.

Unfortunately, I need to call refreshEndCallback(), which is passed to refresh() from the RefreshManager, after rendering is complete to restart the refresh cycle. I can move all of the DOM mutating stuff to layoutCallback, but I'll still need refreshReadyPromise_ so that I can signal within refresh() when to call refreshEndCallback(). Also, timerFor(this.win).delay() returns a cancellation id, not a promise; not sure how much that matters.

Also, it appears that forcing the scheduler to run earlier removes the ad loader, so moving everything into layoutCallback has the effect of essentially never showing the loader, just one ad quickly transitioning to another.

This happens with my code as:

/** @override */ layoutCallback() { if (this.isRefreshing) { this.togglePlaceholder(true); this.destroyFrame(true); } return this.attemptToRenderCreative().then(result => { if (this.isRefreshing) { // We don't want the next creative to appear too suddenly, so we // show the loader for a quarter of a second before switching to // the new creative. timerFor(this.win).delay(() => { this.togglePlaceholder(false); this.refreshReadyPromiseResolver_(result); }, 1000); } return result; }); }

At some point, very quickly, after displaying the loader, the runtime removes it before this.togglePlaceholder(false) is called. Quick enough that you can't see it.

keithwrightbos · 2017-06-20T15:02:32Z

ads/google/a4a/utils.js

-      (containerTypeSet[ValidAdContainerTypes['AMP-FX-FLYING-CARPET']]
-       || containerTypeSet[ValidAdContainerTypes['AMP-STICKY-AD']])
+      (enclosingContainers.indexOf(
+          ValidAdContainerTypes['AMP-FX-FLYING-CARPET']) != -1


can you use includes:

const pfx = enclosingContainers.includes(ValidAdContainerTypes['AMP-FX-FLYING-CARPET']) || enclosingContainers.includes(ValidAdContainerTypes['AMP-STICKY-AD']);

keithwrightbos · 2017-06-20T15:03:18Z

ads/google/utils.js

- * @return {!Array<Array<number>>} An array of dimensions.
+ * @param {boolean} strict If set to true, this indicates that a single
+ *   malformed size should cause the entire multi-size data string to be
+ *   abnadoned. If set to false, then malformed sizes will be ignored, and the


abnadoned -> abandoned

keithwrightbos · 2017-06-20T15:03:30Z

ads/google/utils.js

+ *   abnadoned. If set to false, then malformed sizes will be ignored, and the
+ *   remainder of the string will be parsed for any additional sizes.
+ *   Additionally, errors will only be reported if this flag is set to true.
+ * @return {?Array<Array<number>>} An array of dimensions.


{?Array<!Array<number>>}

It's possible for this function to return an empty array.

keithwrightbos · 2017-06-20T15:07:20Z

ads/google/utils.js

-      return;
+        w => isNaN(w) || w <= 0,
+        h => isNaN(h) || h <= 0,
+        badParams => badParams.map(badParam =>


Seems like you do a similar operation multiple times where you do a badParams mapping and then return or continue based on strict. Consider creating a helper function to save cost & paste

Every call to validateDimensions has slightly varying arguments, so I'm not sure how to take advantage of a helper function here. I'm open to suggestions, though.

keithwrightbos · 2017-06-20T15:08:33Z

extensions/amp-a4a/0.1/amp-a4a.js

+    this.isRefreshing = false;
+
+    /** @private {boolean} */
+    this.isRelayoutNeeded_ = false;


Could consider making refreshing a state variable and combine these.

keithwrightbos · 2017-06-20T15:21:23Z

extensions/amp-a4a/0.1/refresh-manager.js

+        metaTagContent ? metaTagContent.split(',') : [];
+    for (let i = 0; i < networkIntervalPairs.length; i++) {
+      const pair = networkIntervalPairs[i].split('=');
+      if (pair.length != 2) {


Can this just be:

user().assert(pair.length == 2, 'refresh metadata...');
if (pair[0] == this.adType_) {
return ...
}

keithwrightbos · 2017-06-20T15:24:12Z

extensions/amp-a4a/0.1/refresh-manager.js

+    if (isNaN(refreshInterval) || refreshInterval == '') {
+      user().warn(TAG, 'refresh interval must be a number');
+      return null;
+    } else if (refreshInterval < MIN_REFRESH_INTERVAL) {


I don't believe you are properly handling a string based interval here. Perhaps it should be:
const refreshIntervalNum = Number(refreshInterval);
dev().assert(!isNaN(refreshInterval) && refreshInterval >= MIN_REFRESH_INTERVAL, 'invalid refresh interval, must be at least ${MIN_REFRESH_INTERVAL}s: ${refreshInterval}`);

keithwrightbos · 2017-06-20T15:25:36Z

extensions/amp-ad-network-doubleclick-impl/0.1/amp-ad-network-doubleclick-impl.js

+        totalTimeMin: 0,
+        continuousTimeMin: 1,
+      });
+      if (this.refreshManager_.isRefreshable()) {


should refreshManager constructor just initiate the refresh cycle? Then you could minimize this to:

this.refreshManager_ = this.refreshManager_ || new RefreshManager(...);

keithwrightbos · 2017-06-20T15:26:07Z

extensions/amp-ad-network-doubleclick-impl/0.1/amp-ad-network-doubleclick-impl.js

+   * @param {function()} refreshEndCallback
+   *
+   * @override
+   */


This can just be /** @override */

keithwrightbos · 2017-06-20T15:35:52Z

extensions/amp-ad/0.1/amp-ad-xorigin-iframe-handler.js

   * @return {!Promise} awaiting render complete promise
   */
-  init(iframe, opt_isA4A) {
+  init(iframe, opt_isA4A, opt_isRefreshing) {


Let's discuss offline but ideally we wouldn't polluate xorigin handler for this case

rsimha · 2017-06-21T21:28:50Z

The Percy failure was due to #10022, which has now been fixed.

keithwrightbos · 2017-06-23T14:24:41Z

extensions/amp-a4a/0.1/amp-a4a.js

+   *   the refresh function complete. This is particularly handy for testing.
+   */
+  refresh(refreshEndCallback) {
+    return new Promise(resolve => {


Why do you need a new Promise here? You should just be able to have each of the promise chains return

I'd have to wrap the timerFor block in a new Promise at the least. I can do that if you find it preferable.

keithwrightbos · 2017-06-23T14:31:24Z

extensions/amp-a4a/0.1/amp-a4a.js

+          refreshEndCallback();
+          return;
+        }
+        this.getVsync().mutate(() => {


I think this should be:
return this.mutateElement(() => {
this.togglePlaceholder(true);
return timerFor(this.win)....
});

Note that this delays the timer execution until we are able to mutate. I believe this is the correct behavior but I thought we had discussed relying on priority to impose the delay? Essentially the flow would be:

in refresh initiate ad request

after ad request, if valid creative (which I'm not sure you're asserting) toggle placeholder and schedule for layout

ensure that priority is not update to 0 even if an AMP creative (this will ensure 1 second delay is imposed)

layoutCallback can then execute per usual

We discussed both options, changing priority and using timer. It seems to me that messing with the priority to get a 1 second delay seems like roundabout way of doing it when we can just use a timer.

after ad request, if valid creative (which I'm not sure you're asserting)

If the creative would force a collapse, then isRefreshing would be set to false, short circuiting the function early. Also, as previously mentioned, timerFor(...).delay() returns a string cancellation id, not a promise.

You can use timerFor(...).promise(1000) which will return a promise allowing for easier integration

Ah, sorry. Didn't know that function existed.

keithwrightbos · 2017-06-23T15:41:32Z

extensions/amp-a4a/0.1/amp-a4a.js

+        refreshEndCallback();
+        return;
+      }
+      return this.getVsync().mutate(() => {


I still think this should be this.mutateElement(() => {...

keithwrightbos · 2017-06-23T15:49:20Z

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

+      sandbox.restore();
+    });
+
+    it('should effectively reset the slot and invoke given callback', () => {


Can we add a couple more tests:

Verify that if ad promise does not fill, we do nothing (no placeholder, no destroy frame, etc)

Verify that unlayoutCallback called during refresh works correctly

Verify that unlayoutCallback followed by resume works correctly

Verify refresh does not increment ifi as part of the request and include refresh count

Working on bullet points 1, 2, and 4 (this will need to be part of doubleclick impl's tests). Not sure what you mean for the third one, especially by resume. Can you elaborate?

These tests are rather painful to write given how much they rely on the lifecycle methods. Before I continue, could you verify that the rest of the PR looks good, so that I can be sure the tests I'm writing make sense?

keithwrightbos · 2017-06-23T15:50:09Z

extensions/amp-a4a/amp-ad-refresh.md

@@ -0,0 +1,65 @@
+<!---


Let's move this to amp-ad-network-doubleclick-impl md. We should probably also indicate that this is still in beta, meaning we don't yet verify its correctness.

SchnWalter · 2017-06-27T04:46:34Z

extensions/amp-a4a/0.1/refresh-manager.js

+ */
+export let RefreshConfig;
+
+export const MIN_REFRESH_INTERVAL = 3;


MIN_REFRESH_INTERVAL is set to 3 seconds, but the comment for checkAndSanitizeRefreshInterval_() states that the minimum is 30 seconds as well as the documentation in the "amp-ad-network-doubleclick-impl-internal.md" file.

The shorter time delay makes testing and development much easier. I'll be sure to have this changed to 30 before submitting.

lannka · 2017-06-27T21:35:13Z

@zhouyx could you please take a look at this PR?

zhouyx · 2017-06-27T23:05:45Z

ads/google/a4a/utils.js

+ * @param {!Element} adElement
+ * @return {!Array<string>}
+ */
+export function getEnclosingContainerTypes(adElement) {


why we have a counter 20 here? Is checking el.tagName == 'BODY' better, like what we do in #isAdPositionAllowed function

This is considered a best practice in the context of ads tagging. Apparently, even though it should by all rights be impossible, there have been reports of JavaScript tags encountering cyclic DOM trees in the wild. If code that traversed the DOM without a loop counter encountered such a cycle, the browser would hang.

That said, I'm not sure AMP really needs to worry about this.

zhouyx · 2017-06-28T01:43:41Z

extensions/amp-a4a/0.1/refresh-manager.js

+      return Promise.resolve(false);
+    }
+    return new Promise(resolve => {
+      analyticsForDoc(this.element_, true).then(analytics => {


Hmmm I wonder if there's better approach here. I don't like the idea of loading analytics extension only to use the visibilityManager. visibiltyManager is a small part of amp-analytics, we can either refactor the visiblityManager to a shared class, or have refresh manager implement its version.

Initially I had my own implementation, but was explicitly asked to use visibilityManager instead. If we go with the refactoring route, who would own the refactoring?

I'm not too familiar with how visibility-manager, or most of analytics for that matter, works. Is there a reason I can't directly create an instance of VisibilityManager and use that?

I see. if we refactor it I think it will need to be a service. Do you still remember why asked to use visibilityManager?

I think the idea was to not reinvent the wheel. I don't know if there was any reason beyond that.

Would it be possible to leave this as is for now, and optimize it in a later PR? I would gladly own the refactoring task.

There will be the hard question where do we put the code if we want to share.
We certainly don't want it to be in amphtml/core to bloat the size.
We also don't want a4a to have direct code dependency with analytics.

And also, given that your logic might be simpler than the visibilitymanager in amp-analytics, implementing your own might not be bad idea. You can still reuse the lower level IntersectionObserver code (polyfill etc).

Okay, I'm sure I can salvage most of the code that I had already written.

@jridgewell since you're planning to provide visibility info in a central place with your layoutmanager work, for an easier upgrade to that, do you think here we should use InOb directly, or should we rely on the viewportCallback?

keithwrightbos · 2017-06-30T17:50:29Z

extensions/amp-ad-network-doubleclick-impl/0.1/amp-ad-network-doubleclick-impl.js

+        ? this.refreshCount_ + 1
+        : (this.fromResumeCallback ? 1 : this.refreshCount_ || null);
+    this.win['ampAdGoogleIfiCounter'] = this.win['ampAdGoogleIfiCounter'] || 1;
+    this.ifi_ = (this.isRefreshing && this.ifi_) ||


this.ifi_ is initialized as 0 so isRefreshing case will never evaluate to true, right? It will always increment global?

On the first pass this.ifi_ will be set to some non-zero value. On the second pass, if this.isRefreshing is true, then we'll use the value of this.ifi_ (no incrementing), otherwise we'll use the incremented value of this.win['ampAdGoogleIfiCounter'].

keithwrightbos · 2017-06-30T17:52:16Z

extensions/amp-ad-network-doubleclick-impl/0.1/amp-ad-network-doubleclick-impl.js

+  /** @override */
+  layoutCallback() {
+    const superReturnValue = super.layoutCallback();
+    this.refreshManager_ = this.refreshManager_ || new RefreshManager(this, {


Make sure to add check that we do not allow refresh if SRA is enabled. Ideally we would also report this to the pub

keithwrightbos · 2017-06-30T17:53:59Z

extensions/amp-a4a/0.1/refresh-manager.js

+   * IntersectionObserver implementation. It will implement the core logic of
+   * the refresh lifecycle, including the transitions of the DFA.
+   *
+   * @return {function(!Array<IntersectionObserverEntry>)}


{!function(!Array<!IntersectionObserverEntry>>)}

Using {!function(!Array<IntersectionObserverEntry>>)} since the array can be empty.

keithwrightbos · 2017-06-30T17:56:25Z

extensions/amp-a4a/0.1/refresh-manager.js

+   */
+  ioCallbackGenerator_() {
+    const refreshManager = this;
+    return entries => {


Any reason this can't be:
return entries => entries.forEach(entry => {
if (entry.target != refreshManager.element_) return;
switch(refreshManager.state_) {
...
}
});

taymonbeal · 2017-07-06T18:36:40Z

ads/google/a4a/utils.js

+ * @param {!Element} adElement
+ * @return {!Array<string>}
+ */
+export function getEnclosingContainerTypes(adElement) {


This is considered a best practice in the context of ads tagging. Apparently, even though it should by all rights be impossible, there have been reports of JavaScript tags encountering cyclic DOM trees in the wild. If code that traversed the DOM without a loop counter encountered such a cycle, the browser would hang.

That said, I'm not sure AMP really needs to worry about this.

taymonbeal · 2017-07-06T18:54:33Z

ads/google/utils.js

+ *   abandoned. If set to false, then malformed sizes will be ignored, and the
+ *   remainder of the string will be parsed for any additional sizes.
+ *   Additionally, errors will only be reported if this flag is set to true.
+ * @return {?Array<Array<number>>} An array of dimensions.


taymonbeal · 2017-07-06T18:56:40Z

ads/google/utils.js

-      badDim: 'width',
-      badVal: width,
-    };
+    errorBuilder, reportError) {


Optional parameters need to have explicit default values.

taymonbeal · 2017-07-06T18:57:23Z

extensions/amp-a4a/0.1/amp-a4a.js

+   * Attemps to render the returned creative following the resolution of the
+   * adPromise.
+   *
+   * @return {!Promise} Whether the creative was successfully rendered.


Needs generic arguments.

taymonbeal · 2017-07-06T18:57:46Z

extensions/amp-a4a/0.1/amp-a4a.js

+   *   this.isRefreshing is true.
+   * @protected
+   */
+  destroyFrame(force) {


Optional parameter needs explicit default.

taymonbeal · 2017-07-06T19:49:39Z

extensions/amp-a4a/0.1/refresh-manager.js

+/**
+ * Defines the DFA states for the refresh cycle.
+ *
+ * (1) All newly registered elements begin in the INITIAL state.


Better to use 1. as this is a Markdown list.

taymonbeal · 2017-07-06T20:14:36Z

extensions/amp-a4a/0.1/refresh-manager.js

+ *
+ * @const {!Object<string, (!IntersectionObserver|!IntersectionObserverPolyfill)>}
+ */
+const observers = {};


Can you explain why there needs to be mutable state in module-level variables? I tend to think of this as an antipattern.

There's a many to many relationship between RefreshManagers and IntersectionObservers. A RefreshManager needs a way of referencing all previously created IntersectionObservers so as to not create duplicates. Moreover, each IntersectionObserver iterates over a list of entries, each of which is associated with a particular RefreshManager--meaning each IO needs to have a way of referencing all existing RefreshManagers.

If you insist, I can wrap the module-level variables in a singleton, but it seems like overkill to me.

I'm slightly worried about the implications for testing and the more usual approach would be to use a window-level property, but if this works then it works, I suppose.

taymonbeal · 2017-07-06T20:15:15Z

extensions/amp-a4a/0.1/refresh-manager.js

+        // The network has opted-in.
+        !!(this.config_
+        // The publisher has enabled refresh on this slot.
+        && (this.refreshInterval_ || this.refreshInterval_ == '')


Infix operators go before line breaks.

taymonbeal · 2017-07-06T20:15:56Z

extensions/amp-a4a/0.1/refresh-manager.js

+   * IntersectionObserver implementation. It will implement the core logic of
+   * the refresh lifecycle, including the transitions of the DFA.
+   *
+   * @param {!Array<IntersectionObserverEntry>} entries


!IntersectionObserverEntry

taymonbeal · 2017-07-06T20:17:13Z

extensions/amp-a4a/0.1/refresh-manager.js

+  }
+
+  /**
+   * Converts config to appropriate units.


Please state explicitly that this mutates its argument.

taymonbeal · 2017-07-07T20:58:57Z

extensions/amp-a4a/0.1/refresh-manager.js

+ *     The element will remain in this state until reset() is called on the
+ *     element, at which point it will return to the INITIAL state.
+ *
+ * @enum {string}


If the enum is unexported then the tests probably shouldn't be introspecting it. But you can leave it as is.

keithwrightbos · 2017-07-11T16:58:19Z

extensions/amp-a4a/0.1/amp-a4a.js

+    this.isRefreshing = true;
+    this.tearDownSlot();
+    this.initiateAdRequest();
+    const promiseId = this.promiseId_;


Add a dev().assert(this.adPromise_)

keithwrightbos · 2017-07-11T18:00:26Z

extensions/amp-ad-network-doubleclick-impl/0.1/amp-ad-network-doubleclick-impl.js

+  /** @override */
+  layoutCallback() {
+    const superReturnValue = super.layoutCallback();
+    user().assert(


We don't want to throw here so instead just wrap user().warn in if statement

keithwrightbos · 2017-07-11T18:00:48Z

extensions/amp-ad-network-doubleclick-impl/0.1/amp-ad-network-doubleclick-impl.js

+  refresh(refreshEndCallback) {
+    const promise = super.refresh(refreshEndCallback);
+    this.refreshCount_++;
+    return promise;


Why not just return super.refresh(refreshEndCallback)?

keithwrightbos · 2017-07-11T18:01:11Z

extensions/amp-ad-network-doubleclick-impl/0.1/test/test-amp-ad-network-doubleclick-impl.js

@@ -487,6 +489,52 @@ describes.sandboxed('amp-ad-network-doubleclick-impl', {}, () => {
              // Ensure that "auto" doesn't appear anywhere here:
              expect(url).to.match(/sz=[0-9]+x[0-9]+%7C1x2%7C3x4&/));
        });
+    it('should have the correct ifi numbers - no refresh', function() {


Add SRA + refresh test

keithwrightbos

Minor items. Waiting to merge until @lannka or @zhouyx have a chance to complete review.

keithwrightbos · 2017-07-11T19:04:08Z

extensions/amp-a4a/0.1/refresh-manager.js

+        (this.refreshInterval_ || this.refreshInterval_ == '') &&
+        // The slot is contained only within container types eligible for
+        // refresh.
+        !getEnclosingContainerTypes(this.element_).filter(container =>


arguably this constraint should be within doubleclick impl and not refresh manager as other networks will likely not have the same constraint. Mind moving it?

keithwrightbos · 2017-07-11T19:05:18Z

extensions/amp-ad-network-doubleclick-impl/amp-ad-network-doubleclick-impl-internal.md

@@ -72,4 +72,48 @@ Below the term `primary size` refers to the width and height pair specified by t
 - `data-multi-size` A string of comma separated sizes, which if present, forces the tag to request an ad with all of the given sizes, including the primary size. Each individual size must be a number (the width) followed by a lowercase 'x' followed by a number (the height). Each dimension specified this way must not be larger than its counterpart in the primary size. Further, each dimension must be no less than 2/3rds of the corresponding primary dimension, unless `data-mutli-size-validation` is set to false.
 - `data-multi-size-validation` If set to false, this will allow secondary sizes (those specified in the `data-multi-size` attribute) to be less than 2/3rds of the corresponding primary size. By default this is assumed to be true.

+### AMP Ad Refresh


Mind adding a section on SRA and mentioning the refresh+SRA is not supported?

erwinmombay · 2017-07-12T20:42:04Z

is this good to merge?

…ment.

zhouyx · 2017-07-18T16:46:56Z

LGTM

b2adops · 2018-07-03T13:13:19Z

has there been any progress on this project to enable refresh on inline amp ad units? I work with several publishers where this functionality has been requested.

glevitzky · 2018-07-03T13:24:49Z

Yes, refresh is now available on all DoubleClick type amp-ad units, except where SRA is enabled.

b2adops · 2018-07-03T13:27:46Z

Great! Is enabling as simple as adding data-enable-refresh="30" to the amp tag, or is more needed in order to enable? Is it best practice to add a refresh call to each individual ad slot, or in the meta tag?

glevitzky · 2018-07-03T13:44:04Z

data-enable-refresh="30" is all you need. There's no preference between using a meta-tag or doing it at the slot-level--either is fine, depending on your use-case.

b2adops · 2018-07-03T14:01:46Z

Thanks! We're working with a company called adthrive and the amp tags they provide are not DoubleClick type, would the same principles work here, or would we need these ads to run as doubleclick to ensure functionality?

glevitzky · 2018-07-03T14:19:30Z

Ah, my apologies for misunderstanding. Currently, DoubleClick is the only ad network that supports refresh. In order for adthrive to support it, they would need to write their own Fast Fetch extension and implement refresh within it. If this is something they're interested in doing, let me know, and I'll try and provide as much support as I can.

b2adops · 2018-07-03T14:28:15Z

Thanks so much, I really appreciate it. I've got a call with adthrive this afternoon and will have a better idea of next steps then.

thanks again!

googlebot added the cla: yes label May 24, 2017

glevitzky added the WG: monetization label May 24, 2017

taymonbeal suggested changes May 24, 2017

View reviewed changes

keithwrightbos requested changes May 25, 2017

View reviewed changes

keithwrightbos requested changes Jun 1, 2017

View reviewed changes

glevitzky commented Jun 1, 2017

View reviewed changes

keithwrightbos requested changes Jun 2, 2017

View reviewed changes

glevitzky commented Jun 8, 2017

View reviewed changes

keithwrightbos requested changes Jun 8, 2017

View reviewed changes

lannka reviewed Jun 14, 2017

View reviewed changes

keithwrightbos requested changes Jun 14, 2017

View reviewed changes

keithwrightbos requested changes Jun 20, 2017

View reviewed changes

keithwrightbos requested changes Jun 23, 2017

View reviewed changes

glevitzky closed this Jun 26, 2017

glevitzky reopened this Jun 26, 2017

SchnWalter mentioned this pull request Jun 27, 2017

Scroll-dependent header hiding/showing/docking #8268

Closed

SchnWalter reviewed Jun 27, 2017

View reviewed changes

lannka requested a review from zhouyx June 27, 2017 21:33

zhouyx reviewed Jun 28, 2017

View reviewed changes

keithwrightbos approved these changes Jun 30, 2017

View reviewed changes

taymonbeal suggested changes Jul 6, 2017

View reviewed changes

taymonbeal approved these changes Jul 7, 2017

View reviewed changes

keithwrightbos requested changes Jul 11, 2017

View reviewed changes

keithwrightbos approved these changes Jul 11, 2017

View reviewed changes

glevitzky added 17 commits July 18, 2017 12:42

Guarding refresh feature behind experiment flag.

d0b9704

Minor cosmetic changes.

034b7e7

Merges.

b904124

Reverting RefreshManager to use initial implementation of view manage…

448f07b

…ment.

min refresh interval 3 -> 30.

dff0d73

Minor fixes, PR feedback, SRA carveout.

16de942

Removed duplicate file.

e7653bf

Fixed user().assert

8b2499e

Fixed tests.

ce9e829

PR feedback.

26d6afe

Refresh + SRA test, minor misc fixes.

2250818

Moved container-requirement into doubleclick impl + docs update.

6220374

Merges.

54ce29c

Post-merge fixes.

03ee809

PR feedback.

932760f

Removed superfluous 'REFRESHED' state.

afc635d

Fixing comments.

de17663

keithwrightbos merged commit f9f49aa into ampproject:master Jul 18, 2017

keithwrightbos deleted the refresh_poc branch July 18, 2017 19:04

ampprojectbot added the PR use: In Beta / Experimental label Jul 24, 2017

ampprojectbot added the PR use: In Stable label Aug 2, 2017

ampprojectbot mentioned this pull request Apr 30, 2020

🚨 Error: Null promise in layoutCallback #28140

Closed

		@@ -0,0 +1,470 @@
		/**
		* Copyright 2016 The AMP HTML Authors. All Rights Reserved.

		@@ -0,0 +1,117 @@
		/**
		* Copyright 2015 The AMP HTML Authors. All Rights Reserved.

AMP Ad Refresh #9535

AMP Ad Refresh #9535

Conversation

glevitzky commented May 24, 2017 • edited

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

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

taymonbeal commented May 24, 2017

keithwrightbos left a 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

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

keithwrightbos left a 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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glevitzky commented Jun 9, 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

glevitzky commented May 24, 2017 •

edited