Modified: trunk/Websites/bugs.webkit.org/ChangeLog (138652 => 138653)
--- trunk/Websites/bugs.webkit.org/ChangeLog 2013-01-02 22:41:16 UTC (rev 138652)
+++ trunk/Websites/bugs.webkit.org/ChangeLog 2013-01-02 22:45:54 UTC (rev 138653)
@@ -1,3 +1,18 @@
+2013-01-02 Ojan Vafai <o...@chromium.org>
+
+ REGRESSION: Review tool sometimes doesn't include some comments in preview & posts
+ https://bugs.webkit.org/show_bug.cgi?id=105252
+
+ Reviewed by Tony Chang.
+
+ When adding context, the LineContainer for the context line can get removed.
+ In that case, forEachLine needs to know to keep looping past that line number.
+
+ Also, make it so that you can't leave comments on context lines.
+
+ * code-review-test.html:
+ * code-review.js:
+
2012-12-30 Martin Robinson <mrobin...@igalia.com>
PrettyDiff.rb fails to render image diffs with Ruby 1.9.3p194
Modified: trunk/Websites/bugs.webkit.org/PrettyPatch/PrettyPatch.rb (138652 => 138653)
--- trunk/Websites/bugs.webkit.org/PrettyPatch/PrettyPatch.rb 2013-01-02 22:41:16 UTC (rev 138652)
+++ trunk/Websites/bugs.webkit.org/PrettyPatch/PrettyPatch.rb 2013-01-02 22:45:54 UTC (rev 138653)
@@ -506,7 +506,7 @@
}
</style>
<script src=""
-<script src=""
+<script src=""
</head>
EOF
Modified: trunk/Websites/bugs.webkit.org/code-review-test.html (138652 => 138653)
--- trunk/Websites/bugs.webkit.org/code-review-test.html 2013-01-02 22:41:16 UTC (rev 138652)
+++ trunk/Websites/bugs.webkit.org/code-review-test.html 2013-01-02 22:45:54 UTC (rev 138653)
@@ -303,7 +303,6 @@
'</div>' +
'</div>';
- next_line_id = 0;
eraseDraftComments();
crawlDiff();
@@ -401,6 +400,62 @@
ASSERT("ChangeLog-script is not a ChangeLog file", !isChangeLog("Tools/Scripts/ChangeLog-script"));
}
+function testSaveCommentsWithMissingLineIds() {
+ document.getElementById('diff-content').innerHTML =
+ '<div class="FileDiff">' +
+ '<h1><a href="" +
+ '<div class="DiffSection">' +
+ '<div class="DiffBlock">' +
+ '<div class="DiffBlockPart shared">' +
+ '<div class="Line LineContainer">' +
+ '<span class="from lineNumber">6</span><span class="to lineNumber">8</span><span class="text">a</span>' +
+ '</div>' +
+ '<div class="Line LineContainer">' +
+ '<span class="from lineNumber">7</span><span class="to lineNumber">9</span><span class="text">b</span>' +
+ '</div>' +
+ '<div class="Line LineContainer">' +
+ '<span class="from lineNumber">8</span><span class="to lineNumber">10</span><span class="text">c</span>' +
+ '</div>' +
+ '</div><div class="clear_float">' +
+ '</div>' +
+ '</div>' +
+ '<div class="DiffSection">' +
+ '<div class="Line LineContainer context">' +
+ '<span class="from lineNumber">@</span><span class="to lineNumber">@</span><span class="text">static void willRemoveChildren(ContainerNode* container)</span>' +
+ '</div>' +
+ '<div class="DiffBlock">' +
+ '<div class="DiffBlockPart shared">' +
+ '<div class="Line LineContainer">' +
+ '<span class="from lineNumber">15</span><span class="to lineNumber">17</span><span class="text">d</span>' +
+ '</div>' +
+ '</div><div class="clear_float"></div></div>' +
+ '</div>' +
+ '</div>';
+
+ var file_name = "Source/WebCore/dummy.txt";
+ var file_contents = [];
+ for (var i = 0; i < 20; i++)
+ file_contents[i] = i;
+ setFileContents(file_name, file_contents, file_contents);
+
+ eraseDraftComments();
+ crawlDiff();
+
+ var new_comment = addCommentFor($('#line4'));
+ new_comment.find('textarea').val("dummy content");
+ acceptComment(new_comment);
+
+ $('.ExpandLink')[0].click();
+
+ // Strip the link to the context since that points to window.location.
+ ASSERT_EQUAL(serializedComments().replace(/View in context.*code-review-test.html/, ''),
+ '\n\n' +
+ '> Source/WebCore/dummy.txt:17\n\n\n' +
+ 'dummy content');
+ document.getElementById('diff-content').innerHTML = '';
+}
+
+
for (var property in window) {
if (property.indexOf('test') == 0) {
window[property]();
Modified: trunk/Websites/bugs.webkit.org/code-review.js (138652 => 138653)
--- trunk/Websites/bugs.webkit.org/code-review.js 2013-01-02 22:41:16 UTC (rev 138652)
+++ trunk/Websites/bugs.webkit.org/code-review.js 2013-01-02 22:45:54 UTC (rev 138653)
@@ -72,6 +72,7 @@
var WEBKIT_BASE_DIR = "http://svn.webkit.org/repository/webkit/trunk/";
var SIDE_BY_SIDE_DIFFS_KEY = 'sidebysidediffs';
var g_displayed_draft_comments = false;
+ var g_next_line_id = 0;
var KEY_CODE = {
down: 40,
enter: 13,
@@ -90,13 +91,11 @@
function forEachLine(callback) {
var i = 0;
- var line;
- do {
- line = $('#' + idForLine(i));
+ for (var i = 0; i < g_next_line_id; i++) {
+ var line = $('#' + idForLine(i));
if (line[0])
callback(line);
- i++;
- } while (line[0]);
+ }
}
function hoverify() {
@@ -597,9 +596,9 @@
}
function crawlDiff() {
- var next_line_id = 0;
+ g_next_line_id = 0;
var idify = function() {
- this.id = idForLine(next_line_id++);
+ this.id = idForLine(g_next_line_id++);
}
$('.Line').each(idify).each(hoverify);
@@ -740,10 +739,15 @@
return revision[0] ? revision.first().text() : null;
}
+ function setFileContents(file_name, original_contents, patched_contents) {
+ original_file_contents[file_name] = original_contents;
+ patched_file_contents[file_name] = patched_contents;
+ }
+
function getWebKitSourceFile(file_name, onLoad, expand_bar) {
function handleLoad(contents) {
- original_file_contents[file_name] = contents.split('\n');
- patched_file_contents[file_name] = applyDiff(original_file_contents[file_name], file_name);
+ var split_contents = contents.split('\n');
+ setFileContents(file_name, split_contents, applyDiff(split_contents, file_name));
onLoad();
};
@@ -1361,8 +1365,10 @@
$(line).replaceWith(new_line);
- var line = $(document.getElementById(id));
- line.after(commentsToTransferFor(line));
+ if (!line.classList.contains('context')) {
+ var line = $(document.getElementById(id));
+ line.after(commentsToTransferFor(line));
+ }
}
function convertExpansionLine(diff_type, line) {
@@ -1817,9 +1823,11 @@
$('.lineNumber').live('click', function(e) {
var line = lineFromLineDescendant($(this));
- if (line.hasClass('commentContext'))
- trimCommentContextToBefore(previousLineFor(line), line.attr('data-comment-base-line'));
- else if (e.shiftKey)
+ if (line.hasClass('commentContext')) {
+ var previous_line = previousLineFor(line);
+ if (previous_line[0])
+ trimCommentContextToBefore(previous_line, line.attr('data-comment-base-line'));
+ } else if (e.shiftKey)
extendCommentContextTo(line);
}).live('mousedown', function(e) {
// preventDefault to avoid selecting text when dragging to select comment context lines.
@@ -1829,11 +1837,14 @@
return;
var line = lineFromLineDescendant($(this));
+ if (line.hasClass('context'))
+ return;
+
drag_select_start_index = numberFrom(line.attr('id'));
line.addClass('selected');
});
- $('.LineContainer').live('mouseenter', function(e) {
+ $('.LineContainer:not(.context)').live('mouseenter', function(e) {
if (drag_select_start_index == -1 || e.shiftKey)
return;
selectToLineContainer(this);
@@ -2033,7 +2044,7 @@
$('#comment_form .winter').live('click', hideCommentForm);
- function fillInReviewForm() {
+ function serializedComments() {
var comments_in_context = []
forEachLine(function(line) {
if (line.attr('data-has-comment') != 'true')
@@ -2057,8 +2068,12 @@
comment += comments_in_context.join('\n\n');
if (comments_in_context.length > 0)
comment = 'View in context: ' + window.location + '\n\n' + comment;
+ return comment;
+ }
+
+ function fillInReviewForm() {
var review_form = $('#reviewform').contents();
- review_form.find('#comment').val(comment);
+ review_form.find('#comment').val(serializedComments());
review_form.find('#flags select').each(function() {
var control = findControlForFlag(this);
if (!control.size())
@@ -2092,6 +2107,7 @@
if (CODE_REVIEW_UNITTEST) {
window.DraftCommentSaver = DraftCommentSaver;
+ window.addCommentFor = addCommentFor;
window.addPreviousComment = addPreviousComment;
window.tracLinks = tracLinks;
window.crawlDiff = crawlDiff;
@@ -2103,6 +2119,8 @@
window.acceptComment = acceptComment;
window.appendToolbar = appendToolbar;
window.eraseDraftComments = eraseDraftComments;
+ window.serializedComments = serializedComments;
+ window.setFileContents = setFileContents;
window.unfreezeComment = unfreezeComment;
window.g_draftCommentSaver = g_draftCommentSaver;
window.isChangeLog = isChangeLog;
