Sanitize content on copy in the code review tool
authorojan@chromium.org <ojan@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 5 Dec 2012 23:39:09 +0000 (23:39 +0000)
committerojan@chromium.org <ojan@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 5 Dec 2012 23:39:09 +0000 (23:39 +0000)
https://bugs.webkit.org/show_bug.cgi?id=104155

Reviewed by Tony Chang.

Always remove expand/header/annotate links. Provide an option
to remove line numbers as well. Store the option in localStorage so
people can always get whichever behavior they want.

A better solution would be to restructure the DOM, but that would require gutting
the whole code review tool and would make it difficult to include line numbers if
you wanted them.

* PrettyPatch/PrettyPatch.rb:
* code-review-test.html:
* code-review.js:

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

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

index edb84b6..1be2544 100644 (file)
@@ -1,3 +1,22 @@
+2012-12-05  Ojan Vafai  <ojan@chromium.org>
+
+        Sanitize content on copy in the code review tool
+        https://bugs.webkit.org/show_bug.cgi?id=104155
+
+        Reviewed by Tony Chang.
+
+        Always remove expand/header/annotate links. Provide an option
+        to remove line numbers as well. Store the option in localStorage so
+        people can always get whichever behavior they want.
+
+        A better solution would be to restructure the DOM, but that would require gutting
+        the whole code review tool and would make it difficult to include line numbers if
+        you wanted them.
+
+        * PrettyPatch/PrettyPatch.rb:
+        * code-review-test.html:
+        * code-review.js:
+
 2012-12-04  Ojan Vafai  <ojan@chromium.org>
 
         Use sticky positioning for the code review toolbar
index 9b351b4..eeb0278 100644 (file)
@@ -501,7 +501,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=43"></script>
+<script src="code-review.js?version=44"></script>
 </head>
 EOF
 
index 24ac9a8..15fe7e3 100644 (file)
@@ -325,6 +325,75 @@ function testSideBySideDiffWithPreviousCommentsOnSharedLine() {
   document.getElementById('diff-content').innerHTML = '';
 }
 
+function testSanitizeFragmentForCopy() {
+  var fragment = document.createElement('div');
+  fragment.innerHTML = '<div class="FileDiff">' +
+      '<h1><a href="http://trac.webkit.org/browser/trunk/Source/WebCore/ChangeLog">Source/WebCore/ChangeLog</a></h1>' +
+      '<div class="FileDiffLinkContainer LinkContainer" style="opacity: 0;"><a href="javascript:" class="unify-link cremed" style="display: inline;">unified</a></div>' +
+      '<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>' +
+    '</div>';
+
+  var expectedWithLineNumbers = '<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"><br></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>' +
+    '</div>';
+
+  var fragmentCopy = fragment.cloneNode(true);
+  sanitizeFragmentForCopy(fragmentCopy, false);
+  ASSERT_EQUAL(fragmentCopy.innerHTML, expectedWithLineNumbers);
+
+  var expectedWithOutLineNumbers = '<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="text">    layoutFlexItems(*m_orderIterator, lineContexts);</span>' +
+            '</div>' +
+            '<div class="Line LineContainer">' +
+              '<span class="text"><br></span>' +
+            '</div>' +
+            '<div class="Line LineContainer">' +
+              '<span class="text">    LayoutUnit oldClientAfterEdge = clientLogicalBottom();</span>' +
+            '</div>' +
+          '</div><div class="clear_float"></div>' +
+        '</div>' +
+      '</div>' +
+    '</div>';
+
+  var fragmentCopy = fragment.cloneNode(true);
+  sanitizeFragmentForCopy(fragmentCopy, true);
+  ASSERT_EQUAL(fragmentCopy.innerHTML, expectedWithOutLineNumbers);
+}
+
 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"));
