Better tests

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

@@ -97,9 +97,6 @@ describes.realWin(
         get: (name) => {
           return Promise.resolve(storageValue[name]);
         },
-        getUnexpiredValue: () => {
-          return Promise.resolve();
-        },
         set: (name, value) => {
           storageValue[name] = value;
           return Promise.resolve();

spec/amp-var-substitutions.md

@@ -550,7 +550,7 @@ You can pass the following arguments into the Client ID variable like a function
 - `cid scope` (Required): The namespace for the Client ID.
 - `amp-user-notification id` (Optional): Use this argument to make the Client ID substitution dependent on the dismissal of a user notification shown to the visitor of the page. In amp-analytics, this is the same as using the [`data-consent-notification-id`](../extensions/amp-analytics/amp-analytics.md) attribute -- you may choose to use either one for the amp-analytics component.
 - `cookie name` (Optional): The name of the fallback cookie when the document is not served by an AMP proxy. If not provided, `cid scope` will be used as the cookie name.
-- `opt out backup` (Optional): If set to `true`, then will opt-out of storing the AMP generated CID in Storage as a backup when the document is not served by an AMP proxy. Otherwise, will default into this behavior.
+- `disable backup` (Optional): If set to `true`, then will opt-out of storing the AMP generated CID in Storage as a backup when the document is not served by an AMP proxy. Otherwise, will default into this behavior.
 
 #### Content Load Time
 

src/service/cid-impl.js

@@ -51,6 +51,8 @@ const CID_OPTOUT_STORAGE_KEY = 'amp-cid-optout';
 
 const CID_OPTOUT_VIEWER_MESSAGE = 'cidOptOut';
 
+const CID_BACKUP_STORAGE_KEY = 'amp-cid:';
+
 /**
  * Tag for debug logging.
  * @const @private {string}
@@ -90,13 +92,13 @@ let BaseCidInfoDef;
  * - createCookieIfNotPresent: Whether CID is allowed to create a cookie when.
  *   Default value is `false`.
  * - cookieName: Name of the cookie to be used if defined for non-proxy case.
- * - optOutBackup: Whether CID should not be backed up in Storage.
+ * - disableBackup: Whether CID should not be backed up in Storage.
  *   Default value is `false`.
  * @typedef {{
  *   scope: string,
  *   createCookieIfNotPresent: (boolean|undefined),
  *   cookieName: (string|undefined),
- *   optOutBackup: (boolean|undefined),
+ *   disableBackup: (boolean|undefined),
  * }}
  */
 let GetCidDef;
@@ -410,49 +412,38 @@ function setCidBackup(ampdoc, cookieName, cookie) {
  * @return {string}
  */
 function getStorageKey(cookieName) {
-  return 'amp-cid:' + cookieName;
+  return CID_BACKUP_STORAGE_KEY + cookieName;
 }
 
 /**
  * Maybe gets the CID from cookie or, if allowed, gets backup CID
  * from Storage and checks validitiy.
  * @param {!Cid} cid
  * @param {!GetCidDef} getCidStruct
- * @return {!Promise<?string|undefined>}
+ * @return {!Promise<?string>}
  */
 function maybeGetCidFromCookieOrBackup(cid, getCidStruct) {
   const {ampdoc, isBackupCidExpOn} = cid;
   const {win} = ampdoc;
-  const {optOutBackup, scope} = getCidStruct;
+  const {disableBackup, scope} = getCidStruct;
   const cookieName = getCidStruct.cookieName || scope;
   const existingCookie = getCookie(win, cookieName);
 
   if (existingCookie) {
-    // If we created the cookie, update it's expiration time.
-    if (/^amp-/.test(existingCookie)) {
-      setCidCookie(win, cookieName, existingCookie);
-
-      if (isBackupCidExpOn && !optOutBackup) {
-        setCidBackup(ampdoc, cookieName, existingCookie);
-      }
-    }
     return Promise.resolve(existingCookie);
   }
-  if (isBackupCidExpOn && !optOutBackup) {
+  if (isBackupCidExpOn && !disableBackup) {
     return Services.storageForDoc(ampdoc).then((storage) => {
       const key = getStorageKey(cookieName);
-      return storage
-        .getUnexpiredValue(key, BASE_CID_MAX_AGE_MILLIS)
-        .then((backupCid) => {
-          if (backupCid) {
-            setCidCookie(win, cookieName, backupCid);
-            setCidBackup(cid.ampdoc, cookieName, backupCid);
-          }
-          return backupCid;
-        });
+      return storage.get(key, BASE_CID_MAX_AGE_MILLIS).then((backupCid) => {
+        if (!backupCid || typeof backupCid != 'string') {
+          return null;
+        }
+        return backupCid;
+      });
     });
   }
-  return Promise.resolve();
+  return Promise.resolve(null);
 }
 
 /**
@@ -466,7 +457,7 @@ function maybeGetCidFromCookieOrBackup(cid, getCidStruct) {
 function getOrCreateCookie(cid, getCidStruct, persistenceConsent) {
   const {isBackupCidExpOn, ampdoc} = cid;
   const {win} = ampdoc;
-  const {scope, optOutBackup} = getCidStruct;
+  const {scope, disableBackup} = getCidStruct;
   const cookieName = getCidStruct.cookieName || scope;
 
   return maybeGetCidFromCookieOrBackup(cid, getCidStruct).then(
@@ -476,6 +467,13 @@ function getOrCreateCookie(cid, getCidStruct, persistenceConsent) {
       }
 
       if (existingCookie) {
+        // If we created the cookie, update it's expiration time.
+        if (/^amp-/.test(existingCookie)) {
+          setCidCookie(win, cookieName, existingCookie);
+          if (isBackupCidExpOn && !disableBackup) {
+            setCidBackup(ampdoc, cookieName, existingCookie);
+          }
+        }
         return /** @type {!Promise<?string>} */ (Promise.resolve(
           existingCookie
         ));
@@ -498,7 +496,7 @@ function getOrCreateCookie(cid, getCidStruct, persistenceConsent) {
         const relookup = getCookie(win, cookieName);
         if (!relookup) {
           setCidCookie(win, cookieName, newCookie);
-          if (isBackupCidExpOn && !optOutBackup) {
+          if (isBackupCidExpOn && !disableBackup) {
             setCidBackup(ampdoc, cookieName, newCookie);
           }
         }

src/service/storage-impl.js

@@ -76,33 +76,11 @@ export class Storage {
    * Returns the promise that yields the value of the property for the specified
    * key.
    * @param {string} name
+   * @param {number=} opt_duration
    * @return {!Promise<*>}
    */
-  get(name) {
-    return this.getStore_().then((store) => store.get(name));
-  }
-
-  /**
-   * Returns the promise that yields the value of the property for the specified
-   * key, as long as it has a valid expiration.
-   * @param {string} name
-   * @param {number} duration
-   * @return {!Promise<*>}
-   */
-  getUnexpiredValue(name, duration) {
-    return this.getStore_().then((store) => {
-      return Promise.all([store.get(name), store.getTimestamp(name)]).then(
-        (results) => {
-          const value = results[0];
-          const timestamp = results[1];
-          const now = Date.now();
-          if (timestamp != undefined && timestamp + duration > now) {
-            return value;
-          }
-          return;
-        }
-      );
-    });
+  get(name, opt_duration) {
+    return this.getStore_().then((store) => store.get(name, opt_duration));
   }
 
   /**
@@ -246,22 +224,19 @@ export class Store {
 
   /**
    * @param {string} name
+   * @param {number|undefined} opt_duration
    * @return {*|undefined}
    */
-  get(name) {
-    // The structure is {key: {v: *, t: time}}
-    const item = this.values_[name];
-    return item ? item['v'] : undefined;
-  }
-
-  /**
-   * @param {string} name
-   * @return {number|undefined}
-   */
-  getTimestamp(name) {
+  get(name, opt_duration) {
     // The structure is {key: {v: *, t: time}}
     const item = this.values_[name];
-    return item ? item['t'] : undefined;
+    const timestamp = item ? item['t'] : undefined;
+    const isNotExpired =
+      opt_duration && timestamp != undefined
+        ? timestamp + opt_duration > Date.now()
+        : true;
+    const value = item && isNotExpired ? item['v'] : undefined;
+    return value;
   }
 
   /**

src/service/url-replacements-impl.js

@@ -267,7 +267,7 @@ export class GlobalVariableSource extends VariableSource {
         }
         return clientIds[scope];
       },
-      (scope, opt_userNotificationId, opt_cookieName, opt_optOutBackup) => {
+      (scope, opt_userNotificationId, opt_cookieName, opt_disableBackup) => {
         userAssert(
           scope,
           'The first argument to CLIENT_ID, the fallback' +
@@ -287,13 +287,13 @@ export class GlobalVariableSource extends VariableSource {
         }
         return Services.cidForDoc(this.ampdoc)
           .then((cid) => {
-            opt_optOutBackup = opt_optOutBackup == 'true' ? true : false;
+            opt_disableBackup = opt_disableBackup == 'true' ? true : false;
             return cid.get(
               {
                 /** @type {string} */ scope,
                 createCookieIfNotPresent: true,
                 cookieName: opt_cookieName || undefined,
-                optOutBackup: opt_optOutBackup,
+                disableBackup: opt_disableBackup,
               },
               consent
             );