add content type check to deflater #259

Merged
merged 2 commits into from Dec 30, 2012

Projects

None yet

5 participants

@bartuer
Contributor
bartuer commented Oct 24, 2011

try to make the checking as fast as possible, only filter most frequent types

@manveru manveru commented on an outdated diff Oct 24, 2011
test/spec_deflater.rb
+ 'video/vnd.vivo',
+ 'video/x-flv',
+ 'video/x-la-asf',
+ 'video/x-mng',
+ 'video/x-ms-asf',
+ 'video/x-ms-wm',
+ 'video/x-ms-wmv',
+ 'video/x-ms-wmx',
+ 'video/x-ms-wvx',
+ 'video/x-msvideo',
+ 'video/x-sgi-movie',
+ 'x-conference/x-cooltalk',
+ 'x-world/x-vrml',
+ ]
+
+ content_type_deflate_expection = {
@manveru
manveru Oct 24, 2011 Member

Looks like a typo, should that not be content_type_deflate_expectation?

@manveru manveru and 1 other commented on an outdated diff Oct 24, 2011
test/spec_deflater.rb
@@ -166,4 +166,1300 @@ describe Rack::Deflater do
response[1].should.not.include "Content-Encoding"
response[2].join.should.equal("Hello World!")
end
+
+ should "check Content-Type is suitable for deflate" do
+ content_types = [
@manveru
manveru Oct 24, 2011 Member

Any Reason we cannot use Rack::Mime::MIME_TYPES here?

@bartuer
bartuer Oct 24, 2011 Contributor

oh, I should use this, should I update it, also correct the typo and
resend the pull request?

On Oct 24, 2011, at 5:41 PM, Michael Fellinger wrote:

@@ -166,4 +166,1300 @@ describe Rack::Deflater do
response[1].should.not.include "Content-Encoding"
response[2].join.should.equal("Hello World!")
end
+

  • should "check Content-Type is suitable for deflate" do
  • content_types = [

Any Reason we cannot use Rack::Mime::MIME_TYPES here?

Reply to this email directly or view it on GitHub:
https://github.com/rack/rack/pull/259/files#r185977

@bartuer
bartuer Oct 24, 2011 Contributor

just count Rack::Mime::MIME_TYPES, it include 604 MIME types, but content_types include 641 types, I can not say it include all, hope to cover more test cases, this larger list just for test, if Rack::Mime::MIME_TYPES is enough, I will only test against it.

@bartuer bartuer deflate type check against Rack::Mime::MIME_TYPES
- Rack::Mime::MIME_TYPES
  - now has 604 "file type" => "mime" maps
  - these 604 maps include 535 MIME types
  - all MIME types should deflate or not list in content_type_deflate_expectation
- fix typo
  content_type_deflate_expection -> content_type_deflate_expectation
1b3ed3f
@raggi
Member
raggi commented Nov 3, 2012

I'm not too sure I want to maintain this, in that I suspect it'll open up for a number of patches as people want to add to the list of checked mimes.

Is there a demonstrably significant benefit to this?

@spastorino
Member

@bartuer can you point us to a demonstrably significant benefit to this?

@bartuer
Contributor
bartuer commented Dec 29, 2012

Hi

It has no significant benefit, only for developer's convenience, otherwise,
one need write code check MIMES missed in rack's list.

On Sat, Dec 29, 2012 at 11:01 PM, Santiago Pastorino <
notifications@github.com> wrote:

@bartuer https://github.com/bartuer can you point us to a demonstrably
significant benefit to this?


Reply to this email directly or view it on GitHubhttps://github.com/rack/rack/pull/259#issuecomment-11753783.

Please avoid sending me Word or PowerPoint
请勿向本人发送Word和PowerPoint附件

http://www.gnu.org/philosophy/no-word-attachments.html


Stay hungry. Stay foolish.

                                     \  ^__^
O       __O       ,               \ (oo)\____
'Z.    _-\<,_   ~~>-^O~~      (__)\       )\/\
/>    (_)/ (_)      7                       ||----w |
                                                 ||       ||
@raggi raggi merged commit ec5634c into rack:master Dec 30, 2012
@raggi
Member
raggi commented Dec 30, 2012

This code is being used more and more since Heroku shut down their external file serving, so it's probably worth it for those folks.

Thanks!

@raggi raggi added a commit that referenced this pull request Dec 30, 2012
@raggi raggi Revert "Merge pull request #259 from bartuer/master"
This reverts commit ec5634c, reversing
changes made to c23edf4.
c479354
@raggi
Member
raggi commented Dec 30, 2012

Er, correction, reverted due to failure, and very large (unreadable) failure message. Lets get this test written a more manageable way.

@bpinto
bpinto commented Jan 25, 2013

image/svg+xml should be compressed svg => svgz

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment