Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Separate out implementaton of document-info and remove dependency of … #5864

Merged
merged 5 commits into from
Oct 28, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 0 additions & 6 deletions extensions/amp-ad/0.1/test/test-amp-ad-3p-impl.js
Original file line number Diff line number Diff line change
Expand Up @@ -116,12 +116,6 @@ describe('amp-ad-3p-impl', () => {
});
});

it('should reject if no canonical URL provided', () => {
win.document.head.innerHTML = '';
return expect(ad3p.layoutCallback())
.to.eventually.be.rejectedWith('canonical');
});

it('should reject if no type attribute provided', () => {
ad3p.element.removeAttribute('type');
return expect(ad3p.layoutCallback())
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,9 @@ describe('amp-install-serviceworker', () => {
};
resetServiceForTesting(window, 'documentInfo');
getServiceForDoc(document, 'documentInfo', () => {
return documentInfo;
return {
get: () => documentInfo,
};
});
whenVisible = Promise.resolve();
getService(win, 'viewer', () => {
Expand Down
17 changes: 12 additions & 5 deletions src/3p-frame.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@
import {dev, user} from './log';
import {documentInfoForDoc} from './document-info';
import {getLengthNumeral} from '../src/layout';
import {getService} from './service';
import {tryParseJson} from './json';
import {getMode} from './mode';
import {getModeObject} from './mode-object';
Expand Down Expand Up @@ -192,16 +191,24 @@ export function preloadBootstrap(window, preconnect) {
* @visibleForTesting
*/
export function getBootstrapBaseUrl(parentWindow, opt_strictForUnitTest) {
return getService(self, 'bootstrapBaseUrl', () => {
return getCustomBootstrapBaseUrl(parentWindow, opt_strictForUnitTest) ||
getDefaultBootstrapBaseUrl(parentWindow);
});
// The value is cached in a global variable called `bootstrapBaseUrl`;
const bootstrapBaseUrl = parentWindow.bootstrapBaseUrl;
if (bootstrapBaseUrl) {
return bootstrapBaseUrl;
}
return parentWindow.bootstrapBaseUrl =
getCustomBootstrapBaseUrl(parentWindow, opt_strictForUnitTest)
|| getDefaultBootstrapBaseUrl(parentWindow);
}

export function setDefaultBootstrapBaseUrlForTesting(url) {
overrideBootstrapBaseUrl = url;
}

export function resetBootstrapBaseUrlForTesting(win) {
win.bootstrapBaseUrl = undefined;
}

/**
* Returns the default base URL for 3p bootstrap iframes.
* @param {!Window} parentWindow
Expand Down
60 changes: 4 additions & 56 deletions src/document-info.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,66 +14,14 @@
* limitations under the License.
*/

import {getServiceForDoc} from './service';
import {parseUrl, getSourceUrl} from './url';
import {user} from './log';


/**
* Properties:
* - url: The doc's url.
* - sourceUrl: the source url of an amp document.
* - canonicalUrl: The doc's canonical.
* - pageViewId: Id for this page view. Low entropy but should be unique
* for concurrent page views of a user().
*
* @typedef {{
* url: string,
* sourceUrl: string,
* canonicalUrl: string,
* pageViewId: string,
* }}
*/
export let DocumentInfoDef;
import {getExistingServiceForDoc} from './service';


/**
* @param {!Node|!./service/ampdoc-impl.AmpDoc} nodeOrDoc
* @return {!DocumentInfoDef} Info about the doc
* @return {!./service/document-info-impl.DocumentInfoDef} Info about the doc
*/
export function documentInfoForDoc(nodeOrDoc) {
return /** @type {!DocumentInfoDef} */ (getServiceForDoc(nodeOrDoc,
'documentInfo', ampdoc => {
const rootNode = ampdoc.getRootNode();
let canonicalUrl = rootNode && rootNode.AMP
&& rootNode.AMP.canonicalUrl;
if (!canonicalUrl) {
const canonicalTag = user().assert(
rootNode.querySelector('link[rel=canonical]'),
'AMP files are required to have a <link rel=canonical> tag.');
canonicalUrl = parseUrl(canonicalTag.href).href;
}
const pageViewId = getPageViewId(ampdoc.win);
const res = {
get sourceUrl() {
return getSourceUrl(ampdoc.getUrl());
},
canonicalUrl,
pageViewId,
};
return res;
}));
}



/**
* Returns a relatively low entropy random string.
* This should be called once per window and then cached for subsequent
* access to the same value to be persistent per page.
* @param {!Window} win
* @return {string}
*/
function getPageViewId(win) {
return String(Math.floor(win.Math.random() * 10000));
return /** @type {!./service/document-info-impl.DocInfo} */ (
getExistingServiceForDoc(nodeOrDoc, 'documentInfo')).get();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this .get() new? don't think ive seen this before

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nvm looks like its newly added for the cached value

}
2 changes: 2 additions & 0 deletions src/runtime.js
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ import {
} from './shadow-embed';
import {getMode} from './mode';
import {installActionServiceForDoc} from './service/action-impl';
import {installDocumentInfoServiceForDoc} from './service/document-info-impl';
import {installGlobalSubmitListenerForDoc} from './document-submit';
import {extensionsFor} from './extensions';
import {installHistoryServiceForDoc} from './service/history-impl';
Expand Down Expand Up @@ -107,6 +108,7 @@ export function installRuntimeServices(global) {
* @param {!Object<string, string>=} opt_initParams
*/
export function installAmpdocServices(ampdoc, opt_initParams) {
installDocumentInfoServiceForDoc(ampdoc);
installViewerServiceForDoc(ampdoc, opt_initParams);
installViewportServiceForDoc(ampdoc);
installHistoryServiceForDoc(ampdoc);
Expand Down
97 changes: 97 additions & 0 deletions src/service/document-info-impl.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,97 @@
/**
* Copyright 2015 The AMP HTML Authors. All Rights Reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS-IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

import {fromClassForDoc} from '../service';
import {parseUrl, getSourceUrl} from '../url';


/**
* Properties:
* - url: The doc's url.
* - sourceUrl: the source url of an amp document.
* - canonicalUrl: The doc's canonical.
* - pageViewId: Id for this page view. Low entropy but should be unique
* for concurrent page views of a user().
*
* @typedef {{
* sourceUrl: string,
* canonicalUrl: string,
* pageViewId: string,
* }}
*/
export let DocumentInfoDef;


/**
* @param {!Node|!./ampdoc-impl.AmpDoc} nodeOrDoc
* @return {!DocInfo} Info about the doc
*/
export function installDocumentInfoServiceForDoc(nodeOrDoc) {
return fromClassForDoc(nodeOrDoc, 'documentInfo', DocInfo);
}


export class DocInfo {
/**
* @param {!./ampdoc-impl.AmpDoc} ampdoc
*/
constructor(ampdoc) {
/** @private @const */
this.ampdoc_ = ampdoc;
/** @private {?DocumentInfoDef} */
this.info_ = null;
}

/** @return {!DocumentInfoDef} */
get() {
if (this.info_) {
return this.info_;
}
const ampdoc = this.ampdoc_;
const url = ampdoc.getUrl();
const sourceUrl = getSourceUrl(url);
const rootNode = ampdoc.getRootNode();
let canonicalUrl = rootNode && rootNode.AMP
&& rootNode.AMP.canonicalUrl;
if (!canonicalUrl) {
const canonicalTag = rootNode.querySelector('link[rel=canonical]');
canonicalUrl = canonicalTag
? parseUrl(canonicalTag.href).href
: sourceUrl;
}
const pageViewId = getPageViewId(ampdoc.win);
return this.info_ = {
/** @return {string} */
get sourceUrl() {
return getSourceUrl(ampdoc.getUrl());
},
canonicalUrl,
pageViewId,
};
}
}


/**
* Returns a relatively low entropy random string.
* This should be called once per window and then cached for subsequent
* access to the same value to be persistent per page.
* @param {!Window} win
* @return {string}
*/
function getPageViewId(win) {
return String(Math.floor(win.Math.random() * 10000));
}
2 changes: 1 addition & 1 deletion src/service/url-replacements-impl.js
Original file line number Diff line number Diff line change
Expand Up @@ -448,7 +448,7 @@ export class UrlReplacements {

/**
* Resolves the value via document info.
* @param {function(!../document-info.DocumentInfoDef):T} getter
* @param {function(!./document-info-impl.DocumentInfoDef):T} getter
* @return {T}
* @template T
*/
Expand Down
6 changes: 3 additions & 3 deletions test/functional/test-3p-frame.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,11 +21,11 @@ import {
getSubDomain,
preloadBootstrap,
resetCountForTesting,
resetBootstrapBaseUrlForTesting,
} from '../../src/3p-frame';
import {documentInfoForDoc} from '../../src/document-info';
import {loadPromise} from '../../src/event-helper';
import {preconnectForElement} from '../../src/preconnect';
import {resetServiceForTesting} from '../../src/service';
import {validateData} from '../../3p/3p';
import {viewerForDoc} from '../../src/viewer';
import * as sinon from 'sinon';
Expand All @@ -46,8 +46,8 @@ describe('3p-frame', () => {
});

afterEach(() => {
resetBootstrapBaseUrlForTesting(window);
sandbox.restore();
resetServiceForTesting(window, 'bootstrapBaseUrl');
resetCountForTesting();
const m = document.querySelector(
'[name="amp-3p-iframe-src"]');
Expand Down Expand Up @@ -321,7 +321,7 @@ describe('3p-frame', () => {

container.appendChild(div);
const name = getIframe(window, div).name;
resetServiceForTesting(window, 'bootstrapBaseUrl');
resetBootstrapBaseUrlForTesting(window);
resetCountForTesting();
const newName = getIframe(window, div).name;
expect(name).to.match(/d-\d+.ampproject.net__ping__0/);
Expand Down
3 changes: 3 additions & 0 deletions test/functional/test-document-click.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ import {installTimerService} from '../../src/service/timer-impl';
import {
installUrlReplacementsServiceForDoc,
} from '../../src/service/url-replacements-impl';
import {installDocumentInfoServiceForDoc,} from
'../../src/service/document-info-impl';
import * as sinon from 'sinon';
import {toggleExperiment} from '../../src/experiments';

Expand Down Expand Up @@ -95,6 +97,7 @@ describe('test-document-click onDocumentElementClick_', () => {
push: () => {},
};
installTimerService(win);
installDocumentInfoServiceForDoc(ampdoc);
installUrlReplacementsServiceForDoc(ampdoc);
});

Expand Down
18 changes: 8 additions & 10 deletions test/functional/test-document-info.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@

import {createIframePromise} from '../../testing/iframe';
import {documentInfoForDoc} from '../../src/document-info';
import {installDocumentInfoServiceForDoc,} from
'../../src/service/document-info-impl';
import {installDocService} from '../../src/service/ampdoc-impl';
import * as sinon from 'sinon';

Expand All @@ -38,7 +40,10 @@ describe('document-info', () => {
link.setAttribute('rel', 'canonical');
iframe.doc.head.appendChild(link);
}
installDocService(iframe.win, true);
const win = iframe.win;
installDocService(win, true);
sandbox.stub(win.Math, 'random', () => 0.123456789);
installDocumentInfoServiceForDoc(win.document);
return iframe.win;
});
}
Expand All @@ -63,6 +68,7 @@ describe('document-info', () => {
};
win.document.defaultView = win;
installDocService(win, true);
installDocumentInfoServiceForDoc(win.document);
expect(documentInfoForDoc(win.document).sourceUrl).to.equal(
'http://www.origin.com/foo/?f=0');
});
Expand All @@ -80,6 +86,7 @@ describe('document-info', () => {
};
win.document.defaultView = win;
installDocService(win, true);
installDocumentInfoServiceForDoc(win.document);
expect(documentInfoForDoc(win.document).sourceUrl).to.equal(
'http://www.origin.com/foo/?f=0');
win.location.href = 'https://cdn.ampproject.org/v/www.origin.com/foo/?f=1';
Expand All @@ -89,7 +96,6 @@ describe('document-info', () => {

it('should provide the pageViewId', () => {
return getWin('https://twitter.com/').then(win => {
sandbox.stub(win.Math, 'random', () => 0.123456789);
expect(documentInfoForDoc(win.document).pageViewId).to.equal('1234');
expect(documentInfoForDoc(win.document).pageViewId).to.equal('1234');
});
Expand All @@ -101,12 +107,4 @@ describe('document-info', () => {
'http://localhost:' + location.port + '/foo.html');
});
});

it('should throw if no canonical is available.', () => {
return getWin(null).then(win => {
expect(() => {
documentInfoForDoc(win.document).canonicalUrl;
}).to.throw(/AMP files are required to have a/);
});
});
});
10 changes: 7 additions & 3 deletions test/functional/test-performance.js
Original file line number Diff line number Diff line change
Expand Up @@ -534,9 +534,13 @@ describe('performance', () => {
perf.coreServicesAvailable();
resetServiceForTesting(window, 'documentInfo');
const info = {
canonicalUrl: 'https://foo.bar/baz',
pageViewId: 12345,
sourceUrl: 'https://hello.world/baz/#development',
get: () => {
return {
canonicalUrl: 'https://foo.bar/baz',
pageViewId: 12345,
sourceUrl: 'https://hello.world/baz/#development',
};
},
};
getService(window, 'documentInfo', () => info);

Expand Down
Loading