Changeset 136556 in webkit

Timestamp:

Dec 4, 2012, 1:11:01 PM (12 years ago)

Author:

ojan@chromium.org

Message:

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:

Location:

trunk/Websites/bugs.webkit.org

Files:

• ChangeLog (modified) (1 diff)
• code-review-test.html (modified) (2 diffs)
• code-review.js (modified) (6 diffs)

trunk/Websites/bugs.webkit.org/ChangeLog

@@ +1 @@
+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>
 

trunk/Websites/bugs.webkit.org/code-review-test.html

@@ -229 +229 @@
   appendToolbar();
 
-  var line = document.getElementById('line0')
+  var line = document.getElementById('line0');
   var author = "ojan@chromium.org";
   var comment_text = "This change sux.";
@@ -262 +262 @@
 }
 
+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"));

trunk/Websites/bugs.webkit.org/code-review.js

@@ -67 +67 @@
   var maxLeftSideRatio = 90;
   var file_diff_being_resized = null;
-  var next_line_id = 0;
   var files = {};
   var original_file_contents = {};
@@ -90 +89 @@
   }
 
-  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]);
   }
 
@@ -424 +420 @@
         if ($(this).text() != line_number)
           return;
-        var line = $(this).parent();
+        var line = lineContainerFromDescendant($(this));
         addPreviousComment(line, author, comment_text);
       });
@@ -602 +598 @@
 
   function crawlDiff() {
+    var next_line_id = 0;
+    var idify = function() {
+      this.id = idForLine(next_line_id++);
+    }
+
     $('.Line').each(idify).each(hoverify);
     $('.FileDiff').each(function() {
@@ -1929 +1930 @@
   }
 
+  function lineContainerFromDescendant(descendant) {
+    return descendant.hasClass('LineContainer') ? descendant : descendant.parents('.LineContainer');
+  }
+
   function lineFromLineContainer(lineContainer) {
     var line = $(lineContainer);
@@ -2038 +2043 @@
     window.tracLinks = tracLinks;
     window.crawlDiff = crawlDiff;
+    window.convertAllFileDiffs = convertAllFileDiffs;
+    window.displayPreviousComments = displayPreviousComments;
     window.discardComment = discardComment;
     window.addCommentField = addCommentField;