Move details to lookup_context and make resolvers use the cache key.

5 files changed, 118 insertions, 141 deletions

diff --git a/actionpack/lib/abstract_controller/rendering.rb b/actionpack/lib/abstract_controller/rendering.rb
index 46a457e3f4..77c554fa3f 100644
--- a/actionpack/lib/abstract_controller/rendering.rb
+++ b/actionpack/lib/abstract_controller/rendering.rb
@@ -101,19 +101,9 @@ module AbstractController
       end
 
       options[:template] ||= (options[:action] || action_name).to_s
-
-      details = _normalize_details(options)
-      lookup_context.update_details(details)
       options
     end
 
-    def _normalize_details(options)
-      details = {}
-      details[:formats] = Array(options[:format]) if options[:format]
-      details[:locale]  = Array(options[:locale]) if options[:locale]
-      details
-    end
-
     def _process_options(options)
     end
 
diff --git a/actionpack/lib/action_view/lookup_context.rb b/actionpack/lib/action_view/lookup_context.rb
index e259a78c5c..231882185f 100644
--- a/actionpack/lib/action_view/lookup_context.rb
+++ b/actionpack/lib/action_view/lookup_context.rb
@@ -1,14 +1,30 @@
+require 'active_support/core_ext/object/try'
+
 module ActionView
   # LookupContext is the object responsible to hold all information required to lookup
   # templates, i.e. view paths and details. The LookupContext is also responsible to
   # generate a key, given to view paths, used in the resolver cache lookup. Since
   # this key is generated just once during the request, it speeds up all cache accesses.
   class LookupContext #:nodoc:
-    attr_reader :details, :view_paths
-
     mattr_accessor :fallbacks
     @@fallbacks = [FileSystemResolver.new(""), FileSystemResolver.new("/")]
 
+    mattr_accessor :registered_details
+    self.registered_details = {}
+
+    def self.register_detail(name, options = {})
+      registered_details[name] = lambda do |value|
+        value = (value.blank? || options[:accessible] == false) ?
+          Array(yield) : Array(value)
+        value |= [nil] unless options[:allow_nil] == false
+        value
+      end
+    end
+
+    register_detail(:formats) { Mime::SET.symbols }
+    register_detail(:locale, :accessible => false) { [I18n.locale] }
+    register_detail(:handlers, :accessible => false) { Template::Handlers.extensions }
+
     class DetailsKey #:nodoc:
       attr_reader :details
       alias :eql? :equal?
@@ -22,75 +38,108 @@ module ActionView
       def initialize(details)
         @details, @hash = details, details.hash
       end
+
+      def outdated?(details)
+        @details != details
+      end
     end
 
     def initialize(view_paths, details = {})
-      @details, @details_key = details, nil
       self.view_paths = view_paths
+      self.details = details
+      @details_key = nil
     end
 
-    # Shortcut to read formats from details.
-    def formats
-      @details[:formats]
-    end
-
-    # Shortcut to set formats in details.
-    def formats=(value)
-      self.details = @details.merge(:formats => Array(value))
-    end
+    module ViewPaths
+      attr_reader :view_paths
 
-    # Whenever setting view paths, makes a copy so we can manipulate then in
-    # instance objects as we wish.
-    def view_paths=(paths)
-      @view_paths = ActionView::Base.process_view_paths(paths)
-    end
+      # Whenever setting view paths, makes a copy so we can manipulate then in
+      # instance objects as we wish.
+      def view_paths=(paths)
+        @view_paths = ActionView::Base.process_view_paths(paths)
+      end
 
-    # Setter for details. Everything this method is invoked, we need to nullify
-    # the details key if it changed.
-    def details=(details)
-      @details = details
-      @details_key = nil if @details_key && @details_key.details != details
-    end
+      def find_template(name, prefix = nil, partial = false)
+        key = details_key
+        @view_paths.find(name, key.details, prefix, partial || false, key)
+      end
 
-    def details_key
-      @details_key ||= DetailsKey.get(details) unless details.empty?
-    end
+      def template_exists?(name, prefix = nil, partial = false)
+        key = details_key
+        @view_paths.exists?(name, key.details, prefix, partial || false, key)
+      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  = self.details
-      self.details = old_details.merge(new_details)
-
-      if block_given?
-        begin
-          yield
-        ensure
-          self.details = old_details
+      # Add fallbacks to the view paths. Useful in cases you are rendering a file.
+      def with_fallbacks
+        added_resolvers = 0
+        self.class.fallbacks.each do |resolver|
+          next if view_paths.include?(resolver)
+          view_paths.push(resolver)
+          added_resolvers += 1
         end
+        yield
+      ensure
+        added_resolvers.times { view_paths.pop }
       end
     end
 
-    # Added fallbacks to the view paths. Useful in cases you are rendering a file.
-    def with_fallbacks
-      added_resolvers = 0
-      self.class.fallbacks.each do |resolver|
-        next if view_paths.include?(resolver)
-        view_paths.push(resolver)
-        added_resolvers += 1
+    module Details
+      def details
+        @details = normalize_details(@details)
       end
-      yield
-    ensure
-      added_resolvers.times { view_paths.pop }
-    end
 
-    def find_template(name, prefix = nil, partial = false)
-      @view_paths.find(name, details, prefix, partial || false, details_key)
-    end
+      def details=(new_details)
+        @details = new_details
+        details
+      end
 
-    def template_exists?(name, prefix = nil, partial = false)
-      @view_paths.exists?(name, details, prefix, partial || false, details_key)
+      # TODO This is too expensive. Revisit this.
+      def details_key
+        latest_details = self.details
+        @details_key   = nil if @details_key.try(:outdated?, latest_details)
+        @details_key ||= DetailsKey.get(latest_details)
+      end
+
+      # Shortcut to read formats from details.
+      def formats
+        self.details[:formats]
+      end
+
+      # Shortcut to set formats in details.
+      def formats=(value)
+        self.details = @details.merge(:formats => value)
+      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  = self.details
+        self.details = old_details.merge(new_details)
+
+        if block_given?
+          begin
+            yield
+          ensure
+            self.details = old_details
+          end
+        end
+      end
+
+    protected
+
+      def normalize_details(details)
+        details = details.dup
+        # TODO: Refactor this concern out of the resolver
+        details.delete(:formats) if details[:formats] == [:"*/*"]
+        self.class.registered_details.each do |k, v|
+          details[k] = v.call(details[k])
+        end
+        details
+      end
     end
+
+    include Details
+    include ViewPaths
   end
 end
\ No newline at end of file
diff --git a/actionpack/lib/action_view/paths.rb b/actionpack/lib/action_view/paths.rb
index 628546e443..1205d26e3b 100644
--- a/actionpack/lib/action_view/paths.rb
+++ b/actionpack/lib/action_view/paths.rb
@@ -11,7 +11,7 @@ module ActionView #:nodoc:
 
     def find(path, details = {}, prefix = nil, partial = false, key=nil)
       each do |resolver|
-        if template = resolver.find(path, details, prefix, partial)
+        if template = resolver.find(path, details, prefix, partial, key)
           return template
         end
       end
@@ -21,7 +21,7 @@ module ActionView #:nodoc:
     
     def exists?(path, details = {}, prefix = nil, partial = false, key=nil)
       each do |resolver|
-        if resolver.find(path, details, prefix, partial)
+        if resolver.find(path, details, prefix, partial, key)
           return true
         end
       end
diff --git a/actionpack/lib/action_view/template/resolver.rb b/actionpack/lib/action_view/template/resolver.rb
index fde13a4bdf..9a011f7638 100644
--- a/actionpack/lib/action_view/template/resolver.rb
+++ b/actionpack/lib/action_view/template/resolver.rb
@@ -5,24 +5,9 @@ require "action_view/template"
 
 module ActionView
   class Resolver
-
-    class_inheritable_accessor(:registered_details)
-    self.registered_details = {}
-
-    def self.register_detail(name, options = {})
-      registered_details[name] = lambda do |val|
-        val = Array.wrap(val || yield)
-        val |= [nil] unless options[:allow_nil] == false
-        val
-      end
-    end
-
-    register_detail(:locale)   { [I18n.locale] }
-    register_detail(:formats)  { Mime::SET.symbols }
-    register_detail(:handlers) { Template::Handlers.extensions }
-
     def initialize
