Fix lots of type errors and introduces assertElement. (#4885)

* Fix lots of type errors and introduces assertElement.

- Gets rid of many specific `Element` types. In practice these aren't helpful, but introduce a lot of casting.
- Might have fixed on actualy bug in url-replacements.js

142 error(s), 6 warning(s), 93.81603250831178% typed

build-system/amp.extern.js

@@ -131,3 +131,8 @@ twttr.widgets.createTweet;
 
 var FB;
 FB.init;
+
+// Validator
+var amp;
+amp.validator;
+amp.validator.validateUrlAndLog = function(string, doc, filter) {}

build-system/runner/src/org/ampproject/AmpCodingConvention.java

@@ -17,6 +17,7 @@
 package org.ampproject;
 
 import com.google.common.collect.ImmutableList;
+import com.google.javascript.jscomp.ClosureCodingConvention.AssertFunctionByTypeName;
 import com.google.javascript.jscomp.CodingConvention;
 import com.google.javascript.jscomp.CodingConvention.AssertionFunctionSpec;
 import com.google.javascript.jscomp.CodingConventions;
@@ -45,9 +46,8 @@ public AmpCodingConvention(CodingConvention convention) {
     return ImmutableList.of(
         new AssertionFunctionSpec("user.assert", JSType.TRUTHY),
         new AssertionFunctionSpec("dev.assert", JSType.TRUTHY),
-        new AssertionFunctionSpec("module$src$log.user.assert", JSType.TRUTHY),
-        new AssertionFunctionSpec("module$src$log.dev.assert", JSType.TRUTHY),
-        new AssertionFunctionSpec("Log$$module$src$log.prototype.assert", JSType.TRUTHY)
+        new AssertionFunctionSpec("Log$$module$src$log.prototype.assert", JSType.TRUTHY),
+        new AssertFunctionByTypeName("Log$$module$src$log.prototype.assertElement", "Element")
     );
   }
 

src/event-helper.js

@@ -101,9 +101,10 @@ export function isLoaded(element) {
  * Returns a promise that will resolve or fail based on the element's 'load'
  * and 'error' events. Optionally this method takes a timeout, which will reject
  * the promise if the resource has not loaded by then.
- * @param {!Element} element
+ * @param {T} element
  * @param {number=} opt_timeout
- * @return {!Promise<!Element>}
+ * @return {!Promise<T>}
+ * @template T
  */
 export function loadPromise(element, opt_timeout) {
   let unlistenLoad;

src/json.js

@@ -105,7 +105,7 @@ export function getValueForExpr(obj, expr) {
  * Parses the given `json` string without throwing an exception if not valid.
  * Returns `undefined` if parsing fails.
  * Returns the `Object` corresponding to the JSON string when parsing succeeds.
- * @param {string} json JSON string to parse
+ * @param {*} json JSON string to parse
  * @param {function(!Error)=} opt_onFailed Optional function that will be called with
  *     the error if parsing fails.
  * @return {?JSONValueDef|undefined}

src/log.js

@@ -249,7 +249,24 @@ export class Log {
     }
     return shouldBeTrueish;
   }
+
+  /**
+   * Throws an error if the first argument isn't an Element
+   *
+   * Otherwise see `assert` for usage
+   *
+   * @param {*} shouldBeElement
+   * @param {string=} opt_message The assertion message
+   * @return {!Element} The value of shouldBeTrueish.
+   * @template T
+   */
   /*eslint "google-camelcase/google-camelcase": 2*/
+  assertElement(shouldBeElement, opt_message) {
+    const shouldBeTrueish = shouldBeElement && shouldBeElement.nodeType == 1;
+    this.assert(shouldBeTrueish, (opt_message || 'Element expected') + ': %s',
+        shouldBeElement);
+    return /** @type {!Element} */ (shouldBeElement);
+  }
 
   /**
    * Asserts and returns the enum value. If the enum doesn't contain such a value,
@@ -261,6 +278,7 @@ export class Log {
    * @return T
    * @template T
    */
+  /*eslint "google-camelcase/google-camelcase": 2*/
   assertEnumValue(enumObj, s, opt_enumName) {
     for (const k in enumObj) {
       if (enumObj[k] == s) {

src/performance.js

@@ -18,8 +18,9 @@ import {getExistingServiceForWindow} from './service';
 
 /**
  * @param {!Window} window
- * @return {!Performance}
+ * @return {!./service/performance-impl.Performance}
  */
 export function performanceFor(window) {
-  return getExistingServiceForWindow(window, 'performance');
+  return /** @type {!./service/performance-impl.Performance}*/ (
+      getExistingServiceForWindow(window, 'performance'));
 };

src/service/url-replacements-impl.js

@@ -50,7 +50,7 @@ export class UrlReplacements {
     /** @private {!RegExp|undefined} */
     this.replacementExpr_ = undefined;
 
-    /** @private @const {!Object<string, function(*):*>} */
+    /** @private @const {!Object<string, function(*, *):*>} */
     this.replacements_ = this.win_.Object.create(null);
 
     /** @private @const {function():!Promise<?AccessService>} */
@@ -426,7 +426,7 @@ export class UrlReplacements {
    * @template T
    */
   getAccessValue_(getter, expr) {
-    return this.getAccessService_(this.win_).then(accessService => {
+    return this.getAccessService_().then(accessService => {
       if (!accessService) {
         // Access service is not installed.
         user().error(TAG, 'Access service is not installed to access: ', expr);
@@ -441,8 +441,8 @@ export class UrlReplacements {
    * The data for the timing events is retrieved from performance.timing API.
    * If start and end events are both given, the result is the difference between the two.
    * If only start event is given, the result is the timing value at start event.
-   * @param {string} startEvent
-   * @param {string=} endEvent
+   * @param {*} startEvent
+   * @param {*=} endEvent
    * @return {!Promise<string|undefined>}
    * @private
    */
@@ -472,14 +472,15 @@ export class UrlReplacements {
             : String(metric);
       });
     } else {
-      return Promise.resolve(String(metric));
+      return /** @type {!Promise<(string|undefined)>} */ (
+          Promise.resolve(String(metric)));
     }
   }
 
   /**
    * Returns navigation information from the current browsing context.
    * @param {string} attribute
-   * @return {!Promise<string|undefined>}
+   * @return {!Promise<undefined>|string}
    * @private
    */
   getNavigationData_(attribute) {
@@ -497,7 +498,7 @@ export class UrlReplacements {
    * Sets the value resolver for the variable with the specified name. The
    * value resolver may optionally take an extra parameter.
    * @param {string} varName
-   * @param {function(*):*} resolver
+   * @param {function(*, *):*} resolver
    * @return {!UrlReplacements}
    * @private
    */
@@ -604,7 +605,7 @@ export class UrlReplacements {
   /**
    * Method exists to assist stubbing in tests.
    * @param {string} name
-   * @return {function(*):*}
+   * @return {function(*, *):*}
    */
   getReplacement_(name) {
     return this.replacements_[name];
@@ -620,7 +621,7 @@ export class UrlReplacements {
     if (additionalKeys && additionalKeys.length > 0) {
       const allKeys = Object.keys(this.replacements_);
       additionalKeys.forEach(key => {
-        if (allKeys[key] === undefined) {
+        if (this.replacements_[key] === undefined) {
           allKeys.push(key);
         }
       });

src/service/viewer-impl.js

@@ -116,7 +116,7 @@ export class Viewer {
     /** @private {boolean} */
     this.overtakeHistory_ = false;
 
-    /** @private {string} */
+    /** @private {!VisibilityState} */
     this.visibilityState_ = VisibilityState.VISIBLE;
 
     /** @private {string} */
@@ -164,6 +164,15 @@ export class Viewer {
     /** @private {?time} */
     this.firstVisibleTime_ = null;
 
+    /** @private {?Function} */
+    this.messagingReadyResolver_ = null;
+
+    /** @private {?Function} */
+    this.viewerOriginResolver_ = null;
+
+    /** @private {?Function} */
+    this.trustedViewerResolver_ = null;
+
     /**
      * This promise might be resolved right away if the current
      * document is already visible. See end of this constructor where we call
@@ -188,8 +197,8 @@ export class Viewer {
     this.isRuntimeOn_ = !parseInt(this.params_['off'], 10);
     dev().fine(TAG_, '- runtimeOn:', this.isRuntimeOn_);
 
-    this.overtakeHistory_ = !!(parseInt(this.params_['history'], 10)) ||
-        this.overtakeHistory_;
+    this.overtakeHistory_ = !!(parseInt(this.params_['history'], 10) ||
+        this.overtakeHistory_);
     dev().fine(TAG_, '- history:', this.overtakeHistory_);
 
     this.setVisibilityState_(this.params_['visibilityState']);
@@ -237,9 +246,6 @@ export class Viewer {
     // Wait for document to become visible.
     this.docState_.onVisibilityChanged(this.recheckVisibilityState_.bind(this));
 
-    /** @private {function()|undefined} */
-    this.messagingReadyResolver_ = undefined;
-
     /**
      * This promise will resolve when communications channel has been
      * established or timeout in 20 seconds. The timeout is needed to avoid
@@ -287,9 +293,6 @@ export class Viewer {
           this.isTrustedViewerOrigin_(this.win.location.ancestorOrigins[0]));
       trustedViewerPromise = Promise.resolve(trustedViewerResolved);
     } else {
-
-      /** @private {!function(boolean)|undefined} */
-      this.trustedViewerResolver_ = undefined;
       // Wait for comms channel to confirm the origin.
       trustedViewerResolved = undefined;
       trustedViewerPromise = new Promise(resolve => {
@@ -300,9 +303,6 @@ export class Viewer {
     /** @const @private {!Promise<boolean>} */
     this.isTrustedViewer_ = trustedViewerPromise;
 
-    /** @private {!function(string)|undefined} */
-    this.viewerOriginResolver_ = undefined;
-
     /** @const @private {!Promise<string>} */
     this.viewerOrigin_ = new Promise(resolve => {
       if (!this.isEmbedded()) {
@@ -775,7 +775,8 @@ export class Viewer {
    * @return {!Promise}
    */
   requestFullOverlay() {
-    return this.sendMessageUnreliable_('requestFullOverlay', {}, true);
+    return /** @type {!Promise} */ (
+        this.sendMessageUnreliable_('requestFullOverlay', {}, true));
   }
 
   /**
@@ -784,7 +785,8 @@ export class Viewer {
    * @return {!Promise}
    */
   cancelFullOverlay() {
-    return this.sendMessageUnreliable_('cancelFullOverlay', {}, true);
+    return /** @type {!Promise} */ (
+        this.sendMessageUnreliable_('cancelFullOverlay', {}, true));
   }
 
   /**
@@ -793,8 +795,8 @@ export class Viewer {
    * @return {!Promise}
    */
   postPushHistory(stackIndex) {
-    return this.sendMessageUnreliable_(
-        'pushHistory', {stackIndex}, true);
+    return /** @type {!Promise} */ (this.sendMessageUnreliable_(
+        'pushHistory', {stackIndex}, true));
   }
 
   /**
@@ -803,8 +805,8 @@ export class Viewer {
    * @return {!Promise}
    */
   postPopHistory(stackIndex) {
-    return this.sendMessageUnreliable_(
-        'popHistory', {stackIndex}, true);
+    return /** @type {!Promise} */ (this.sendMessageUnreliable_(
+        'popHistory', {stackIndex}, true));
   }
 
   /**
@@ -833,7 +835,7 @@ export class Viewer {
       // it should resolve in milli seconds.
       return timerFor(this.win).timeoutPromise(10000, cidPromise, 'base cid')
           .catch(error => {
-            dev().error(error);
+            dev().error(TAG_, error);
             return undefined;
           });
     });
@@ -925,7 +927,8 @@ export class Viewer {
       return Promise.resolve();
     }
     if (eventType == 'broadcast') {
-      this.broadcastObservable_.fire(data);
+      this.broadcastObservable_.fire(
+          /** @type {!JSONType|undefined} */ (data));
       return Promise.resolve();
     }
     dev().fine(TAG_, 'unknown message:', eventType);
@@ -1018,7 +1021,7 @@ export class Viewer {
    * @param {string} eventType
    * @param {*} data
    * @param {boolean} awaitResponse
-   * @return {!Promise<*>|undefined}
+   * @return {?Promise<*>|undefined}
    * @private
    */
   sendMessageUnreliable_(eventType, data, awaitResponse) {
@@ -1094,7 +1097,7 @@ function parseParams_(str, allParams) {
 
 /**
  * Creates an error for the case where a channel cannot be established.
- * @param {!Error|string=} opt_reason
+ * @param {*=} opt_reason
  * @return {!Error}
  */
 function getChannelError(opt_reason) {

src/service/viewport-impl.js

@@ -119,7 +119,7 @@ export class Viewport {
     /** @private @const {!Observable} */
     this.scrollObservable_ = new Observable();
 
-    /** @private {?HTMLMetaElement|undefined} */
+    /** @private {?Element|undefined} */
     this.viewportMeta_ = undefined;
 
     /** @private {string|undefined} */
@@ -483,7 +483,7 @@ export class Viewport {
   }
 
   /**
-   * @return {?HTMLMetaElement}
+   * @return {?Element}
    * @private
    */
   getViewportMeta_() {
@@ -646,9 +646,7 @@ export class ViewportBindingDef {
    * independent fixed layer.
    * @return {boolean}
    */
-  requiresFixedLayerTransfer() {
-    return false;
-  }
+  requiresFixedLayerTransfer() {}
 
   /**
    * Register a callback for scroll events.
@@ -934,6 +932,9 @@ export class ViewportBindingNaturalIosEmbed_ {
     /** @private {?Element} */
     this.scrollMoveEl_ = null;
 
+    /** @private {?Element} */
+    this.endPosEl_ = null;
+
     /** @private {!{x: number, y: number}} */
     this.pos_ = {x: 0, y: 0};
 
@@ -962,7 +963,8 @@ export class ViewportBindingNaturalIosEmbed_ {
   /** @private */
   setup_() {
     const documentElement = this.win.document.documentElement;
-    const documentBody = this.win.document.body;
+    const documentBody = /** @type {!Element} */ (
+        this.win.document.body);
 
     // Embedded scrolling on iOS is rather complicated. IFrames cannot be sized
     // and be scrollable. Sizing iframe by scrolling height has a big negative
@@ -1221,7 +1223,7 @@ export class ViewportBindingNaturalIosEmbed_ {
  * width=device-width,initial-scale=1,minimum-scale=1
  * ```
  * @param {string} content
- * @return {!Object<string, string>}
+ * @return {!Object<string, (string|undefined)>}
  * @private Visible for testing only.
  */
 export function parseViewportMeta(content) {
@@ -1324,7 +1326,7 @@ function createViewport_(window) {
  * @return {!Viewport}
  */
 export function installViewportService(window) {
-  return getService(window, 'viewport', () => {
+  return /** @type !Viewport} */ (getService(window, 'viewport', () => {
     return createViewport_(window);
-  });
+  }));
 };