Can't add followup comment to a previous comment

https://bugs.webkit.org/show_bug.cgi?id=104025

Reviewed by Adam Barth.

If we side-by-sidify a shared diff line, and then apply
a previous comment, we would incorrectly put the comment
on the Line instead of the LineContainer.

Also, get rid of global next_line_id to simplify testing.

* code-review-test.html:
* code-review.js:

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

diff --git a/Websites/bugs.webkit.org/ChangeLog b/Websites/bugs.webkit.org/ChangeLog
index 95819314d1030a621c6412a6ef75630862fd67bf..c17b79bf9d3100f70681ce825be9a4576db03b67 100644 (file)
--- a/Websites/bugs.webkit.org/ChangeLog
+++ b/Websites/bugs.webkit.org/ChangeLog
@@ -1,3 +1,19 @@
+2012-12-04  Ojan Vafai  <ojan@chromium.org>
+
+        Can't add followup comment to a previous comment
+        https://bugs.webkit.org/show_bug.cgi?id=104025
+
+        Reviewed by Adam Barth.
+
+        If we side-by-sidify a shared diff line, and then apply
+        a previous comment, we would incorrectly put the comment
+        on the Line instead of the LineContainer.
+
+        Also, get rid of global next_line_id to simplify testing.
+
+        * code-review-test.html:
+        * code-review.js:
+
 2012-11-06  Ryosuke Niwa  <rniwa@webkit.org>
 
         committers-autocomplete.js works only with WebKit based browsers

diff --git a/Websites/bugs.webkit.org/code-review-test.html b/Websites/bugs.webkit.org/code-review-test.html
index a2907730e50b075605f85d5b40c2114c07a731ce..95feea203cc87824c3646f3a00a0532d5336fe64 100644 (file)
--- a/Websites/bugs.webkit.org/code-review-test.html
+++ b/Websites/bugs.webkit.org/code-review-test.html
@@ -228,7 +228,7 @@ function testReaddDiscardedCommentWithPreviousComment() {
   crawlDiff();
   appendToolbar();
 
-  var line = document.getElementById('line0')
+  var line = document.getElementById('line0');
   var author = "ojan@chromium.org";
   var comment_text = "This change sux.";
   addPreviousComment(line, author, comment_text);
@@ -261,6 +261,49 @@ function testReaddDiscardedCommentWithPreviousComment() {
   document.getElementById('diff-content').innerHTML = '';
 }
 
+function testSideBySideDiffWithPreviousCommentsOnSharedLine() {
+  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 shared">' +
+              '<div class="Line LineContainer">' +
+              '<span class="from lineNumber">336</span><span class="to lineNumber">338</span><span class="text">    layoutFlexItems(*m_orderIterator, lineContexts);</span>' +
+              '</div>' +
+              '<div class="Line LineContainer">' +
+              '<span class="from lineNumber">337</span><span class="to lineNumber">339</span><span class="text"></span>' +
+              '</div>' +
+              '<div class="Line LineContainer">' +
+              '<span class="from lineNumber">338</span><span class="to lineNumber">340</span><span class="text">    LayoutUnit oldClientAfterEdge = clientLogicalBottom();</span>' +
+              '</div>' +
+            '</div><div class="clear_float">' +
+          '</div>' +
+        '</div>' +
+      '</div>';
+
+  next_line_id = 0;
+  eraseDraftComments();
+  crawlDiff();
+
+  convertAllFileDiffs('sidebyside', $('.FileDiff'));
+
+  displayPreviousComments([{
+    author: 'ojan@chromium.org',
+    file_name: 'Source/WebCore/ChangeLog',
+    line_number: 338,
+    comment_text: 'This change sux.'
+  }]);
+
+  var previous_comment = document.querySelector('.previousComment');
+  ASSERT_EQUAL(previous_comment.getAttribute('data-comment-for'), 'line0');
+
+  var new_comment = addCommentField(previous_comment);
+  ASSERT("New comment should exist and contain a textarea.", new_comment.find('textarea'));
+
+  document.getElementById('diff-content').innerHTML = '';
+}
+
 function testIsChangeLog() {
   ASSERT("Top-level ChangeLog file is a ChangeLog file", isChangeLog("ChangeLog"));
   ASSERT("Second-level ChangeLog file is a ChangeLog file", isChangeLog("Tools/ChangeLog"));

diff --git a/Websites/bugs.webkit.org/code-review.js b/Websites/bugs.webkit.org/code-review.js
index d133484583a339d6749d1209462d009614c57578..32c7d13a32e25151ebd2ae2f247667c6c210ffae 100644 (file)
--- a/Websites/bugs.webkit.org/code-review.js
+++ b/Websites/bugs.webkit.org/code-review.js
@@ -66,7 +66,6 @@ var CODE_REVIEW_UNITTEST;
   var minLeftSideRatio = 10;
   var maxLeftSideRatio = 90;
   var file_diff_being_resized = null;
-  var next_line_id = 0;
   var files = {};
   var original_file_contents = {};
   var patched_file_contents = {};
@@ -89,18 +88,15 @@ var CODE_REVIEW_UNITTEST;
     return 'line' + number;
   }
 
-  function nextLineID() {
-    return idForLine(next_line_id++);
-  }
-
   function forEachLine(callback) {
-    for (var i = 0; i < next_line_id; ++i) {
-      callback($('#' + idForLine(i)));
-    }
-  }
-
-  function idify() {
-    this.id = nextLineID();
+    var i = 0;
+    var line;
+    do {
+      line = $('#' + idForLine(i));
+      if (line[0])
+        callback(line);
+      i++;
+    } while (line[0]);
   }
 
   function hoverify() {
@@ -423,7 +419,7 @@ var CODE_REVIEW_UNITTEST;
       $(file).find(query).each(function() {
         if ($(this).text() != line_number)
           return;
-        var line = $(this).parent();
+        var line = lineContainerFromDescendant($(this));
         addPreviousComment(line, author, comment_text);
       });
     }
@@ -601,6 +597,11 @@ var CODE_REVIEW_UNITTEST;
   }
 
   function crawlDiff() {
+    var next_line_id = 0;
+    var idify = function() {
+      this.id = idForLine(next_line_id++);
+    }
+
     $('.Line').each(idify).each(hoverify);
     $('.FileDiff').each(function() {
       var header = $(this).children('h1');
@@ -1928,6 +1929,10 @@ var CODE_REVIEW_UNITTEST;
     return descendant.hasClass('Line') ? descendant : descendant.parents('.Line');
   }
 
+  function lineContainerFromDescendant(descendant) {
+    return descendant.hasClass('LineContainer') ? descendant : descendant.parents('.LineContainer');
+  }
+
   function lineFromLineContainer(lineContainer) {
     var line = $(lineContainer);
     if (!line.hasClass('Line'))
@@ -2037,6 +2042,8 @@ var CODE_REVIEW_UNITTEST;
     window.addPreviousComment = addPreviousComment;
     window.tracLinks = tracLinks;
     window.crawlDiff = crawlDiff;
+    window.convertAllFileDiffs = convertAllFileDiffs;
+    window.displayPreviousComments = displayPreviousComments;
     window.discardComment = discardComment;
     window.addCommentField = addCommentField;
     window.acceptComment = acceptComment;