Browse files

Improve performance of the rendering stack by freezing formats as a s…

…ign that they shouldn't be further modified.
  • Loading branch information...
1 parent fbe3565 commit f28d856cece877d1b2f306f54aeb12cce1db1023 @josevalim josevalim committed Mar 19, 2010
View
2 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
View
11 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
View
1 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
View
58 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)
View
1 actionmailer/test/fixtures/base_mailer/explicit_multipart_with_one_template.erb
@@ -0,0 +1 @@
+<%= self.formats.inspect %>
View
22 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
View
1 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
View
8 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
View
72 actionpack/lib/action_view/lookup_context.rb
@@ -1,4 +1,4 @@
-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
View
2 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
View
16 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
View
12 actionpack/lib/action_view/template.rb
@@ -1,7 +1,7 @@
# 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)
View
19 actionpack/lib/action_view/template/resolver.rb
@@ -1,6 +1,5 @@
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,26 +66,32 @@ 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|
query << '{' << ext.map {|e| e && ".#{e}" }.join(',') << ',}'
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
View
15 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
View
4 actionpack/test/lib/fixture_template.rb
@@ -9,15 +9,15 @@ 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('|') << '|)'
end
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
View
43 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

0 comments on commit f28d856

Please sign in to comment.