Changeset 66849 in webkit


Ignore:
Timestamp:
Sep 6, 2010 2:09:21 PM (14 years ago)
Author:
abarth@webkit.org
Message:

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:
Location:
trunk/BugsSite
Files:
3 edited

Legend:

Unmodified
Added
Removed
  • trunk/BugsSite/ChangeLog

    r66838 r66849  
     12010-09-06  Adam Barth  <abarth@webkit.org>
     2
     3        Reviewed by Eric Seidel.
     4
     5        [reviewtool] Add a field for overall comments
     6        https://bugs.webkit.org/show_bug.cgi?id=45273
     7
     8        This patch does a couple logically separate things that could be
     9        separated into smaller patches:
     10
     11        1) This patch adds an "overall comments" field where you can enter
     12           overall comments about the patch.  These comments appear at the top
     13           of the bugzilla posting.  Currently, these aren't redisplayed when
     14           viewing the patch, but I plan to add that in a future patch.
     15
     16        2) This patch renames some of the CSS classes to more consistently
     17           follow the camelCase style that PrettyPatch uses.
     18
     19        3) This patch moves the "prepare comments" button to the left of the
     20           toolbar and renames is to "publish comments".  This makes more sense
     21           when you scroll to the bottom of the page and enter in some overall
     22           comments.
     23
     24        4) When you attempt to add a comment to a line that already has a
     25           "frozen" comment, we now unfreeze the comment instead of doing
     26           nothing.  The old behavior was kind of frustrating if you didn't
     27           know that you could unfreeze a comment by clicking on it.
     28
     29        * PrettyPatch/PrettyPatch.rb:
     30            - Update CSS.
     31        * code-review.js:
     32
    1332010-09-06  Adam Barth  <abarth@webkit.org>
    234
  • trunk/BugsSite/PrettyPatch/PrettyPatch.rb

    r66553 r66849  
    189189/* Support for inline comments */
    190190
    191 .previous_comment {
    192   border: inset 1px;
    193   padding: 5px;
    194   background-color: #ffd;
    195 }
    196 
    197191.author {
    198192  font-style: italic;
     
    207201}
    208202
    209 .comment textarea {
     203.comment textarea, .overallComments textarea {
    210204  width: 100%;
    211205  height: 6em;
     
    227221
    228222#toolbar .actions {
     223  float: left;
     224}
     225
     226#toolbar .message {
    229227  float: right;
    230228}
     
    263261}
    264262
    265 .help {
    266  color: gray;
    267  font-style: italic;
    268 }
    269 
    270263.commentContext .lineNumber {
    271264  background-color: yellow;
     
    276269  border-bottom-color: #69F;
    277270  border-right-color: #69F;
     271}
     272
     273.help {
     274 color: gray;
     275 font-style: italic;
     276}
     277
     278.description {
     279  font-style: italic;
     280}
     281
     282.comment, .overallComments, .previousComment, .frozenComment {
     283  background-color: #ffd;
     284}
     285
     286.overallComments {
     287  padding: 5px;
     288}
     289
     290.previousComment, .frozenComment {
     291  border: inset 1px; padding: 5px;
    278292}
    279293</style>
  • trunk/BugsSite/code-review.js

    r66838 r66849  
    4646  function findCommentPositionFor(line) {
    4747    var position = line;
    48     while (position.next() && position.next().hasClass('previous_comment'))
     48    while (position.next() && position.next().hasClass('previousComment'))
    4949      position = position.next();
    5050    return position;
     
    6363
    6464  function addCommentFor(line) {
    65     if (line.attr('data-has-comment'))
    66       return;
     65    if (line.attr('data-has-comment')) {
     66      // FIXME: This query is overly complex because we place comment blocks
     67      // after Lines.  Instead, comment blocks should be children of Lines.
     68      findCommentPositionFor(line).next().next().filter('.frozenComment').each(unfreezeComment);
     69      return;
     70    }
    6771    line.attr('data-has-comment', 'true');
    6872    line.addClass('commentContext');
     
    8387
    8488  function addPreviousComment(line, author, comment_text) {
    85     var comment_block = $('<div data-comment-for="' + line.attr('id') + '" class="previous_comment"></div>');
     89    var comment_block = $('<div data-comment-for="' + line.attr('id') + '" class="previousComment"></div>');
    8690    var author_block = $('<div class="author"></div>').text(author + ':');
    8791    comment_block.text(comment_text).prepend(author_block).each(hoverify).click(addCommentField);
     
    181185    crawlDiff();
    182186    fetchHistory();
    183     $(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>');
     187    $(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>');
    184188    $(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>');
     189    $(document.body).append('<div class="overallComments"><div class="description">Overall comments:</div><textarea></textarea></div>');
    185190  });
    186191
     
    193198  }
    194199
     200  function unfreezeComment() {
     201    $(this).prev().show();
     202    $(this).remove();
     203  }
     204
    195205  $('.comment .cancel').live('click', cancelComment);
    196206
     
    203213    var line_id = comment_textarea.attr('data-comment-for');
    204214    var line = $('#' + line_id)
    205     findCommentBlockFor(line).hide().after($('<div class="frozen-comment"></div>').text(comment_textarea.val()));
    206   });
    207 
    208   $('.frozen-comment').live('click', function() {
    209     $(this).prev().show();
    210     $(this).remove();
    211   });
     215    findCommentBlockFor(line).hide().after($('<div class="frozenComment"></div>').text(comment_textarea.val()));
     216  });
     217
     218  $('.frozenComment').live('click', unfreezeComment);
    212219
    213220  function focusOn(comment) {
     
    219226
    220227  function focusNextComment() {
    221     var comments = $('.previous_comment');
     228    var comments = $('.previousComment');
    222229    if (comments.length == 0)
    223230      return;
     
    228235
    229236  function focusPreviousComment() {
    230     var comments = $('.previous_comment');
     237    var comments = $('.previousComment');
    231238    if (comments.length == 0)
    232239      return;
     
    347354        return;
    348355      var snippet = snippetFor(line);
    349       var comment = findCommentBlockFor(line).children('textarea').val();
     356      var comment = findCommentBlockFor(line).children('textarea').val().trim();
    350357      if (comment == '')
    351358        return;
     
    353360    });
    354361    $('#comment_form').removeClass('inactive');
    355     var comment = comments_in_context.join('\n\n');
     362    var comment = $('.overallComments textarea').val().trim();
     363    if (comment != '')
     364      comment += '\n\n';
     365    comment += comments_in_context.join('\n\n');
    356366    if (comment != '')
    357367      comment = 'View in context: ' + window.location + '\n\n' + comment;
Note: See TracChangeset for help on using the changeset viewer.