amp-instagram placeholder should not prerender #4700

erwinmombay · 2016-08-24T23:46:03Z

this is temporary until we can hit the image url on the cdn directly without having to go through the www.instagram.com link (which redirects to the cdn url)

at minimum i wish we could use a generic instagram placeholder image like their gray logo or one of their assets (like in https://www.instagram-brand.com/)

cc @rudygalfi @adewale

mkhatib · 2016-08-24T23:50:57Z

extensions/amp-instagram/0.1/amp-instagram.js

-    const image = this.win.document.createElement('amp-img');
-    // This will redirect to the image URL. By experimentation this is
-    // always the same URL that is actually used inside of the embed.
-    image.setAttribute('src', 'https://www.instagram.com/p/' +


Would just calling sanitizer.resolveUrlAttr against this URL allow us to serve it from the CDN instead? cc/ @dvoytenko

Also it looks like using api.instagram.com instead of www.instagram.com works similarly though the documentation only mentions the naked domain.

awesome, this seems fixed and we go through the cache.

erwinmombay · 2016-08-25T00:43:50Z

disregard original statements since it looks like the instagram imaged is proxied

mkhatib · 2016-08-25T00:55:34Z

LGTM

dvoytenko · 2016-08-25T14:54:13Z

extensions/amp-instagram/0.1/amp-instagram.js

@@ -85,8 +86,10 @@ class AmpInstagram extends AMP.BaseElement {
    const image = this.win.document.createElement('amp-img');
    // This will redirect to the image URL. By experimentation this is
    // always the same URL that is actually used inside of the embed.
-    image.setAttribute('src', 'https://www.instagram.com/p/' +
-        encodeURIComponent(this.shortcode_) + '/media/?size=l');
+    const imgSrc = resolveUrlAttr('amp-img', 'src',


In my opinion, we shouldn't proxy an Instagram image. We should either find a way to get it from their cookieless CDN or exclude it from prerendering. The reasons are:

They will most likely immediately throttle any AMP CDN

This prefetch will miss the cache when the actual widget is downloaded, which makes pre-fetch a lot less useful.

so i think last time i mentioned the only way I know of how to get the cdn url is to hit an endpoint:

http://api.instagram.com/oembed?callback=&url=https%3A%2F%2Finstagram.com%2Fp%2FfBwFP

Would that be viable?

response looks like:

{ "provider_url": "https://www.instagram.com", "media_id": "520552783_242517", "author_name": "cramforce", "height": null, "thumbnail_url": "https://instagram.fsnc1-1.fna.fbcdn.net/t51.2885-15/e15/924722_1467037386920481_2122071451_n.jpg?ig_cache_key=NTIwNTUyNzgz.2", "thumbnail_width": 612, "thumbnail_height": 612, "provider_name": "Instagram", "title": "", "html": "<blockquote class=\"instagram-media\" data-instgrm-version=\"7\" style=\" background:#FFF; border:0; border-radius:3px; box-shadow:0 0 1px 0 rgba(0,0,0,0.5),0 1px 10px 0 rgba(0,0,0,0.15); margin: 1px; max-width:658px; padding:0; width:99.375%; width:-webkit-calc(100% - 2px); width:calc(100% - 2px);\"><div style=\"padding:8px;\"> <div style=\" background:#F8F8F8; line-height:0; margin-top:40px; padding:50% 0; text-align:center; width:100%;\"> <div style=\" background:url(data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAACwAAAAsCAMAAAApWqozAAAABGdBTUEAALGPC/xhBQAAAAFzUkdCAK7OHOkAAAAMUExURczMzPf399fX1+bm5mzY9AMAAADiSURBVDjLvZXbEsMgCES5/P8/t9FuRVCRmU73JWlzosgSIIZURCjo/ad+EQJJB4Hv8BFt+IDpQoCx1wjOSBFhh2XssxEIYn3ulI/6MNReE07UIWJEv8UEOWDS88LY97kqyTliJKKtuYBbruAyVh5wOHiXmpi5we58Ek028czwyuQdLKPG1Bkb4NnM+VeAnfHqn1k4+GPT6uGQcvu2h2OVuIf/gWUFyy8OWEpdyZSa3aVCqpVoVvzZZ2VTnn2wU8qzVjDDetO90GSy9mVLqtgYSy231MxrY6I2gGqjrTY0L8fxCxfCBbhWrsYYAAAAAElFTkSuQmCC); display:block; height:44px; margin:0 auto -44px; position:relative; top:-22px; width:44px;\"></div></div><p style=\" color:#c9c8cd; font-family:Arial,sans-serif; font-size:14px; line-height:17px; margin-bottom:0; margin-top:8px; overflow:hidden; padding:8px 0 7px; text-align:center; text-overflow:ellipsis; white-space:nowrap;\"><a href=\"https://www.instagram.com/p/fBwFP/\" style=\" color:#c9c8cd; font-family:Arial,sans-serif; font-size:14px; font-style:normal; font-weight:normal; line-height:17px; text-decoration:none;\" target=\"_blank\">A photo posted by Malte Ubl (@cramforce)</a> on <time style=\" font-family:Arial,sans-serif; font-size:14px; line-height:17px;\" datetime=\"2012-01-07T03:37:39+00:00\">Jan 6, 2012 at 7:37pm PST</time></p></div></blockquote>\n<script async defer src=\"//platform.instagram.com/en_US/embeds.js\"></script>", "width": 658, "version": "1.0", "author_url": "https://www.instagram.com/cramforce", "author_id": 242517, "type": "rich" }

I could see this possible but:

Not sure additional latency justifies it.

Can we actually make requests to this API from AMP? Does it need some sort of a key?

Is this API itself cookie-less? We can, of course, send a uncredentialed CORS request to ensure that it is, but I'm not sure what requirements are there.

There would actually be no extra latency. The current request pattern is

Request to instagram

Redirect to CDN.

I don't think it'll work, though. The API @erwinmombay mentioned is designed for JSONP (no CORS support), then their real API (not the oembed endpoint) requires an access token.

I think for now we can just guard the existing code behind whenFirstVisible promise. Having an instagram in the first viewport is quite uncommon anyway, which makes this essentially a null change.

Ideally we should do it via property. These calls are made often and reading an attribute is pretty slow.

SGTM

On Tue, Aug 30, 2016 at 5:22 PM, Dima Voytenko notifications@github.com
wrote:

In extensions/amp-instagram/0.1/amp-instagram.js
#4700 (comment):

@@ -85,8 +86,10 @@ class AmpInstagram extends AMP.BaseElement {
const image = this.win.document.createElement('amp-img');
// This will redirect to the image URL. By experimentation this is
// always the same URL that is actually used inside of the embed.

image.setAttribute('src', 'https://www.instagram.com/p/' +

encodeURIComponent(this.shortcode_) + '/media/?size=l');

const imgSrc = resolveUrlAttr('amp-img', 'src',

Ideally we should do it via property. These calls are made often and
reading an attribute is pretty slow.

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
https://github.com/ampproject/amphtml/pull/4700/files/1aa0d710d3dccdfc767a4c932c5020abf0660c6e#r76904683,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAFeT7p6vZ5D9vuqPnsNYHIBllEE-hFFks5qlMkygaJpZM4JskC5
.

It should be internal only anyway.

On Tue, Aug 30, 2016 at 5:51 PM, Malte Ubl malte.ubl@gmail.com wrote:

SGTM

On Tue, Aug 30, 2016 at 5:22 PM, Dima Voytenko notifications@github.com
wrote:

In extensions/amp-instagram/0.1/amp-instagram.js
#4700 (comment):

@@ -85,8 +86,10 @@ class AmpInstagram extends AMP.BaseElement {
const image = this.win.document.createElement('amp-img');
// This will redirect to the image URL. By experimentation this is
// always the same URL that is actually used inside of the embed.

image.setAttribute('src', 'https://www.instagram.com/p/' +

encodeURIComponent(this.shortcode_) + '/media/?size=l');

const imgSrc = resolveUrlAttr('amp-img', 'src',

Ideally we should do it via property. These calls are made often and
reading an attribute is pretty slow.

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
https://github.com/ampproject/amphtml/pull/4700/files/1aa0d710d3dccdfc767a4c932c5020abf0660c6e#r76904683,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAFeT7p6vZ5D9vuqPnsNYHIBllEE-hFFks5qlMkygaJpZM4JskC5
.

@dvoytenko does this need to be done in base-element?

you answers offline, this should just be done for amp-img for now

mkhatib · 2016-08-25T20:33:16Z

Un-LGTM to address @dvoytenko comments.

They will most likely immediately throttle any AMP CDN
This prefetch will miss the cache when the actual widget is downloaded, which makes pre-fetch a lot less useful.

These are good points, I guess the quick solution is for now to drop the placeholder as you had before @erwinmombay until we can maybe get in touch with someone from Instagram and see if they have a better way to get the media.

erwinmombay · 2016-09-08T15:09:59Z

@dvoytenko @mkhatib PTAL

erwinmombay · 2016-09-08T16:18:56Z

also had to fix some viewer.html code so i could test prerender correctly

mkhatib · 2016-09-08T18:05:19Z

LGTM

dvoytenko · 2016-09-09T17:10:01Z

builtins/amp-img.js

@@ -40,6 +40,12 @@ export class AmpImg extends BaseElement {
  }

  /** @override */
+  buildCallback() {
+    /** @private @const {boolean} */
+    this.isPrerenderAllowed_ = !this.element.hasAttribute('no-prerender');


Let's call it noprerender since it's our most typical "no" form.

dvoytenko · 2016-09-09T17:11:10Z

LGTM with one request

* remove amp-instagram default placeholder * remove amp-instagram default placeholder * no-prerender -> noprerender

erwinmombay added the NEEDS REVIEW label Aug 24, 2016

erwinmombay assigned mkhatib and dvoytenko Aug 24, 2016

erwinmombay force-pushed the stop-instagram-prerender branch 4 times, most recently from 5d65184 to 9fbb633 Compare August 24, 2016 23:47

mkhatib reviewed Aug 24, 2016
View reviewed changes

erwinmombay force-pushed the stop-instagram-prerender branch 2 times, most recently from 33d6fdb to 1e08477 Compare August 25, 2016 00:38

mkhatib added LGTM and removed NEEDS REVIEW labels Aug 25, 2016

erwinmombay force-pushed the stop-instagram-prerender branch 2 times, most recently from e77c92d to 1aa0d71 Compare August 25, 2016 01:04

erwinmombay changed the title ~~remove amp-instagram default placeholder which hits www.instagram.com~~ amp-instagram placeholder should hit proxy if possible Aug 25, 2016

dvoytenko reviewed Aug 25, 2016
View reviewed changes

mkhatib added NEEDS REVIEW and removed LGTM labels Aug 25, 2016

erwinmombay force-pushed the stop-instagram-prerender branch 7 times, most recently from 620ee14 to 39adf0a Compare September 8, 2016 09:27

mkhatib added LGTM and removed NEEDS REVIEW labels Sep 8, 2016

dvoytenko changed the title ~~amp-instagram placeholder should hit proxy if possible~~ amp-instagram placeholder should not prerender Sep 9, 2016

dvoytenko reviewed Sep 9, 2016
View reviewed changes

erwinmombay force-pushed the stop-instagram-prerender branch from 39adf0a to ee6bf26 Compare September 9, 2016 17:20

erwinmombay added 3 commits September 9, 2016 10:21

remove amp-instagram default placeholder

2dda3c4

remove amp-instagram default placeholder

604714b

no-prerender -> noprerender

ee6bf26

erwinmombay merged commit cfc616d into ampproject:master Sep 11, 2016

dreamofabear pushed a commit to dreamofabear/amphtml that referenced this pull request Sep 16, 2016

amp-instagram placeholder should not prerender (ampproject#4700)

f127869

* remove amp-instagram default placeholder * remove amp-instagram default placeholder * no-prerender -> noprerender

mityaha pushed a commit to ooyala/amphtml that referenced this pull request Nov 30, 2016

amp-instagram placeholder should not prerender (ampproject#4700)

0d2f5ee

* remove amp-instagram default placeholder * remove amp-instagram default placeholder * no-prerender -> noprerender

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

amp-instagram placeholder should not prerender #4700

amp-instagram placeholder should not prerender #4700

erwinmombay commented Aug 24, 2016

mkhatib Aug 24, 2016

mkhatib Aug 24, 2016

erwinmombay Aug 25, 2016

erwinmombay commented Aug 25, 2016

mkhatib commented Aug 25, 2016

dvoytenko Aug 25, 2016

erwinmombay Aug 25, 2016

erwinmombay Aug 25, 2016

dvoytenko Aug 25, 2016

cramforce Aug 30, 2016

dvoytenko Aug 31, 2016

cramforce Aug 31, 2016

cramforce Aug 31, 2016

erwinmombay Sep 7, 2016

erwinmombay Sep 7, 2016

mkhatib commented Aug 25, 2016

erwinmombay commented Sep 8, 2016

erwinmombay commented Sep 8, 2016

mkhatib commented Sep 8, 2016

dvoytenko Sep 9, 2016

erwinmombay Sep 9, 2016

dvoytenko commented Sep 9, 2016

amp-instagram placeholder should not prerender #4700

amp-instagram placeholder should not prerender #4700

Conversation

erwinmombay commented Aug 24, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erwinmombay commented Aug 25, 2016

mkhatib commented Aug 25, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkhatib commented Aug 25, 2016

erwinmombay commented Sep 8, 2016

erwinmombay commented Sep 8, 2016

mkhatib commented Sep 8, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dvoytenko commented Sep 9, 2016