Fix bug in the code review tool when readding a discarded comment
authorojan@chromium.org <ojan@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 13 Dec 2011 23:26:58 +0000 (23:26 +0000)
committerojan@chromium.org <ojan@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 13 Dec 2011 23:26:58 +0000 (23:26 +0000)
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:

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

Websites/bugs.webkit.org/ChangeLog
Websites/bugs.webkit.org/code-review-test.html
Websites/bugs.webkit.org/code-review.js

index 289dba5..70e87c6 100644 (file)
@@ -1,3 +1,24 @@
+2011-12-13  Ojan Vafai  <ojan@chromium.org>
+
+        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  <tony@chromium.org>
 
         set a max-width on the codereview overall comments textarea
index 48342c6..e46ce64 100644 (file)
@@ -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="https://ajax.googleapis.com/ajax/libs/jquery/1.4.2/jquery.min.js"></script> 
 <script src="code-review.js"></script>
 <pre id="output"></pre>
+<div id="diff-content"></div>
 <script>
-
 function inherits(childConstructor, parentConstructor) {
   function tempConstructor() {};
   tempConstructor.prototype = parentConstructor.prototype;
@@ -69,11 +76,18 @@ function log(msg) {
     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 @@ var comments = new MockDraftCommentSaver('1', ls).saved_comments().comments;
 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="http://trac.webkit.org/browser/trunk/Source/WebCore/ChangeLog">Source/WebCore/ChangeLog</a></h1>' +
+        '<div class="DiffSection">' +
+          '<div class="DiffBlock">' +
+            '<div class="DiffBlockPart add">' +
+              '<div class="Line LineContainer add">' +
+                '<span class="from lineNumber">&nbsp;</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">&nbsp;</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 = "ojan@chromium.org";
+  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>
index bac2224..78223bd 100644 (file)
@@ -165,10 +165,20 @@ var CODE_REVIEW_UNITTEST;
     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 @@ var CODE_REVIEW_UNITTEST;
     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 @@ var CODE_REVIEW_UNITTEST;
     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 @@ var CODE_REVIEW_UNITTEST;
       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 @@ var CODE_REVIEW_UNITTEST;
     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 @@ var CODE_REVIEW_UNITTEST;
   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 @@ var CODE_REVIEW_UNITTEST;
   }
   
   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 @@ var CODE_REVIEW_UNITTEST;
     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 @@ var CODE_REVIEW_UNITTEST;
   }
 
   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 @@ var CODE_REVIEW_UNITTEST;
     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 @@ var CODE_REVIEW_UNITTEST;
       '<a href="javascript:" 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 @@ var CODE_REVIEW_UNITTEST;
           '</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.cgi?id=' + attachment_id + '&action=reviewform"></iframe></div></div>');
     $('#reviewform').bind('load', handleReviewFormLoad);
@@ -1128,7 +1135,7 @@ var CODE_REVIEW_UNITTEST;
 
     updateToolbarAnchorState();
     loadDiffState();
-  });
+  };
 
   function handleReviewFormLoad() {
     var review_form_contents = $('#reviewform').contents();
@@ -1330,12 +1337,12 @@ var CODE_REVIEW_UNITTEST;
   }
 
   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 @@ var CODE_REVIEW_UNITTEST;
   }
   
   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 @@ var CODE_REVIEW_UNITTEST;
       if (numberFrom(id) > line_to_trim_to)
         return;
 
-      removeDataCommentBaseLine(this, comment_base_line);
+      if (!$('[data-comment-for=' + comment_base_line + ']').length)
+        removeDataCommentBaseLine(this, comment_base_line);
     });
   }
 
@@ -1972,4 +1981,20 @@ var CODE_REVIEW_UNITTEST;
     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)
+  }
 })();