From 159b97e0aa5dfac93f5434c9ba04c170a8172f82 Mon Sep 17 00:00:00 2001 From: Enrique Marroquin Date: Fri, 26 Jul 2019 17:18:57 -0400 Subject: [PATCH 01/16] batch preconnects and clean AmpEvents.BUILT --- src/custom-element.js | 41 +++++++++++++++++--------------- src/service/resource.js | 7 ++++++ src/service/resources-impl.js | 44 ++++++++++++++++++++++++++++++++++- 3 files changed, 72 insertions(+), 20 deletions(-) diff --git a/src/custom-element.js b/src/custom-element.js index 2dd2fe7f26d3..024e1069f73c 100644 --- a/src/custom-element.js +++ b/src/custom-element.js @@ -162,7 +162,7 @@ function createBaseCustomElementClass(win) { */ createdCallback() { // Flag "notbuilt" is removed by Resource manager when the resource is - // considered to be built. See "setBuilt" method. + // considered to be built. See "isBuilt" method. /** @private {boolean} */ this.built_ = false; @@ -535,7 +535,7 @@ function createBaseCustomElementClass(win) { } }).then( () => { - this.preconnect(/* onLayout */ false); + this.maybePreconnect(/* onLayout */ false); this.built_ = true; this.classList.remove('i-amphtml-notbuilt'); this.classList.remove('amp-notbuilt'); @@ -573,29 +573,32 @@ function createBaseCustomElementClass(win) { /** * Called to instruct the element to preconnect to hosts it uses during - * layout. + * layout. If attempting an early preconnect, it requests to preconnect + * to resources beforehand. * @param {boolean} onLayout Whether this was called after a layout. * @this {!Element} */ - preconnect(onLayout) { + maybePreconnect(onLayout) { if (onLayout) { this.implementation_.preconnectCallback(onLayout); } else { - // If we do early preconnects we delay them a bit. This is kind of - // an unfortunate trade off, but it seems faster, because the DOM - // operations themselves are not free and might delay - Services.timerFor(toWin(this.ownerDocument.defaultView)).delay(() => { - const TAG = this.tagName; - if (!this.ownerDocument) { - dev().error(TAG, 'preconnect without ownerDocument'); - return; - } else if (!this.ownerDocument.defaultView) { - dev().error(TAG, 'preconnect without defaultView'); - return; - } - this.implementation_.preconnectCallback(onLayout); - }, 1); + this.getResources().requestPreconnect(this); + } + } + + /** + * Preconnect to host. + */ + preconnect() { + const TAG = this.tagName; + if (!this.ownerDocument) { + dev().error(TAG, 'preconnect without ownerDocument'); + return; + } else if (!this.ownerDocument.defaultView) { + dev().error(TAG, 'preconnect without defaultView'); + return; } + this.implementation_.preconnectCallback(false); } /** @@ -1201,7 +1204,7 @@ function createBaseCustomElementClass(win) { } const promise = tryResolve(() => this.implementation_.layoutCallback()); - this.preconnect(/* onLayout */ true); + this.maybePreconnect(/* onLayout */ true); this.classList.add('i-amphtml-layout'); return promise.then( diff --git a/src/service/resource.js b/src/service/resource.js index 6458662a7136..2993970fde54 100644 --- a/src/service/resource.js +++ b/src/service/resource.js @@ -1091,4 +1091,11 @@ export class Resource { delete this.element[RESOURCE_PROP_]; this.element.disconnect(/* opt_pretendDisconnected */ true); } + + /** + * Calls element's preconnectCallback. + */ + preconnect() { + this.element.preconnect(); + } } diff --git a/src/service/resources-impl.js b/src/service/resources-impl.js index 5112030dda31..5e48172fda84 100644 --- a/src/service/resources-impl.js +++ b/src/service/resources-impl.js @@ -25,6 +25,7 @@ import {VisibilityState} from '../visibility-state'; import {areMarginsChanged, expandLayoutRect} from '../layout-rect'; import {closest, hasNextNodeInDocumentOrder} from '../dom'; import {computedStyle} from '../style'; +import {debounce} from '../utils/rate-limit'; import {dev, devAssert} from '../log'; import {dict, hasOwn} from '../utils/object'; import {getSourceUrl} from '../url'; @@ -425,6 +426,18 @@ export class Resources { /** @private {?Array} */ this.pendingBuildResources_ = []; + /** @private {?Array} */ + this.pendingPreconnectResources_ = []; + + // If we do early preconnects we delay them a bit. This is kind of + // an unfortunate trade off, but it seems faster, because the DOM + // operations themselves are not free and might delay + this.eventuallyCallPreconnects_ = debounce( + this.win, + this.eventuallyCallPreconnects_.bind(this), + 1 + ); + /** @private {boolean} */ this.isCurrentlyBuildingPendingResources_ = false; @@ -840,7 +853,36 @@ export class Resources { dev().fine(TAG_, 'element upgraded:', resource.debugid); } - /** @override */ + /** + * Schedule resources for preconnecting. + * @param {!Element} element + */ + requestPreconnect(element) { + const resource = Resource.forElement(element); + this.pendingPreconnectResources_.push(resource); + this.eventuallyCallPreconnects_(); + } + + /** + * Calls resource manager's preconnect and removes resource from the pending + * buffer. + * @private + */ + eventuallyCallPreconnects_() { + for (let i = 0; i < this.pendingPreconnectResources_.length; i++) { + const resource = this.pendingPreconnectResources_[i]; + resource.preconnect(); + this.pendingPreconnectResources_.splice(i, 1); + } + } + + /** + * Assigns an owner for the specified element. This means that the resources + * within this element will be managed by the owner and not Resources manager. + * @param {!Element} element + * @param {!AmpElement} owner + * @package + */ setOwner(element, owner) { Resource.setOwner(element, owner); } From 088ab7f99b050cf6a5b6bd1206bc7e0eabbf8434 Mon Sep 17 00:00:00 2001 From: Enrique Marroquin Date: Fri, 26 Jul 2019 17:50:13 -0400 Subject: [PATCH 02/16] fix conflict --- src/service/resources-impl.js | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/src/service/resources-impl.js b/src/service/resources-impl.js index 5e48172fda84..05e634311274 100644 --- a/src/service/resources-impl.js +++ b/src/service/resources-impl.js @@ -876,13 +876,7 @@ export class Resources { } } - /** - * Assigns an owner for the specified element. This means that the resources - * within this element will be managed by the owner and not Resources manager. - * @param {!Element} element - * @param {!AmpElement} owner - * @package - */ + /** @override */ setOwner(element, owner) { Resource.setOwner(element, owner); } From 09a881a798f9fa9ff63d48acbde496c7290f72d8 Mon Sep 17 00:00:00 2001 From: Enrique Marroquin Date: Fri, 26 Jul 2019 18:09:22 -0400 Subject: [PATCH 03/16] fix call --- src/service/resources-impl.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/service/resources-impl.js b/src/service/resources-impl.js index 05e634311274..457f6288bd17 100644 --- a/src/service/resources-impl.js +++ b/src/service/resources-impl.js @@ -434,7 +434,7 @@ export class Resources { // operations themselves are not free and might delay this.eventuallyCallPreconnects_ = debounce( this.win, - this.eventuallyCallPreconnects_.bind(this), + this.callPreconnects_.bind(this), 1 ); @@ -868,7 +868,7 @@ export class Resources { * buffer. * @private */ - eventuallyCallPreconnects_() { + callPreconnects_() { for (let i = 0; i < this.pendingPreconnectResources_.length; i++) { const resource = this.pendingPreconnectResources_[i]; resource.preconnect(); From 9651056a48fdca10eb47baaa6f95eaa812a61303 Mon Sep 17 00:00:00 2001 From: Enrique Marroquin Date: Mon, 29 Jul 2019 10:42:02 -0400 Subject: [PATCH 04/16] types --- src/service/resources-impl.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/service/resources-impl.js b/src/service/resources-impl.js index 457f6288bd17..bdf7418585a1 100644 --- a/src/service/resources-impl.js +++ b/src/service/resources-impl.js @@ -435,7 +435,7 @@ export class Resources { this.eventuallyCallPreconnects_ = debounce( this.win, this.callPreconnects_.bind(this), - 1 + 5 ); /** @private {boolean} */ From ce7723eb847d325d69b62520fe765e5838572449 Mon Sep 17 00:00:00 2001 From: Enrique Marroquin Date: Mon, 29 Jul 2019 16:08:50 -0400 Subject: [PATCH 05/16] change debounce to 1ms, polish comments --- src/custom-element.js | 5 ++++- src/service/resources-impl.js | 6 ++---- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/src/custom-element.js b/src/custom-element.js index 024e1069f73c..a8f8261d50fe 100644 --- a/src/custom-element.js +++ b/src/custom-element.js @@ -582,6 +582,9 @@ function createBaseCustomElementClass(win) { if (onLayout) { this.implementation_.preconnectCallback(onLayout); } else { + // If we do early preconnects we delay them a bit. This is kind of + // an unfortunate trade off, but it seems faster, because the DOM + // operations themselves are not free and might delay this.getResources().requestPreconnect(this); } } @@ -598,7 +601,7 @@ function createBaseCustomElementClass(win) { dev().error(TAG, 'preconnect without defaultView'); return; } - this.implementation_.preconnectCallback(false); + this.implementation_.preconnectCallback(/* opt_onLayout */ false); } /** diff --git a/src/service/resources-impl.js b/src/service/resources-impl.js index bdf7418585a1..6560969526ca 100644 --- a/src/service/resources-impl.js +++ b/src/service/resources-impl.js @@ -429,13 +429,11 @@ export class Resources { /** @private {?Array} */ this.pendingPreconnectResources_ = []; - // If we do early preconnects we delay them a bit. This is kind of - // an unfortunate trade off, but it seems faster, because the DOM - // operations themselves are not free and might delay + /** @private */ this.eventuallyCallPreconnects_ = debounce( this.win, this.callPreconnects_.bind(this), - 5 + 1 ); /** @private {boolean} */ From f8b75433809a2b75c20b0b6f473a727244d0f00b Mon Sep 17 00:00:00 2001 From: Enrique Marroquin Date: Wed, 7 Aug 2019 14:16:36 -0700 Subject: [PATCH 06/16] revert commits --- src/custom-element.js | 38 +++++++++++++++-------------------- src/service/resource.js | 11 ++++------ src/service/resources-impl.js | 34 ------------------------------- 3 files changed, 20 insertions(+), 63 deletions(-) diff --git a/src/custom-element.js b/src/custom-element.js index a8f8261d50fe..2dd2fe7f26d3 100644 --- a/src/custom-element.js +++ b/src/custom-element.js @@ -162,7 +162,7 @@ function createBaseCustomElementClass(win) { */ createdCallback() { // Flag "notbuilt" is removed by Resource manager when the resource is - // considered to be built. See "isBuilt" method. + // considered to be built. See "setBuilt" method. /** @private {boolean} */ this.built_ = false; @@ -535,7 +535,7 @@ function createBaseCustomElementClass(win) { } }).then( () => { - this.maybePreconnect(/* onLayout */ false); + this.preconnect(/* onLayout */ false); this.built_ = true; this.classList.remove('i-amphtml-notbuilt'); this.classList.remove('amp-notbuilt'); @@ -573,35 +573,29 @@ function createBaseCustomElementClass(win) { /** * Called to instruct the element to preconnect to hosts it uses during - * layout. If attempting an early preconnect, it requests to preconnect - * to resources beforehand. + * layout. * @param {boolean} onLayout Whether this was called after a layout. * @this {!Element} */ - maybePreconnect(onLayout) { + preconnect(onLayout) { if (onLayout) { this.implementation_.preconnectCallback(onLayout); } else { // If we do early preconnects we delay them a bit. This is kind of // an unfortunate trade off, but it seems faster, because the DOM // operations themselves are not free and might delay - this.getResources().requestPreconnect(this); - } - } - - /** - * Preconnect to host. - */ - preconnect() { - const TAG = this.tagName; - if (!this.ownerDocument) { - dev().error(TAG, 'preconnect without ownerDocument'); - return; - } else if (!this.ownerDocument.defaultView) { - dev().error(TAG, 'preconnect without defaultView'); - return; + Services.timerFor(toWin(this.ownerDocument.defaultView)).delay(() => { + const TAG = this.tagName; + if (!this.ownerDocument) { + dev().error(TAG, 'preconnect without ownerDocument'); + return; + } else if (!this.ownerDocument.defaultView) { + dev().error(TAG, 'preconnect without defaultView'); + return; + } + this.implementation_.preconnectCallback(onLayout); + }, 1); } - this.implementation_.preconnectCallback(/* opt_onLayout */ false); } /** @@ -1207,7 +1201,7 @@ function createBaseCustomElementClass(win) { } const promise = tryResolve(() => this.implementation_.layoutCallback()); - this.maybePreconnect(/* onLayout */ true); + this.preconnect(/* onLayout */ true); this.classList.add('i-amphtml-layout'); return promise.then( diff --git a/src/service/resource.js b/src/service/resource.js index 2993970fde54..4cdffbfa2ff4 100644 --- a/src/service/resource.js +++ b/src/service/resource.js @@ -14,6 +14,7 @@ * limitations under the License. */ +import {AmpEvents} from '../amp-events'; import {Deferred, tryResolve} from '../utils/promise'; import {Layout} from '../layout'; import {Services} from '../services'; @@ -339,6 +340,9 @@ export class Resource { } // TODO(dvoytenko): merge with the standard BUILT signal. this.element.signals().signal('res-built'); + // TODO(dvoytenko, #7389): cleanup once amp-sticky-ad signals are + // in PROD. + this.element.dispatchCustomEvent(AmpEvents.BUILT); }, reason => { this.maybeReportErrorOnBuildFailure(reason); @@ -1091,11 +1095,4 @@ export class Resource { delete this.element[RESOURCE_PROP_]; this.element.disconnect(/* opt_pretendDisconnected */ true); } - - /** - * Calls element's preconnectCallback. - */ - preconnect() { - this.element.preconnect(); - } } diff --git a/src/service/resources-impl.js b/src/service/resources-impl.js index 6560969526ca..5112030dda31 100644 --- a/src/service/resources-impl.js +++ b/src/service/resources-impl.js @@ -25,7 +25,6 @@ import {VisibilityState} from '../visibility-state'; import {areMarginsChanged, expandLayoutRect} from '../layout-rect'; import {closest, hasNextNodeInDocumentOrder} from '../dom'; import {computedStyle} from '../style'; -import {debounce} from '../utils/rate-limit'; import {dev, devAssert} from '../log'; import {dict, hasOwn} from '../utils/object'; import {getSourceUrl} from '../url'; @@ -426,16 +425,6 @@ export class Resources { /** @private {?Array} */ this.pendingBuildResources_ = []; - /** @private {?Array} */ - this.pendingPreconnectResources_ = []; - - /** @private */ - this.eventuallyCallPreconnects_ = debounce( - this.win, - this.callPreconnects_.bind(this), - 1 - ); - /** @private {boolean} */ this.isCurrentlyBuildingPendingResources_ = false; @@ -851,29 +840,6 @@ export class Resources { dev().fine(TAG_, 'element upgraded:', resource.debugid); } - /** - * Schedule resources for preconnecting. - * @param {!Element} element - */ - requestPreconnect(element) { - const resource = Resource.forElement(element); - this.pendingPreconnectResources_.push(resource); - this.eventuallyCallPreconnects_(); - } - - /** - * Calls resource manager's preconnect and removes resource from the pending - * buffer. - * @private - */ - callPreconnects_() { - for (let i = 0; i < this.pendingPreconnectResources_.length; i++) { - const resource = this.pendingPreconnectResources_[i]; - resource.preconnect(); - this.pendingPreconnectResources_.splice(i, 1); - } - } - /** @override */ setOwner(element, owner) { Resource.setOwner(element, owner); From f6fb13e8f7001ccaa5ae53c18e62544baec26663 Mon Sep 17 00:00:00 2001 From: Enrique Marroquin Date: Wed, 7 Aug 2019 16:28:37 -0700 Subject: [PATCH 07/16] feedback --- src/service/timer-impl.js | 38 ++++++++++++++++++++++++++++++++++++-- 1 file changed, 36 insertions(+), 2 deletions(-) diff --git a/src/service/timer-impl.js b/src/service/timer-impl.js index 7772c9199cc7..6a90e877c228 100644 --- a/src/service/timer-impl.js +++ b/src/service/timer-impl.js @@ -15,6 +15,7 @@ */ import {installServiceInEmbedScope, registerServiceBuilder} from '../service'; +import {map} from '../utils/object'; import {reportError} from '../error'; import {user} from '../log'; @@ -38,6 +39,8 @@ export class Timer { this.canceled_ = {}; + this.delayQueue_ = map(); + /** @const {number} */ this.startTime_ = Date.now(); } @@ -61,10 +64,10 @@ export class Timer { * @return {number|string} */ delay(callback, opt_delay) { + let id = 'p' + this.taskCount_++; if (!opt_delay) { // For a delay of zero, schedule a promise based micro task since // they are predictably fast. - const id = 'p' + this.taskCount_++; this.resolved_ .then(() => { if (this.canceled_[id]) { @@ -78,13 +81,44 @@ export class Timer { } const wrapped = () => { try { + if (this.canceled_[id]) { + delete this.canceled_[id]; + return; + } callback(); } catch (e) { reportError(e); throw e; } }; - return this.win.setTimeout(wrapped, opt_delay); + + const delayEnd = Date.now() + opt_delay; + if (!this.delayQueue_[delayEnd]) { + this.delayQueue_[delayEnd] = []; + id = this.win.setTimeout( + this.callCallbacksForDelay_.bind(this), + opt_delay, + delayEnd + ); + } + + this.delayQueue_[delayEnd].push(wrapped); + return id; + } + + /** + * Calls previously batched callbacks for a given ms key. + * @param {number} delay + * @private + */ + callCallbacksForDelay_(delay) { + const callbacks = this.delayQueue_[delay]; + delete this.delayQueue_[delay]; + + for (let i = 0; i < callbacks.length; i++) { + const cb = callbacks[i]; + cb(); + } } /** From f9c72409c4c62166eb7f0525e221193e33f6f341 Mon Sep 17 00:00:00 2001 From: Enrique Marroquin Date: Wed, 7 Aug 2019 16:47:15 -0700 Subject: [PATCH 08/16] don't return timeout id --- src/service/timer-impl.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/service/timer-impl.js b/src/service/timer-impl.js index 6a90e877c228..6622aff319c8 100644 --- a/src/service/timer-impl.js +++ b/src/service/timer-impl.js @@ -64,7 +64,7 @@ export class Timer { * @return {number|string} */ delay(callback, opt_delay) { - let id = 'p' + this.taskCount_++; + const id = 'p' + this.taskCount_++; if (!opt_delay) { // For a delay of zero, schedule a promise based micro task since // they are predictably fast. @@ -95,7 +95,7 @@ export class Timer { const delayEnd = Date.now() + opt_delay; if (!this.delayQueue_[delayEnd]) { this.delayQueue_[delayEnd] = []; - id = this.win.setTimeout( + this.win.setTimeout( this.callCallbacksForDelay_.bind(this), opt_delay, delayEnd From 599189ca75c1f5d7fded56544780b7eef1e9f78e Mon Sep 17 00:00:00 2001 From: Enrique Marroquin Date: Wed, 7 Aug 2019 16:49:56 -0700 Subject: [PATCH 09/16] fix jsdoc --- src/service/timer-impl.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/service/timer-impl.js b/src/service/timer-impl.js index 6622aff319c8..cf775e24571c 100644 --- a/src/service/timer-impl.js +++ b/src/service/timer-impl.js @@ -61,7 +61,7 @@ export class Timer { * Returns the timer ID that can be used to cancel the timer (cancel method). * @param {function()} callback * @param {number=} opt_delay - * @return {number|string} + * @return {string} */ delay(callback, opt_delay) { const id = 'p' + this.taskCount_++; From 74f65a423ef3c4685928c61a44bb7bf59c260649 Mon Sep 17 00:00:00 2001 From: Enrique Marroquin Date: Wed, 7 Aug 2019 22:49:43 -0700 Subject: [PATCH 10/16] replace delay call with startupchunk --- src/custom-element.js | 5 +++-- src/service/timer-impl.js | 40 +++------------------------------------ 2 files changed, 6 insertions(+), 39 deletions(-) diff --git a/src/custom-element.js b/src/custom-element.js index 2dd2fe7f26d3..5f39cc9d80b9 100644 --- a/src/custom-element.js +++ b/src/custom-element.js @@ -16,6 +16,7 @@ import * as dom from './dom'; import {AmpEvents} from './amp-events'; +import {ChunkPriority, chunk, startupChunk} from './chunk'; import {CommonSignals} from './common-signals'; import {ElementStub} from './element-stub'; import { @@ -584,7 +585,7 @@ function createBaseCustomElementClass(win) { // If we do early preconnects we delay them a bit. This is kind of // an unfortunate trade off, but it seems faster, because the DOM // operations themselves are not free and might delay - Services.timerFor(toWin(this.ownerDocument.defaultView)).delay(() => { + startupChunk(this.ownerDocument, () => { const TAG = this.tagName; if (!this.ownerDocument) { dev().error(TAG, 'preconnect without ownerDocument'); @@ -594,7 +595,7 @@ function createBaseCustomElementClass(win) { return; } this.implementation_.preconnectCallback(onLayout); - }, 1); + }); } } diff --git a/src/service/timer-impl.js b/src/service/timer-impl.js index cf775e24571c..7772c9199cc7 100644 --- a/src/service/timer-impl.js +++ b/src/service/timer-impl.js @@ -15,7 +15,6 @@ */ import {installServiceInEmbedScope, registerServiceBuilder} from '../service'; -import {map} from '../utils/object'; import {reportError} from '../error'; import {user} from '../log'; @@ -39,8 +38,6 @@ export class Timer { this.canceled_ = {}; - this.delayQueue_ = map(); - /** @const {number} */ this.startTime_ = Date.now(); } @@ -61,13 +58,13 @@ export class Timer { * Returns the timer ID that can be used to cancel the timer (cancel method). * @param {function()} callback * @param {number=} opt_delay - * @return {string} + * @return {number|string} */ delay(callback, opt_delay) { - const id = 'p' + this.taskCount_++; if (!opt_delay) { // For a delay of zero, schedule a promise based micro task since // they are predictably fast. + const id = 'p' + this.taskCount_++; this.resolved_ .then(() => { if (this.canceled_[id]) { @@ -81,44 +78,13 @@ export class Timer { } const wrapped = () => { try { - if (this.canceled_[id]) { - delete this.canceled_[id]; - return; - } callback(); } catch (e) { reportError(e); throw e; } }; - - const delayEnd = Date.now() + opt_delay; - if (!this.delayQueue_[delayEnd]) { - this.delayQueue_[delayEnd] = []; - this.win.setTimeout( - this.callCallbacksForDelay_.bind(this), - opt_delay, - delayEnd - ); - } - - this.delayQueue_[delayEnd].push(wrapped); - return id; - } - - /** - * Calls previously batched callbacks for a given ms key. - * @param {number} delay - * @private - */ - callCallbacksForDelay_(delay) { - const callbacks = this.delayQueue_[delay]; - delete this.delayQueue_[delay]; - - for (let i = 0; i < callbacks.length; i++) { - const cb = callbacks[i]; - cb(); - } + return this.win.setTimeout(wrapped, opt_delay); } /** From 22086c45bccfb39b70b42fe743cfb313036ad6ac Mon Sep 17 00:00:00 2001 From: Enrique Marroquin Date: Wed, 7 Aug 2019 22:57:04 -0700 Subject: [PATCH 11/16] revert type changes --- src/custom-element.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/custom-element.js b/src/custom-element.js index 5f39cc9d80b9..b778d85a1f7c 100644 --- a/src/custom-element.js +++ b/src/custom-element.js @@ -16,7 +16,6 @@ import * as dom from './dom'; import {AmpEvents} from './amp-events'; -import {ChunkPriority, chunk, startupChunk} from './chunk'; import {CommonSignals} from './common-signals'; import {ElementStub} from './element-stub'; import { @@ -44,6 +43,7 @@ import {isExperimentOn} from './experiments'; import {parseSizeList} from './size-list'; import {setStyle} from './style'; import {shouldBlockOnConsentByMeta} from '../src/consent'; +import {startupChunk} from './chunk'; import {toWin} from './types'; import {tryResolve} from '../src/utils/promise'; From 14ce7b2e57a7238e3b7505c8589f83e22e5a3cb9 Mon Sep 17 00:00:00 2001 From: Enrique Marroquin Date: Thu, 8 Aug 2019 09:44:00 -0700 Subject: [PATCH 12/16] whitelist startupchunk --- build-system/tasks/presubmit-checks.js | 1 + 1 file changed, 1 insertion(+) diff --git a/build-system/tasks/presubmit-checks.js b/build-system/tasks/presubmit-checks.js index 4adc5048f04b..e0a72465049c 100644 --- a/build-system/tasks/presubmit-checks.js +++ b/build-system/tasks/presubmit-checks.js @@ -631,6 +631,7 @@ const forbiddenTerms = { 'src/chunk.js', 'src/inabox/amp-inabox.js', 'src/runtime.js', + 'src/custom-element.js', ], }, 'AMP_CONFIG': { From 7195051b25436ced894bb0be65b8fcba2d45f8c5 Mon Sep 17 00:00:00 2001 From: Enrique Marroquin Date: Thu, 8 Aug 2019 10:10:05 -0700 Subject: [PATCH 13/16] fix conflicts --- src/service/resource.js | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/service/resource.js b/src/service/resource.js index 4cdffbfa2ff4..6458662a7136 100644 --- a/src/service/resource.js +++ b/src/service/resource.js @@ -14,7 +14,6 @@ * limitations under the License. */ -import {AmpEvents} from '../amp-events'; import {Deferred, tryResolve} from '../utils/promise'; import {Layout} from '../layout'; import {Services} from '../services'; @@ -340,9 +339,6 @@ export class Resource { } // TODO(dvoytenko): merge with the standard BUILT signal. this.element.signals().signal('res-built'); - // TODO(dvoytenko, #7389): cleanup once amp-sticky-ad signals are - // in PROD. - this.element.dispatchCustomEvent(AmpEvents.BUILT); }, reason => { this.maybeReportErrorOnBuildFailure(reason); From 80406424063e8e39f90baa02b04c63b1609d0120 Mon Sep 17 00:00:00 2001 From: Enrique Marroquin Date: Thu, 8 Aug 2019 15:25:33 -0700 Subject: [PATCH 14/16] fix test --- test/unit/test-custom-element.js | 6 ------ 1 file changed, 6 deletions(-) diff --git a/test/unit/test-custom-element.js b/test/unit/test-custom-element.js index d1a371751f38..a3cd4074c27e 100644 --- a/test/unit/test-custom-element.js +++ b/test/unit/test-custom-element.js @@ -734,14 +734,11 @@ describes.realWin('CustomElement', {amp: true}, env => { return element.buildingPromise_.then(() => { expect(element.isBuilt()).to.equal(true); expect(testElementBuildCallback).to.be.calledOnce; - expect(testElementPreconnectCallback).to.have.not.been.called; // Call again. return element.build().then(() => { expect(element.isBuilt()).to.equal(true); expect(testElementBuildCallback).to.be.calledOnce; - expect(testElementPreconnectCallback).to.have.not.been.called; - clock.tick(1); expect(testElementPreconnectCallback).to.be.calledOnce; }); }); @@ -968,9 +965,6 @@ describes.realWin('CustomElement', {amp: true}, env => { return element.build().then(() => { expect(element.isBuilt()).to.equal(true); expect(testElementLayoutCallback).to.have.not.been.called; - clock.tick(1); - expect(testElementPreconnectCallback).to.be.calledOnce; - expect(testElementPreconnectCallback.getCall(0).args[0]).to.be.false; const p = element.layoutCallback(); expect(testElementLayoutCallback).to.be.calledOnce; From 83a917561b908441d1c6eb2f00cff5ee2a690d05 Mon Sep 17 00:00:00 2001 From: Enrique Marroquin Date: Fri, 9 Aug 2019 13:41:54 -0700 Subject: [PATCH 15/16] add fortesting --- test/unit/test-custom-element.js | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/test/unit/test-custom-element.js b/test/unit/test-custom-element.js index a3cd4074c27e..69b235d9b191 100644 --- a/test/unit/test-custom-element.js +++ b/test/unit/test-custom-element.js @@ -22,6 +22,10 @@ import {ElementStub} from '../../src/element-stub'; import {LOADING_ELEMENTS_, Layout} from '../../src/layout'; import {ResourceState} from '../../src/service/resource'; import {Services} from '../../src/services'; +import { + activateChunkingForTesting, + chunkInstanceForTesting, +} from '../../src/chunk'; import {createAmpElementForTesting} from '../../src/custom-element'; describes.realWin('CustomElement', {amp: true}, env => { @@ -142,6 +146,8 @@ describes.realWin('CustomElement', {amp: true}, env => { testElementUnlayoutCallback = sandbox.spy(); testElementPauseCallback = sandbox.spy(); testElementResumeCallback = sandbox.spy(); + + chunkInstanceForTesting(env.ampdoc); }); afterEach(() => { From a6463ff1879926bf16b2a19f46a59b0bbdf70674 Mon Sep 17 00:00:00 2001 From: Enrique Marroquin Date: Fri, 9 Aug 2019 14:07:19 -0700 Subject: [PATCH 16/16] fix test: force macrotask before checking if preconnect was called --- src/custom-element.js | 2 +- test/unit/test-custom-element.js | 18 +++++++++--------- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/src/custom-element.js b/src/custom-element.js index b778d85a1f7c..55c0eb73fad1 100644 --- a/src/custom-element.js +++ b/src/custom-element.js @@ -585,7 +585,7 @@ function createBaseCustomElementClass(win) { // If we do early preconnects we delay them a bit. This is kind of // an unfortunate trade off, but it seems faster, because the DOM // operations themselves are not free and might delay - startupChunk(this.ownerDocument, () => { + startupChunk(self.document, () => { const TAG = this.tagName; if (!this.ownerDocument) { dev().error(TAG, 'preconnect without ownerDocument'); diff --git a/test/unit/test-custom-element.js b/test/unit/test-custom-element.js index 69b235d9b191..2df6d5a3d587 100644 --- a/test/unit/test-custom-element.js +++ b/test/unit/test-custom-element.js @@ -22,10 +22,7 @@ import {ElementStub} from '../../src/element-stub'; import {LOADING_ELEMENTS_, Layout} from '../../src/layout'; import {ResourceState} from '../../src/service/resource'; import {Services} from '../../src/services'; -import { - activateChunkingForTesting, - chunkInstanceForTesting, -} from '../../src/chunk'; +import {chunkInstanceForTesting} from '../../src/chunk'; import {createAmpElementForTesting} from '../../src/custom-element'; describes.realWin('CustomElement', {amp: true}, env => { @@ -119,6 +116,7 @@ describes.realWin('CustomElement', {amp: true}, env => { resourcesMock = sandbox.mock(resources); container = doc.createElement('div'); doc.body.appendChild(container); + chunkInstanceForTesting(env.ampdoc); ElementClass = createAmpElementForTesting(win, 'amp-test', TestElement); StubElementClass = createAmpElementForTesting( @@ -146,8 +144,6 @@ describes.realWin('CustomElement', {amp: true}, env => { testElementUnlayoutCallback = sandbox.spy(); testElementPauseCallback = sandbox.spy(); testElementResumeCallback = sandbox.spy(); - - chunkInstanceForTesting(env.ampdoc); }); afterEach(() => { @@ -745,7 +741,9 @@ describes.realWin('CustomElement', {amp: true}, env => { return element.build().then(() => { expect(element.isBuilt()).to.equal(true); expect(testElementBuildCallback).to.be.calledOnce; - expect(testElementPreconnectCallback).to.be.calledOnce; + setTimeout(() => { + expect(testElementPreconnectCallback).to.be.calledOnce; + }, 0); }); }); }); @@ -974,10 +972,12 @@ describes.realWin('CustomElement', {amp: true}, env => { const p = element.layoutCallback(); expect(testElementLayoutCallback).to.be.calledOnce; - expect(testElementPreconnectCallback).to.have.callCount(2); - expect(testElementPreconnectCallback.getCall(1).args[0]).to.be.true; expect(element.signals().get(CommonSignals.LOAD_START)).to.be.ok; expect(element.signals().get(CommonSignals.LOAD_END)).to.be.null; + setTimeout(() => { + expect(testElementPreconnectCallback).to.have.callCount(2); + expect(testElementPreconnectCallback.getCall(1).args[0]).to.be.true; + }, 0); return p.then(() => { expect(element.readyState).to.equal('complete'); expect(element.signals().get(CommonSignals.LOAD_END)).to.be.ok;