Design Review 2019-06-12 15:30 UTC (wg-runtime, Cloudinary extension, localStorage for experiments)

[mrjoro]

Time: 2019-06-12 15:30 UTC (add to Google Calendar)
Location: Video conference via Google Meet

The AMP Project holds weekly engineering design reviews. We encourage everyone in the community to participate in these design reviews.

If you are interested in bringing your design to design review, read the design review documentation and add a link to your design doc by the Monday before your design review.

When attending a design review please read through the designs before the design review starts. This allows us to spend more time on discussion of the design.

We rotate our design review between times that work better for different parts of the world as described in our design review documentation, but you are welcome to attend any design review. If you cannot make any of the design reviews but have a design to discuss please let mrjoro@ know on Slack and we will find a time that works for you.

[mrjoro]

The Runtime Working Group will be presenting their periodic update at this Design Review (10-15 minutes).

/cc @jridgewell @ampproject/wg-runtime

[lannka]

I'd like to discuss issues about the now and future of vendor integration in AMP. slides

[Note: we did not get to this topic this week.]

[nitzanj]

Hi all,

We'd like to bring the Cloudinary AMP extension to this review,

These are the main points that came up on the I2I:

• Pre-rendering/caching issues
• srcset/sizes vs run-time dimension querying
• lightbox support (can we perhaps base our extension on amp-img and have everything play along nicely)

Note: There's already a WIP PR with working code for those who want to take a better look.

Thanks,
Nitzan

[dreamofabear]

If time allows, I'd like to review a change for AMP experiments to use localStorage instead of cookies (#22796).

[jridgewell]

wg-runtime's quarterly status update is at ampproject/wg-runtime#12

[mrjoro]

wg-runtime update

• scrolling: there's a path to ship on Safari without extra hack?
    - no, started to flicker when we had more complex pages
    - still some bug in Safari GPU rendering
    - Safari 13 on iOS switching to a completely different codepath will require some changes; need to invest in AMP working here (they completely swapped out scrolling implementation)
    - is iPad implementation different than iPhone?  some things, @cramforce thinks scrolling implementation is the same; currently no one that we know of supports embedding in iPad

[mrjoro]

Cloudinary AMP extension

• pre-rendering/caching
    - @colinbendell: optimization in Google AMP Cache ends up being bigger/lower quality because they were already optimized, no way to signal "don't optimize this"; Rory was going to look into this
    - this tag is intentionally trying to avoid the AMP Cache to avoid this issue; this sacrifices the pre-rendering case
    - Cloudinary customers use them to manage life-cycle plus delivery optimization (based on criteria the user has given)
    - there are some proposals on Google AMP Cache to solve this issue
• @aghassemi: is this the primary purpose of this extension?  if the Google AMP Cache problem is solved would the need for this extension go away?
    - Colin: this is a big part of it, but also since Cloudinary has an understanding of where the focus of the attention is it can handle overflow/etc.
    - @cramforce: do you also already know the dimensions of the eventual image?  always use JS?  Colin: outside of AMP there are a number of different strategies used by customers, e.g. client hints, backend-side
      - @kristoferbaxter: would prefer as much as possible to move all of this to the server; JS isn't performant enough for this; but there are some cases where this isn't feasible; a compromise where some things are set to preload + all of the images on the page being amp-img for now
        - a lot of things depend on images being amp-img, e.g. lightbox; if we want to remove these constraints we should remove these for all images
        - Colin: this is an alternate/enhanced experience, there's a combinatorial explosion of dprs, can't really be derived beforehand without being very verbose; if Google AMP Cache can support variants to address this, that would help with this case
        - @kristoferbaxter: recommendation would be to add the tradeoffs/alternatives that achieve the same set of results to the I2I so we can do a full evaluation of them for not just these documents but other documents
    - don't need srcset because you're planning on knowing the size of the target image by the time the request is made?  Colin: yes, bypassing pre-renderer allows the request to include the dimensions
