Fixed #17552 -- Removed a hack for IE6 and earlier. #2781
Conversation
| ctype = response.get('Content-Type', '').lower() | ||
| if not ctype.startswith("text/") or "javascript" in ctype: | ||
| return response | ||
| patch_vary_headers(response, ('Accept-Encoding',)) |
There was a problem hiding this comment.
Can you explain why it's okay to move patch_vary_headers()? (I know nothing about it and am just reading about it now.)
Also, remove (I think) the failing test.
There was a problem hiding this comment.
Moving patch_vary_headers is a small performance optimization.
If this method returns before testing for the Accept-Encoding header, the response doesn't depend on Accept-Encoding, so it needs not be added to the Vary header. That's why it makes sense to put patch_vary_headers just before testing for Accept-Encoding.
(In fact, if the response already has a Content-Encoding header, a proper implementation willl have tested the Accept-Encoding header and called patch_vary_headers, so this doesn't change caching behavior in practice.)
There was a problem hiding this comment.
I also removed the failing test (well done, pull request testing!)
There was a problem hiding this comment.
Okay, I buy it. How about a more descriptive commit message (at least mention GZipMiddleware)?
Thanks Aaron Cannon for the report and Tim Graham for the review.
|
merged in df09d85. |
Thanks Aaron Cannon for the report.