-      @cached = {}
+      @cached = Hash.new { |h1,k1| h1[k1] =
+        Hash.new { |h2,k2| h2[k2] = Hash.new { |h3, k3| h3[k3] = {} } } }
     end
 
     def find(*args)
@@ -30,11 +15,10 @@ module ActionView
     end
 
     # Normalizes the arguments and passes it on to find_template.
-    def find_all(name, details = {}, prefix = nil, partial = nil)
-      details = normalize_details(details)
+    def find_all(name, details = {}, prefix = nil, partial = nil, key=nil)
       name, prefix = normalize_name(name, prefix)
 
-      cached([name, details, prefix, partial]) do
+      cached(key, prefix, name, partial) do
         find_templates(name, details, prefix, partial)
       end
     end
@@ -52,16 +36,6 @@ module ActionView
       raise NotImplementedError
     end
 
-    def normalize_details(details)
-      details = details.dup
-      # TODO: Refactor this concern out of the resolver
-      details.delete(:formats) if details[:formats] == [:"*/*"]
-      registered_details.each do |k, v|
-        details[k] = v.call(details[k])
-      end
-      details
-    end
-
     # Support legacy foo.erb names even though we now ignore .erb
     # as well as incorrectly putting part of the path in the template
     # name instead of the prefix.
@@ -73,10 +47,14 @@ module ActionView
       return parts.pop, [prefix, *parts].compact.join("/")
     end
 
-    def cached(key)
-      return yield unless caching?
-      return @cached[key] if @cached.key?(key)
-      @cached[key] = yield
+    def cached(key, prefix, name, partial)
+      return yield unless key && caching?
+      scope = @cached[key][prefix][name]
+      if scope.key?(partial)
+        scope[partial]
+      else
+        scope[partial] = yield
+      end
     end
   end
 
diff --git a/actionpack/test/abstract/render_test.rb b/actionpack/test/abstract/render_test.rb
index e54d056666..25dc8bd804 100644
--- a/actionpack/test/abstract/render_test.rb
+++ b/actionpack/test/abstract/render_test.rb
@@ -16,11 +16,7 @@ module AbstractController
         "renderer/string.erb" => "With String",
         "renderer/symbol.erb" => "With Symbol",
         "string/with_path.erb" => "With String With Path",
-        "some/file.erb" => "With File",
-        "with_format.html.erb" => "With html format",
-        "with_format.xml.erb" => "With xml format",
-        "with_locale.en.erb" => "With en locale",
-        "with_locale.pl.erb" => "With pl locale"
+        "some/file.erb" => "With File"
       )]
 
       def template
@@ -54,22 +50,6 @@ module AbstractController
       def symbol
         render :symbol
       end
-
-      def with_html_format
-        render :template => "with_format", :format => :html
-      end
-
-      def with_xml_format
-        render :template => "with_format", :format => :xml
-      end
-
-      def with_en_locale
-        render :template => "with_locale"
-      end
-
-      def with_pl_locale
-        render :template => "with_locale", :locale => :pl
-      end
     end
 
     class TestRenderer < ActiveSupport::TestCase
@@ -117,26 +97,6 @@ module AbstractController
         @controller.process(:string_with_path)
         assert_equal "With String With Path", @controller.response_body
       end
-
-      def test_render_with_html_format
-        @controller.process(:with_html_format)
-        assert_equal "With html format", @controller.response_body
-      end
-
-      def test_render_with_xml_format
-        @controller.process(:with_xml_format)
-        assert_equal "With xml format", @controller.response_body
-      end
-
-      def test_render_with_en_locale
-        @controller.process(:with_en_locale)
-        assert_equal "With en locale", @controller.response_body
-      end
-
-      def test_render_with_pl_locale
-        @controller.process(:with_pl_locale)
-        assert_equal "With pl locale", @controller.response_body
-      end
     end
   end
 end