Move analytics services into extension directory and make deps check …

…stricter. (#3147)

The dep check now has a whitelist that allows fine grained whitelisting.

build-system/dep-check-config.js

@@ -19,7 +19,8 @@
  * - filesMatching - Is assumed to be all files if not provided.
  * - mustNotDependOn - If type is "forbidden" (default) then the files
  *     matched must not match the glob(s) provided.
- * - whitelist - Skip rule if file matches whitelist pattern(s).
+ * - whitelist - Skip rule if this particular dependency is found.
+ *     Syntax: fileA->fileB where -> reads "depends on"
  * @typedef {{
  *   type: (string|undefined),
  *   filesMatching: (string|!Array<string>|undefined),
@@ -30,10 +31,25 @@
 var RuleConfigDef;
 
 exports.rules = [
-  // Extensions must not import any services directly.
+  // Extensions must not import any central services directly.
   {
     filesMatching: 'extensions/**/*.js',
     mustNotDependOn: 'src/service/**/*.js',
-    whitelist: 'extensions/**/amp-analytics.js',
+  },
+  // Files under src must not depend on extensions.
+  {
+    filesMatching: 'src/**/*.js',
+    mustNotDependOn: 'extensions/**/*.js',
+  },
+  // Files under src must not depend on ads code.
+  {
+    filesMatching: 'src/**/*.js',
+    mustNotDependOn: 'ads/**/*.js',
+    whitelist: 'src/ad-cid.js->ads/_config.js',
+  },
+  // Files under src must not depend on 3p code.
+  {
+    filesMatching: 'src/**/*.js',
+    mustNotDependOn: '3p/**/*.js',
   },
 ];

build-system/tasks/dep-check.js

@@ -75,7 +75,7 @@ function Rule(config) {
   /** @private @const {!GlobsDef} */
   this.mustNotDependOn_ = toArrayOrDefault(config.mustNotDependOn, []);
 
-  /** @private @const {!GlobsDef} */
+  /** @private @const {!Array<string>} */
   this.whitelist_ = toArrayOrDefault(config.whitelist, []);
 }
 
@@ -86,13 +86,6 @@ function Rule(config) {
  */
 Rule.prototype.run = function(moduleName, deps) {
   var errors = [];
-  var inWhitelist = this.whitelist_.length ?
-      this.whitelist_.some(x => minimist(moduleName, x)) : false;
-
-  // Bail out early if its in the current rules whitelist.
-  if (inWhitelist) {
-    return errors;
-  }
 
   // If forbidden rule and current module has no dependencies at all
   // then no need to match.
@@ -125,6 +118,18 @@ Rule.prototype.matchBadDeps = function(moduleName, deps) {
   deps.forEach(dep => {
     this.mustNotDependOn_.forEach(badDepPattern => {
       if (minimatch(dep, badDepPattern)) {
+        var inWhitelist = this.whitelist_.some(entry => {
+          var pair = entry.split('->');
+          var whitelistedModuleName = pair[0];
+          var whitelistedDep = pair[1];
+          if (moduleName != whitelistedModuleName) {
+            return false;
+          }
+          return dep == whitelistedDep;
+        });
+        if (inWhitelist) {
+          return;
+        }
         mustNotDependErrors.push(`${moduleName} must not depend on ${dep}. ` +
             `Rule: ${JSON.stringify(this.config_)}.`);
       }

build-system/tasks/presubmit-checks.js

@@ -87,14 +87,14 @@ var forbiddenTerms = {
   'installActivityService': {
     message: privateServiceFactory,
     whitelist: [
-      'src/service/activity-impl.js',
+      'extensions/amp-analytics/0.1/activity-impl.js',
       'extensions/amp-analytics/0.1/amp-analytics.js'
     ]
   },
   'installCidService': {
     message: privateServiceFactory,
     whitelist: [
-      'src/service/cid-impl.js',
+      'extensions/amp-analytics/0.1/cid-impl.js',
       'extensions/amp-access/0.1/amp-access.js',
       'extensions/amp-analytics/0.1/amp-analytics.js',
     ],
@@ -103,7 +103,7 @@ var forbiddenTerms = {
     message: privateServiceFactory,
     whitelist: [
       'extensions/amp-analytics/0.1/amp-analytics.js',
-      'src/service/storage-impl.js',
+      'extensions/amp-analytics/0.1/storage-impl.js',
     ],
   },
   'installUrlReplacementsService': {
@@ -160,7 +160,7 @@ var forbiddenTerms = {
     message: privateServiceFactory,
     whitelist: [
       'src/service/viewer-impl.js',
-      'src/service/storage-impl.js',
+      'extensions/amp-analytics/0.1/storage-impl.js',
       'examples/viewer-integr-messaging.js',
       'extensions/amp-access/0.1/login-dialog.js',
     ],
@@ -180,21 +180,21 @@ var forbiddenTerms = {
   'getBaseCid': {
     message: requiresReviewPrivacy,
     whitelist: [
-      'src/service/cid-impl.js',
+      'extensions/amp-analytics/0.1/cid-impl.js',
       'src/service/viewer-impl.js',
     ],
   },
   'cookie\\W': {
     message: requiresReviewPrivacy,
     whitelist: [
       'src/cookies.js',
-      'src/service/cid-impl.js',
+      'extensions/amp-analytics/0.1/cid-impl.js',
     ],
   },
   'getCookie\\W': {
     message: requiresReviewPrivacy,
     whitelist: [
-      'src/service/cid-impl.js',
+      'extensions/amp-analytics/0.1/cid-impl.js',
       'src/cookies.js',
       'src/experiments.js',
       'tools/experiments/experiments.js',
@@ -203,7 +203,7 @@ var forbiddenTerms = {
   'setCookie\\W': {
     message: requiresReviewPrivacy,
     whitelist: [
-      'src/service/cid-impl.js',
+      'extensions/amp-analytics/0.1/cid-impl.js',
       'src/cookies.js',
       'src/experiments.js',
       'tools/experiments/experiments.js',
@@ -244,8 +244,8 @@ var forbiddenTerms = {
   'localStorage': {
     message: requiresReviewPrivacy,
     whitelist: [
-      'src/service/cid-impl.js',
-      'src/service/storage-impl.js',
+      'extensions/amp-analytics/0.1/cid-impl.js',
+      'extensions/amp-analytics/0.1/storage-impl.js',
     ],
   },
   'sessionStorage': requiresReviewPrivacy,

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

@@ -18,7 +18,8 @@ import {AccessClientAdapter} from '../amp-access-client';
 import {AccessOtherAdapter} from '../amp-access-other';
 import {AccessService} from '../amp-access';
 import {Observable} from '../../../../src/observable';
-import {installCidService} from '../../../../src/service/cid-impl';
+import {installCidService,} from
+    '../../../../extensions/amp-analytics/0.1/cid-impl';
 import {markElementScheduledForTesting} from '../../../../src/custom-element';
 import * as sinon from 'sinon';
 

src/service/activity-impl.js → extensions/amp-analytics/0.1/activity-impl.js

@@ -19,10 +19,10 @@
  * has performed on the page.
  */
 
-import {getService} from '../service';
-import {viewerFor} from '../viewer';
-import {viewportFor} from '../viewport';
-import {listen} from '../event-helper';
+import {getService} from '../../../src/service';
+import {viewerFor} from '../../../src/viewer';
+import {viewportFor} from '../../../src/viewport';
+import {listen} from '../../../src/event-helper';
 
 
 /**

extensions/amp-analytics/0.1/amp-analytics.js

@@ -19,10 +19,10 @@ import {addListener, instrumentationServiceFor} from './instrumentation';
 import {assertHttpsUrl, addParamsToUrl} from '../../../src/url';
 import {dev, user} from '../../../src/log';
 import {expandTemplate} from '../../../src/string';
-import {installCidService} from '../../../src/service/cid-impl';
-import {installStorageService} from '../../../src/service/storage-impl';
-import {installActivityService} from '../../../src/service/activity-impl';
-import {installVisibilityService} from '../../../src/service/visibility-impl';
+import {installCidService} from './cid-impl';
+import {installStorageService} from './storage-impl';
+import {installActivityService} from './activity-impl';
+import {installVisibilityService} from './visibility-impl';
 import {isArray, isObject} from '../../../src/types';
 import {sendRequest, sendRequestUsingIframe} from './transport';
 import {urlReplacementsFor} from '../../../src/url-replacements';

src/service/cid-impl.js → extensions/amp-analytics/0.1/cid-impl.js

@@ -22,15 +22,19 @@
  * For details, see https://goo.gl/Mwaacs
  */
 
-import {getCookie, setCookie} from '../cookies';
-import {getService} from '../service';
-import {getSourceOrigin, isProxyOrigin, parseUrl} from '../url';
-import {timer} from '../timer';
-import {viewerFor} from '../viewer';
+import {getCookie, setCookie} from '../../../src/cookies';
+import {getService} from '../../../src/service';
+import {
+  getSourceOrigin,
+  isProxyOrigin,
+  parseUrl,
+} from '../../../src/url';
+import {timer} from '../../../src/timer';
+import {viewerFor} from '../../../src/viewer';
 import {
   sha384Base64,
-} from '../../third_party/closure-library/sha384-generated';
-import {user} from '../log';
+} from '../../../third_party/closure-library/sha384-generated';
+import {user} from '../../../src/log';
 
 
 const ONE_DAY_MILLIS = 24 * 3600 * 1000;

extensions/amp-analytics/0.1/instrumentation.js

@@ -15,7 +15,7 @@
  */
 
 import {isExperimentOn} from '../../../src/experiments';
-import {isVisibilitySpecValid} from '../../../src/service/visibility-impl';
+import {isVisibilitySpecValid} from './visibility-impl';
 import {Observable} from '../../../src/observable';
 import {getService} from '../../../src/service';
 import {timer} from '../../../src/timer';

src/service/storage-impl.js → extensions/amp-analytics/0.1/storage-impl.js

@@ -14,12 +14,12 @@
  * limitations under the License.
  */
 
-import {getService} from '../service';
-import {getSourceOrigin} from '../url';
-import {dev} from '../log';
-import {recreateNonProtoObject} from '../json';
-import {timer} from '../timer';
-import {viewerFor} from '../viewer';
+import {getService} from '../../../src/service';
+import {getSourceOrigin} from '../../../src/url';
+import {dev} from '../../../src/log';
+import {recreateNonProtoObject} from '../../../src/json';
+import {timer} from '../../../src/timer';
+import {viewerFor} from '../../../src/viewer';
 
 /** @const */
 const TAG = 'Storage';

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

@@ -23,7 +23,8 @@ import {adopt} from '../../../../src/runtime';
 import {createIframePromise} from '../../../../testing/iframe';
 import {getService, resetServiceForTesting} from '../../../../src/service';
 import {markElementScheduledForTesting} from '../../../../src/custom-element';
-import {installCidService} from '../../../../src/service/cid-impl';
+import {installCidService,} from
+    '../../../../extensions/amp-analytics/0.1/cid-impl';
 import {installViewerService} from '../../../../src/service/viewer-impl';
 import {installViewportService} from '../../../../src/service/viewport-impl';
 import {

src/service/visibility-impl.js → extensions/amp-analytics/0.1/visibility-impl.js

@@ -14,12 +14,12 @@
  * limitations under the License.
  */
 
-import {dev} from '../log';
-import {getService} from '../service';
-import {resourcesFor} from '../resources';
-import {timer} from '../timer';
-import {user} from '../log';
-import {viewportFor} from '../viewport';
+import {dev} from '../../../src/log';
+import {getService} from '../../../src/service';
+import {resourcesFor} from '../../../src/resources';
+import {timer} from '../../../src/timer';
+import {user} from '../../../src/log';
+import {viewportFor} from '../../../src/viewport';
 
 /** @const {number} */
 const LISTENER_INITIAL_RUN_DELAY_ = 20;

test/functional/test-activity.js

@@ -14,7 +14,8 @@
  * limitations under the License.
  */
 
-import {installActivityService} from '../../src/service/activity-impl';
+import {installActivityService,} from
+    '../../extensions/amp-analytics/0.1/activity-impl';
 import {activityFor} from '../../src/activity';
 import {installViewerService} from '../../src/service/viewer-impl';
 import {viewerFor} from '../../src/viewer';

test/functional/test-ad-cid.js

@@ -18,7 +18,7 @@ import {clientIdScope} from '../../ads/_config';
 import {createAdPromise} from '../../testing/ad-iframe';
 import {installAd} from '../../builtins/amp-ad';
 import {installEmbed} from '../../builtins/amp-embed';
-import {installCidService} from '../../src/service/cid-impl';
+import {installCidService} from '../../extensions/amp-analytics/0.1/cid-impl';
 import {
   installUserNotificationManager,
 } from '../../extensions/amp-user-notification/0.1/amp-user-notification';

test/functional/test-cid.js

@@ -18,7 +18,7 @@ import {cidFor} from '../../src/cid';
 import {
   installCidService,
   getProxySourceOrigin,
-} from '../../src/service/cid-impl';
+} from '../../extensions/amp-analytics/0.1/cid-impl';
 import {parseUrl} from '../../src/url';
 import {timer} from '../../src/timer';
 import {installViewerService} from '../../src/service/viewer-impl';

test/functional/test-storage.js

@@ -19,7 +19,7 @@ import {
   Store,
   LocalStorageBinding,
   ViewerStorageBinding,
-} from '../../src/service/storage-impl';
+} from '../../extensions/amp-analytics/0.1/storage-impl';
 import * as sinon from 'sinon';
 
 

test/functional/test-url-replacements.js

@@ -19,9 +19,10 @@ import {createIframePromise} from '../../testing/iframe';
 import {user} from '../../src/log';
 import {urlReplacementsFor} from '../../src/url-replacements';
 import {markElementScheduledForTesting} from '../../src/custom-element';
-import {installCidService} from '../../src/service/cid-impl';
+import {installCidService} from '../../extensions/amp-analytics/0.1/cid-impl';
 import {installViewerService} from '../../src/service/viewer-impl';
-import {installActivityService} from '../../src/service/activity-impl';
+import {installActivityService,} from
+    '../../extensions/amp-analytics/0.1/activity-impl';
 import {
   installUrlReplacementsService,
 } from '../../src/service/url-replacements-impl';

test/functional/test-visibility.js

@@ -14,8 +14,12 @@
  * limitations under the License.
  */
 
-import {isPositiveNumber_, isValidPercentage_, isVisibilitySpecValid,
-  installVisibilityService} from '../../src/service/visibility-impl';
+import {
+  isPositiveNumber_,
+  isValidPercentage_,
+  isVisibilitySpecValid,
+  installVisibilityService,
+} from '../../extensions/amp-analytics/0.1/visibility-impl';
 import {installResourcesService} from '../../src/service/resources-impl';
 import {visibilityFor} from '../../src/visibility';
 import * as sinon from 'sinon';