• @sparhami: we'd still like to include srcset so we can handle things like lightbox zoom
    - Colin: you don't just want srcset here, you need art direction because the crop may be different
    - @sparhami: non-cropped image is preferred so that we can do transforms/animations; this isn't critical but is nice-to-have; we'd disable lightbox animation otherwise; Colin: you could use the original image and then blend it into the new image, a little weird but there are some clever things you could do
    - Colin: part of the value Cloudinary brings is they have an understanding of the content of the image--it's not just the center of the image, the focus may move around, e.g. a kid throwing leaves in the air the focus shifts based on portrait mode/etc.
    - @sparhami: could still use a client-side extension to provide these hints?
• @cramforce: the extension as is probably fine; we should strongly consider having a pre-render mode (using Google AMP Cache when there), but we should fix the Google AMP Cache should support no-transform
    - complication in web package based deployments; we need to do pure link rel preload-based loading of images, which requires image selection to be JS free
    - Colin: another complication is it's cross domain; Cloudinary customer's contents refer to images on a different domain; wouldn't necessarily be part of the web package itself; @cramforce this isn't a problem since the resources aren't part of the package, so they can come from arbitrary places (only in unbundled signed exchanges); non-JS selection happens based on srcset
• @sparhami: is it just the image dimensions?
    - Colin: dimensions/intrinsic dimensions, dpr (not just multiplication but also viewing distance)
    - @sparhami: could it be done in a generic way where amp-img adds some extra query params; could be useful to caches/other people; Colin: absolutely, this is what client hints is intended to do (provides headers: viewport and actual container dimensions, accept header), if Google AMP Cache supports surrogate variants it would help here
    - @sparhami: if you have a different set of client hints does it break caching?  Colin: Chrome's disk cache can include information on variations ("this image varies based on dpr"), code allows inspecting the cache for other possible variants, looks for nearest neighbor (WebKit only looks for exact hints)
    - @kristoferbaxter: another proposal is amp-cloudinary is a proto ascii that says client hints must be enabled for the base document so image requests from the document always contain that document (and then "polyfill"/"poorly fill" this for devices that don't support it); include in the I2I the enforcement of client hints on the doc; Colin: ultimately the JS implementation is doing this; @kristoferbaxter: benefit of this approach is for browsers that support it (Chrome)
• @sparhami: can we have a shorter term solution while we wait for changes to Google AMP Cache?  build extension now, then eventually transform them to amp-img
    - @kristoferbaxter: we're doing something similar with experiments now; as long as the API design allows us to do a simple transform in the future this is a possibility
    - Colin: sounds like we're philosophically aligned here
• @aghassemi: look at amp-img js to see how we're measuring/etc.
• update the I2I:
    - alternatives considered, list all possible alternatives that solve the same set of constraints
    - using amp-img vs. using img directly in component
    - the preload thing that @cramforce mentioned earlier
    - mention in I2I that the goal is to get to the point where we remove the JS as much as possible, design should support this

[mrjoro]

localStorage for AMP experiments

• @sparhami: when you opt in using local storage, how would it serve the right v0.js?  this is client-experiments, not binary
• @jridgewell: we could make it an http cookie, which makes it unsettable by JS (set in a response header)
    - @kristoferbaxter: agreed, localStorage is synchronous
    - we typically have a couple of dozen experiments at a time; would be a decently sized refactor to make them async
    - doesn't completely eliminate cookie tossing
    - we can't use only an http cookie because we have to read it in JS
    - format would be the same as it is in the cookie (comma separated list in a string)
• what about publisher origin pages?
    - can use dev console, but annoying for mobile devices
    - @choumx looking into whether bookmarklets would work
    - looking into the ability to copy/paste a viewer URL into experiment page so you don't have to figure out the cache URL
• @aghassemi: can we include this in the Chrome extension? (the one with the validator in it)
• @kristoferbaxter: take advantage of new error messaging stuff that points to amp.dev with more in-depth explanations (runtime message in dev console)

[cramforce]

Comment on sync localStorage:

We should definitely save all experiments as a single localStorage key. We can use the (future) async API and just make sure that no call that reads experiments happens before we once waited for the promise to resolve. We already always read from localStorage I think. Since (if any) only the first read is slow, this should not regress us.