[SCM] WebKit Debian packaging branch, debian/experimental, updated. upstream/1.3.3-9427-gc2be6fc
abarth at webkit.org
abarth at webkit.org
Wed Dec 22 13:05:05 UTC 2010
The following commit has been merged in the debian/experimental branch:
commit 1414c99fb262fe69219c968f1cd50a5355248bf7
Author: abarth at webkit.org <abarth at webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Date: Mon Sep 6 21:09:21 2010 +0000
2010-09-06 Adam Barth <abarth at 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:
git-svn-id: http://svn.webkit.org/repository/webkit/trunk@66849 268f45cc-cd09-0410-ab3c-d52691b4dbfc
diff --git a/BugsSite/ChangeLog b/BugsSite/ChangeLog
index ea56be1..52d689f 100644
--- a/BugsSite/ChangeLog
+++ b/BugsSite/ChangeLog
@@ -1,5 +1,37 @@
2010-09-06 Adam Barth <abarth at 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 at 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.
diff --git a/BugsSite/PrettyPatch/PrettyPatch.rb b/BugsSite/PrettyPatch/PrettyPatch.rb
index 27a5843..ffccd39 100644
--- a/BugsSite/PrettyPatch/PrettyPatch.rb
+++ b/BugsSite/PrettyPatch/PrettyPatch.rb
@@ -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>
diff --git a/BugsSite/code-review.js b/BugsSite/code-review.js
index b94382f..98ff999 100644
--- a/BugsSite/code-review.js
+++ b/BugsSite/code-review.js
@@ -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;
}
@@ -62,8 +62,12 @@
}
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);
@@ -180,8 +184,9 @@
$(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() {
@@ -192,6 +197,11 @@
trimCommentContextToBefore(line);
}
+ function unfreezeComment() {
+ $(this).prev().show();
+ $(this).remove();
+ }
+
$('.comment .cancel').live('click', cancelComment);
$('.comment .ok').live('click', function() {
@@ -202,13 +212,10 @@
}
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');
@@ -218,7 +225,7 @@
}
function focusNextComment() {
- var comments = $('.previous_comment');
+ var comments = $('.previousComment');
if (comments.length == 0)
return;
var index = comments.index($('.focused'));
@@ -227,7 +234,7 @@
}
function focusPreviousComment() {
- var comments = $('.previous_comment');
+ var comments = $('.previousComment');
if (comments.length == 0)
return;
var index = comments.index($('.focused'));
@@ -346,13 +353,16 @@
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);
--
WebKit Debian packaging
More information about the Pkg-webkit-commits
mailing list