Skip to content

Commit

Permalink
Tighten types related to element attribute modification.
Browse files Browse the repository at this point in the history
Part of #9876
  • Loading branch information
cramforce committed Jun 22, 2017
1 parent faf9745 commit 925186d
Show file tree
Hide file tree
Showing 13 changed files with 80 additions and 68 deletions.
7 changes: 4 additions & 3 deletions builtins/amp-pixel.js
Expand Up @@ -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';
Expand Down Expand Up @@ -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;
Expand Down
57 changes: 30 additions & 27 deletions extensions/amp-a4a/0.1/amp-a4a.js
Expand Up @@ -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';
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -1343,22 +1345,23 @@ export class AmpA4A extends AMP.BaseElement {

/**
* Shared functionality for cross-domain iframe-based rendering methods.
* @param {!Object<string, string>} attributes The attributes of the iframe.
* @param {!JsonObject<string, string>} 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.
Expand Down Expand Up @@ -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)),
});
}));
}

/**
Expand Down Expand Up @@ -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}));
});
}

Expand Down
7 changes: 4 additions & 3 deletions extensions/amp-auto-ads/0.1/ad-network-config.js
Expand Up @@ -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';
Expand Down Expand Up @@ -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<string, string>}
* @return {!JsonObject<string, string>}
*/
getAttributes() {}

Expand Down Expand Up @@ -102,10 +103,10 @@ class AdSenseNetworkConfig {

/** @override */
getAttributes() {
return {
return dict({
'type': 'adsense',
'data-ad-client': this.autoAmpAdsElement_.getAttribute('data-ad-client'),
};
});
}

/** @override */
Expand Down
5 changes: 3 additions & 2 deletions extensions/amp-auto-ads/0.1/ad-strategy.js
Expand Up @@ -32,14 +32,15 @@ export class AdStrategy {

/**
* @param {!Array<!./placement.Placement>} placements
* @param {!Object<string, string>} baseAttributes Any attributes that should
* be added to any inserted ads. These will be combined with any
* @param {!JsonObject<string, string>} 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<string, string>} */
this.baseAttributes_ = baseAttributes;

/** @type {!./ad-tracker.AdTracker} */
Expand Down
5 changes: 3 additions & 2 deletions extensions/amp-auto-ads/0.1/amp-auto-ads.js
Expand Up @@ -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();
Expand Down
18 changes: 10 additions & 8 deletions extensions/amp-auto-ads/0.1/anchor-ad-strategy.js
Expand Up @@ -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 */
Expand All @@ -26,15 +27,15 @@ const OPT_IN_STATUS_ANCHOR_ADS = 2;
export class AnchorAdStrategy {
/**
* @param {!Window} win
* @param {!Object<string, string>} baseAttributes Any attributes that should
* be added to any inserted ads.
* @param {!JsonObject<string, string>} 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<string, string>} */
/** @const @private {!JsonObject<string, string>} */
this.baseAttributes_ = baseAttributes;

/** @const @private {!JSONType} */
Expand Down Expand Up @@ -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);
Expand Down
15 changes: 8 additions & 7 deletions extensions/amp-auto-ads/0.1/attributes.js
Expand Up @@ -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';

Expand All @@ -29,26 +30,26 @@ const NON_DATA_ATTRIBUTE_WHITELIST = {
};

/**
* @param {!Object} configObj
* @return {!Object<string, string>}
* @param {!JsonObject} configObj
* @return {!JsonObject<string, string>}
*/
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<string, string>}
* @param {!JsonObject} attributeObject
* @return {!JsonObject<string, string>}
*/
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);
Expand Down
15 changes: 8 additions & 7 deletions extensions/amp-auto-ads/0.1/placement.js
Expand Up @@ -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 {
Expand Down Expand Up @@ -88,7 +89,7 @@ export class Placement {
* @param {!Element} anchorElement
* @param {!Position} position
* @param {!function(!Element, !Element)} injector
* @param {!Object<string, string>} attributes
* @param {!JsonObject<string, string>} attributes
* @param {!../../../src/layout-rect.LayoutMarginsChangeDef=} opt_margins
*/
constructor(win, resources, anchorElement, position, injector, attributes,
Expand All @@ -108,7 +109,7 @@ export class Placement {
/** @const @private {!function(!Element, !Element)} */
this.injector_ = injector;

/** @const @private {!Object<string, string>} */
/** @const @private {!JsonObject<string, string>} */
this.attributes_ = attributes;

/**
Expand Down Expand Up @@ -164,7 +165,7 @@ export class Placement {
}

/**
* @param {!Object<string, string>} baseAttributes Any attributes to add to
* @param {!JsonObject<string, string>} baseAttributes Any attributes to add to
* injected <amp-ad>. Specific attributes will override defaults, but be
* overridden by placement specific attributes defined in the
* configuration.
Expand Down Expand Up @@ -194,16 +195,16 @@ export class Placement {
}

/**
* @param {!Object<string, string>} baseAttributes
* @param {!JsonObject<string, string>} 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);
}
Expand Down Expand Up @@ -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<!Placement>} placements
*/
function getPlacementsFromObject(win, placementObj, placements) {
Expand Down
2 changes: 1 addition & 1 deletion extensions/amp-call-tracking/0.1/amp-call-tracking.js
Expand Up @@ -32,7 +32,7 @@ let cachedResponsePromises_ = {};
* Fetches vendor response.
* @param {!Window} win
* @param {!string} url
* @return {!Promise<Object>}
* @return {!Promise<JsonObject>}
*/
function fetch_(win, url) {
if (!(url in cachedResponsePromises_)) {
Expand Down
9 changes: 5 additions & 4 deletions src/analytics.js
Expand Up @@ -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';


/**
Expand Down Expand Up @@ -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;
Expand Down
2 changes: 1 addition & 1 deletion src/base-element.js
Expand Up @@ -932,7 +932,7 @@ export class BaseElement {
* @note Boolean attributes have a value of `true` and `false` when
* present and missing, respectively.
* @param {
* !Object<string, (null|boolean|string|number|Array|Object)>
* !JsonObject<string, (null|boolean|string|number|Array|Object)>
* } unusedMutations
*/
mutatedAttributesCallback(unusedMutations) {
Expand Down

0 comments on commit 925186d

Please sign in to comment.