Modified: trunk/Websites/bugs.webkit.org/ChangeLog (102710 => 102711)
--- trunk/Websites/bugs.webkit.org/ChangeLog 2011-12-13 23:19:19 UTC (rev 102710)
+++ trunk/Websites/bugs.webkit.org/ChangeLog 2011-12-13 23:26:58 UTC (rev 102711)
@@ -1,3 +1,24 @@
+2011-12-13 Ojan Vafai <[email protected]>
+
+ Fix bug in the code review tool when readding a discarded comment
+ https://bugs.webkit.org/show_bug.cgi?id=74450
+
+ Reviewed by Adam Barth.
+
+ If you discard a comment that has a corresponding previousComment,
+ then we would incorrectly remove the comment baseline. So, the next
+ time you added a comment by clicking on the previousComment, we
+ would get undefined as the start line for the new comment.
+
+ All of this works fine until you try to restore the comment from
+ localStorage, at which point we throw an error because the start
+ line is undefined.
+
+ Also added some failsafes to better handle the case of corrupted comments.
+
+ * code-review-test.html:
+ * code-review.js:
+
2011-11-15 Tony Chang <[email protected]>
set a max-width on the codereview overall comments textarea
Modified: trunk/Websites/bugs.webkit.org/code-review-test.html (102710 => 102711)
--- trunk/Websites/bugs.webkit.org/code-review-test.html 2011-12-13 23:19:19 UTC (rev 102710)
+++ trunk/Websites/bugs.webkit.org/code-review-test.html 2011-12-13 23:26:58 UTC (rev 102711)
@@ -1,12 +1,19 @@
-<div>Tests for some of the easily unittestable parts of code-review.js. You should see a series of "PASSED" lines.</div>
+<div>Tests for some of the easily unittestable parts of code-review.js. You should see a series of "PASS" lines.</div>
<div>FIXME: Run these as part of the layout test suite?</div>
-<script>CODE_REVIEW_UNITTEST = true</script>
+<script>
+CODE_REVIEW_UNITTEST = true;
+
+window._onerror_ = function(errorMsg, url, lineNumber) {
+ log('FAIL: Received an error at line ' + lineNumber);
+ return false;
+}
+</script>
<script src=""
<script src=""
<pre id="output"></pre>
+<div id="diff-content"></div>
<script>
-
function inherits(childConstructor, parentConstructor) {
function tempConstructor() {};
tempConstructor.prototype = parentConstructor.prototype;
@@ -69,11 +76,18 @@
document.getElementById('output').innerHTML += msg + '\n\n';
}
+function ASSERT(msg, assertion) {
+ if (assertion)
+ log('PASS');
+ else
+ log('FAIL: ' + msg);
+}
+
function ASSERT_EQUAL(actual, expected) {
if (actual == expected)
- log('PASSED');
+ log('PASS');
else
- log('FAILED:\ngot:\n' + actual + '\nexpected:\n' + expected + '');
+ log('FAIL:\ngot:\n' + actual + '\nexpected:\n' + expected + '');
}
var links = tracLinks('foo/bar/baz.cpp', '');
@@ -168,4 +182,80 @@
ASSERT_EQUAL(ls.log_string(), 'getItem:draft-comments-for-attachment-1\nremoveItem:draft-comments-for-attachment-1');
+function stripBornOn(json) {
+ return json.replace(/\"born\-on\"\:\d+,/, "");
+}
+
+function strippedSavedComments() {
+ return stripBornOn(localStorage[g_draftCommentSaver.localStorageKey()]);
+}
+
+function syncSlideUp(type, handler) {
+ handler.call(this);
+}
+
+function testReaddDiscardedCommentWithPreviousComment() {
+ document.getElementById('diff-content').innerHTML =
+ '<div class="FileDiff">' +
+ '<h1><a href="" +
+ '<div class="DiffSection">' +
+ '<div class="DiffBlock">' +
+ '<div class="DiffBlockPart add">' +
+ '<div class="Line LineContainer add">' +
+ '<span class="from lineNumber"> </span><span class="to lineNumber">1</span><span class="text">first diff block first line</span>' +
+ '</div>' +
+ '<div class="Line LineContainer add">' +
+ '<span class="from lineNumber"> </span><span class="to lineNumber">2</span><span class="text"></span>' +
+ '</div>' +
+ '</div>' +
+ '<div class="clear_float"></div>' +
+ '</div>' +
+ '<div class="DiffBlock">' +
+ '<div class="DiffBlockPart shared">' +
+ '<div class="Line LineContainer">' +
+ '<span class="from lineNumber">1</span><span class="to lineNumber">12</span><span class="text">second diff block first line</span>' +
+ '</div>' +
+ '</div><div class="clear_float">' +
+ '</div>' +
+ '</div>' +
+ '</div>' +
+ '</div>';
+
+ eraseDraftComments();
+ crawlDiff();
+ appendToolbar();
+
+ var line = document.getElementById('line0')
+ var author = "[email protected]";
+ var comment_text = "This change sux.";
+ addPreviousComment(line, author, comment_text);
+ var previous_comment = document.querySelector('.previousComment');
+ ASSERT("Line with only a previous comment should not have 'data-has-comment' attribute.", !line.getAttribute('data-has-comment'));
+
+ var new_comment = addCommentField(previous_comment);
+ new_comment.find('textarea').val("dummy content");
+ var frozen_comment = acceptComment(new_comment);
+
+ ASSERT_EQUAL(document.querySelectorAll('.comment').length, 1);
+ ASSERT_EQUAL(strippedSavedComments(), '{"comments":[{"start_line_id":"line0","end_line_id":"line0","contents":"dummy content"}],"overall-comments":""}');
+
+ unfreezeComment(frozen_comment);
+ // This is a hack to make slideUp synchronous so that we can keep this test from needing to be async.
+ jQuery.fn.slideUp = syncSlideUp;
+ discardComment(new_comment);
+
+ ASSERT('There should be no draft comments.', !document.querySelector('.comment'));
+ ASSERT_EQUAL(strippedSavedComments(), '{"comments":[],"overall-comments":""}');
+ ASSERT("Line with only a previous comment should not have 'data-has-comment' attribute.", !line.getAttribute('data-has-comment'));
+
+ new_comment = addCommentField(previous_comment);
+ new_comment.find('textarea').val("dummy content");
+ acceptComment(new_comment);
+
+ ASSERT_EQUAL(document.querySelectorAll('.comment').length, 1);
+ ASSERT_EQUAL(strippedSavedComments(), '{"comments":[{"start_line_id":"line0","end_line_id":"line0","contents":"dummy content"}],"overall-comments":""}');
+
+ document.getElementById('diff-content').innerHTML = '';
+}
+testReaddDiscardedCommentWithPreviousComment();
</script>
Modified: trunk/Websites/bugs.webkit.org/code-review.js (102710 => 102711)
--- trunk/Websites/bugs.webkit.org/code-review.js 2011-12-13 23:19:19 UTC (rev 102710)
+++ trunk/Websites/bugs.webkit.org/code-review.js 2011-12-13 23:26:58 UTC (rev 102711)
@@ -165,10 +165,20 @@
g_displayed_draft_comments = true;
var comments = g_draftCommentSaver.saved_comments();
+ var errors = [];
$(comments.comments).each(function() {
- addDraftComment(this.start_line_id, this.end_line_id, this.contents);
+ try {
+ addDraftComment(this.start_line_id, this.end_line_id, this.contents);
+ } catch (e) {
+ errors.push({'start': this.start_line_id, 'end': this.end_line_id, 'contents': this.contents});
+ }
});
+ if (errors.length) {
+ console.log('DRAFT COMMENTS WITH ERRORS:', JSON.stringify(errors));
+ alert('Some draft comments failed to be added. See the console to manually resolve.');
+ }
+
var overall_comments = comments['overall-comments'];
if (overall_comments) {
openOverallComments();
@@ -182,9 +192,6 @@
this._save_comments = true;
}
- if (CODE_REVIEW_UNITTEST)
- window['DraftCommentSaver'] = DraftCommentSaver;
-
DraftCommentSaver.prototype._json = function() {
var comments = $('.comment');
var comment_store = [];
@@ -210,8 +217,12 @@
return JSON.stringify({'born-on': Date.now(), 'comments': comment_store, 'overall-comments': overall_comments});
}
+ DraftCommentSaver.prototype.localStorageKey = function() {
+ return DraftCommentSaver._keyPrefix + this._attachment_id;
+ }
+
DraftCommentSaver.prototype.saved_comments = function() {
- var serialized_comments = this._localStorage.getItem(DraftCommentSaver._keyPrefix + this._attachment_id);
+ var serialized_comments = this._localStorage.getItem(this.localStorageKey());
if (!serialized_comments)
return [];
@@ -222,17 +233,12 @@
this._erase_corrupt_comments();
return {};
}
-
+
var individual_comments = comments.comments;
- if (comments && !individual_comments.length)
- return comments;
-
- // Sanity check comments are as expected.
- if (!comments || !individual_comments[0].contents) {
+ if (!comments || !comments['born-on'] || !individual_comments || (individual_comments.length && !individual_comments[0].contents)) {
this._erase_corrupt_comments();
return {};
- }
-
+ }
return comments;
}
@@ -246,7 +252,7 @@
if (!this._save_comments)
return;
- var key = DraftCommentSaver._keyPrefix + this._attachment_id;
+ var key = this.localStorageKey();
var value = this._json();
if (this._attemptToWrite(key, value))
@@ -286,7 +292,7 @@
DraftCommentSaver._keyPrefix = 'draft-comments-for-attachment-';
DraftCommentSaver.prototype.erase = function() {
- this._localStorage.removeItem(DraftCommentSaver._keyPrefix + this._attachment_id);
+ this._localStorage.removeItem(this.localStorageKey());
}
DraftCommentSaver.prototype._eraseOldCommentsForAllReviews = function() {
@@ -342,9 +348,9 @@
}
function unfreezeCommentFor(line) {
- // 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(handleUnfreezeComment);
+ // 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(handleUnfreezeComment);
}
function createCommentFor(line) {
@@ -369,13 +375,14 @@
comment_block.hide().slideDown('fast', function() {
$(this).children('textarea').focus();
});
+ return comment_block;
}
function addCommentField(comment_block) {
var id = $(comment_block).attr('data-comment-for');
if (!id)
id = comment_block.id;
- addCommentFor($('#' + id));
+ return addCommentFor($('#' + id));
}
function handleAddCommentField() {
@@ -383,13 +390,13 @@
}
function addPreviousComment(line, author, comment_text) {
- var line_id = line.attr('id');
+ var line_id = $(line).attr('id');
var comment_block = $('<div data-comment-for="" + line_id + '" class="previousComment"></div>');
var author_block = $('<div class="author"></div>').text(author + ':');
var text_block = $('<div class="content"></div>').text(comment_text);
comment_block.append(author_block).append(text_block).each(hoverify).click(handleAddCommentField);
- addDataCommentBaseLine(line, line_id);
- insertCommentFor(line, comment_block);
+ addDataCommentBaseLine($(line), line_id);
+ insertCommentFor($(line), comment_block);
}
function displayPreviousComments(comments) {
@@ -635,9 +642,6 @@
return trac_links;
}
- if (CODE_REVIEW_UNITTEST)
- window.tracLinks = tracLinks;
-
function addExpandLinks(file_name) {
if (file_name.indexOf('ChangeLog') != -1)
return;
@@ -1070,9 +1074,28 @@
'<a href="" class="side-by-side-link">side-by-side</a>';
}
- $(document).ready(function() {
- if (CODE_REVIEW_UNITTEST)
- return;
+ function appendToolbar() {
+ $(document.body).append('<div id="toolbar">' +
+ '<div class="overallComments">' +
+ '<textarea placeholder="Overall comments"></textarea>' +
+ '</div>' +
+ '<div>' +
+ '<span id="statusBubbleContainer"></span>' +
+ '<span class="actions">' +
+ '<span class="links"><span class="bugLink"></span></span>' +
+ '<span id="flagContainer"></span>' +
+ '<button id="preview_comments">Preview</button>' +
+ '<button id="post_comments">Publish</button> ' +
+ '</span>' +
+ '</div>' +
+ '<div class="autosave-state"></div>' +
+ '</div>');
+
+ $('.overallComments textarea').bind('click', openOverallComments);
+ $('.overallComments textarea').bind('input', handleOverallCommentsInput);
+ }
+
+ function handleDocumentReady() {
crawlDiff();
fetchHistory();
$(document.body).prepend('<div id="message">' +
@@ -1097,24 +1120,8 @@
'</div>' +
'</div>' +
'</div>');
- $(document.body).append('<div id="toolbar">' +
- '<div class="overallComments">' +
- '<textarea placeholder="Overall comments"></textarea>' +
- '</div>' +
- '<div>' +
- '<span id="statusBubbleContainer"></span>' +
- '<span class="actions">' +
- '<span class="links"><span class="bugLink"></span></span>' +
- '<span id="flagContainer"></span>' +
- '<button id="preview_comments">Preview</button>' +
- '<button id="post_comments">Publish</button> ' +
- '</span>' +
- '</div>' +
- '<div class="autosave-state"></div>' +
- '</div>');
- $('.overallComments textarea').bind('click', openOverallComments);
- $('.overallComments textarea').bind('input', handleOverallCommentsInput);
+ appendToolbar();
$(document.body).prepend('<div id="comment_form" class="inactive"><div class="winter"></div><div class="lightbox"><iframe id="reviewform" src="" + attachment_id + '&action=""
$('#reviewform').bind('load', handleReviewFormLoad);
@@ -1128,7 +1135,7 @@
updateToolbarAnchorState();
loadDiffState();
- });
+ };
function handleReviewFormLoad() {
var review_form_contents = $('#reviewform').contents();
@@ -1330,12 +1337,12 @@
}
function discardComment(comment_block) {
- var line_id = comment_block.find('textarea').attr('data-comment-for');
+ var line_id = $(comment_block).find('textarea').attr('data-comment-for');
var line = $('#' + line_id)
- findCommentBlockFor(line).slideUp('fast', function() {
+ $(comment_block).slideUp('fast', function() {
$(this).remove();
line.removeAttr('data-has-comment');
- trimCommentContextToBefore(line, line.attr('data-comment-base-line'));
+ trimCommentContextToBefore(line, line_id);
saveDraftComments();
});
}
@@ -1368,9 +1375,10 @@
}
function acceptComment(comment) {
- var frozen_comment = freezeComment(comment);
+ var frozen_comment = freezeComment($(comment));
focusOn(frozen_comment);
saveDraftComments();
+ return frozen_comment;
}
$('.FileDiff').live('mouseenter', showFileDiffLinks);
@@ -1679,7 +1687,8 @@
if (numberFrom(id) > line_to_trim_to)
return;
- removeDataCommentBaseLine(this, comment_base_line);
+ if (!$('[data-comment-for='']').length)
+ removeDataCommentBaseLine(this, comment_base_line);
});
}
@@ -1972,4 +1981,20 @@
fillInReviewForm();
$('#reviewform').contents().find('form').submit();
});
+
+ if (CODE_REVIEW_UNITTEST) {
+ window.DraftCommentSaver = DraftCommentSaver;
+ window.addPreviousComment = addPreviousComment;
+ window.tracLinks = tracLinks;
+ window.crawlDiff = crawlDiff;
+ window.discardComment = discardComment;
+ window.addCommentField = addCommentField;
+ window.acceptComment = acceptComment;
+ window.appendToolbar = appendToolbar;
+ window.eraseDraftComments = eraseDraftComments;
+ window.unfreezeComment = unfreezeComment;
+ window.g_draftCommentSaver = g_draftCommentSaver;
+ } else {
+ $(document).ready(handleDocumentReady)
+ }
})();
