Title: [136772] trunk/Websites/bugs.webkit.org
- Revision
- 136772
- Author
- o...@chromium.org
- Date
- 2012-12-05 15:39:09 -0800 (Wed, 05 Dec 2012)
Log Message
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:
Modified Paths
Diff
Modified: trunk/Websites/bugs.webkit.org/ChangeLog (136771 => 136772)
--- trunk/Websites/bugs.webkit.org/ChangeLog 2012-12-05 23:28:42 UTC (rev 136771)
+++ trunk/Websites/bugs.webkit.org/ChangeLog 2012-12-05 23:39:09 UTC (rev 136772)
@@ -1,3 +1,22 @@
+2012-12-05 Ojan Vafai <o...@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 <o...@chromium.org>
Use sticky positioning for the code review toolbar
Modified: trunk/Websites/bugs.webkit.org/PrettyPatch/PrettyPatch.rb (136771 => 136772)
--- trunk/Websites/bugs.webkit.org/PrettyPatch/PrettyPatch.rb 2012-12-05 23:28:42 UTC (rev 136771)
+++ trunk/Websites/bugs.webkit.org/PrettyPatch/PrettyPatch.rb 2012-12-05 23:39:09 UTC (rev 136772)
@@ -501,7 +501,7 @@
}
</style>
<script src=""
-<script src=""
+<script src=""
</head>
EOF
Modified: trunk/Websites/bugs.webkit.org/code-review-test.html (136771 => 136772)
--- trunk/Websites/bugs.webkit.org/code-review-test.html 2012-12-05 23:28:42 UTC (rev 136771)
+++ trunk/Websites/bugs.webkit.org/code-review-test.html 2012-12-05 23:39:09 UTC (rev 136772)
@@ -325,6 +325,75 @@
document.getElementById('diff-content').innerHTML = '';
}
+function testSanitizeFragmentForCopy() {
+ var fragment = document.createElement('div');
+ fragment.innerHTML = '<div class="FileDiff">' +
+ '<h1><a href="" +
+ '<div class="FileDiffLinkContainer LinkContainer" style="opacity: 0;"><a href="" 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="" +
+ '<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="" +
+ '<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"));
Modified: trunk/Websites/bugs.webkit.org/code-review.js (136771 => 136772)
--- trunk/Websites/bugs.webkit.org/code-review.js 2012-12-05 23:28:42 UTC (rev 136771)
+++ trunk/Websites/bugs.webkit.org/code-review.js 2012-12-05 23:39:09 UTC (rev 136772)
@@ -1095,9 +1095,13 @@
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="" id="line-number-on-copy-link"></a> ' +
+ diffLinksHtml() +
+ '</div>' +
'<a href="" class="more">[more]</a>' +
'<div class="more-help inactive">' +
'<div class="winter"></div>' +
@@ -1125,8 +1129,72 @@
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 @@
window.tracLinks = tracLinks;
window.crawlDiff = crawlDiff;
window.convertAllFileDiffs = convertAllFileDiffs;
+ window.sanitizeFragmentForCopy = sanitizeFragmentForCopy;
window.displayPreviousComments = displayPreviousComments;
window.discardComment = discardComment;
window.addCommentField = addCommentField;
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
http://lists.webkit.org/mailman/listinfo/webkit-changes
