suggested changes

ampproject · Sep 29, 2020 · 6f62b0f · 6f62b0f
1 parent 8a1ecb6
commit 6f62b0f
Show file tree

Hide file tree

Showing 18 changed files with 40 additions and 95 deletions.
diff --git a/build-system/tasks/presubmit-checks.js b/build-system/tasks/presubmit-checks.js
@@ -1144,6 +1144,7 @@ const forbiddenTermsSrcInclusive = {
   '\\.setNonBoolean\\(': {
     message: requiresReviewPrivacy,
     allowlist: [
+      'src/service/cid-impl.js',
       'src/service/storage-impl.js',
       'extensions/amp-consent/0.1/consent-state-manager.js',
     ],

diff --git a/extensions/amp-access/0.1/amp-access.js b/extensions/amp-access/0.1/amp-access.js
@@ -167,11 +167,7 @@ export class AccessService {
       const consent = Promise.resolve();
       this.readerIdPromise_ = this.cid_.then((cid) => {
         return cid.get(
-          {
-            scope: 'amp-access',
-            createCookieIfNotPresent: true,
-            backupToStorage: true,
-          },
+          {scope: 'amp-access', createCookieIfNotPresent: true},
           consent
         );
       });

diff --git a/extensions/amp-access/0.1/test/test-amp-access.js b/extensions/amp-access/0.1/test/test-amp-access.js
@@ -373,11 +373,7 @@ describes.fakeWin(
       cidMock
         .expects('get')
         .withExactArgs(
-          {
-            scope: 'amp-access',
-            createCookieIfNotPresent: true,
-            backupToStorage: true,
-          },
+          {scope: 'amp-access', createCookieIfNotPresent: true},
           env.sandbox.match(() => true)
         )
         .returns(Promise.resolve(result))
@@ -803,11 +799,7 @@ describes.fakeWin(
       cidMock
         .expects('get')
         .withExactArgs(
-          {
-            scope: 'amp-access',
-            createCookieIfNotPresent: true,
-            backupToStorage: true,
-          },
+          {scope: 'amp-access', createCookieIfNotPresent: true},
           env.sandbox.match(() => true)
         )
         .returns(Promise.resolve(result))
@@ -1301,11 +1293,7 @@ describes.fakeWin(
       cidMock
         .expects('get')
         .withExactArgs(
-          {
-            scope: 'amp-access',
-            createCookieIfNotPresent: true,
-            backupToStorage: true,
-          },
+          {scope: 'amp-access', createCookieIfNotPresent: true},
           env.sandbox.match(() => true)
         )
         .returns(Promise.resolve('reader1'))
@@ -1326,11 +1314,7 @@ describes.fakeWin(
       cidMock
         .expects('get')
         .withExactArgs(
-          {
-            scope: 'amp-access',
-            createCookieIfNotPresent: true,
-            backupToStorage: true,
-          },
+          {scope: 'amp-access', createCookieIfNotPresent: true},
           env.sandbox.match(() => true)
         )
         .returns(Promise.resolve('reader1'))
@@ -1369,11 +1353,7 @@ describes.fakeWin(
       cidMock
         .expects('get')
         .withExactArgs(
-          {
-            scope: 'amp-access',
-            createCookieIfNotPresent: true,
-            backupToStorage: true,
-          },
+          {scope: 'amp-access', createCookieIfNotPresent: true},
           env.sandbox.match(() => true)
         )
         .returns(Promise.resolve('reader1'))
@@ -1891,11 +1871,7 @@ describes.fakeWin(
       cidMock
         .expects('get')
         .withExactArgs(
-          {
-            scope: 'amp-access',
-            createCookieIfNotPresent: true,
-            backupToStorage: true,
-          },
+          {scope: 'amp-access', createCookieIfNotPresent: true},
           env.sandbox.match(() => true)
         )
         .returns(Promise.resolve(result))

diff --git a/extensions/amp-consent/0.1/consent-config.js b/extensions/amp-consent/0.1/consent-config.js
@@ -342,7 +342,7 @@ export function expandConsentEndpointUrl(element, url) {
 export function getConsentCID(node) {
   return Services.cidForDoc(node).then((cid) => {
     return cid.get(
-      {scope: CID_SCOPE, createCookieIfNotPresent: true, backupToStorage: true},
+      {scope: CID_SCOPE, createCookieIfNotPresent: true},
       /** consent */ Promise.resolve()
     );
   });

diff --git a/extensions/amp-consent/0.1/test/test-amp-consent.js b/extensions/amp-consent/0.1/test/test-amp-consent.js
@@ -97,6 +97,9 @@ describes.realWin(
         get: (name) => {
           return Promise.resolve(storageValue[name]);
         },
+        getTimestamp: () => {
+          return Promise.resolve();
+        },
         set: (name, value) => {
           storageValue[name] = value;
           return Promise.resolve();

diff --git a/extensions/amp-experiment/0.1/test/test-variant.js b/extensions/amp-experiment/0.1/test/test-variant.js
@@ -261,7 +261,6 @@ describes.sandboxed('allocateVariant', {}, (env) => {
       .withArgs({
         scope: 'amp-experiment',
         createCookieIfNotPresent: true,
-        backupToStorage: true,
       })
       .returns(Promise.resolve('123abc'));
     uniformStub.withArgs('name:123abc').returns(Promise.resolve(0.4));
@@ -280,7 +279,6 @@ describes.sandboxed('allocateVariant', {}, (env) => {
       .withArgs({
         scope: 'custom-scope',
         createCookieIfNotPresent: true,
-        backupToStorage: true,
       })
       .returns(Promise.resolve('123abc'));
     uniformStub.withArgs('name:123abc').returns(Promise.resolve(0.4));
@@ -300,7 +298,6 @@ describes.sandboxed('allocateVariant', {}, (env) => {
       .withArgs({
         scope: 'amp-experiment',
         createCookieIfNotPresent: true,
-        backupToStorage: true,
       })
       .returns(Promise.resolve('123abc'));
     uniformStub.withArgs('custom-group:123abc').returns(Promise.resolve(0.4));
@@ -328,7 +325,6 @@ describes.sandboxed('allocateVariant', {}, (env) => {
       .withArgs({
         scope: 'amp-experiment',
         createCookieIfNotPresent: true,
-        backupToStorage: true,
       })
       .returns(Promise.resolve('123abc'));
     uniformStub.withArgs('name:123abc').returns(Promise.resolve(0.4));

diff --git a/extensions/amp-experiment/0.1/variant.js b/extensions/amp-experiment/0.1/variant.js
@@ -183,7 +183,6 @@ function getBucketTicket(ampdoc, group, opt_cidScope) {
       {
         scope: dev().assertString(opt_cidScope),
         createCookieIfNotPresent: true,
-        backupToStorage: true,
       },
       Promise.resolve()
     )

diff --git a/extensions/amp-experiment/1.0/test/test-variant.js b/extensions/amp-experiment/1.0/test/test-variant.js
@@ -366,7 +366,6 @@ describes.sandboxed('allocateVariant', {}, (env) => {
       .withArgs({
         scope: 'amp-experiment',
         createCookieIfNotPresent: true,
-        backupToStorage: true,
       })
       .returns(Promise.resolve('123abc'));
     uniformStub.withArgs('name:123abc').returns(Promise.resolve(0.4));
@@ -391,7 +390,6 @@ describes.sandboxed('allocateVariant', {}, (env) => {
       .withArgs({
         scope: 'custom-scope',
         createCookieIfNotPresent: true,
-        backupToStorage: true,
       })
       .returns(Promise.resolve('123abc'));
     uniformStub.withArgs('name:123abc').returns(Promise.resolve(0.4));
@@ -417,7 +415,6 @@ describes.sandboxed('allocateVariant', {}, (env) => {
       .withArgs({
         scope: 'amp-experiment',
         createCookieIfNotPresent: true,
-        backupToStorage: true,
       })
       .returns(Promise.resolve('123abc'));
     uniformStub.withArgs('custom-group:123abc').returns(Promise.resolve(0.4));
@@ -451,7 +448,6 @@ describes.sandboxed('allocateVariant', {}, (env) => {
       .withArgs({
         scope: 'amp-experiment',
         createCookieIfNotPresent: true,
-        backupToStorage: true,
       })
       .returns(Promise.resolve('123abc'));
     uniformStub.withArgs('name:123abc').returns(Promise.resolve(0.4));

diff --git a/extensions/amp-experiment/1.0/variant.js b/extensions/amp-experiment/1.0/variant.js
@@ -190,7 +190,6 @@ function getBucketTicket(ampdoc, group, opt_cidScope) {
       {
         scope: dev().assertString(opt_cidScope),
         createCookieIfNotPresent: true,
-        backupToStorage: true,
       },
       Promise.resolve()
     )

diff --git a/extensions/amp-story-interactive/0.1/amp-story-interactive-abstract.js b/extensions/amp-story-interactive/0.1/amp-story-interactive-abstract.js
@@ -378,11 +378,7 @@ export class AmpStoryInteractive extends AMP.BaseElement {
     if (!this.clientIdPromise_) {
       this.clientIdPromise_ = this.clientIdService_.then((data) => {
         return data.get(
-          {
-            scope: 'amp-story',
-            createCookieIfNotPresent: true,
-            backupToStorage: true,
-          },
+          {scope: 'amp-story', createCookieIfNotPresent: true},
           /* consent */ Promise.resolve()
         );
       });

diff --git a/extensions/amp-subscriptions/0.1/amp-subscriptions.js b/extensions/amp-subscriptions/0.1/amp-subscriptions.js
@@ -245,10 +245,7 @@ export class SubscriptionService {
       const scope =
         'amp-access' + (serviceId == 'local' ? '' : '-' + serviceId);
       readerIdPromise = this.cid_.then((cid) =>
-        cid.get(
-          {scope, createCookieIfNotPresent: true, backupToStorage: true},
-          consent
-        )
+        cid.get({scope, createCookieIfNotPresent: true}, consent)
       );
       this.serviceIdToReaderIdPromiseMap_[serviceId] = readerIdPromise;
     }

diff --git a/extensions/amp-subscriptions/0.1/test/test-amp-subscriptions.js b/extensions/amp-subscriptions/0.1/test/test-amp-subscriptions.js
@@ -264,7 +264,6 @@ describes.fakeWin('AmpSubscriptions', {amp: true}, (env) => {
         // Local service is default to "amp-access" scope.
         scope: 'amp-access',
         createCookieIfNotPresent: true,
-        backupToStorage: true,
       });
     });
 
@@ -273,7 +272,6 @@ describes.fakeWin('AmpSubscriptions', {amp: true}, (env) => {
       expect(cidGet).to.be.calledOnce.calledWith({
         scope: 'amp-access-service1',
         createCookieIfNotPresent: true,
-        backupToStorage: true,
       });
     });
 

diff --git a/extensions/amp-user-notification/0.1/amp-user-notification.js b/extensions/amp-user-notification/0.1/amp-user-notification.js
@@ -357,7 +357,7 @@ export class AmpUserNotification extends AMP.BaseElement {
       // the notification or have the nagging notification sitting there
       // (to never resolve).
       return cid.get(
-        {scope: TAG, createCookieIfNotPresent: true, backupToStorage: true},
+        {scope: TAG, createCookieIfNotPresent: true},
         Promise.resolve(),
         this.dialogPromise_
       );

diff --git a/src/ad-cid.js b/src/ad-cid.js
@@ -61,7 +61,6 @@ export function getOrCreateAdCid(
           scope: dev().assertString(clientIdScope),
           createCookieIfNotPresent: true,
           cookieName: opt_clientIdCookieName,
-          backupToStorage: true,
         },
         Promise.resolve(undefined)
       )

diff --git a/src/service/cid-impl.js b/src/service/cid-impl.js
@@ -91,14 +91,14 @@ let BaseCidInfoDef;
  * The "get CID" parameters.
  * - createCookieIfNotPresent: Whether CID is allowed to create a cookie when.
  *   Default value is `false`.
- * - cookieName: CookieName to be used if defined for non-proxy case.
- * - backupToStorage: Whether CID should be backed up in Storage.
+ * - cookieName: Name of the cookie to be used if defined for non-proxy case.
+ * - optOutBackup: Whether CID should not be backed up in Storage.
  *   Default value is `false`.
  * @typedef {{
  *   scope: string,
  *   createCookieIfNotPresent: (boolean|undefined),
  *   cookieName: (string|undefined),
- *   backupToStorage: (boolean|undefined),
+ *   optOutBackup: (boolean|undefined),
  * }}
  */
 let GetCidDef;
@@ -239,30 +239,25 @@ class Cid {
     const url = parseUrlDeprecated(this.ampdoc.win.location.href);
     if (!isProxyOrigin(url)) {
       const apiKey = this.isScopeOptedIn_(scope);
-      const cookieName = getCidStruct.cookieName || scope;
       if (apiKey) {
         return this.cidApi_.getScopedCid(apiKey, scope).then((scopedCid) => {
           if (scopedCid == TokenStatus.OPT_OUT) {
             return null;
           }
           if (scopedCid) {
+            const cookieName = getCidStruct.cookieName || scope;
             setCidCookie(this.ampdoc.win, cookieName, scopedCid);
             return scopedCid;
           }
           return maybeGetCidFromCookieOrBackup(
             this,
-            getCidStruct,
-            cookieName
+            getCidStruct
           ).then((cookie) =>
             returnOrCreateCookie(this, getCidStruct, persistenceConsent, cookie)
           );
         });
       }
-      return maybeGetCidFromCookieOrBackup(
-        this,
-        getCidStruct,
-        cookieName
-      ).then((cookie) =>
+      return maybeGetCidFromCookieOrBackup(this, getCidStruct).then((cookie) =>
         returnOrCreateCookie(this, getCidStruct, persistenceConsent, cookie)
       );
     }
@@ -408,8 +403,11 @@ function setCidCookie(win, scope, cookie) {
  */
 function setCidBackup(ampdoc, cookieName, cookie) {
   Services.storageForDoc(ampdoc).then((storage) => {
-    const key = getStorageKey(cookieName);
-    storage.setNonBoolean(key, cookie);
+    const isViewerStorage = storage.isViewerStorage();
+    if (!isViewerStorage) {
+      const key = getStorageKey(cookieName);
+      storage.setNonBoolean(key, cookie);
+    }
   });
 }
 
@@ -426,26 +424,27 @@ function getStorageKey(cookieName) {
  * from Storage and checks validitiy.
  * @param {!Cid} cid
  * @param {!GetCidDef} getCidStruct
- * @param {string} cookieName
  * @return {!Promise<?string|undefined>}
  */
-function maybeGetCidFromCookieOrBackup(cid, getCidStruct, cookieName) {
+function maybeGetCidFromCookieOrBackup(cid, getCidStruct) {
   const {ampdoc} = cid;
   const {win} = ampdoc;
-  const {backupToStorage} = getCidStruct;
+  const {optOutBackup, 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 (backupToStorage) {
+      if (!optOutBackup) {
         setCidBackup(ampdoc, cookieName, existingCookie);
       }
     }
     return Promise.resolve(existingCookie);
   }
-  if (backupToStorage) {
+  if (!optOutBackup) {
     return Services.storageForDoc(ampdoc).then((storage) => {
       const key = getStorageKey(cookieName);
       const backupCidPromise = storage.get(key);
@@ -486,7 +485,7 @@ function returnOrCreateCookie(
   existingCookie
 ) {
   const {win} = cid.ampdoc;
-  const {scope, backupToStorage} = getCidStruct;
+  const {scope, optOutBackup} = getCidStruct;
   const cookieName = getCidStruct.cookieName || scope;
 
   if (!existingCookie && !getCidStruct.createCookieIfNotPresent) {
@@ -514,7 +513,7 @@ function returnOrCreateCookie(
     const relookup = getCookie(win, cookieName);
     if (!relookup) {
       setCidCookie(win, cookieName, newCookie);
-      if (backupToStorage) {
+      if (!optOutBackup) {
         setCidBackup(cid.ampdoc, cookieName, newCookie);
       }
     }

diff --git a/src/service/url-replacements-impl.js b/src/service/url-replacements-impl.js
@@ -267,7 +267,7 @@ export class GlobalVariableSource extends VariableSource {
         }
         return clientIds[scope];
       },
-      (scope, opt_userNotificationId, opt_cookieName, opt_storage) => {
+      (scope, opt_userNotificationId, opt_cookieName, opt_optOutBackup) => {
         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_storage = opt_storage == 'false' ? false : true;
+            opt_optOutBackup = opt_optOutBackup == 'true' ? true : false;
             return cid.get(
               {
                 /** @type {string} */ scope,
                 createCookieIfNotPresent: true,
                 cookieName: opt_cookieName,
-                backupToStorage: opt_storage,
+                optOutBackup: opt_optOutBackup,
               },
               consent
             );