Improve performance of the rendering stack by freezing formats as a sign that...

Improve performance of the rendering stack by freezing formats as a sign that they shouldn't be further modified.

actionmailer/lib/action_mailer/base.rb

@@ -605,6 +605,8 @@ def collect_responses_and_parts_order(headers) #:nodoc:
         templates_name = headers.delete(:template_name) || action_name
 
         each_template(templates_path, templates_name) do |template|
+          self.formats = template.formats
+
           responses << {
             :body => render(:template => template),
             :content_type => template.mime_type.to_s

actionmailer/lib/action_mailer/collector.rb

@@ -11,7 +11,6 @@ def initialize(context, &block)
       @context = context
       @responses = []
       @default_render = block
-      @default_formats = context.formats
     end
 
     def any(*args, &block)
@@ -21,16 +20,12 @@ def any(*args, &block)
     end
     alias :all :any
 
-    def custom(mime, options={}, &block)
+    def custom(mime, options={})
       options.reverse_merge!(:content_type => mime.to_s)
       @context.formats = [mime.to_sym]
-      options[:body] = if block
-        block.call
-      else
-        @default_render.call
-      end
+      @context.formats.freeze
+      options[:body] = block_given? ? yield : @default_render.call
       @responses << options
-      @context.formats = @default_formats
     end
   end
 end
\ No newline at end of file

actionmailer/lib/action_mailer/old_api.rb

@@ -204,6 +204,7 @@ def create_parts
         @parts.unshift create_inline_part(@body)
       elsif @parts.empty? || @parts.all? { |p| p.content_disposition =~ /^attachment/ }
         lookup_context.find_all(@template, @mailer_name).each do |template|
+          self.formats = template.formats
           @parts << create_inline_part(render(:template => template), template.mime_type)
         end
 

actionmailer/test/base_test.rb

@@ -74,13 +74,20 @@ def explicit_multipart_with_any(hash = {})
       end
     end
 
-    def custom_block(include_html=false)
+    def explicit_multipart_with_options(include_html = false)
       mail do |format|
         format.text(:content_transfer_encoding => "base64"){ render "welcome" }
         format.html{ render "welcome" } if include_html
       end
     end
 
+    def explicit_multipart_with_one_template(hash = {})
+      mail(hash) do |format|
+        format.html
+        format.text
+      end
+    end
+
     def implicit_different_template(template_name='')
       mail(:template_name => template_name)
     end
@@ -148,6 +155,13 @@ def different_layout(layout_name='')
     assert_equal("Hello there", email.body.encoded)
   end
 
+  test "should set template content type if mail has only one part" do
+    mail = BaseMailer.html_only
+    assert_equal('text/html', mail.mime_type)
+    mail = BaseMailer.plain_text_only
+    assert_equal('text/plain', mail.mime_type)
+  end
+
   # Custom headers
   test "custom headers" do
     email = BaseMailer.welcome
@@ -162,7 +176,7 @@ def different_layout(layout_name='')
     assert_equal('1234@mikel.me.com', mail['In-Reply-To'].decoded)
   end
 
-  test "can pass random headers in as a hash" do
+  test "can pass random headers in as a hash to headers" do
     hash = {'X-Special-Domain-Specific-Header' => "SecretValue",
             'In-Reply-To' => '1234@mikel.me.com' }
     mail = BaseMailer.welcome_with_headers(hash)
@@ -366,6 +380,11 @@ def different_layout(layout_name='')
     assert_equal("HTML Explicit Multipart", email.parts[1].body.encoded)
   end
 
+  test "explicit multipart have a boundary" do
+    mail = BaseMailer.explicit_multipart
+    assert_not_nil(mail.content_type_parameters[:boundary])
+  end
+
   test "explicit multipart does not sort order" do
     order = ["text/html", "text/plain"]
     with_default BaseMailer, :parts_order => order do
@@ -399,7 +418,7 @@ def different_layout(layout_name='')
     assert_equal("TEXT Explicit Multipart Templates", email.parts[1].body.encoded)
   end
 
-  test "explicit multipart with any" do
+  test "explicit multipart with format.any" do
     email = BaseMailer.explicit_multipart_with_any
     assert_equal(2, email.parts.size)
     assert_equal("multipart/alternative", email.mime_type)
@@ -409,8 +428,8 @@ def different_layout(layout_name='')
     assert_equal("Format with any!", email.parts[1].body.encoded)
   end
 
-  test "explicit multipart with options" do
-    email = BaseMailer.custom_block(true)
+  test "explicit multipart with format(Hash)" do
+    email = BaseMailer.explicit_multipart_with_options(true)
     email.ready_to_send!
     assert_equal(2, email.parts.size)
     assert_equal("multipart/alternative", email.mime_type)
@@ -420,28 +439,23 @@ def different_layout(layout_name='')
     assert_equal("7bit", email.parts[1].content_transfer_encoding)
   end
 
-  test "explicit multipart should be multipart" do
-    mail = BaseMailer.explicit_multipart
-    assert_not_nil(mail.content_type_parameters[:boundary])
-  end
-
-  test "should set a content type if only has an html part" do
-    mail = BaseMailer.html_only
-    assert_equal('text/html', mail.mime_type)
-  end
-  
-  test "should set a content type if only has an plain text part" do
-    mail = BaseMailer.plain_text_only
-    assert_equal('text/plain', mail.mime_type)
-  end
-
-  test "explicit multipart with one part is rendered as body" do
-    email = BaseMailer.custom_block
+  test "explicit multipart with one part is rendered as body and options are merged" do
+    email = BaseMailer.explicit_multipart_with_options
     assert_equal(0, email.parts.size)
     assert_equal("text/plain", email.mime_type)
     assert_equal("base64", email.content_transfer_encoding)
   end
 
+  test "explicit multipart with one template has the expected format" do
+    email = BaseMailer.explicit_multipart_with_one_template
+    assert_equal(2, email.parts.size)
+    assert_equal("multipart/alternative", email.mime_type)
+    assert_equal("text/html", email.parts[0].mime_type)
+    assert_equal("[:html]", email.parts[0].body.encoded)
+    assert_equal("text/plain", email.parts[1].mime_type)
+    assert_equal("[:text]", email.parts[1].body.encoded)
+  end
+
   # Class level API with method missing
   test "should respond to action methods" do
     assert BaseMailer.respond_to?(:welcome)

actionmailer/test/fixtures/base_mailer/explicit_multipart_with_one_template.erb

+<%= self.formats.inspect %>
\ No newline at end of file

actionmailer/test/old_base/mail_layout_test.rb

@@ -69,47 +69,25 @@ def test_should_pickup_default_layout
 
   def test_should_pickup_multipart_layout
     mail = AutoLayoutMailer.multipart
-    # CHANGED: content_type returns an object
-    # assert_equal "multipart/alternative", mail.content_type
     assert_equal "multipart/alternative", mail.mime_type
     assert_equal 2, mail.parts.size
 
-    # CHANGED: content_type returns an object
-    # assert_equal 'text/plain', mail.parts.first.content_type
     assert_equal 'text/plain', mail.parts.first.mime_type
-
-    # CHANGED: body returns an object
-    # assert_equal "text/plain layout - text/plain multipart", mail.parts.first.body
     assert_equal "text/plain layout - text/plain multipart", mail.parts.first.body.to_s
 
-    # CHANGED: content_type returns an object
-    # assert_equal 'text/html', mail.parts.last.content_type
     assert_equal 'text/html', mail.parts.last.mime_type
-
-    # CHANGED: body returns an object
-    # assert_equal "Hello from layout text/html multipart", mail.parts.last.body
     assert_equal "Hello from layout text/html multipart", mail.parts.last.body.to_s
   end
 
   def test_should_pickup_multipartmixed_layout
     mail = AutoLayoutMailer.multipart("multipart/mixed")
-    # CHANGED: content_type returns an object
-    # assert_equal "multipart/mixed", mail.content_type
     assert_equal "multipart/mixed", mail.mime_type
     assert_equal 2, mail.parts.size
 
-    # CHANGED: content_type returns an object
-    # assert_equal 'text/plain', mail.parts.first.content_type
     assert_equal 'text/plain', mail.parts.first.mime_type
-    # CHANGED: body returns an object
-    # assert_equal "text/plain layout - text/plain multipart", mail.parts.first.body
     assert_equal "text/plain layout - text/plain multipart", mail.parts.first.body.to_s
 
-    # CHANGED: content_type returns an object
-    # assert_equal 'text/html', mail.parts.last.content_type
     assert_equal 'text/html', mail.parts.last.mime_type
-    # CHANGED: body returns an object
-    # assert_equal "Hello from layout text/html multipart", mail.parts.last.body
     assert_equal "Hello from layout text/html multipart", mail.parts.last.body.to_s
   end
 

actionpack/lib/action_controller/metal/mime_responds.rb

@@ -263,6 +263,7 @@ def retrieve_response_from_mimes(mimes=nil, &block)
       if format = request.negotiate_mime(collector.order)
         self.content_type ||= format.to_s
         self.formats = [format.to_sym]
+        self.formats.freeze
         collector.response_for(format)
       else
         head :not_acceptable

actionpack/lib/action_view/helpers/prototype_helper.rb

@@ -148,11 +148,9 @@ def remote_function(options)
       class JavaScriptGenerator #:nodoc:
         def initialize(context, &block) #:nodoc:
           @context, @lines = context, []
-          @context.update_details(:formats => [:js, :html]) do
-            include_helpers_from_context
-            @context.with_output_buffer(@lines) do
-              @context.instance_exec(self, &block)
-            end
+          include_helpers_from_context
+          @context.with_output_buffer(@lines) do
+            @context.instance_exec(self, &block)
           end
         end
 

actionpack/lib/action_view/lookup_context.rb

-require 'active_support/core_ext/object/try'
+require 'active_support/core_ext/array/wrap'
 require 'active_support/core_ext/object/blank'
 
 module ActionView
@@ -15,21 +15,23 @@ class LookupContext #:nodoc:
 
     def self.register_detail(name, options = {}, &block)
       self.registered_details << name
-      Setters.send :define_method, :"_#{name}_defaults", &block
-      Setters.module_eval <<-METHOD, __FILE__, __LINE__ + 1
-        def #{name}=(value)
-          value = Array(value.presence || _#{name}_defaults)
+      Accessors.send :define_method, :"_#{name}_defaults", &block
+      Accessors.module_eval <<-METHOD, __FILE__, __LINE__ + 1
+        def #{name}
+          @details[:#{name}]
+        end
 
-          unless value == @details[:#{name}]
-            @details_key, @details = nil, @details.merge(:#{name} => value)
-            @details.freeze
-          end
+        def #{name}=(value)
+          value = Array.wrap(value.presence || _#{name}_defaults)
+          @details_key = nil unless value == @details[:#{name}]
+          # Always set the value to handle frozen arrays
+          @details[:#{name}] = value
         end
       METHOD
     end
 
-    # Holds raw setters for the registered details.
-    module Setters #:nodoc:
+    # Holds accessors for the registered details.
+    module Accessors #:nodoc:
     end
 
     register_detail(:formats) { Mime::SET.symbols }
@@ -52,9 +54,9 @@ def initialize
     end
 
     def initialize(view_paths, details = {})
-      @details, @details_key = {}, nil
+      @details, @details_key = { :handlers => default_handlers }, nil
       self.view_paths = view_paths
-      self.details = details
+      self.update_details(details, true)
     end
 
     module ViewPaths
@@ -97,9 +99,7 @@ def with_fallbacks
 
       def args_for_lookup(name, prefix, partial) #:nodoc:
         name, prefix = normalize_name(name, prefix)
-        details_key  = self.details_key
-        details      = self.details.merge(:handlers => default_handlers)
-        [name, prefix, partial || false, details, details_key]
+        [name, prefix, partial || false, @details, details_key]
       end
 
       # Support legacy foo.erb names even though we now ignore .erb
@@ -121,48 +121,44 @@ def handlers_regexp #:nodoc:
     end
 
     module Details
-      attr_reader :details
-
-      def details=(given_details)
-        registered_details.each { |key| send(:"#{key}=", given_details[key]) }
-      end
-
-      def details_key
+      # Calculate the details key. Remove the handlers from calculation to improve performance
+      # since the user cannot modify it explicitly.
+      def details_key #:nodoc:
         @details_key ||= DetailsKey.get(@details)
       end
 
-      # Shortcut to read formats from details.
-      def formats
-        @details[:formats].compact
-      end
-
       # Overload formats= to reject [:"*/*"] values.
       def formats=(value)
-        value = nil if value == [:"*/*"]
+        value = nil    if value == [:"*/*"]
+        value << :html if value == [:js]
         super(value)
       end
 
-      # Shortcut to read locale.
+      # Overload locale to return a symbol instead of array
       def locale
-        I18n.locale
+        @details[:locale].first
       end
 
       # Overload locale= to also set the I18n.locale. If the current I18n.config object responds
       # to i18n_config, it means that it's has a copy of the original I18n configuration and it's
       # acting as proxy, which we need to skip.
       def locale=(value)
-        value = value.first if value.is_a?(Array)
-        config = I18n.config.respond_to?(:i18n_config) ? I18n.config.i18n_config : I18n.config
-        config.locale = value if value
+        if value
+          config = I18n.config.respond_to?(:i18n_config) ? I18n.config.i18n_config : I18n.config
+          config.locale = value
+        end
         super(I18n.locale)
       end
 
       # Update the details keys by merging the given hash into the current
       # details hash. If a block is given, the details are modified just during
       # the execution of the block and reverted to the previous value after.
-      def update_details(new_details)
-        old_details  = @details
-        self.details = old_details.merge(new_details)
+      def update_details(new_details, force=false)
+        old_details = @details.dup
+
+        registered_details.each do |key|
+          send(:"#{key}=", new_details[key]) if force || new_details.key?(key)
+        end
 
         if block_given?
           begin
@@ -174,7 +170,7 @@ def update_details(new_details)
       end
     end
 
-    include Setters
+    include Accessors
     include Details
     include ViewPaths
   end

actionpack/lib/action_view/render/layouts.rb

@@ -51,7 +51,7 @@ def find_layout(layout)
         if formats.size == 1
           _find_layout(layout)
         else
-          update_details(:formats => self.formats[0,1]){ _find_layout(layout) }
+          update_details(:formats => self.formats.first){ _find_layout(layout) }
         end
       rescue ActionView::MissingTemplate => e
         update_details(:formats => nil) do

actionpack/lib/action_view/render/rendering.rb

@@ -21,7 +21,7 @@ def render(options = {}, locals = {}, &block)
           _render_partial(options)
         else
           template = _determine_template(options)
-          self.formats = template.formats
+          _freeze_formats(template.formats)
           _render_template(template, options[:layout], options)
         end
       when :update
@@ -58,12 +58,18 @@ def _render_template(template, layout = nil, options = {}) #:nodoc:
         content = template.render(self, locals) { |*name| _layout_for(*name) }
         @_content_for[:layout] = content
 
-        if layout
-          content  = _render_layout(layout, locals)
-        end
-
+        content = _render_layout(layout, locals) if layout
         content
       end
     end
+
+    # Freeze the current formats in the lookup context. By freezing them, you are guaranteeing
+    # that next template lookups are not going to modify the formats. The controller can also
+    # use this, to ensure that formats won't be further modified (as it does in respond_to blocks).
+    def _freeze_formats(formats) #:nodoc:
+      return if self.formats.frozen?
+      self.formats = formats
+      self.formats.freeze
+    end
   end
 end

actionpack/lib/action_view/template.rb

 # encoding: utf-8
 # This is so that templates compiled in this file are UTF-8
-
-require 'set'
+require 'active_support/core_ext/array/wrap'
+require 'active_support/core_ext/object/blank'
 
 module ActionView
   class Template
@@ -26,12 +26,8 @@ def initialize(source, identifier, handler, details)
       @virtual_path = details[:virtual_path]
       @method_names = {}
 
-      format    = details[:format]
-      format  ||= handler.default_format.to_sym if handler.respond_to?(:default_format)
-      format  ||= :html
-
-      @formats  = [format.to_sym]
-      @formats << :html if @formats.first == :js
+      format   = details[:format] || :html
+      @formats = Array.wrap(format).map(&:to_sym)
     end
 
     def render(view, locals, &block)

actionpack/lib/action_view/template/resolver.rb

 require "pathname"
 require "active_support/core_ext/class"
-require "active_support/core_ext/array/wrap"
 require "action_view/template"
 
 module ActionView
@@ -57,7 +56,7 @@ def to_s
 
     def find_templates(name, prefix, partial, details)
       path = build_path(name, prefix, partial, details)
-      query(path, EXTENSION_ORDER.map { |ext| details[ext] })
+      query(path, EXTENSION_ORDER.map { |ext| details[ext] }, details[:formats])
     end
 
     def build_path(name, prefix, partial, details)
@@ -67,7 +66,7 @@ def build_path(name, prefix, partial, details)
       path
     end
 
-    def query(path, exts)
+    def query(path, exts, formats)
       query = File.join(@path, path)
 
       exts.each do |ext|
@@ -75,18 +74,24 @@ def query(path, exts)
       end
 
       Dir[query].reject { |p| File.directory?(p) }.map do |p|
-        handler, format = extract_handler_and_format(p)
+        handler, format = extract_handler_and_format(p, formats)
         Template.new(File.read(p), File.expand_path(p), handler,
           :virtual_path => path, :format => format)
       end
     end
 
-    def extract_handler_and_format(path)
+    # Extract handler and formats from path. If a format cannot be a found neither
+    # from the path, or the handler, we should return the array of formats given
+    # to the resolver.
+    def extract_handler_and_format(path, default_formats)
       pieces = File.basename(path).split(".")
       pieces.shift
 
-      handler = Template.handler_class_for_extension(pieces.pop)
-      format  = pieces.last && Mime[pieces.last] && pieces.pop.to_sym
+      handler  = Template.handler_class_for_extension(pieces.pop)
+      format   = pieces.last && Mime[pieces.last] && pieces.pop.to_sym
+      format ||= handler.default_format if handler.respond_to?(:default_format)
+      format ||= default_formats
+
       [handler, format]
     end
   end

actionpack/test/controller/new_base/render_rjs_test.rb

@@ -5,7 +5,7 @@ class BasicController < ActionController::Base
     self.view_paths = [ActionView::FixtureResolver.new(
       "render_rjs/basic/index.js.rjs"          => "page[:customer].replace_html render(:partial => 'customer')",
       "render_rjs/basic/index_html.js.rjs"     => "page[:customer].replace_html :partial => 'customer'",
-      "render_rjs/basic/index_no_js.js.rjs"    => "page[:developer].replace_html render(:partial => 'developer')",
+      "render_rjs/basic/index_no_js.js.erb"    => "<%= render(:partial => 'developer') %>",
       "render_rjs/basic/_customer.js.erb"      => "JS Partial",
       "render_rjs/basic/_customer.html.erb"    => "HTML Partial",
       "render_rjs/basic/_developer.html.erb"   => "HTML Partial",
@@ -18,6 +18,12 @@ def index
       render
     end
 
+    def index_respond_to
+      respond_to do |format|
+        format.js { render :action => "index_no_js" }
+      end
+    end
+
     def index_locale
       self.locale = :da
     end
@@ -41,7 +47,12 @@ def teardown
 
     test "rendering a partial in an RJS template should pick the HTML one if no JS is available" do
       get :index_no_js, "format" => "js"
-      assert_response("$(\"developer\").update(\"HTML Partial\");")
+      assert_response("HTML Partial")
+    end
+
+    test "rendering a partial in an RJS template should pick the HTML one if no JS is available on respond_to" do
+      get :index_respond_to, "format" => "js"
+      assert_response("HTML Partial")
     end
 
     test "replacing an element with a partial in an RJS template should pick the HTML template over the JS one" do

actionpack/test/lib/fixture_template.rb

@@ -9,7 +9,7 @@ def initialize(hash = {})
 
   private
 
-    def query(path, exts)
+    def query(path, exts, formats)
       query = Regexp.escape(path)
       exts.each do |ext|
         query << '(' << ext.map {|e| e && Regexp.escape(".#{e}") }.join('|') << '|)'
@@ -17,7 +17,7 @@ def query(path, exts)
 
       templates = []
       @hash.select { |k,v| k =~ /^#{query}$/ }.each do |path, source|
-        handler, format = extract_handler_and_format(path)
+        handler, format = extract_handler_and_format(path, formats)
         templates << Template.new(source, path, handler,
           :virtual_path => path, :format => format)
       end

actionpack/test/template/lookup_context_test.rb

@@ -22,34 +22,35 @@ def teardown
   end
 
   test "normalizes details on initialization" do
-    formats = Mime::SET
-    locale  = [I18n.locale]
-    assert_equal Hash[:formats => formats, :locale => locale], @lookup_context.details
-  end
-
-  test "allows me to set details" do
-    @lookup_context.details = { :formats => [:html], :locale => :pt }
-    assert_equal Hash[:formats => [:html], :locale => [:pt]], @lookup_context.details
+    assert_equal Mime::SET, @lookup_context.formats
+    assert_equal :en, @lookup_context.locale
   end
 
-  test "does not allow details to be modified in place" do
-    assert @lookup_context.details.frozen?
+  test "allows me to update details" do
+    @lookup_context.update_details(:formats => [:html], :locale => :pt)
+    assert_equal [:html], @lookup_context.formats
+    assert_equal :pt, @lookup_context.locale
   end
 
   test "allows me to update an specific detail" do
     @lookup_context.update_details(:locale => :pt)
     assert_equal :pt, I18n.locale
-    formats = Mime::SET
-    locale  = [I18n.locale]
-    assert_equal Hash[:formats => formats, :locale => locale], @lookup_context.details
+    assert_equal :pt, @lookup_context.locale
+  end
+
+  test "allows me to freeze and retrieve frozen formats" do
+    @lookup_context.formats.freeze
+    assert @lookup_context.formats.frozen?
   end
 
   test "allows me to change some details to execute an specific block of code" do
     formats = Mime::SET
     @lookup_context.update_details(:locale => :pt) do
-      assert_equal Hash[:formats => formats, :locale => [:pt]], @lookup_context.details
+      assert_equal formats, @lookup_context.formats
+      assert_equal :pt, @lookup_context.locale
     end
-    assert_equal Hash[:formats => formats, :locale => [:en]], @lookup_context.details
+    assert_equal formats, @lookup_context.formats
+    assert_equal :en, @lookup_context.locale
   end
 
   test "provides getters and setters for formats" do
@@ -62,6 +63,11 @@ def teardown
     assert_equal Mime::SET, @lookup_context.formats
   end
 
+  test "adds :html fallback to :js formats" do
+    @lookup_context.formats = [:js]
+    assert_equal [:js, :html], @lookup_context.formats
+  end
+
   test "provides getters and setters for locale" do
     @lookup_context.locale = :pt
     assert_equal :pt, @lookup_context.locale
@@ -94,6 +100,13 @@ def teardown
     assert_equal "Hey verden", template.source
   end
 
+  test "found templates respects given formats if one cannot be found from template or handler" do
+    ActionView::Template::Handlers::ERB.expects(:default_format).returns(nil)
+    @lookup_context.formats = [:text]
+    template = @lookup_context.find("hello_world", "test")
+    assert_equal [:text], template.formats
+  end
+
   test "adds fallbacks to view paths when required" do
     assert_equal 1, @lookup_context.view_paths.size