Skip GZip generation depend on specific file parameters

[iggant]

Allow to skip generation gzip file for specific file

If you specific preprocessor for images

    app.config.assets.configure do |env|
      env.register_preprocessor 'image/png', :image_optim, ImageOptim::ImageOptimProcessor
    end

then sprocket generate gzip file for image which doesn't make sence because png file was already optimized

when we add skip_gzip: true in the processor return and sproket will skip gzip generation

[rails-bot]

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @schneems (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[rails-bot]

Warning

• Pull requests are usually filed against the master branch for this repo, but this one is against 3.x. Please double check that you specified the right target!

[rafaelfranca]

Does generating the gzip for this example cause any issue? Or does it just an optimization to avoid unnecessary work?

[iggant]

Browsers can't open this gzip image. and if we have them in our assets directory, apache/nginx/CDN will serve them instead of normal image.
Also in many cases gz version of image file is bigger (even for 43K images)

[schneems]

Sprockets shouldn't be compressing a PNG it explicitly checks a field in the mine type to see if it is text data before zipping. I'm on mobile right now but I even put a comment on the line that does the check. So a good question would be how did that mime type get modified in your example? Can you give me a minimal example app that reproduces the problem?

[iggant]

@schneems

I have created new rails application to show this
https://github.com/iggant/image_sprocket_preprocessor

actually in new application add in file (config/initializers/assets.rb)

  class ImageOptimProcessor
    def self.call(input)
      { data: input[:data] }
    end
  end

  Rails.application.config.assets.configure do |env|
    env.register_preprocessor 'image/png', :image_optim, ImageOptimProcessor
  end

then after (before add some png files to assets/images)
RAILS_ENV=production bundle exec rake assets:precompile

output:

I, [2016-01-18T13:02:14.001103 #45813]  INFO -- : Writing /......./image_sprocket_preprocessor/public/assets/img-stamp-yes-62743315bfbef289c1bb6e41067dfe8e32a3abedc5d18a1081c72c989bc064eb.png
I, [2016-01-18T13:02:14.007210 #45813]  INFO -- : Writing /......./image_sprocket_preprocessor/public/assets/img-stamp-yes-62743315bfbef289c1bb6e41067dfe8e32a3abedc5d18a1081c72c989bc064eb.png.gz

probably you are refering to this comment in method can_compress?(mime_types)

        # The "charset" of a mime type is present if the value is
        # encoded text. We can check this value to see if the asset
        # can be compressed.
        #
        # SVG images are text but do not have
        # a charset defined, this is special cased.
        @charset || @content_type == "image/svg+xml".freeze

there is no charset for images, because it doesn't have any preprocessor (loader.rb), but when we add some it will add charset as ascii-8bit

[schneems]

Here's the problem line:

sprockets/lib/sprockets/loader.rb

Line 164 in 65504dd

charset: source.encoding.name.downcase,

.

The charset is assigned automatically based on the source encoding in the text that was read from the file which in this case is ascii-8bit. This is incorrect. Unfortunately due to the direction of the merge you can't manually tell it to do the right thing by specifying a charset. I think that's the correct way to move forwards, to check and see if the result had an explicit charset returned. Unfortunately i'm pretty sure that's going to blow other stuff up, it's just a question of if we can fix that somewhere else or not.

[schneems]

Here's my proposed solution. Need to add tests for this new behavior but it looks like it won't break anything which is good.

[schneems]

Link: #224

[schneems]

Can you comment on #224?