Amp Ad Fast Fetch allow amp-image prefetch #11391
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jonkeller
approved these changes
Sep 22, 2017
| a4a.onLayoutMeasure(); | ||
| return a4a.layoutCallback().then(() => { | ||
| expect(doc.querySelector('link[rel=preload]' + | ||
| '[href="https://prefetch.me.com?a=b"]')).to.be.ok; |
There was a problem hiding this comment.
String continuation onto next line should be indented 4 spaces, correct? (I suspect this is a case of the indentation being wrong but the file is omitted by the linter)
There was a problem hiding this comment.
It passes gulp lint and I am fairly positive it is enforced on test files.
jonkeller
reviewed
Sep 22, 2017
extensions/amp-a4a/0.1/amp-a4a.js
Outdated
| @@ -106,7 +106,8 @@ export let SizeInfoDef; | |||
| /** @typedef {{ | |||
| minifiedCreative: string, | |||
| customElementExtensions: !Array<string>, | |||
| customStylesheets: !Array<{href: string}> | |||
| customStylesheets: !Array<{href: string}>, | |||
| ampImages: Array<string>|undefined, | |||
There was a problem hiding this comment.
gulp check-types didn't like this. I suggest:
ampImages: (Array<string>|undefined),
See:
jonkeller
requested changes
Sep 22, 2017
jonkeller
approved these changes
Sep 22, 2017
zhouyx
reviewed
Sep 22, 2017
extensions/amp-a4a/0.1/amp-a4a.js
Outdated
| @@ -738,6 +739,9 @@ export class AmpA4A extends AMP.BaseElement { | |||
| // Preload any fonts. | |||
| (creativeMetaDataDef.customStylesheets || []).forEach(font => | |||
| this.preconnect.preload(font.href)); | |||
| // Preload any AMP images. | |||
| (creativeMetaDataDef.ampImages || []).forEach(image => | |||
| isSecureUrl(image) && this.preconnect.preload(image)); | |||
There was a problem hiding this comment.
LGTM. @dvoytenko to confirm, we are ok to preload an array of images?
There was a problem hiding this comment.
Yeah. We discussed. Should be ok as long as this list is not super long.
jridgewell
reviewed
Oct 2, 2017
| @@ -733,6 +734,9 @@ export class AmpA4A extends AMP.BaseElement { | |||
| // Preload any fonts. | |||
| (creativeMetaDataDef.customStylesheets || []).forEach(font => | |||
| this.preconnect.preload(font.href)); | |||
| // Preload any AMP images. | |||
| (creativeMetaDataDef.images || []).forEach(image => | |||
| isSecureUrl(image) && this.preconnect.preload(image)); | |||
There was a problem hiding this comment.
Who controls the origin of these images? How do we know we're not preconneting to a tracking service?
| throw new Error(errorMsg); | ||
| } | ||
| }); | ||
| } | ||
| if (isArray(metaDataObj['images'])) { | ||
| // Load maximum of 5 images. | ||
| metaData.images = metaDataObj['images'].splice(0, 5); |
There was a problem hiding this comment.
This does not match the comment! #slice is not #splice.
There was a problem hiding this comment.
Actually, this will work since #splice returns the removed elements. But still, not the canonical use, #slice would be better here.
cramforce
reviewed
Oct 3, 2017
| @@ -733,6 +734,9 @@ export class AmpA4A extends AMP.BaseElement { | |||
| // Preload any fonts. | |||
| (creativeMetaDataDef.customStylesheets || []).forEach(font => | |||
| this.preconnect.preload(font.href)); | |||
| // Preload any AMP images. | |||
| (creativeMetaDataDef.images || []).forEach(image => | |||
| isSecureUrl(image) && this.preconnect.preload(image)); | |||
There was a problem hiding this comment.
This should pass in an as value. Otherwise it'll be downloaded 2x in Safari.
dmvjs
pushed a commit
to dmvjs/amphtml
that referenced
this pull request
Jan 31, 2018
* Amp Ad Fast Fetch allow amp-image prefetch * type check fix * feedback from @dvoytenko
dmvjs
added a commit
to dmvjs/amphtml
that referenced
this pull request
Feb 6, 2018
…o jr/amp-addthis * 'jr/amp-addthis' of https://github.com/dmvjs/amphtml: (54 commits) Add required logic for view/engagement analytics without an iframe Lojson call refactor; saving WIP Code review cleanup Make all images in AMP have the `async` attribute. (ampproject#11366) skip test (ampproject#11459) Fix having multiple Criteo standalone ads on one page (ampproject#11285) add `bt` (binary type) query param to error reporting query string (ampproject#11452) Enables vendor iframe to send response to creative (ampproject#11306) Remove noisy 3p error for user error reporting (ampproject#11382) Ramp Fast Fetch crypto verifier experiment to 10% (ampproject#11447) Updates doubleclick impl to include docs on fluid (ampproject#11437) Revert "Add a visual diff test for amp-pinterest" (ampproject#11442) Ramp experiment down to 0 (ampproject#11441) Updated changelog with recent validator release (ampproject#11440) Remove pfx parameter from Doubleclick Fast Fetch reuests (ampproject#11439) Allow element to accept custom share attributes (with fallback); remove other ownerDocument refs Add wait code for amp.article.html (ampproject#11429) fix inabox-viewport test (ampproject#11428) Remove camelcase from data attributes Amp Ad Fast Fetch allow amp-image prefetch (ampproject#11391) ...
dmvjs
added a commit
to dmvjs/amphtml
that referenced
this pull request
Feb 6, 2018
…o jr/amp-addthis * 'jr/amp-addthis' of https://github.com/dmvjs/amphtml: (54 commits) Add required logic for view/engagement analytics without an iframe Lojson call refactor; saving WIP Code review cleanup Make all images in AMP have the `async` attribute. (ampproject#11366) skip test (ampproject#11459) Fix having multiple Criteo standalone ads on one page (ampproject#11285) add `bt` (binary type) query param to error reporting query string (ampproject#11452) Enables vendor iframe to send response to creative (ampproject#11306) Remove noisy 3p error for user error reporting (ampproject#11382) Ramp Fast Fetch crypto verifier experiment to 10% (ampproject#11447) Updates doubleclick impl to include docs on fluid (ampproject#11437) Revert "Add a visual diff test for amp-pinterest" (ampproject#11442) Ramp experiment down to 0 (ampproject#11441) Updated changelog with recent validator release (ampproject#11440) Remove pfx parameter from Doubleclick Fast Fetch reuests (ampproject#11439) Allow element to accept custom share attributes (with fallback); remove other ownerDocument refs Add wait code for amp.article.html (ampproject#11429) fix inabox-viewport test (ampproject#11428) Remove camelcase from data attributes Amp Ad Fast Fetch allow amp-image prefetch (ampproject#11391) ...
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Allow for prefetching amp-image src resources after AMP creative validation but in advance of layoutCallback in order to reduce time to render/load for down page slots.
cc/ @ampproject/a4a