From c69a0779fb499fb3c8352eede0b5c6d7bb1117d1 Mon Sep 17 00:00:00 2001
From: Douwe Maan <douwe@selenight.nl>
Date: Fri, 21 Apr 2017 13:22:04 -0500
Subject: [PATCH] Address feedback

---
 app/helpers/blob_helper.rb                      | 16 ++++++++++++++++
 app/models/blob.rb                              |  8 +++-----
 app/models/blob_viewer/base.rb                  |  4 ++--
 app/models/blob_viewer/download.rb              |  5 +++--
 app/views/projects/blob/_content.html.haml      |  6 +++---
 app/views/projects/blob/_render_error.html.haml | 13 ++-----------
 6 files changed, 29 insertions(+), 23 deletions(-)

diff --git a/app/helpers/blob_helper.rb b/app/helpers/blob_helper.rb
index 0e369a63c87..472662c4ba9 100644
--- a/app/helpers/blob_helper.rb
+++ b/app/helpers/blob_helper.rb
@@ -224,4 +224,20 @@ module BlobHelper
       "it is stored in LFS"
     end
   end
+
+  def blob_render_error_options(viewer, error)
+    options = []
+
+    if error == :too_large && viewer.can_override_max_size?
+      options << link_to('load it anyway', url_for(params.merge(viewer: viewer.type, override_max_size: true, format: nil)))
+    end
+
+    if viewer.rich? && viewer.blob.rendered_as_text?(override_max_size: true)
+      options << link_to('view the source', '#', class: 'js-blob-viewer-switcher', data: { viewer: 'simple' })
+    end
+
+    options << link_to('download it', blob_raw_url, target: '_blank', rel: 'noopener noreferrer')
+
+    options
+  end
 end
diff --git a/app/models/blob.rb b/app/models/blob.rb
index 1bb9ed03c11..65356f01cc2 100644
--- a/app/models/blob.rb
+++ b/app/models/blob.rb
@@ -107,11 +107,9 @@ class Blob < SimpleDelegator
   end
 
   def rich_viewer_class
-    if invalid_lfs_pointer? || empty?
-      nil
-    else
-      rich_viewers_classes.find { |viewer_class| viewer_class.can_render?(self) }
-    end
+    return if invalid_lfs_pointer? || empty?
+
+    rich_viewers_classes.find { |viewer_class| viewer_class.can_render?(self) }
   end
 
   def simple_viewer
diff --git a/app/models/blob_viewer/base.rb b/app/models/blob_viewer/base.rb
index ce4f129232d..8e6919f1054 100644
--- a/app/models/blob_viewer/base.rb
+++ b/app/models/blob_viewer/base.rb
@@ -2,7 +2,7 @@ module BlobViewer
   class Base
     class_attribute :partial_name, :type, :extensions, :client_side, :text_based, :switcher_icon, :switcher_title, :max_size, :absolute_max_size
 
-    delegate :partial_path, :rich?, :simple?, :client_side?, :text_based?, to: :class
+    delegate :partial_path, :rich?, :simple?, :client_side?, :server_side?, :text_based?, to: :class
 
     attr_reader :blob
 
@@ -26,7 +26,7 @@ module BlobViewer
       client_side
     end
 
-    def server_side?
+    def self.server_side?
       !client_side?
     end
 
diff --git a/app/models/blob_viewer/download.rb b/app/models/blob_viewer/download.rb
index e406cfc05f7..8f293ea6008 100644
--- a/app/models/blob_viewer/download.rb
+++ b/app/models/blob_viewer/download.rb
@@ -1,8 +1,9 @@
 module BlobViewer
   class Download < Base
     include Simple
-    # We pretend the Download viewer is rendered client-side so that it doesn't
-    # attempt to load the entire blob contents.
+    # We treat the Download viewer as if it renders the content client-side,
+    # so that it doesn't attempt to load the entire blob contents and is
+    # rendered synchronously instead of loaded asynchronously.
     include ClientSide
 
     self.partial_name = 'download'
diff --git a/app/views/projects/blob/_content.html.haml b/app/views/projects/blob/_content.html.haml
index 7bbc8627a2b..cd4d85591ae 100644
--- a/app/views/projects/blob/_content.html.haml
+++ b/app/views/projects/blob/_content.html.haml
@@ -1,8 +1,8 @@
 - simple_viewer = blob.simple_viewer
 - rich_viewer = blob.rich_viewer
-- active_viewer = rich_viewer && params[:viewer] != 'simple' ? :rich : :simple
+- rich_viewer_active = rich_viewer && params[:viewer] != 'simple'
 
-= render 'projects/blob/viewer_wrapper', viewer: simple_viewer, hidden: (active_viewer != :simple)
+= render 'projects/blob/viewer_wrapper', viewer: simple_viewer, hidden: rich_viewer_active
 
 - if rich_viewer
-  = render 'projects/blob/viewer_wrapper', viewer: rich_viewer, hidden: (active_viewer != :rich)
+  = render 'projects/blob/viewer_wrapper', viewer: rich_viewer, hidden: !rich_viewer_active
diff --git a/app/views/projects/blob/_render_error.html.haml b/app/views/projects/blob/_render_error.html.haml
index 56bcc2b018c..026b7c95163 100644
--- a/app/views/projects/blob/_render_error.html.haml
+++ b/app/views/projects/blob/_render_error.html.haml
@@ -1,16 +1,7 @@
-- reason = blob_render_error_reason(viewer, error)
-
-- options = []
-- if error == :too_large && viewer.can_override_max_size?
-  - options << link_to('load it anyway', url_for(params.merge(viewer: viewer.type, override_max_size: true, format: nil)))
-- if viewer.rich? && viewer.blob.rendered_as_text?(override_max_size: true)
-  - options << link_to('view the source', '#', class: 'js-blob-viewer-switcher', data: { viewer: 'simple' })
-- options << link_to('download it', blob_raw_url, target: '_blank', rel: 'noopener noreferrer')
-
 .file-content.code
   .nothing-here-block
-    The #{viewer.switcher_title} could not be displayed because #{reason}.
+    The #{viewer.switcher_title} could not be displayed because #{blob_render_error_reason(viewer, error)}.
 
     You can
-    = options.to_sentence(two_words_connector: ' or ', last_word_connector: ', or ').html_safe
+    = blob_render_error_options(viewer, error).to_sentence(two_words_connector: ' or ', last_word_connector: ', or ').html_safe
     instead.
-- 
GitLab