New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PageSpeed's cache lookup needs to become Vary:Accept aware, at least for webp. #846

Closed
GoogleCodeExporter opened this Issue Apr 6, 2015 · 7 comments

Comments

Projects
None yet
1 participant
@GoogleCodeExporter
Copy link

GoogleCodeExporter commented Apr 6, 2015

Currently PageSpeed does not incorporate any request-header fields into its 
HTTP cache keys.  But when we address Issue 817 (trigger webp transcoding 
accept:*webp*, we'll need to do that.  Otherwise once we have an http cache 
entry for a .pagespeed.*.webp file we'll serve it to any client that doesn't 
accept webp.

Original issue reported on code.google.com by jmara...@google.com on 27 Nov 2013 at 4:30

@GoogleCodeExporter

This comment has been minimized.

Copy link

GoogleCodeExporter commented Apr 6, 2015

Thoughts on implementation:

Our new strategy (see Issue 817) for avoiding serving of webp to browsers that 
can't display it is to rely on browsers that *do* support it sending an Accept 
header that includes the substring "image/webp".  This is superior to comparing 
the User-Agent to a browser whitelist for a few reasons:
  1. we don't have to rely on keeping a browser whitelist up to date
  2. cache fragmentation for proxy caches (CDNs) due to Vary:Accept is annoying but
     not disastrous compared to Vary:User-Agent.

However, it would be nice, since PageSpeed only cares about webp or not, if we 
could just add that one bit (webp or not) to our HTTP Cache key, rather than 
including the entire Accept header in the lookup.  This might require some 
non-obvious layering over the HTTPCache class so we can deploy the 
functionality cleanly in a way that can be extended to more bits of Accept if 
there's something in the future we care about other than webp.

Original comment by jmara...@google.com on 27 Nov 2013 at 11:17

@GoogleCodeExporter

This comment has been minimized.

Copy link

GoogleCodeExporter commented Apr 6, 2015

Note that our internal caches already do contain a specific "supports webp" 
bit.  It's currently set off user-agent, but there's no reason it couldn't be 
set from accept: as well.

The trickier bit will be getting the vary: header correct in all cases if we 
permit older user-agents to fetch webp without the accept: header.  So if we 
accept webp, we vary: accept, otherwise we vary: user-agent.  Sadly, I think we 
need to do that.

Original comment by jmaes...@google.com on 2 Dec 2013 at 1:45

@GoogleCodeExporter

This comment has been minimized.

Copy link

GoogleCodeExporter commented Apr 6, 2015

I think the only old UA we might want to send WEBPs to is the Android Browser.  
We don't care that much about old Chrome since to a first proximation zero 
people use it, as Chrome auto-updates itself.

This discussion should really be in Issue 817.

Original comment by jmara...@google.com on 2 Dec 2013 at 2:12

@GoogleCodeExporter

This comment has been minimized.

Copy link

GoogleCodeExporter commented Apr 6, 2015

Yeah, the discussion of what browsers need ongoing support belongs in 817.  
This is about the tech details of handling vary: accept.  One other work item 
specific to this particular issue is being consistent about what files we serve 
on first and repeat view when a webp url is requested *without* accept: webp.

Original comment by jmaes...@google.com on 2 Dec 2013 at 2:59

@GoogleCodeExporter

This comment has been minimized.

Copy link

GoogleCodeExporter commented Apr 6, 2015

Original comment by igrigo...@google.com on 3 Dec 2013 at 4:52

@GoogleCodeExporter

This comment has been minimized.

Copy link

GoogleCodeExporter commented Apr 6, 2015

This issue was closed by revision r3681.

Original comment by jmara...@google.com on 13 Dec 2013 at 10:06

  • Changed state: Fixed
@GoogleCodeExporter

This comment has been minimized.

Copy link

GoogleCodeExporter commented Apr 6, 2015

Original comment by jmara...@google.com on 13 Dec 2013 at 10:07

  • Added labels: Milestone-v31, release-note
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment