Skip to content

Commit

Permalink
DocInfo: replace metaTags with viewport in API (#26687)
Browse files Browse the repository at this point in the history
* DocInfo: replace metaTags with viewport in API

- Add getMeta to AmpDoc
- Replace metaTags with viewport in DocInfo API and propagate change
- Update 'documentLoaded' message to viewer to include viewport
  property, but still provide metaTags.viewport for backwards
  compatibility
- Update amp-doc-viewer documentation
- Update unit tests with phony document to include head

* Revions from review

- Cache name/content pairs
- Minor optimizations
  • Loading branch information
mdmower authored Feb 27, 2020
1 parent 97bcfa1 commit 88e75b8
Show file tree
Hide file tree
Showing 16 changed files with 166 additions and 129 deletions.
2 changes: 1 addition & 1 deletion examples/amp-consent/amp-consent-client.html
Original file line number Diff line number Diff line change
Expand Up @@ -231,7 +231,7 @@ <h1 itemprop="headline">Lorem Ipsum</h1>
Maecenas sollicitudin felis aliquam tortor vulputate,
ac posuere velit semper.
</p>
<h1>_ping_ Ad Blocked by metaTags</h1>
<h1>_ping_ Ad Blocked by meta tags</h1>
<amp-ad width=300 height=250
type="_ping_"
data-url='/examples/img/bigbuckbunny.jpg'
Expand Down
2 changes: 1 addition & 1 deletion examples/amp-consent/amp-consent-geo.html
Original file line number Diff line number Diff line change
Expand Up @@ -280,7 +280,7 @@ <h1 itemprop="headline">Lorem Ipsum</h1>
Maecenas sollicitudin felis aliquam tortor vulputate,
ac posuere velit semper.
</p>
<h1>_ping_ Ad Blocked by metaTags</h1>
<h1>_ping_ Ad Blocked by meta tags</h1>
<amp-ad width=300 height=250
type="_ping_"
data-url='https://lh3.googleusercontent.com/pSECrJ82R7-AqeBCOEPGPM9iG9OEIQ_QXcbubWIOdkY=w400-h300-no-n'
Expand Down
2 changes: 1 addition & 1 deletion examples/amp-consent/amp-consent-server.html
Original file line number Diff line number Diff line change
Expand Up @@ -218,7 +218,7 @@ <h1 itemprop="headline">Lorem Ipsum</h1>
Maecenas sollicitudin felis aliquam tortor vulputate,
ac posuere velit semper.
</p>
<h1>_ping_ Ad Blocked by metaTags</h1>
<h1>_ping_ Ad Blocked by meta tags</h1>
<amp-ad width=300 height=250
type="_ping_"
data-url='/examples/img/bigbuckbunny.jpg'
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -184,9 +184,7 @@ export class LinkRewriterManager {
* @private
*/
getPriorityList_(ampdoc) {
const docInfo = Services.documentInfoForDoc(ampdoc);
const value = docInfo.metaTags[PRIORITY_META_TAG_NAME];

const value = ampdoc.getMetaByName(PRIORITY_META_TAG_NAME);
return value ? value.trim().split(/\s+/) : [];
}

Expand Down
8 changes: 4 additions & 4 deletions extensions/amp-skimlinks/0.1/test/test-link-rewriter.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ import {
PRIORITY_META_TAG_NAME,
EVENTS as linkRewriterEvents,
} from '../link-rewriter/constants';
import {Services} from '../../../../src/services';
import {TwoStepsResponse} from '../link-rewriter/two-steps-response';
import {createCustomEvent} from '../../../../src/event-helper';

Expand Down Expand Up @@ -67,9 +66,10 @@ describes.fakeWin('LinkRewriterManager', {amp: true}, env => {
};

addPriorityMetaTagHelper = priorityRule => {
env.sandbox.stub(Services, 'documentInfoForDoc').returns({
metaTags: {[PRIORITY_META_TAG_NAME]: priorityRule},
});
env.sandbox
.stub(env.ampdoc, 'getMetaByName')
.withArgs(PRIORITY_META_TAG_NAME)
.returns(priorityRule);

linkRewriterManager = new LinkRewriterManager(env.ampdoc);
};
Expand Down
63 changes: 31 additions & 32 deletions extensions/amp-viewer-integration/amp-doc-viewer-api.md

Large diffs are not rendered by default.

29 changes: 5 additions & 24 deletions src/consent.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ import {
CONSENT_POLICY_STATE, // eslint-disable-line no-unused-vars
} from './consent-state';
import {Services} from './services';
import {user} from './log';

/**
* Returns a promise that resolve when all consent state the policy wait
Expand Down Expand Up @@ -80,38 +79,20 @@ export function getConsentPolicyInfo(element, policyId) {
}

/**
* Determine if an element needs to be blocked by consent based on metaTags.
* Determine if an element needs to be blocked by consent based on meta tags.
* @param {*} element
* @return {boolean}
*/
export function shouldBlockOnConsentByMeta(element) {
const ampdoc = element.getAmpDoc();
let content = Services.documentInfoForDoc(ampdoc).metaTags[
'amp-consent-blocking'
];

let content = ampdoc.getMetaByName('amp-consent-blocking');
if (!content) {
return false;
}

// validator enforce uniqueness of <meta name='amp-consent-blocking'>
// content will not be an array.
if (typeof content !== 'string') {
user().error(
'CONSENT',
'Invalid amp-consent-blocking value, ignore meta tag'
);
return false;
}

// Handles whitespace
content = content
.toUpperCase()
.replace(/\s/g, '')
.split(',');
content = content.toUpperCase().replace(/\s+/g, '');

if (content.includes(element.tagName)) {
return true;
}
return false;
const contents = /** @type {Array<string>} */ (content.split(','));
return contents.includes(element.tagName);
}
55 changes: 39 additions & 16 deletions src/service/ampdoc-impl.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,14 +24,13 @@ import {
removeDocumentVisibilityChangeListener,
} from '../utils/document-visibility';
import {dev, devAssert} from '../log';
import {escapeCssSelectorIdent} from '../css';
import {getParentWindowFrameElement, registerServiceBuilder} from '../service';
import {getShadowRootNode} from '../shadow-embed';
import {isDocumentReady, whenDocumentReady} from '../document-ready';
import {isInAmpdocFieExperiment} from '../ampdoc-fie';
import {iterateCursor, rootNodeFor, waitForBodyOpenPromise} from '../dom';
import {map} from '../utils/object';
import {parseQueryString} from '../url';
import {rootNodeFor, waitForBodyOpenPromise} from '../dom';

/** @const {string} */
const AMPDOC_PROP = '__AMPDOC';
Expand Down Expand Up @@ -318,8 +317,8 @@ export class AmpDoc {
/** @private {!Object<string, string>} */
this.params_ = (opt_options && opt_options.params) || map();

/** @protected {!Object<string, string>} */
this.meta_ = (opt_options && opt_options.meta) || map();
/** @protected {?Object<string, string>} */
this.meta_ = null;

/** @private @const {!Array<string>} */
this.declaredExtensions_ = [];
Expand Down Expand Up @@ -418,6 +417,35 @@ export class AmpDoc {
return v == null ? null : v;
}

/**
* Initializes (if necessary) cached map of an ampdoc's meta name values to
* their associated content values and returns the map.
* @return {!Object<string, string>}
*/
getMeta() {
if (this.meta_) {
return map(this.meta_);
}

this.meta_ = map();
const metaEls = dev()
.assertElement(this.win.document.head)
.querySelectorAll('meta[name]');
iterateCursor(metaEls, metaEl => {
const name = metaEl.getAttribute('name');
const content = metaEl.getAttribute('content');
if (!name || content === null) {
return;
}

// Retain only the first meta content value for a given name
if (this.meta_[name] === undefined) {
this.meta_[name] = content;
}
});
return map(this.meta_);
}

/**
* Returns the value of an ampdoc's meta tag content for a given name, or
* `null` if the meta tag does not exist.
Expand All @@ -429,11 +457,8 @@ export class AmpDoc {
return null;
}

const el = dev()
.assertElement(this.win.document.head)
.querySelector(`meta[name="${escapeCssSelectorIdent(name)}"]`);

return el ? el.getAttribute('content') || '' : null;
const content = this.getMeta()[name];
return content !== undefined ? content : null;
}

/**
Expand Down Expand Up @@ -905,18 +930,16 @@ export class AmpDocShadow extends AmpDoc {
}

/** @override */
getMetaByName(name) {
return this.meta_[name] !== undefined ? this.meta_[name] : null;
getMeta() {
return /** @type {!Object<string,string>} */ (map(this.meta_));
}

/** @override */
setMetaByName(name, content) {
if (!name) {
throw dev().createError(
'Attempted to store invalid meta name/content pair'
);
devAssert(name, 'Attempted to store invalid meta name/content pair');
if (!this.meta_) {
this.meta_ = map();
}

this.meta_[name] = content;
}
}
Expand Down
43 changes: 10 additions & 33 deletions src/service/document-info-impl.js
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,7 @@ const filteredLinkRels = ['prefetch', 'preload', 'preconnect', 'dns-prefetch'];
* for concurrent page views of a user().
* - linkRels: A map object of link tag's rel (key) and corresponding
* hrefs (value). rel could be 'canonical', 'icon', etc.
* - metaTags: A map object of meta tag's name (key) and corresponding
* contents (value).
* - viewport: The global doc's viewport.
* - replaceParams: A map object of extra query string parameter names (key)
* to corresponding values, used for custom analytics.
* Null if not applicable.
Expand All @@ -50,7 +49,7 @@ const filteredLinkRels = ['prefetch', 'preload', 'preconnect', 'dns-prefetch'];
* pageViewId: string,
* pageViewId64: !Promise<string>,
* linkRels: !Object<string, string|!Array<string>>,
* metaTags: !Object<string, string|!Array<string>>,
* viewport: ?string,
* replaceParams: ?Object<string, string|!Array<string>>
* }}
*/
Expand Down Expand Up @@ -95,7 +94,7 @@ export class DocInfo {
}
const pageViewId = getPageViewId(ampdoc.win);
const linkRels = getLinkRels(ampdoc.win.document);
const metaTags = getMetaTags(ampdoc.win.document);
const viewport = getViewport(ampdoc.win.document);
const replaceParams = getReplaceParams(ampdoc);

return (this.info_ = {
Expand All @@ -115,7 +114,7 @@ export class DocInfo {
return this.pageViewId64_;
},
linkRels,
metaTags,
viewport,
replaceParams,
});
}
Expand Down Expand Up @@ -172,36 +171,14 @@ function getLinkRels(doc) {
}

/**
* Returns a map object of meta tags in document head.
* Key is the meta name, value is a list of corresponding content values.
* Returns the viewport of the document. Note that this is the viewport of the
* host document for AmpDocShadow instances.
* @param {!Document} doc
* @return {!JsonObject<string, string|!Array<string>>}
* @return {?string}
*/
function getMetaTags(doc) {
const metaTags = map();
if (doc.head) {
const metas = doc.head.querySelectorAll('meta[name]');
for (let i = 0; i < metas.length; i++) {
const meta = metas[i];
const content = meta.getAttribute('content');
const name = meta.getAttribute('name');
if (!name || !content) {
continue;
}

let value = metaTags[name];
if (value) {
// Change to array if more than one content for the same name
if (!isArray(value)) {
value = metaTags[name] = [value];
}
value.push(content);
} else {
metaTags[name] = content;
}
}
}
return metaTags;
function getViewport(doc) {
const viewportEl = doc.head.querySelector('meta[name="viewport"]');
return viewportEl ? viewportEl.getAttribute('content') : null;
}

/**
Expand Down
6 changes: 5 additions & 1 deletion src/service/resources-impl.js
Original file line number Diff line number Diff line change
Expand Up @@ -595,14 +595,18 @@ export class ResourcesImpl {
this.firstPassAfterDocumentReady_ = false;
const doc = this.win.document;
const documentInfo = Services.documentInfoForDoc(this.ampdoc);

// TODO(choumx, #26687): Update viewers to read data.viewport instead of
// data.metaTags.viewport from 'documentLoaded' message.
this.viewer_.sendMessage(
'documentLoaded',
dict({
'title': doc.title,
'sourceUrl': getSourceUrl(this.ampdoc.getUrl()),
'serverLayout': doc.documentElement.hasAttribute('i-amphtml-element'),
'linkRels': documentInfo.linkRels,
'metaTags': documentInfo.metaTags,
'metaTags': {'viewport': documentInfo.viewport} /* deprecated */,
'viewport': documentInfo.viewport,
}),
/* cancelUnsent */ true
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -308,7 +308,7 @@ <h1 itemprop="headline">Lorem Ipsum</h1>
pellentesque, id placerat elit ornare.
</p>

<h1>_ping_ Ad Blocked by metaTags</h1>
<h1>_ping_ Ad Blocked by meta tags</h1>
<amp-ad width=300 height=250
type="_ping_"
data-url='https://lh3.googleusercontent.com/pSECrJ82R7-AqeBCOEPGPM9iG9OEIQ_QXcbubWIOdkY=w400-h300-no-n'
Expand Down
5 changes: 5 additions & 0 deletions test/unit/test-cid.js
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,11 @@ describes.sandboxed('cid', {}, env => {
nodeType: /* DOCUMENT */ 9,
body: {},
querySelector: () => {},
head: {
nodeType: /* ELEMENT */ 1,
querySelector: () => null,
querySelectorAll: () => [],
},
},
navigator: window.navigator,
setTimeout: window.setTimeout,
Expand Down
2 changes: 1 addition & 1 deletion test/unit/test-consent.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
import {shouldBlockOnConsentByMeta} from '../../src/consent';

describes.fakeWin('consent', {amp: true}, env => {
describe('block by metaTags', () => {
describe('block by meta tags', () => {
let doc;
let head;
let meta;
Expand Down
Loading

0 comments on commit 88e75b8

Please sign in to comment.