1074514 - Investigate why we do ?modified=crushed

Status: RESOLVED WORKSFORME

(Marketplace Graveyard :: Consumer Pages, defect)

Description

[Christopher Van Wiemeersch [:cvan]]

1. Load https://marketplace.firefox.com/
2. Load the Network tab of the Developer Tools
3. Notice lots of icons have `?modified=crushed`

https://www.dropbox.com/s/grqc8as21rcrjll/Screenshot%202014-09-29%2015.23.24.png?dl=0

I realise this likely means the icon was pngcrushed, but why are we not cachebusting with a hash of the file contents?

Comment 1

[Mathieu Pillard [:mat]]

"crushed" is a value that I set manually on all images when I ran the script to manually crush all images a while ago. Why ? Let me explain:

- We only have one field where we store the suffix hash, but multiple icons sizes per webapp. For this reason, and for simplicity, we use the hash of the original icon image. All icons sizes for a given webapp share the same hash.
- Back then, we used to not crush the images by default. So a hash had been generated for all existing webapp icons, from the source file.
- When we added crushing I wrote a script to crush all existing icons. But since the original icon had not changed, re-generating the hash was pointless - it'd end up being the same.
- So I decided to use a special suffix, and that suffix is "crushed".

New webapps, or webapps that have changed their icon since, don't have this and simply use the icon hash.

TL:DR : It's fine.

Comment 2

[Christopher Van Wiemeersch [:cvan]]

So what happens when the icon changes on the server but since the filename hasn't changed (and the response has a far-future `Expires` header), we keep getting a 304?

Comment 3

[Mathieu Pillard [:mat]]

If the icon changes the hash changes with it when we regenerate the icon sizes. Unless the icon changes outside our code, obviously. That's why old webapps that have since changed their icon don't have ?modified=crushed anymore but instead ?modified={hash}.

Comment 4

[Christopher Van Wiemeersch [:cvan]]

Oh, okay - understood. That's pretty confusing. But I assume you want to leave it as is. If so, feel free to close this bug.