2010-09-06 Adam Barth <abarth@webkit.org>
authorabarth@webkit.org <abarth@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 6 Sep 2010 21:09:21 +0000 (21:09 +0000)
committerabarth@webkit.org <abarth@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 6 Sep 2010 21:09:21 +0000 (21:09 +0000)
        Reviewed by Eric Seidel.

        [reviewtool] Add a field for overall comments
        https://bugs.webkit.org/show_bug.cgi?id=45273

        This patch does a couple logically separate things that could be
        separated into smaller patches:

        1) This patch adds an "overall comments" field where you can enter
           overall comments about the patch.  These comments appear at the top
           of the bugzilla posting.  Currently, these aren't redisplayed when
           viewing the patch, but I plan to add that in a future patch.

        2) This patch renames some of the CSS classes to more consistently
           follow the camelCase style that PrettyPatch uses.

        3) This patch moves the "prepare comments" button to the left of the
           toolbar and renames is to "publish comments".  This makes more sense
           when you scroll to the bottom of the page and enter in some overall
           comments.

        4) When you attempt to add a comment to a line that already has a
           "frozen" comment, we now unfreeze the comment instead of doing
           nothing.  The old behavior was kind of frustrating if you didn't
           know that you could unfreeze a comment by clicking on it.

        * PrettyPatch/PrettyPatch.rb:
            - Update CSS.
        * code-review.js:

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@66849 268f45cc-cd09-0410-ab3c-d52691b4dbfc

BugsSite/ChangeLog
BugsSite/PrettyPatch/PrettyPatch.rb
BugsSite/code-review.js

index ea56be1..52d689f 100644 (file)
@@ -1,5 +1,37 @@
 2010-09-06  Adam Barth  <abarth@webkit.org>
 
+        Reviewed by Eric Seidel.
+
+        [reviewtool] Add a field for overall comments
+        https://bugs.webkit.org/show_bug.cgi?id=45273
+
+        This patch does a couple logically separate things that could be
+        separated into smaller patches:
+
+        1) This patch adds an "overall comments" field where you can enter
+           overall comments about the patch.  These comments appear at the top
+           of the bugzilla posting.  Currently, these aren't redisplayed when
+           viewing the patch, but I plan to add that in a future patch.
+
+        2) This patch renames some of the CSS classes to more consistently
+           follow the camelCase style that PrettyPatch uses.
+
+        3) This patch moves the "prepare comments" button to the left of the
+           toolbar and renames is to "publish comments".  This makes more sense
+           when you scroll to the bottom of the page and enter in some overall
+           comments.
+
+        4) When you attempt to add a comment to a line that already has a
+           "frozen" comment, we now unfreeze the comment instead of doing
+           nothing.  The old behavior was kind of frustrating if you didn't
+           know that you could unfreeze a comment by clicking on it.
+
+        * PrettyPatch/PrettyPatch.rb:
+            - Update CSS.
+        * code-review.js:
+
+2010-09-06  Adam Barth  <abarth@webkit.org>
+
         [reviewtool] Tweak the ok button to cancel the comment if the comment
         is empty.  Previously we would get into a bad state where a line had a
         comment but there was no longer any way to access it.
index 27a5843..ffccd39 100644 (file)
@@ -188,12 +188,6 @@ h1 :hover {
 
 /* Support for inline comments */
 
-.previous_comment {
-  border: inset 1px;
-  padding: 5px;
-  background-color: #ffd;
-}
-
 .author {
   font-style: italic;
 }
@@ -206,7 +200,7 @@ h1 :hover {
   position: relative;
 }
 
-.comment textarea {
+.comment textarea, .overallComments textarea {
   width: 100%;
   height: 6em;
 }
@@ -226,6 +220,10 @@ body {
 }
 
 #toolbar .actions {
+  float: left;
+}
+
+#toolbar .message {
   float: right;
 }
 
@@ -262,11 +260,6 @@ body {
   height: 100%;
 }
 
-.help {
- color: gray;
- font-style: italic;
-}
-
 .commentContext .lineNumber {
   background-color: yellow;
 }
@@ -276,6 +269,27 @@ body {
   border-bottom-color: #69F;
   border-right-color: #69F;
 }
+
+.help {
+ color: gray;
+ font-style: italic;
+}
+
+.description {
+  font-style: italic;
+}
+
+.comment, .overallComments, .previousComment, .frozenComment {
+  background-color: #ffd;
+}
+
+.overallComments {
+  padding: 5px;
+}
+
+.previousComment, .frozenComment {
+  border: inset 1px; padding: 5px;
+}
 </style>
 <script src="https://ajax.googleapis.com/ajax/libs/jquery/1.4.2/jquery.min.js"></script> 
 <script src="code-review.js?version=5"></script> 
