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:

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

diff --git a/Websites/bugs.webkit.org/ChangeLog b/Websites/bugs.webkit.org/ChangeLog
index 1e61196..98ff4db 100644 (file)
--- a/Websites/bugs.webkit.org/ChangeLog
+++ b/Websites/bugs.webkit.org/ChangeLog
@@ -1,3 +1,18 @@
+2013-01-02  Ojan Vafai  <ojan@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  <mrobinson@igalia.com>
 
         PrettyDiff.rb fails to render image diffs with Ruby 1.9.3p194

diff --git a/Websites/bugs.webkit.org/PrettyPatch/PrettyPatch.rb b/Websites/bugs.webkit.org/PrettyPatch/PrettyPatch.rb
index 4087296..d2d4bf3 100644 (file)
--- a/Websites/bugs.webkit.org/PrettyPatch/PrettyPatch.rb
+++ b/Websites/bugs.webkit.org/PrettyPatch/PrettyPatch.rb
@@ -506,7 +506,7 @@ div:focus {
 }
 </style>
 <script src="https://ajax.googleapis.com/ajax/libs/jquery/1.4.2/jquery.min.js"></script> 
-<script src="code-review.js?version=46"></script>
+<script src="code-review.js?version=47"></script>
 </head>
 EOF
 

diff --git a/Websites/bugs.webkit.org/code-review-test.html b/Websites/bugs.webkit.org/code-review-test.html
index 15fe7e3..d2469ba 100644 (file)
--- a/Websites/bugs.webkit.org/code-review-test.html
+++ b/Websites/bugs.webkit.org/code-review-test.html
@@ -303,7 +303,6 @@ function testSideBySideDiffWithPreviousCommentsOnSharedLine() {
         '</div>' +
       '</div>';
 
-  next_line_id = 0;
   eraseDraftComments();
   crawlDiff();
 
@@ -401,6 +400,62 @@ function testIsChangeLog() {
   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="http://trac.webkit.org/browser/trunk/Source/WebCore/ChangeLog">Source/WebCore/dummy.txt</a></h1>' +
+        '<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]();

diff --git a/Websites/bugs.webkit.org/code-review.js b/Websites/bugs.webkit.org/code-review.js
index 412fa19..4f61878 100644 (file)
--- a/Websites/bugs.webkit.org/code-review.js
+++ b/Websites/bugs.webkit.org/code-review.js
@@ -72,6 +72,7 @@ var CODE_REVIEW_UNITTEST;
   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 @@ var CODE_REVIEW_UNITTEST;
 
   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 @@ var CODE_REVIEW_UNITTEST;
   }
 
   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 @@ var CODE_REVIEW_UNITTEST;
     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 @@ var CODE_REVIEW_UNITTEST;
 
     $(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 @@ var CODE_REVIEW_UNITTEST;
 
   $('.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 @@ var CODE_REVIEW_UNITTEST;
       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 @@ var CODE_REVIEW_UNITTEST;
 
   $('#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 @@ var CODE_REVIEW_UNITTEST;
     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 @@ var CODE_REVIEW_UNITTEST;
   
   if (CODE_REVIEW_UNITTEST) {
     window.DraftCommentSaver = DraftCommentSaver;
+    window.addCommentFor = addCommentFor;
     window.addPreviousComment = addPreviousComment;
     window.tracLinks = tracLinks;
     window.crawlDiff = crawlDiff;
@@ -2103,6 +2119,8 @@ var CODE_REVIEW_UNITTEST;
     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;