index 8102c6b..44c136a 100644 (file)
@@ -1095,9 +1095,13 @@ var CODE_REVIEW_UNITTEST;
   function handleDocumentReady() {
     crawlDiff();
     fetchHistory();
+
     $(document.body).prepend('<div id="message">' +
         '<div class="help">Select line numbers to add a comment. Scroll though diffs with the "j" and "k" keys.' +
-          '<div class="DiffLinks LinkContainer">' + diffLinksHtml() + '</div>' +
+          '<div class="DiffLinks LinkContainer">' +
+            '<a href="javascript:" id="line-number-on-copy-link"></a> ' +
+            diffLinksHtml() +
+          '</div>' +
           '<a href="javascript:" class="more">[more]</a>' +
           '<div class="more-help inactive">' +
             '<div class="winter"></div>' +
@@ -1125,8 +1129,72 @@ var CODE_REVIEW_UNITTEST;
 
     loadDiffState();
     generateFileDiffResizeStyleElement();
+    updateLineNumberOnCopyLinkContents();
+
+    document.body.addEventListener('copy', handleCopy);
   };
 
+  function forEachNode(nodeList, callback) {
+    Array.prototype.forEach.call(nodeList, callback);
+  }
+
+  $('#line-number-on-copy-link').live('click', toggleShouldStripLineNumbersOnCopy);
+
+  function updateLineNumberOnCopyLinkContents() {
+    var link = document.getElementById('line-number-on-copy-link');
+    link.textContent = shouldStripLineNumbersOnCopy() ? 'Don\'t strip line numbers on copy' : 'Strip line numbers on copy';
+  }
+
+  function shouldStripLineNumbersOnCopy() {
+    return localStorage.getItem('code-review-line-numbers-on-copy') == 'true';
+  }
+
+  function toggleShouldStripLineNumbersOnCopy() {
+    localStorage.setItem('code-review-line-numbers-on-copy', !shouldStripLineNumbersOnCopy());
+    updateLineNumberOnCopyLinkContents();
+  }
+
+  function sanitizeFragmentForCopy(fragment, shouldStripLineNumbers) {
+    var classesToRemove = ['LinkContainer'];
+    if (shouldStripLineNumbers)
+      classesToRemove.push('lineNumber');
+
+    classesToRemove.forEach(function(className) {
+      forEachNode(fragment.querySelectorAll('.' + className), function(node) {
+        node.remove();
+      });
+    });
+
+    // Ensure that empty newlines show up in the copy now that
+    // the line might collapse since the line number doesn't take up space.
+    forEachNode(fragment.querySelectorAll('.text'), function(node) {
+      if (node.textContent.match(/^\s*$/))
+        node.innerHTML = '<br>';
+    });
+  }
+
+  function handleCopy(event) {
+    if (event.target.tagName == 'TEXTAREA')
+      return;
+    var selection = window.getSelection();
+    var range = selection.getRangeAt(0);
+    var selectionFragment = range.cloneContents();
+    sanitizeFragmentForCopy(selectionFragment, shouldStripLineNumbersOnCopy())
+
+    // FIXME: When event.clipboardData.setData supports text/html, remove all the code below.
+    // https://bugs.webkit.org/show_bug.cgi?id=104179
+    var container = document.createElement('div');
+    container.appendChild(selectionFragment);
+    document.body.appendChild(container);
+    selection.selectAllChildren(container);
+
+    setTimeout(function() {
+      container.remove();
+      selection.removeAllRanges();
+      selection.addRange(range);
+    });
+  }
+
   function handleReviewFormLoad() {
     var review_form_contents = $('#reviewform').contents();
     if (review_form_contents[0].querySelector('#form-controls #flags')) {
@@ -2029,6 +2097,7 @@ var CODE_REVIEW_UNITTEST;
     window.tracLinks = tracLinks;
     window.crawlDiff = crawlDiff;
     window.convertAllFileDiffs = convertAllFileDiffs;
+    window.sanitizeFragmentForCopy = sanitizeFragmentForCopy;
     window.displayPreviousComments = displayPreviousComments;
     window.discardComment = discardComment;
     window.addCommentField = addCommentField;