index b94382f..98ff999 100644 (file)
@@ -45,7 +45,7 @@
 
   function findCommentPositionFor(line) {
     var position = line;
-    while (position.next() && position.next().hasClass('previous_comment'))
+    while (position.next() && position.next().hasClass('previousComment'))
       position = position.next();
     return position;
   }
   }
 
   function addCommentFor(line) {
-    if (line.attr('data-has-comment'))
+    if (line.attr('data-has-comment')) {
+      // FIXME: This query is overly complex because we place comment blocks
+      // after Lines.  Instead, comment blocks should be children of Lines.
+      findCommentPositionFor(line).next().next().filter('.frozenComment').each(unfreezeComment);
       return;
+    }
     line.attr('data-has-comment', 'true');
     line.addClass('commentContext');
 
@@ -82,7 +86,7 @@
   var files = {}
 
   function addPreviousComment(line, author, comment_text) {
-    var comment_block = $('<div data-comment-for="' + line.attr('id') + '" class="previous_comment"></div>');
+    var comment_block = $('<div data-comment-for="' + line.attr('id') + '" class="previousComment"></div>');
     var author_block = $('<div class="author"></div>').text(author + ':');
     comment_block.text(comment_text).prepend(author_block).each(hoverify).click(addCommentField);
     insertCommentFor(line, comment_block);
   $(document).ready(function() {
     crawlDiff();
     fetchHistory();
-    $(document.body).prepend('<div id="toolbar"><div class="actions"><button id="post_comments">Prepare comments</button></div><span class="commentStatus"></span> <span class="help">Double-click a line to add a comment.</span></div>');
+    $(document.body).prepend('<div id="toolbar"><div class="actions"><button id="post_comments">Publish comments</button></div><div class="message"><span class="commentStatus"></span> <span class="help">Double-click a line to add a comment.</span></div></div>');
     $(document.body).prepend('<div id="comment_form" class="inactive"><div class="winter"></div><div class="lightbox"><iframe src="attachment.cgi?id=' + attachment_id + '&action=reviewform"></iframe></div></div>');
+    $(document.body).append('<div class="overallComments"><div class="description">Overall comments:</div><textarea></textarea></div>');
   });
 
   function cancelComment() {
     trimCommentContextToBefore(line);
   }
 
+  function unfreezeComment() {
+    $(this).prev().show();
+    $(this).remove();
+  }
+
   $('.comment .cancel').live('click', cancelComment);
 
   $('.comment .ok').live('click', function() {
     }
     var line_id = comment_textarea.attr('data-comment-for');
     var line = $('#' + line_id)
-    findCommentBlockFor(line).hide().after($('<div class="frozen-comment"></div>').text(comment_textarea.val()));
+    findCommentBlockFor(line).hide().after($('<div class="frozenComment"></div>').text(comment_textarea.val()));
   });
 
-  $('.frozen-comment').live('click', function() {
-    $(this).prev().show();
-    $(this).remove();
-  });
+  $('.frozenComment').live('click', unfreezeComment);
 
   function focusOn(comment) {
     $('.focused').removeClass('focused');
   }
 
   function focusNextComment() {
-    var comments = $('.previous_comment');
+    var comments = $('.previousComment');
     if (comments.length == 0)
       return;
     var index = comments.index($('.focused'));
   }
 
   function focusPreviousComment() {
-    var comments = $('.previous_comment');
+    var comments = $('.previousComment');
     if (comments.length == 0)
       return;
     var index = comments.index($('.focused'));
       if (line.attr('data-has-comment') != 'true')
         return;
       var snippet = snippetFor(line);
-      var comment = findCommentBlockFor(line).children('textarea').val();
+      var comment = findCommentBlockFor(line).children('textarea').val().trim();
       if (comment == '')
         return;
       comments_in_context.push(snippet + '\n' + comment);
     });
     $('#comment_form').removeClass('inactive');
-    var comment = comments_in_context.join('\n\n');
+    var comment = $('.overallComments textarea').val().trim();
+    if (comment != '')
+      comment += '\n\n';
+    comment += comments_in_context.join('\n\n');
     if (comment != '')
       comment = 'View in context: ' + window.location + '\n\n' + comment;
     $('#comment_form').find('iframe').contents().find('#comment').val(comment);