Bug 1143874: Improve load time of bug comments

r=dkl a=sgreen

3 files changed, 31 insertions, 23 deletions

diff --git a/Bugzilla/BugMail.pm b/Bugzilla/BugMail.pm
index f655c4ae6..bfb57f158 100644
--- a/Bugzilla/BugMail.pm
+++ b/Bugzilla/BugMail.pm
@@ -38,9 +38,6 @@ sub relationships {
     return %relationships;
 }
 
-# This is a bit of a hack, basically keeping the old system()
-# cmd line interface. Should clean this up at some point.
-#
 # args: bug_id, and an optional hash ref which may have keys for:
 # changer, owner, qa, reporter, cc
 # Optional hash contains values of people which will be forced to those
@@ -88,6 +85,8 @@ sub Send {
         @diffs = _get_new_bugmail_fields($bug);
     }
 
+    my $comments = [];
+
     if ($params->{dep_only}) {
         push(@diffs, { field_name => 'bug_status',
                        old        => $params->{changes}->{bug_status}->[0],
@@ -104,14 +103,14 @@ sub Send {
     }
     else {
         push(@diffs, _get_diffs($bug, $end, \%user_cache));
-    }
 
-    my $comments = $bug->comments({ after => $start, to => $end });
-    # Skip empty comments.
-    @$comments = grep { $_->type || $_->body =~ /\S/ } @$comments;
+        $comments = $bug->comments({ after => $start, to => $end });
+        # Skip empty comments.
+        @$comments = grep { $_->type || $_->body =~ /\S/ } @$comments;
 
-    # If no changes have been made, there is no need to process further.
-    return {'sent' => []} unless scalar(@diffs) || scalar(@$comments);
+        # If no changes have been made, there is no need to process further.
+        return {'sent' => []} unless scalar(@diffs) || scalar(@$comments);
+    }
 
     ###########################################################################
     # Start of email filtering code
diff --git a/Bugzilla/Comment.pm b/Bugzilla/Comment.pm
index 3dabe6702..8232f5ac1 100644
--- a/Bugzilla/Comment.pm
+++ b/Bugzilla/Comment.pm
@@ -162,11 +162,15 @@ sub preload {
         my $rows = $dbh->selectall_arrayref(
             "SELECT comment_id, " . $dbh->sql_group_concat('tag', "','") . "
                FROM longdescs_tags
-              WHERE " . $dbh->sql_in('comment_id', \@comment_ids) . "
-              GROUP BY comment_id");
+              WHERE " . $dbh->sql_in('comment_id', \@comment_ids) . ' ' .
+              $dbh->sql_group_by('comment_id'));
         foreach my $row (@$rows) {
             $comment_map{$row->[0]}->{tags} = [ split(/,/, $row->[1]) ];
         }
+        # Also sets the 'tags' attribute for comments which have no entry
+        # in the longdescs_tags table, else calling $comment->tags will
+        # trigger another SQL query again.
+        $comment_map{$_}->{tags} ||= [] foreach @comment_ids;
     }
 }
 
@@ -190,7 +194,8 @@ sub extra_data  { return $_[0]->{'extra_data'} }
 
 sub tags {
     my ($self) = @_;
-    return [] unless Bugzilla->params->{'comment_taggers_group'};
+    state $comment_taggers_group = Bugzilla->params->{'comment_taggers_group'};
+    return [] unless $comment_taggers_group;
     $self->{'tags'} ||= Bugzilla->dbh->selectcol_arrayref(
         "SELECT tag
            FROM longdescs_tags
@@ -202,11 +207,14 @@ sub tags {
 
 sub collapsed {
     my ($self) = @_;
-    return 0 unless Bugzilla->params->{'comment_taggers_group'};
+    state $comment_taggers_group = Bugzilla->params->{'comment_taggers_group'};
+    return 0 unless $comment_taggers_group;
     return $self->{collapsed} if exists $self->{collapsed};
+
+    state $collapsed_comment_tags = Bugzilla->params->{'collapsed_comment_tags'};
     $self->{collapsed} = 0;
     Bugzilla->request_cache->{comment_tags_collapsed}
-            ||= [ split(/\s*,\s*/, Bugzilla->params->{'collapsed_comment_tags'}) ];
+            ||= [ split(/\s*,\s*/, $collapsed_comment_tags) ];
     my @collapsed_tags = @{ Bugzilla->request_cache->{comment_tags_collapsed} };
     foreach my $my_tag (@{ $self->tags }) {
         $my_tag = lc($my_tag);
diff --git a/template/en/default/bug/comments.html.tmpl b/template/en/default/bug/comments.html.tmpl
index 63196a1ce..fdefa9b19 100644
--- a/template/en/default/bug/comments.html.tmpl
+++ b/template/en/default/bug/comments.html.tmpl
@@ -19,6 +19,7 @@
 [% DEFAULT mode = "show" %]
 [% user_cache = template_cache.users %]
 [% markdown_enabled = feature_enabled('jsonrpc') AND user.settings.use_markdown.value == "on" %]
+[% can_edit_comments = bug.check_can_change_field('longdesc', 0, 1) %]
 
 <!-- This auto-sizes the comments and positions the collapse/expand links 
      to the right. -->
@@ -26,9 +27,7 @@
 <tr>
 <td>
 
-[% FOREACH comment = comments %]
-  [% PROCESS a_comment %]
-[% END %]
+[% PROCESS display_comments %]
 
 [% IF mode == "edit" && user.id
       && user.settings.comment_box_position.value == "before_comments" %]
@@ -69,10 +68,11 @@
 [%# Block for individual comments                                            #%]
 [%############################################################################%]
 
-[% BLOCK a_comment %]
-  [% RETURN IF comment.is_private AND NOT (user.is_insider || user.id == comment.author.id) %]
-  [% comment_text = comment.body_full %]
-  [% RETURN IF comment_text == '' AND (comment.work_time - 0) != 0 AND !user.is_timetracker %]
+[% BLOCK display_comments %]
+  [% FOREACH comment = comments %]
+    [% NEXT IF comment.is_private AND NOT (user.is_insider || user.id == comment.author.id) %]
+    [% comment_text = comment.body_full %]
+    [% NEXT IF comment_text == '' AND (comment.work_time - 0) != 0 AND !user.is_timetracker %]
 
     <div id="c[% comment.count %]" class="bz_comment[% " bz_private" IF comment.is_private %]
                 [% " bz_default_collapsed" IF comment.collapsed %]
@@ -92,7 +92,7 @@
             [% IF comment.collapsed %]
               <span class="bz_collapsed_actions">
             [% END %]
-            [% IF bug.check_can_change_field('longdesc', 0, 1) %]
+            [% IF can_edit_comments %]
               [% IF user.can_tag_comments %]
                 [<a href="#"
                     onclick="YAHOO.bugzilla.commentTagging.toggle([% comment.id %], [% comment.count %]);return false">tag</a>]
@@ -125,7 +125,7 @@
           </span>
         [% END %]
 
-        [% IF mode == "edit" && user.is_insider && bug.check_can_change_field('longdesc', 0, 1) %]
+        [% IF mode == "edit" && can_edit_comments && user.is_insider %]
           <div class="bz_private_checkbox">
             <input type="hidden" value="1"
                    name="defined_isprivate_[% comment.id %]">
@@ -214,4 +214,5 @@
 </[% user.use_markdown(comment) ? "div" : "pre" %]>
     [% Hook.process('a_comment-end', 'bug/comments.html.tmpl') %]
     </div>
+  [% END %]
 [% END %]