From b4ce6cee81c8b07bce500adc86cc8ed2296774df Mon Sep 17 00:00:00 2001 From: Erwin Mombay Date: Thu, 3 Dec 2015 12:39:44 -0800 Subject: [PATCH 01/17] feature(xhr): modify `fetchJson` to accept method "POST" --- src/xhr.js | 85 +++++++++++++++++-- test/functional/test-xhr.js | 158 +++++++++++++++++++++++++++++++++++- 2 files changed, 234 insertions(+), 9 deletions(-) diff --git a/src/xhr.js b/src/xhr.js index 8791ba7b208c..1b21a1ceca69 100644 --- a/src/xhr.js +++ b/src/xhr.js @@ -25,24 +25,36 @@ import {getService} from './service'; * See https://developer.mozilla.org/en-US/docs/Web/API/GlobalFetch/fetch * * @typedef {{ - * credentials: string + * body: (!Object|!Array|undefined), + * credentials: (string|undefined), + * headers: (!Object|undefined), + * method: (string|undefined) * }} */ const FetchInit = {}; +/** @private @const {!Array} */ +const allowedMethods_ = ['GET', 'POST']; + +/** @private @const {!Array} */ +const allowedBodyTypes_ = ['[object Object]', '[object Array]']; + /** * A service that polyfills Fetch API for use within AMP. */ class Xhr { + /** * @param {!Window} win */ constructor(win) { /** - * @private @type {function(string, ?FetchInit=):!Promise} + * We want to call `fetch_` unbound from any context since it could + * be either the native fetch or our polyfill. + * @private @const {function(string, ?FetchInit=):!Promise} */ - this.fetch_ = win.fetch || fetchPolyfill; + this.fetch_ = (win.fetch || fetchPolyfill).bind(null); } /** @@ -55,13 +67,59 @@ class Xhr { * @return {!Promise} */ fetchJson(input, opt_init) { - return this.fetch_.call(null, input, opt_init).then(response => { + const init = opt_init || {}; + init.method = normalizeMethod_(init.method); + setupJson_(init); + + return this.fetch_(input, init).then(response => { return assertSuccess(response).json(); }); } } +/** + * Normalized method name by uppercasing. + * @param {string|undefined} method + * @return {string} + * @private + */ +export function normalizeMethod_(method) { + if (method === undefined) { + return 'GET'; + } + return method.toUpperCase(); +} + +/** +* Initialize init object with headers and stringifies the body. + * @param {!FetchInit} init + * @private + */ +function setupJson_(init) { + assert(allowedMethods_.indexOf(init.method) != -1, 'Only one of ' + + allowedMethods_.join(', ') + ' is currently allowed. Got %s', + init.method); + + init.headers = { + 'Accept': 'application/json' + }; + + if (init.method == 'POST') { + const bodyType = Object.prototype.toString.call(init.body); + + // Assume JSON strict mode where only objects or arrays are allowed + // as body. + assert(allowedBodyTypes_.indexOf(bodyType) > -1, + 'body must be of type object or array. %s', + init.body); + + init.headers['Content-Type'] = 'application/json;charset=utf-8'; + init.body = JSON.stringify(init.body); + } +} + + /** * A minimal polyfill of Fetch API. It only polyfills what we currently use. * @@ -71,7 +129,7 @@ class Xhr { * us to immediately support a much wide API. * * @param {string} input - * @param {?FetchInit=} opt_init + * @param {!FetchInit=} opt_init * @return {!Promise} * @private Visible for testing */ @@ -82,7 +140,7 @@ export function fetchPolyfill(input, opt_init) { 'Only credentials=include support: %s', init.credentials); return new Promise(function(resolve, reject) { - const xhr = createXhrRequest(init.method || 'GET', input); + const xhr = createXhrRequest(init.method || 'GET', input, init); if (init.credentials == 'include') { xhr.withCredentials = true; @@ -112,7 +170,11 @@ export function fetchPolyfill(input, opt_init) { reject(new Error('Request aborted')); }; - xhr.send(); + if (init.method == 'POST') { + xhr.send(init.body); + } else { + xhr.send(); + } }); } @@ -120,10 +182,11 @@ export function fetchPolyfill(input, opt_init) { /** * @param {string} method * @param {string} url + * @param {!FetchInit} init * @return {!XMLHttpRequest} * @private */ -function createXhrRequest(method, url) { +function createXhrRequest(method, url, init) { let xhr = new XMLHttpRequest(); if ('withCredentials' in xhr) { xhr.open(method, url, true); @@ -134,6 +197,12 @@ function createXhrRequest(method, url) { } else { throw new Error('CORS is not supported'); } + + if (init.headers) { + Object.keys(init.headers).forEach(function(header) { + xhr.setRequestHeader(header, init.headers[header]); + }); + } return xhr; } diff --git a/test/functional/test-xhr.js b/test/functional/test-xhr.js index f41ddd5559f3..94b66038bf6c 100644 --- a/test/functional/test-xhr.js +++ b/test/functional/test-xhr.js @@ -14,9 +14,13 @@ * limitations under the License. */ +import * as sinon from 'sinon'; import {xhrFor, fetchPolyfill} from '../../src/xhr'; describe('XHR', function() { + let mockXhr; + let requests; + // Given XHR calls give tests more time. this.timeout(5000); @@ -24,9 +28,86 @@ describe('XHR', function() { {xhr: xhrFor(window), desc: 'Native'}, {xhr: xhrFor({fetch: fetchPolyfill}), desc: 'Polyfill'} ]; + + function setupMockXhr() { + mockXhr = sinon.useFakeXMLHttpRequest(); + requests = []; + mockXhr.onCreate = function(xhr) { + requests.push(xhr); + }; + } + + afterEach(() => { + if (mockXhr) { + mockXhr.restore(); + mockXhr = null; + requests = null; + } + }); + scenarios.forEach(test => { + const xhr = test.xhr; + + // Since if its the Native fetch, it wont use the XHR object so + // mocking and testing the request becomes not doable. + if (test.desc != 'Native') { + + it('should allow GET and POST methods', () => { + setupMockXhr(); + const get = xhr.fetchJson.bind(xhr, '/get?k=v1'); + const post = xhr.fetchJson.bind(xhr, '/post', { + method: 'POST', + body: { + hello: 'world' + } + }); + const put = xhr.fetchJson.bind(xhr, '/put', { + method: 'PUT', + body: { + hello: 'world' + } + }); + const patch = xhr.fetchJson.bind(xhr, '/patch', { + method: 'PATCH', + body: { + hello: 'world' + } + }); + const deleteMethod = xhr.fetchJson.bind(xhr, '/delete', { + method: 'DELETE', + body: { + id: 3 + } + }); + + expect(get).to.not.throw(); + expect(post).to.not.throw(); + expect(put).to.throw(); + expect(patch).to.throw(); + expect(deleteMethod).to.throw(); + }); + + it('should do `GET` as default method', () => { + setupMockXhr(); + xhr.fetchJson('/get?k=v1'); + expect(requests[0].method).to.equal('GET'); + }); + + it('should normalize method names to uppercase', () => { + setupMockXhr(); + xhr.fetchJson('/abc'); + xhr.fetchJson('/abc', { + method: 'post', + body: { + hello: 'world' + } + }); + expect(requests[0].method).to.equal('GET'); + expect(requests[1].method).to.equal('POST'); + }); + } + describe(test.desc, () => { - const xhr = test.xhr; it('should do simple JSON fetch', () => { return xhr.fetchJson('https://httpbin.org/get?k=v1').then(res => { @@ -86,4 +167,79 @@ describe('XHR', function() { }); }); }); + + scenarios.forEach(test => { + const url = 'https://httpbin.org/post'; + + describe(test.desc + ' POST', () => { + const xhr = test.xhr; + + if (test.desc != 'Native') { + it('should have required json POST headers by default', () => { + setupMockXhr(); + xhr.fetchJson(url, { + method: 'POST', + body: { + hello: 'world' + } + }); + expect(JSON.stringify(requests[0].requestHeaders)) + .to.eql(JSON.stringify({ + 'Accept': 'application/json', + 'Content-Type': 'application/json;charset=utf-8' + })); + }); + } + + it('should get an echo\'d response back', () => { + return xhr.fetchJson(url, { + method: 'POST', + body: { + hello: 'world' + } + }).then(res => { + expect(res.json).to.jsonEqual({ + hello: 'world' + }); + }); + }); + + it('should throw when `body` is not an object or array', () => { + const objectFn = xhr.fetchJson.bind(xhr, url, { + method: 'POST', + body: { + hello: 'world' + } + }); + const arrayFn = xhr.fetchJson.bind(xhr, url, { + method: 'POST', + body: ['hello', 'world'] + }); + const stringFn = xhr.fetchJson.bind(xhr, url, { + method: 'POST', + body: 'hello world' + }); + const numberFn = xhr.fetchJson.bind(xhr, url, { + method: 'POST', + body: 3 + }); + const booleanFn = xhr.fetchJson.bind(xhr, url, { + method: 'POST', + body: true + }); + const nullFn = xhr.fetchJson.bind(xhr, url, { + method: 'POST', + body: null + }); + + expect(objectFn).to.not.throw(); + expect(arrayFn).to.not.throw(); + expect(stringFn).to.throw(); + expect(numberFn).to.throw(); + expect(booleanFn).to.throw(); + expect(nullFn).to.throw(); + }); + + }); + }); }); From e5cb401ff2c276ff4012dcde9a9612f07f2e3967 Mon Sep 17 00:00:00 2001 From: Brad Townsend Date: Wed, 16 Dec 2015 12:33:34 -0800 Subject: [PATCH 02/17] Make request transport for amp-analytics configurable. --- examples/analytics.amp.html | 4 +- extensions/amp-analytics/0.1/amp-analytics.js | 33 ++---- .../0.1/test/test-amp-analytics.js | 103 +++++----------- .../amp-analytics/0.1/test/test-transport.js | 112 ++++++++++++++++++ extensions/amp-analytics/0.1/transport.js | 110 +++++++++++++++++ extensions/amp-analytics/0.1/vendors.js | 24 ++-- 6 files changed, 280 insertions(+), 106 deletions(-) create mode 100644 extensions/amp-analytics/0.1/test/test-transport.js create mode 100644 extensions/amp-analytics/0.1/transport.js diff --git a/examples/analytics.amp.html b/examples/analytics.amp.html index e8527833b538..2cefc8f8589a 100644 --- a/examples/analytics.amp.html +++ b/examples/analytics.amp.html @@ -22,9 +22,9 @@ @@ -48,29 +50,33 @@ "vars": { "account": "UA-XXXX-Y" }, - "triggers": [{ - "on": "visible", - "request": "pageview", - "vars": { - "title": "Example Pageview" + "triggers": { + "default pageview": { + "on": "visible", + "request": "pageview", + "vars": { + "title": "Example Pageview" + } + }, + "click on #test1 trigger": { + "on": "click", + "selector": "#test1", + "request": "event", + "vars": { + "eventCategory": "examples", + "eventAction": "clicked-test1" + } + }, + "click on #top trigger": { + "on": "click", + "selector": "#top", + "request": "event", + "vars": { + "eventCategory": "examples", + "eventAction": "clicked-header" + } } - }, { - "on": "click", - "selector": "#test1", - "request": "event", - "vars": { - "eventCategory": "examples", - "eventAction": "clicked-test1" - } - }, { - "on": "click", - "selector": "#top", - "request": "event", - "vars": { - "eventCategory": "examples", - "eventAction": "clicked-header" - } - }] + } } diff --git a/extensions/amp-analytics/0.1/amp-analytics.js b/extensions/amp-analytics/0.1/amp-analytics.js index 4ef4c46398d2..39a0f3e08f72 100644 --- a/extensions/amp-analytics/0.1/amp-analytics.js +++ b/extensions/amp-analytics/0.1/amp-analytics.js @@ -94,22 +94,24 @@ export class AmpAnalytics extends AMP.BaseElement { this.generateRequests_(); - if (!Array.isArray(this.config_['triggers'])) { + if (!this.config_['triggers']) { log.error(this.getName_(), 'No triggers were found in the config. No ' + 'analytics data will be sent.'); return Promise.resolve(); } // Trigger callback can be synchronous. Do the registration at the end. - for (let k = 0; k < this.config_['triggers'].length; k++) { - const trigger = this.config_['triggers'][k]; - if (!trigger['on'] || !trigger['request']) { - log.warn(this.getName_(), '"on" and "request" attributes are ' + - 'required for data to be collected.'); - continue; + for (const k in this.config_['triggers']) { + if (this.config_['triggers'].hasOwnProperty(k)) { + const trigger = this.config_['triggers'][k]; + if (!trigger['on'] || !trigger['request']) { + log.warn(this.getName_(), '"on" and "request" attributes are ' + + 'required for data to be collected.'); + continue; + } + addListener(this.getWin(), trigger['on'], + this.handleEvent_.bind(this, trigger), trigger['selector']); } - addListener(this.getWin(), trigger['on'], - this.handleEvent_.bind(this, trigger), trigger['selector']); } return Promise.resolve(); } From 354322658f6f0b3994a7b387b662ac2118e47754 Mon Sep 17 00:00:00 2001 From: Malte Ubl Date: Tue, 15 Dec 2015 17:36:28 -0800 Subject: [PATCH 04/17] Simplify scroll tracking and greatly increase frequency in which we emit viewport change events on scroll. - The scroll event now always fires in vsync and always measures scrollTop. - The viewport change event now happens at the latest 3 frames after scrolling ended. - Increased speed at which we send change event. Needed due to resolution at slow speed, but I also feel the value is better. Changes ad intersection to fire "on scroll". Fixes #873 Fixes a bug where only the first viewability provider in an ad would get an initial viewability change record. Fixes #1126 --- builtins/amp-ad.js | 13 ++++-- src/viewport.js | 69 +++++++++++++++++--------------- test/functional/test-amp-ad.js | 6 +-- test/functional/test-viewport.js | 45 ++++++++------------- 4 files changed, 65 insertions(+), 68 deletions(-) diff --git a/builtins/amp-ad.js b/builtins/amp-ad.js index 3ecb95780a3d..75c745ee537e 100644 --- a/builtins/amp-ad.js +++ b/builtins/amp-ad.js @@ -20,7 +20,7 @@ import {getIntersectionChangeEntry} from '../src/intersection-observer'; import {isLayoutSizeDefined} from '../src/layout'; import {loadPromise} from '../src/event-helper'; import {registerElement} from '../src/custom-element'; -import {getIframe, listenOnce, postMessage, prefetchBootstrap} from +import {getIframe, listen, listenOnce, postMessage, prefetchBootstrap} from '../src/3p-frame'; import {adPrefetch, adPreconnect} from '../ads/_prefetch'; import {timer} from '../src/timer'; @@ -212,7 +212,12 @@ export function installAd(win) { this.deferMutate(this.noContentHandler_.bind(this)); }); // Triggered by context.observeIntersection(…) inside the ad iframe. - listenOnce(this.iframe_, 'send-intersections', () => { + // We use listen instead of listenOnce, because a single ad might + // have multiple parties wanting to receive viewability data. + // The second time this is called, it doesn't do much but it + // guarantees that the receiver gets an initial intersection change + // record. + listen(this.iframe_, 'send-intersections', () => { this.startSendingIntersectionChanges_(); }); } @@ -227,7 +232,7 @@ export function installAd(win) { // it is visible. if (inViewport) { this.unlistenViewportChanges_ = - this.getViewport().onChanged(this.sendAdIntersection_.bind(this)); + this.getViewport().onScroll(this.sendAdIntersection_.bind(this)); } else if (this.unlistenViewportChanges_) { this.unlistenViewportChanges_(); this.unlistenViewportChanges_ = null; @@ -239,6 +244,8 @@ export function installAd(win) { * observing its position in the viewport. * Sets a flag, measures the iframe position if necessary and sends * one change record to the iframe. + * Note that this method may be called more than once if a single ad + * has multiple parties interested in viewability data. * @private */ startSendingIntersectionChanges_() { diff --git a/src/viewport.js b/src/viewport.js index 17fa4cd6847f..8399bafc6f60 100644 --- a/src/viewport.js +++ b/src/viewport.js @@ -23,6 +23,7 @@ import {onDocumentReady} from './document-state'; import {platform} from './platform'; import {px, setStyle, setStyles} from './style'; import {timer} from './timer'; +import {vsyncFor} from './vsync'; import {viewerFor} from './viewer'; @@ -74,6 +75,9 @@ export class Viewport { /** @private {?number} */ this./*OK*/scrollTop_ = null; + /** @private {?number} */ + this.lastMeasureScrollTop_ = null; + /** @private {?number} */ this./*OK*/scrollLeft_ = null; @@ -83,6 +87,9 @@ export class Viewport { /** @private {number} */ this.scrollMeasureTime_ = 0; + /** @private {Vsync} */ + this.vsync_ = vsyncFor(win); + /** @private {boolean} */ this.scrollTracking_ = false; @@ -101,6 +108,9 @@ export class Viewport { /** @private {string|undefined} */ this.originalViewportMetaString_ = undefined; + /** @private @const (function()) */ + this.boundThrottledScroll_ = this.throttledScroll_.bind(this); + this.viewer_.onViewportEvent(() => { this.binding_.updateViewerViewport(this.viewer_); const paddingTop = this.viewer_.getPaddingTop(); @@ -155,7 +165,7 @@ export class Viewport { * @return {number} */ getScrollLeft() { - if (this./*OK*/scrollleft_ == null) { + if (this./*OK*/scrollLeft_ == null) { this./*OK*/scrollLeft_ = this.binding_.getScrollLeft(); } return this./*OK*/scrollLeft_; @@ -379,14 +389,7 @@ export class Viewport { /** @private */ scroll_() { this.scrollCount_++; - this.scrollObservable_.fire(); - this.scrollLeft_ = this.binding_.getScrollLeft(); - - if (this.scrollTracking_) { - return; - } - const newScrollTop = this.binding_.getScrollTop(); if (newScrollTop < 0) { // iOS and some other browsers use negative values of scrollTop for @@ -394,44 +397,44 @@ export class Viewport { // be ignored here. return; } - - this.scrollTracking_ = true; this.scrollTop_ = newScrollTop; - this.scrollMeasureTime_ = timer.now(); - timer.delay(() => this.scrollDeferred_(), 500); + if (!this.scrollTracking_) { + this.scrollTracking_ = true; + const now = timer.now(); + // Wait 2 frames and then request an animation frame. + timer.delay(() => this.vsync_.measure( + this.throttledScroll_.bind(this, now, newScrollTop)), 36); + } + this.scrollObservable_.fire(); } - /** @private */ - scrollDeferred_() { + /** + * This method is called about every 3 frames (assuming 60hz) and it + * is called in a vsync measure task. + * @param {number} referenceTime Time when the scroll measurement, that + * triggered this call made, was made. + * @param {number} referenceTop Scrolltop at that time. + * @private + */ + throttledScroll_(referenceTime, referenceTop) { this.scrollTracking_ = false; - const newScrollTop = this.binding_.getScrollTop(); - if (this.scrollTop_ === null) { - // If the scrollTop was reset while waiting for the next scroll event - // we have to assume that velocity is 0 - there's no other way we can - // calculate it. - this.scrollTop_ = newScrollTop; - } + const newScrollTop = this.scrollTop_ = this.binding_.getScrollTop(); const now = timer.now(); let velocity = 0; - if (now != this.scrollMeasureTime_) { - velocity = (newScrollTop - this.scrollTop_) / - (now - this.scrollMeasureTime_); + if (now != referenceTime) { + velocity = (newScrollTop - referenceTop) / + (now - referenceTime); } log.fine(TAG_, 'scroll: ' + 'scrollTop=' + newScrollTop + '; ' + 'velocity=' + velocity); - this.scrollTop_ = newScrollTop; - this.scrollMeasureTime_ = now; // TODO(dvoytenko): confirm the desired value and document it well. - // Currently, this is 20px/second -> 0.02px/millis - if (Math.abs(velocity) < 0.02) { + // Currently, this is 30px/second -> 0.03px/millis + if (Math.abs(velocity) < 0.03) { this.changed_(/* relayoutAll */ false, velocity); } else { - timer.delay(() => { - if (!this.scrollTracking_) { - this.scrollDeferred_(); - } - }, 250); + timer.delay(() => this.vsync_.measure( + this.throttledScroll_.bind(this, now, newScrollTop)), 20); } } diff --git a/test/functional/test-amp-ad.js b/test/functional/test-amp-ad.js index 48ad2a649fda..8ab93eeecbc2 100644 --- a/test/functional/test-amp-ad.js +++ b/test/functional/test-amp-ad.js @@ -19,7 +19,7 @@ import {installAd, AD_LOAD_TIME_MS} from '../../builtins/amp-ad'; import {viewportFor} from '../../src/viewport'; import {timer} from '../../src/timer'; -import {vsync} from '../../src/vsync'; +import {vsyncFor} from '../../src/vsync'; import * as sinon from 'sinon'; describe('amp-ad', () => { @@ -230,12 +230,12 @@ describe('amp-ad', () => { expect(posts).to.have.length(1); ampAd.viewportCallback(true); expect(posts).to.have.length(2); - viewport.changed_(false, 0); + viewport.scroll_(); expect(posts).to.have.length(3); ampAd.viewportCallback(false); expect(posts).to.have.length(4); // No longer listening. - viewport.changed_(false, 0); + viewport.scroll_(); expect(posts).to.have.length(4); }); }); diff --git a/test/functional/test-viewport.js b/test/functional/test-viewport.js index 509001b49ef5..bd9988c56c1d 100644 --- a/test/functional/test-viewport.js +++ b/test/functional/test-viewport.js @@ -48,7 +48,10 @@ describe('Viewport', () => { windowApi = { document: { documentElement: {style: {}} - } + }, + location: {}, + setTimeout: window.setTimeout, + requestAnimationFrame: fn => window.setTimeout(fn, 16) }; binding = new ViewportBindingVirtual_(windowApi, viewer); viewport = new Viewport(windowApi, binding, viewer); @@ -132,44 +135,27 @@ describe('Viewport', () => { viewport.onChanged(event => { changeEvent = event; }); - viewer.getScrollTop = () => {return 34;}; + viewer.getScrollTop = () => 34; viewerViewportHandler(); expect(changeEvent).to.equal(null); // Not enough time past. - clock.tick(100); - viewer.getScrollTop = () => {return 35;}; - viewerViewportHandler(); + clock.tick(8); expect(changeEvent).to.equal(null); - - // A bit more time. - clock.tick(750); - expect(changeEvent).to.not.equal(null); - expect(changeEvent.relayoutAll).to.equal(false); - expect(changeEvent.velocity).to.be.closeTo(0.002, 1e-4); - }); - - it('should defer scroll events and react to reset of scroll pos', () => { - let changeEvent = null; - viewport.onChanged(event => { - changeEvent = event; - }); - viewer.getScrollTop = () => {return 34;}; - viewerViewportHandler(); + clock.tick(8); expect(changeEvent).to.equal(null); - - // Not enough time past. - clock.tick(100); - viewer.getScrollTop = () => {return 35;}; + viewer.getScrollTop = () => 35; viewerViewportHandler(); + clock.tick(16); expect(changeEvent).to.equal(null); - // Reset and wait a bit more time. - viewport./*OK*/scrollTop_ = null; - clock.tick(750); + // A bit more time. + clock.tick(16); + expect(changeEvent).to.equal(null); + clock.tick(4); expect(changeEvent).to.not.equal(null); expect(changeEvent.relayoutAll).to.equal(false); - expect(changeEvent.velocity).to.equal(0); + expect(changeEvent.velocity).to.be.closeTo(0.019230, 1e-4); }); it('should update scroll pos and reset cache', () => { @@ -363,7 +349,8 @@ describe('Viewport META', () => { } return undefined; } - } + }, + location: {} }; binding = new ViewportBindingVirtual_(windowApi, viewer); From e438c134fea172f90c2fb2d52223140a3c2565a6 Mon Sep 17 00:00:00 2001 From: Dima Voytenko Date: Fri, 18 Dec 2015 09:11:11 -0800 Subject: [PATCH 05/17] More documentation on amp-list layout --- extensions/amp-list/amp-list.md | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/extensions/amp-list/amp-list.md b/extensions/amp-list/amp-list.md index 71a78dbc6827..148783996d82 100644 --- a/extensions/amp-list/amp-list.md +++ b/extensions/amp-list/amp-list.md @@ -47,7 +47,8 @@ element will be shown if AMP Runtime cannot resize the `amp-list` element as req An example: ```html - +