From 925186d78da3a17d4fd384f69c8fef43e4dd7280 Mon Sep 17 00:00:00 2001 From: Malte Ubl Date: Thu, 22 Jun 2017 06:49:24 -0700 Subject: [PATCH] Tighten types related to element attribute modification. Part of #9876 --- builtins/amp-pixel.js | 7 ++- extensions/amp-a4a/0.1/amp-a4a.js | 57 ++++++++++--------- .../amp-auto-ads/0.1/ad-network-config.js | 7 ++- extensions/amp-auto-ads/0.1/ad-strategy.js | 5 +- extensions/amp-auto-ads/0.1/amp-auto-ads.js | 5 +- .../amp-auto-ads/0.1/anchor-ad-strategy.js | 18 +++--- extensions/amp-auto-ads/0.1/attributes.js | 15 ++--- extensions/amp-auto-ads/0.1/placement.js | 15 ++--- .../0.1/amp-call-tracking.js | 2 +- src/analytics.js | 9 +-- src/base-element.js | 2 +- src/custom-element.js | 2 +- src/dom.js | 4 +- 13 files changed, 80 insertions(+), 68 deletions(-) diff --git a/builtins/amp-pixel.js b/builtins/amp-pixel.js index 2af5c65d6171..addebf6f4089 100644 --- a/builtins/amp-pixel.js +++ b/builtins/amp-pixel.js @@ -16,6 +16,7 @@ import {BaseElement} from '../src/base-element'; import {dev, user} from '../src/log'; +import {dict} from '../src/utils/object'; import {registerElement} from '../src/custom-element'; import {timerFor} from '../src/services'; import {urlReplacementsForDoc} from '../src/services'; @@ -119,9 +120,9 @@ function createNoReferrerPixel(parentElement, src) { // if "referrerPolicy" is not supported, use iframe wrapper // to scrub the referrer. const iframe = createElementWithAttributes( - /** @type {!Document} */ (parentElement.ownerDocument), 'iframe', { - src: 'about:blank', - }); + /** @type {!Document} */ (parentElement.ownerDocument), 'iframe', dict({ + 'src': 'about:blank', + })); parentElement.appendChild(iframe); createImagePixel(iframe.contentWindow, src); return iframe; diff --git a/extensions/amp-a4a/0.1/amp-a4a.js b/extensions/amp-a4a/0.1/amp-a4a.js index caefa8ef7716..375990de63d9 100644 --- a/extensions/amp-a4a/0.1/amp-a4a.js +++ b/extensions/amp-a4a/0.1/amp-a4a.js @@ -37,6 +37,7 @@ import { import {isLayoutSizeDefined} from '../../../src/layout'; import {isAdPositionAllowed} from '../../../src/ad-helper'; import {dev, user, duplicateErrorIfNecessary} from '../../../src/log'; +import {dict} from '../../../src/utils/object'; import {getMode} from '../../../src/mode'; import {isArray, isObject, isEnumValue} from '../../../src/types'; import {some} from '../../../src/utils/promise'; @@ -97,14 +98,14 @@ export const XORIGIN_MODE = { }; /** @type {!Object} @private */ -const SHARED_IFRAME_PROPERTIES = { - frameborder: '0', - allowfullscreen: '', - allowtransparency: '', - scrolling: 'no', - marginwidth: '0', - marginheight: '0', -}; +const SHARED_IFRAME_PROPERTIES = dict({ + 'frameborder': '0', + 'allowfullscreen': '', + 'allowtransparency': '', + 'scrolling': 'no', + 'marginwidth': '0', + 'marginheight': '0', +}); /** @typedef {{ * creative: ArrayBuffer, @@ -1269,16 +1270,17 @@ export class AmpA4A extends AMP.BaseElement { // Create and setup friendly iframe. this.iframe = /** @type {!HTMLIFrameElement} */( createElementWithAttributes( - /** @type {!Document} */(this.element.ownerDocument), 'iframe', { + /** @type {!Document} */(this.element.ownerDocument), 'iframe', + dict({ // NOTE: It is possible for either width or height to be 'auto', // a non-numeric value. - height: this.creativeSize_.height, - width: this.creativeSize_.width, - frameborder: '0', - allowfullscreen: '', - allowtransparency: '', - scrolling: 'no', - })); + 'height': this.creativeSize_.height, + 'width': this.creativeSize_.width, + 'frameborder': '0', + 'allowfullscreen': '', + 'allowtransparency': '', + 'scrolling': 'no', + }))); this.applyFillContent(this.iframe); const fontsArray = []; if (creativeMetaData.customStylesheets) { @@ -1343,22 +1345,23 @@ export class AmpA4A extends AMP.BaseElement { /** * Shared functionality for cross-domain iframe-based rendering methods. - * @param {!Object} attributes The attributes of the iframe. + * @param {!JsonObject} attributes The attributes of the iframe. * @return {!Promise} awaiting load event for ad frame * @private */ iframeRenderHelper_(attributes) { - const mergedAttributes = Object.assign(attributes, { - height: this.creativeSize_.height, - width: this.creativeSize_.width, - }); + const mergedAttributes = Object.assign(attributes, dict({ + 'height': this.creativeSize_.height, + 'width': this.creativeSize_.width, + })); if (this.sentinel) { mergedAttributes['data-amp-3p-sentinel'] = this.sentinel; } this.iframe = createElementWithAttributes( /** @type {!Document} */ (this.element.ownerDocument), - 'iframe', Object.assign(mergedAttributes, SHARED_IFRAME_PROPERTIES)); + 'iframe', /** @type {!JsonObject} */ ( + Object.assign(mergedAttributes, SHARED_IFRAME_PROPERTIES))); // TODO(keithwrightbos): noContentCallback? this.xOriginIframeHandler_ = new AMP.AmpAdXOriginIframeHandler(this); // Iframe is appended to element as part of xorigin frame handler init. @@ -1390,11 +1393,11 @@ export class AmpA4A extends AMP.BaseElement { */ renderViaCachedContentIframe_(adUrl) { this.protectedEmitLifecycleEvent_('renderCrossDomainStart'); - return this.iframeRenderHelper_({ - src: xhrFor(this.win).getCorsUrl(this.win, adUrl), - name: JSON.stringify( + return this.iframeRenderHelper_(dict({ + 'src': xhrFor(this.win).getCorsUrl(this.win, adUrl), + 'name': JSON.stringify( getContextMetadata(this.win, this.element, this.sentinel)), - }); + })); } /** @@ -1446,7 +1449,7 @@ export class AmpA4A extends AMP.BaseElement { name = `${this.safeframeVersion_};${creative.length};${creative}` + `${contextMetadata}`; } - return this.iframeRenderHelper_({src: srcPath, name}); + return this.iframeRenderHelper_(dict({'src': srcPath, name})); }); } diff --git a/extensions/amp-auto-ads/0.1/ad-network-config.js b/extensions/amp-auto-ads/0.1/ad-network-config.js index 60ab5a5d61dd..ede899d56e6b 100644 --- a/extensions/amp-auto-ads/0.1/ad-network-config.js +++ b/extensions/amp-auto-ads/0.1/ad-network-config.js @@ -19,6 +19,7 @@ import { getAdSenseAmpAutoAdsExpBranch, } from '../../../ads/google/adsense-amp-auto-ads'; import {buildUrl} from '../../../ads/google/a4a/url-builder'; +import {dict} from '../../../src/utils/object'; import {documentInfoForDoc} from '../../../src/services'; import {parseUrl} from '../../../src/url'; import {viewportForDoc} from '../../../src/services'; @@ -46,7 +47,7 @@ class AdNetworkConfigDef { /** * Any attributes derived from either the page or the auto-amp-ads tag that * should be applied to any ads inserted. - * @return {!Object} + * @return {!JsonObject} */ getAttributes() {} @@ -102,10 +103,10 @@ class AdSenseNetworkConfig { /** @override */ getAttributes() { - return { + return dict({ 'type': 'adsense', 'data-ad-client': this.autoAmpAdsElement_.getAttribute('data-ad-client'), - }; + }); } /** @override */ diff --git a/extensions/amp-auto-ads/0.1/ad-strategy.js b/extensions/amp-auto-ads/0.1/ad-strategy.js index 7084e8739862..e4575fc94efb 100644 --- a/extensions/amp-auto-ads/0.1/ad-strategy.js +++ b/extensions/amp-auto-ads/0.1/ad-strategy.js @@ -32,14 +32,15 @@ export class AdStrategy { /** * @param {!Array} placements - * @param {!Object} baseAttributes Any attributes that should - * be added to any inserted ads. These will be combined with any + * @param {!JsonObject} baseAttributes Any attributes that + * should be added to any inserted ads. These will be combined with any * additional data atrributes specified by the placement. * @param {!./ad-tracker.AdTracker} adTracker */ constructor(placements, baseAttributes, adTracker) { this.availablePlacements_ = placements.slice(0); + /** @private {!JsonObject} */ this.baseAttributes_ = baseAttributes; /** @type {!./ad-tracker.AdTracker} */ diff --git a/extensions/amp-auto-ads/0.1/amp-auto-ads.js b/extensions/amp-auto-ads/0.1/amp-auto-ads.js index 8d54adf3049d..c6f66c54e6a6 100644 --- a/extensions/amp-auto-ads/0.1/amp-auto-ads.js +++ b/extensions/amp-auto-ads/0.1/amp-auto-ads.js @@ -53,8 +53,9 @@ export class AmpAutoAds extends AMP.BaseElement { } const placements = getPlacementsFromConfigObj(this.win, configObj); - const attributes = Object.assign(adNetwork.getAttributes(), - getAttributesFromConfigObj(configObj)); + const attributes = /** @type {!JsonObject} */ ( + Object.assign(adNetwork.getAttributes(), + getAttributesFromConfigObj(configObj))); const adTracker = new AdTracker(getExistingAds(this.win), adNetwork.getAdConstraints()); new AdStrategy(placements, attributes, adTracker).run(); diff --git a/extensions/amp-auto-ads/0.1/anchor-ad-strategy.js b/extensions/amp-auto-ads/0.1/anchor-ad-strategy.js index 56d1b1845a7f..6af262b0bdda 100644 --- a/extensions/amp-auto-ads/0.1/anchor-ad-strategy.js +++ b/extensions/amp-auto-ads/0.1/anchor-ad-strategy.js @@ -15,6 +15,7 @@ */ import {createElementWithAttributes} from '../../../src/dom'; import {user} from '../../../src/log'; +import {dict} from '../../../src/utils/object'; import {viewportForDoc} from '../../../src/services'; /** @const */ @@ -26,15 +27,15 @@ const OPT_IN_STATUS_ANCHOR_ADS = 2; export class AnchorAdStrategy { /** * @param {!Window} win - * @param {!Object} baseAttributes Any attributes that should - * be added to any inserted ads. + * @param {!JsonObject} baseAttributes Any attributes that + * should be added to any inserted ads. * @param {!JSONType} configObj */ constructor(win, baseAttributes, configObj) { /** @const @private {!Window} */ this.win_ = win; - /** @const @private {!Object} */ + /** @const @private {!JsonObject} */ this.baseAttributes_ = baseAttributes; /** @const @private {!JSONType} */ @@ -83,14 +84,15 @@ export class AnchorAdStrategy { placeStickyAd_() { const viewportWidth = viewportForDoc(this.win_.document).getWidth(); - const attributes = Object.assign({}, this.baseAttributes_, { - 'width': String(viewportWidth), - 'height': '100', - }); + const attributes = /** @type {!JsonObject} */ ( + Object.assign(dict(), this.baseAttributes_, dict({ + 'width': String(viewportWidth), + 'height': '100', + }))); const ampAd = createElementWithAttributes( this.win_.document, 'amp-ad', attributes); const stickyAd = createElementWithAttributes( - this.win_.document, 'amp-sticky-ad', {'layout': 'nodisplay'}); + this.win_.document, 'amp-sticky-ad', dict({'layout': 'nodisplay'})); stickyAd.appendChild(ampAd); this.win_.document.body.insertBefore( stickyAd, this.win_.document.body.firstChild); diff --git a/extensions/amp-auto-ads/0.1/attributes.js b/extensions/amp-auto-ads/0.1/attributes.js index 2158dc0051c9..2997cd109aa2 100644 --- a/extensions/amp-auto-ads/0.1/attributes.js +++ b/extensions/amp-auto-ads/0.1/attributes.js @@ -15,6 +15,7 @@ */ import {user} from '../../../src/log'; +import {dict} from '../../../src/utils/object'; import {startsWith} from '../../../src/string'; import {isArray, isObject} from '../../../src/types'; @@ -29,26 +30,26 @@ const NON_DATA_ATTRIBUTE_WHITELIST = { }; /** - * @param {!Object} configObj - * @return {!Object} + * @param {!JsonObject} configObj + * @return {!JsonObject} */ export function getAttributesFromConfigObj(configObj) { if (!configObj['attributes']) { - return {}; + return dict(); } if (!isObject(configObj['attributes']) || isArray(configObj['attributes'])) { user().warn(TAG, 'attributes property not an object'); - return {}; + return dict(); } return parseAttributes(configObj['attributes']); } /** - * @param {!Object} attributeObject - * @return {!Object} + * @param {!JsonObject} attributeObject + * @return {!JsonObject} */ function parseAttributes(attributeObject) { - const attributes = {}; + const attributes = dict(); for (const key in attributeObject) { if (!NON_DATA_ATTRIBUTE_WHITELIST[key] && !startsWith(key, 'data-')) { user().warn(TAG, 'Attribute not whitlisted: ' + key); diff --git a/extensions/amp-auto-ads/0.1/placement.js b/extensions/amp-auto-ads/0.1/placement.js index a0b61553b5ba..5068e9674e58 100644 --- a/extensions/amp-auto-ads/0.1/placement.js +++ b/extensions/amp-auto-ads/0.1/placement.js @@ -15,6 +15,7 @@ */ import {dev, user} from '../../../src/log'; +import {dict} from '../../../src/utils/object'; import {getAttributesFromConfigObj} from './attributes'; import {resourcesForDoc} from '../../../src/services'; import { @@ -88,7 +89,7 @@ export class Placement { * @param {!Element} anchorElement * @param {!Position} position * @param {!function(!Element, !Element)} injector - * @param {!Object} attributes + * @param {!JsonObject} attributes * @param {!../../../src/layout-rect.LayoutMarginsChangeDef=} opt_margins */ constructor(win, resources, anchorElement, position, injector, attributes, @@ -108,7 +109,7 @@ export class Placement { /** @const @private {!function(!Element, !Element)} */ this.injector_ = injector; - /** @const @private {!Object} */ + /** @const @private {!JsonObject} */ this.attributes_ = attributes; /** @@ -164,7 +165,7 @@ export class Placement { } /** - * @param {!Object} baseAttributes Any attributes to add to + * @param {!JsonObject} baseAttributes Any attributes to add to * injected . Specific attributes will override defaults, but be * overridden by placement specific attributes defined in the * configuration. @@ -194,16 +195,16 @@ export class Placement { } /** - * @param {!Object} baseAttributes + * @param {!JsonObject} baseAttributes * @return {!Element} * @private */ createAdElement_(baseAttributes) { - const attributes = Object.assign({ + const attributes = /** @type {!JsonObject} */ (Object.assign(dict({ 'layout': 'fixed-height', 'height': '0', 'class': 'i-amphtml-layout-awaiting-size', - }, baseAttributes, this.attributes_); + }), baseAttributes, this.attributes_)); return createElementWithAttributes( this.win_.document, 'amp-ad', attributes); } @@ -231,7 +232,7 @@ export function getPlacementsFromConfigObj(win, configObj) { * Validates that the placementObj represents a valid placement and if so * constructs and returns an instance of the Placement class for it. * @param {!Window} win - * @param {!Object} placementObj + * @param {!JsonObject} placementObj * @param {!Array} placements */ function getPlacementsFromObject(win, placementObj, placements) { diff --git a/extensions/amp-call-tracking/0.1/amp-call-tracking.js b/extensions/amp-call-tracking/0.1/amp-call-tracking.js index 59de59e3c114..930dfc7110ec 100644 --- a/extensions/amp-call-tracking/0.1/amp-call-tracking.js +++ b/extensions/amp-call-tracking/0.1/amp-call-tracking.js @@ -32,7 +32,7 @@ let cachedResponsePromises_ = {}; * Fetches vendor response. * @param {!Window} win * @param {!string} url - * @return {!Promise} + * @return {!Promise} */ function fetch_(win, url) { if (!(url in cachedResponsePromises_)) { diff --git a/src/analytics.js b/src/analytics.js index b5233a027d41..af46b1a7d163 100644 --- a/src/analytics.js +++ b/src/analytics.js @@ -22,6 +22,7 @@ import {createElementWithAttributes} from './dom'; import {getAmpdoc} from './service'; import {extensionsFor} from './services'; import {dev} from './log'; +import {dict} from './utils/object'; /** @@ -79,15 +80,15 @@ export function insertAnalyticsElement( const doc = /** @type {!Document} */ (parentElement.ownerDocument); const analyticsElem = createElementWithAttributes( doc, - 'amp-analytics', { + 'amp-analytics', dict({ 'sandbox': 'true', 'trigger': 'immediate', - }); + })); const scriptElem = createElementWithAttributes( doc, - 'script', { + 'script', dict({ 'type': 'application/json', - }); + })); scriptElem.textContent = JSON.stringify(config); analyticsElem.appendChild(scriptElem); analyticsElem.CONFIG = config; diff --git a/src/base-element.js b/src/base-element.js index a26d656796eb..c1f2ed9e4120 100644 --- a/src/base-element.js +++ b/src/base-element.js @@ -932,7 +932,7 @@ export class BaseElement { * @note Boolean attributes have a value of `true` and `false` when * present and missing, respectively. * @param { - * !Object + * !JsonObject * } unusedMutations */ mutatedAttributesCallback(unusedMutations) { diff --git a/src/custom-element.js b/src/custom-element.js index 359ad2a7d30a..062d4de5ef38 100644 --- a/src/custom-element.js +++ b/src/custom-element.js @@ -1463,7 +1463,7 @@ function createBaseCustomElementClass(win) { * @note Boolean attributes have a value of `true` and `false` when * present and missing, respectively. * @param { - * !Object + * !JsonObject * } mutations */ mutatedAttributesCallback(mutations) { diff --git a/src/dom.js b/src/dom.js index 1457c3dd569f..00228ce53914 100644 --- a/src/dom.js +++ b/src/dom.js @@ -149,7 +149,7 @@ export function copyChildren(from, to) { /** * Add attributes to an element. * @param {!Element} element - * @param {!Object} attributes + * @param {!JsonObject} attributes * @return {!Element} created element */ export function addAttributesToElement(element, attributes) { @@ -163,7 +163,7 @@ export function addAttributesToElement(element, attributes) { * Create a new element on document with specified tagName and attributes. * @param {!Document} doc * @param {string} tagName - * @param {!Object} attributes + * @param {!JsonObject} attributes * @return {!Element} created element */ export function createElementWithAttributes(doc, tagName, attributes) {