2011-01-31 Ojan Vafai <ojan@chromium.org>
authorojan@chromium.org <ojan@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 2 Feb 2011 00:00:36 +0000 (00:00 +0000)
committerojan@chromium.org <ojan@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 2 Feb 2011 00:00:36 +0000 (00:00 +0000)
        Reviewed by Adam Barth.

        Store draft comments in localStorage
        https://bugs.webkit.org/show_bug.cgi?id=52866

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

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

Websites/bugs.webkit.org/ChangeLog
Websites/bugs.webkit.org/code-review-test.html [new file with mode: 0644]
Websites/bugs.webkit.org/code-review.js

index 3127055..95ba069 100644 (file)
@@ -1,3 +1,13 @@
+2011-01-31  Ojan Vafai  <ojan@chromium.org>
+
+        Reviewed by Adam Barth.
+
+        Store draft comments in localStorage
+        https://bugs.webkit.org/show_bug.cgi?id=52866
+
+        * code-review.js:
+        * code-review-test.html
+
 2011-01-20  Ojan Vafai  <ojan@chromium.org>
 
         Fix the review tool for image diffs. We would get a javascript error
diff --git a/Websites/bugs.webkit.org/code-review-test.html b/Websites/bugs.webkit.org/code-review-test.html
new file mode 100644 (file)
index 0000000..8e74dc5
--- /dev/null
@@ -0,0 +1,164 @@
+<div>Tests for some of the easily unittestable parts of code-review.js. You should see a series of "PASSED" lines.</div>
+<div>FIXME: Run these as part of the layout test suite?</div>
+
+<script>CODE_REVIEW_UNITTEST = true</script>
+<script src="code-review.js"></script>
+<pre id="output"></pre>
+<script>
+
+function inherits(childConstructor, parentConstructor) {
+  function tempConstructor() {};
+  tempConstructor.prototype = parentConstructor.prototype;
+  childConstructor.prototype = new tempConstructor();
+  childConstructor.prototype.constructor = childConstructor;
+}
+
+function MockLocalStorage() {
+    this.localStorageStore = {};
+    this.log = [];
+
+    this.getItem = function(key) {
+        this.log.push('getItem:' + key);
+        return this.localStorageStore[key];
+    };
+
+    this.setItem = function(key, value) {
+        // For testing sake, consider having more than 2 items to exceed the storage quota.
+        if (Object.keys(this.localStorageStore).length > 2) {
+            this.log.push('QuotaExceeded on setItem:' + key + ',' + value);
+            throw "QuotaExceeded";
+        }
+        this.log.push('setItem:' + key + ',' + value);
+        this.localStorageStore[key] = value;
+    };
+
+    this.removeItem = function(key) {
+        this.log.push('removeItem:' + key);
+        delete this.localStorageStore[key];
+    };
+
+    this.log_string = function() {
+        return this.log.join('\n');
+    };
+
+    this.key = function(i) {
+        return Object.keys(this.localStorageStore)[i];
+    };
+
+    this.__defineGetter__('length', function() {
+        return Object.keys(this.localStorageStore).length;
+    });
+}
+
+function MockDraftCommentSaver(attachment_id, opt_localStorage) {
+    DraftCommentSaver.call(this, attachment_id, opt_localStorage);
+}
+
+inherits(MockDraftCommentSaver, DraftCommentSaver)
+
+MockDraftCommentSaver.prototype._json = function() {
+    return "{MOCK JSON}";
+}
+
+MockDraftCommentSaver.prototype._should_remove_comments = function(message) {
+    return false;
+}
+
+function log(msg) {
+    document.getElementById('output').innerHTML += msg + '\n\n';
+}
+
+function ASSERT_EQUAL(actual, expected) {
+    if (actual == expected)
+        log('PASSED');
+    else
+        log('FAILED:\ngot:\n' + actual + '\nexpected:\n' + expected + '');
+}
+
+// Basic setItem.
+var ls = new MockLocalStorage();
+new MockDraftCommentSaver('1234', ls).save();
+ASSERT_EQUAL(ls.log_string(), 'setItem:draft-comments-for-attachment-1234,{MOCK JSON}');
+
+// Exceed quota, but succeed after erasing old reviews.
+var ls = new MockLocalStorage();
+ls.localStorageStore = {
+    'draft-comments-for-attachment-1': '{"born-on": 100, "comments":[]}',
+    'draft-comments-for-attachment-2': '{"born-on": 100, "comments":[]}',
+    'draft-comments-for-attachment-3': '{"born-on": 100, "comments":[]}',
+    'draft-comments-for-attachment-4': '{"born-on": ' + (Date.now() - 100) + ', "comments":[]}'
+};
+new MockDraftCommentSaver('1234', ls).save();
+ASSERT_EQUAL(ls.log_string(), 'QuotaExceeded on setItem:draft-comments-for-attachment-1234,{MOCK JSON}\ngetItem:draft-comments-for-attachment-1\ngetItem:draft-comments-for-attachment-2\ngetItem:draft-comments-for-attachment-3\ngetItem:draft-comments-for-attachment-4\nremoveItem:draft-comments-for-attachment-1\nremoveItem:draft-comments-for-attachment-2\nremoveItem:draft-comments-for-attachment-3\nsetItem:draft-comments-for-attachment-1234,{MOCK JSON}');
+
+// Exceed quota after erasing old reviews and fail after prompt.
+var ls = new MockLocalStorage();
+ls.localStorageStore = {
+    'draft-comments-for-attachment-1': '{"born-on": 100, "comments":[]}',
+    'draft-comments-for-attachment-2': '{"born-on": ' + (Date.now() - 100) + ', "comments":[]}',
+    'draft-comments-for-attachment-3': '{"born-on": ' + (Date.now() - 100) + ', "comments":[]}',
+    'draft-comments-for-attachment-4': '{"born-on": ' + (Date.now() - 100) + ', "comments":[]}'
+};
+var mockDraftSaver = new MockDraftCommentSaver('1234', ls);
+mockDraftSaver.save();
+// Second save to ensure that we stop trying to save when we fail the prompt.
+mockDraftSaver.save();
+ASSERT_EQUAL(ls.log_string(), 'QuotaExceeded on setItem:draft-comments-for-attachment-1234,{MOCK JSON}\ngetItem:draft-comments-for-attachment-1\ngetItem:draft-comments-for-attachment-2\ngetItem:draft-comments-for-attachment-3\ngetItem:draft-comments-for-attachment-4\nremoveItem:draft-comments-for-attachment-1\nQuotaExceeded on setItem:draft-comments-for-attachment-1234,{MOCK JSON}');
+
+// Exceed quota after erasing old reviews, but succeed after prompt.
+var ls = new MockLocalStorage();
+ls.localStorageStore = {
+    'draft-comments-for-attachment-1': '{"born-on": 100, "comments":[]}',
+    'draft-comments-for-attachment-2': '{"born-on": ' + (Date.now() - 100) + ', "comments":[]}',
+    'draft-comments-for-attachment-3': '{"born-on": ' + (Date.now() - 100) + ', "comments":[]}',
+    'draft-comments-for-attachment-4': '{"born-on": ' + (Date.now() - 100) + ', "comments":[]}'
+};
+var mockDraftSaver = new MockDraftCommentSaver('1234', ls);
+mockDraftSaver._should_remove_comments = function() { return true; };
+mockDraftSaver.save();
+ASSERT_EQUAL(ls.log_string(), 'QuotaExceeded on setItem:draft-comments-for-attachment-1234,{MOCK JSON}\ngetItem:draft-comments-for-attachment-1\ngetItem:draft-comments-for-attachment-2\ngetItem:draft-comments-for-attachment-3\ngetItem:draft-comments-for-attachment-4\nremoveItem:draft-comments-for-attachment-1\nQuotaExceeded on setItem:draft-comments-for-attachment-1234,{MOCK JSON}\nremoveItem:draft-comments-for-attachment-2\nremoveItem:draft-comments-for-attachment-3\nremoveItem:draft-comments-for-attachment-4\nsetItem:draft-comments-for-attachment-1234,{MOCK JSON}');
+
+// Always exceeds quota, even after erasing all review comments. There should be no setItem calls.
+var ls = new MockLocalStorage();
+ls.setItem = function() {
+    this.log.push('QuotaExceeded on setItem:' + key + ',' + value);
+    throw "QuotaExceeded"; 
+}
+ls.localStorageStore = {
+    'draft-comments-for-attachment-1': '{"born-on": 100, "comments":[]}',
+    'draft-comments-for-attachment-2': '{"born-on": ' + (Date.now() - 100) + ', "comments":[]}',
+    'draft-comments-for-attachment-3': '{"born-on": ' + (Date.now() - 100) + ', "comments":[]}',
+    'draft-comments-for-attachment-4': '{"born-on": ' + (Date.now() - 100) + ', "comments":[]}'
+};
+var mockDraftSaver = new MockDraftCommentSaver('1234', ls);
+mockDraftSaver._should_remove_comments = function() { return true; };
+mockDraftSaver.save();
+// Second save to ensure that we stop trying to save when we fail the prompt.
+mockDraftSaver.save();
+ASSERT_EQUAL(ls.log_string(), 'getItem:draft-comments-for-attachment-1\ngetItem:draft-comments-for-attachment-2\ngetItem:draft-comments-for-attachment-3\ngetItem:draft-comments-for-attachment-4\nremoveItem:draft-comments-for-attachment-1\nremoveItem:draft-comments-for-attachment-2\nremoveItem:draft-comments-for-attachment-3\nremoveItem:draft-comments-for-attachment-4');
+
+var ls = new MockLocalStorage();
+ls.localStorageStore = {
+    'draft-comments-for-attachment-1': '{"born-on": 100, "comments":[]}',
+    'draft-comments-for-attachment-2': '{"born-on": 100, "comments":[{"start_line_id": 1, "end_line_id": 2, "contents": "DUMMY CONTENTS"}, {"start_line_id": 3, "end_line_id": 4, "contents": "DUMMY CONTENTS 2"}]}'
+};
+var comments = new MockDraftCommentSaver('2', ls).saved_comments();
+ASSERT_EQUAL(comments.length, 2);
+ASSERT_EQUAL(ls.log_string(), 'getItem:draft-comments-for-attachment-2');
+
+var ls = new MockLocalStorage();
+ls.localStorageStore = {
+    'draft-comments-for-attachment-1': 'corrupt comments'
+};
+var comments = new MockDraftCommentSaver('1', ls).saved_comments();
+ASSERT_EQUAL(ls.log_string(), 'getItem:draft-comments-for-attachment-1\nremoveItem:draft-comments-for-attachment-1');
+
+var ls = new MockLocalStorage();
+ls.localStorageStore = {
+    'draft-comments-for-attachment-1': '["also corrupt comments"]'
+};
+var comments = new MockDraftCommentSaver('1', ls).saved_comments();
+ASSERT_EQUAL(ls.log_string(), 'getItem:draft-comments-for-attachment-1\nremoveItem:draft-comments-for-attachment-1');
+
+
+</script>
index 79b4975..d626d3a 100644 (file)
@@ -21,6 +21,7 @@
 // LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY
 // OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH
 // DAMAGE.
+var CODE_REVIEW_UNITTEST;
 
 (function() {
   /**
@@ -54,7 +55,7 @@
   if (window.top != window)
     return;
 
-  if (!window.location.search.match(/action=review/)
+  if (!CODE_REVIEW_UNITTEST && !window.location.search.match(/action=review/)
       && !window.location.toString().match(/bugs\.webkit\.org\/PrettyPatch/))
     return;
 
@@ -68,6 +69,7 @@
   var patched_file_contents = {};
   var WEBKIT_BASE_DIR = "http://svn.webkit.org/repository/webkit/trunk/";
   var SIDE_BY_SIDE_DIFFS_KEY = 'sidebysidediffs';
+  var g_displayed_draft_comments = false;
 
   function idForLine(number) {
     return 'line' + number;
@@ -97,7 +99,7 @@
   }
 
   function fileDiffFor(line) {
-    return line.parents('.FileDiff');
+    return $(line).parents('.FileDiff');
   }
 
   function activeCommentFor(line) {
     findCommentPositionFor(line).after(block);
   }
 
-  function addCommentFor(line) {
+  function addDraftComment(start_line_id, end_line_id, contents) {
+    var line = $('#' + end_line_id);
+    var start = numberFrom(start_line_id);
+    var end = numberFrom(end_line_id);
+    for (var i = start; i <= end; i++) {
+      var line = $('#line' + i);
+      line.addClass('commentContext');
+      addDataCommentBaseLine(line, end_line_id);
+    }
+
+    var comment_block = createCommentFor(line);
+    $(comment_block).children('textarea').val(contents);
+    freezeComment(comment_block);
+  }
+
+  function ensureDraftCommentsDisplayed() {
+    if (g_displayed_draft_comments)
+      return;
+    g_displayed_draft_comments = true;
+
+    var comments = g_draftCommentSaver.saved_comments();
+    $(comments).each(function() {
+      addDraftComment(this.start_line_id, this.end_line_id, this.contents);
+    });
+  }
+
+  function DraftCommentSaver(opt_attachment_id, opt_localStorage) {
+    this._attachment_id = opt_attachment_id || attachment_id;
+    this._localStorage = opt_localStorage || localStorage;
+    this._save_comments = true;
+  }
+
+  if (CODE_REVIEW_UNITTEST)
+    window['DraftCommentSaver'] = DraftCommentSaver;
+  
+  DraftCommentSaver.prototype._json = function() {
+    var comments = $('.comment');
+    var comment_store = [];
+    comments.each(function () {
+      var file_diff = fileDiffFor(this);
+      var textarea = $('textarea', this);
+
+      var contents = textarea.val().trim();
+      if (!contents)
+        return;
+
+      var comment_base_line = textarea.attr('data-comment-for');
+      var lines = contextLinesFor(comment_base_line, file_diff);
+
+      comment_store.push({
+        start_line_id: lines.first().attr('id'),
+        end_line_id: comment_base_line,
+        contents: contents
+      });
+    });
+
+    return JSON.stringify({'born-on': Date.now(), 'comments': comment_store});
+  }
+  
+  DraftCommentSaver.prototype.saved_comments = function() {
+    var serialized_comments = this._localStorage.getItem(DraftCommentSaver._keyPrefix + this._attachment_id);
+    if (!serialized_comments)
+      return [];
+
+    var comments = [];
+    try {
+      comments = JSON.parse(serialized_comments).comments;
+    } catch (e) {
+      this._erase_corrupt_comments();
+      return [];
+    }
+    
+    if (comments && !comments.length)
+      return comments;
+    
+    // Sanity check comments are as expected.
+    if (!comments || !comments[0].contents) {
+      this._erase_corrupt_comments();
+      return [];
+    }
+    
+    return comments;
+  }
+  
+  DraftCommentSaver.prototype._erase_corrupt_comments = function() {
+    // FIXME: Show an error to the user instead of logging.
+    console.log('Draft comments were corrupted. Erasing comments.');
+    this.erase();
+  }
+  
+  DraftCommentSaver.prototype.save = function() {
+    if (!this._save_comments)
+      return;
+
+    var key = DraftCommentSaver._keyPrefix + this._attachment_id;
+    var value = this._json();
+
+    if (this._attemptToWrite(key, value))
+      return;
+
+    this._eraseOldCommentsForAllReviews();
+    if (this._attemptToWrite(key, value))
+      return;
+
+    var remove_comments = this._should_remove_comments();
+    if (!remove_comments) {
+      this._save_comments = false;
+      return;
+    }
+
+    this._eraseCommentsForAllReviews();
+    if (this._attemptToWrite(key, value))
+      return;
+
+    this._save_comments = false;
+    // FIXME: Show an error to the user.
+  }
+
+  DraftCommentSaver.prototype._should_remove_comments = function(message) {
+    return prompt('Local storage quota is full. Remove draft comments from all previous reviews to make room?');
+  }
+
+  DraftCommentSaver.prototype._attemptToWrite = function(key, value) {
+    try {
+      this._localStorage.setItem(key, value);
+      return true;
+    } catch (e) {
+      return false;
+    }
+  }
+
+  DraftCommentSaver._keyPrefix = 'draft-comments-for-attachment-';
+
+  DraftCommentSaver.prototype.erase = function() {
+    this._localStorage.removeItem(DraftCommentSaver._keyPrefix + this._attachment_id);
+  }
+
+  DraftCommentSaver.prototype._eraseOldCommentsForAllReviews = function() {
+    this._eraseComments(true);
+  }
+  DraftCommentSaver.prototype._eraseCommentsForAllReviews = function() {
+    this._eraseComments(false);
+  }
+
+  var MONTH_IN_MS = 1000 * 60 * 60 * 24 * 30;
+
+  DraftCommentSaver.prototype._eraseComments = function(only_old_reviews) {
+    var length = this._localStorage.length;
+    var keys_to_delete = [];
+    for (var i = 0; i < length; i++) {
+      var key = this._localStorage.key(i);
+      if (key.indexOf(DraftCommentSaver._keyPrefix) != 0)
+        continue;
+        
+      if (only_old_reviews) {
+        try {
+          var born_on = JSON.parse(this._localStorage.getItem(key))['born-on'];
+          if (Date.now() - born_on < MONTH_IN_MS)
+            continue;
+        } catch (e) {
+          console.log('Deleting JSON. JSON for code review is corrupt: ' + key);
+        }        
+      }
+      keys_to_delete.push(key);
+    }
+
+    for (var i = 0; i < keys_to_delete.length; i++) {
+      this._localStorage.removeItem(keys_to_delete[i]);
+    }
+  }
+  
+  var g_draftCommentSaver = new DraftCommentSaver();
+
+  function saveDraftComments() {
+    ensureDraftCommentsDisplayed();
+    g_draftCommentSaver.save();
+  }
+
+  function createCommentFor(line) {
     if (line.attr('data-has-comment')) {
       // FIXME: This query is overly complex because we place comment blocks
       // after Lines.  Instead, comment blocks should be children of Lines.
 
     var comment_block = $('<div class="comment"><textarea data-comment-for="' + line.attr('id') + '"></textarea><div class="actions"><button class="ok">OK</button><button class="discard">Discard</button></div></div>');
     insertCommentFor(line, comment_block);
+    return comment_block;
+  }
+
+  function addCommentFor(line) {
+    var comment_block = createCommentFor(line);
     comment_block.hide().slideDown('fast', function() {
       $(this).children('textarea').focus();
     });
             $.merge(comments, scanForStyleQueueComments(text));
         });
         displayPreviousComments(comments);
+        ensureDraftCommentsDisplayed();
       });
 
       var details = $(data);
       '<a href="javascript:" class="side-by-side-link">side-by-side</a>';
   }
 
+
   $(document).ready(function() {
     crawlDiff();
     fetchHistory();
     $('.overallComments textarea').bind('click', openOverallComments);
 
     $(document.body).prepend('<div id="comment_form" class="inactive"><div class="winter"></div><div class="lightbox"><iframe id="reviewform" src="attachment.cgi?id=' + attachment_id + '&action=reviewform"></iframe></div></div>');
+    $('#reviewform').bind('load', handleReviewFormLoad);
 
     // Create a dummy iframe and monitor resizes in it's contentWindow to detect when the top document's body changes size.
     // FIXME: Should we setTimeout throttle these?
     loadDiffState();
   });
 
+  function handleReviewFormLoad() {
+    var form = $('#reviewform').contents().find('form')[0];
+    form.addEventListener('submit', eraseDraftComments);
+  }
+  
+  function eraseDraftComments() {
+    g_draftCommentSaver.erase();
+  }
+
   function loadDiffState() {
     var diffstate = localStorage.getItem('code-review-diffstate');
     if (diffstate != 'sidebyside' && diffstate != 'unified')
     return fragment;
   }
 
-  function discardComment() {
-    var line_id = $(this).parentsUntil('.comment').parent().find('textarea').attr('data-comment-for');
+  function discardComment(comment_block) {
+    var line_id = comment_block.find('textarea').attr('data-comment-for');
     var line = $('#' + line_id)
     findCommentBlockFor(line).slideUp('fast', function() {
       $(this).remove();
       line.removeAttr('data-has-comment');
       trimCommentContextToBefore(line, line.attr('data-comment-base-line'));
+      saveDraftComments();
     });
   }
 
     $('.LinkContainer', this).each(function() { this.style.opacity = 0; });
   }
 
+  function handleDiscardComment() {
+    discardComment($(this).parents('.comment'));
+  }
+  
+  function handleAcceptComment() {
+    freezeComment($(this).parents('.comment'));
+    saveDraftComments();
+  }
+
   $('.FileDiff').live('mouseenter', showFileDiffLinks);
   $('.FileDiff').live('mouseleave', hideFileDiffLinks);
   $('.side-by-side-link').live('click', handleSideBySideLinkClick);
   $('.unify-link').live('click', handleUnifyLinkClick);
   $('.ExpandLink').live('click', handleExpandLinkClick);
-  $('.comment .discard').live('click', discardComment);
   $('.frozenComment').live('click', unfreezeComment);
+  $('.comment .discard').live('click', handleDiscardComment);
+  $('.comment .ok').live('click', handleAcceptComment);
 
-  $('.comment .ok').live('click', function() {
-    var comment_textarea = $(this).parentsUntil('.comment').parent().find('textarea');
+  function freezeComment(comment_block) {
+    var comment_textarea = comment_block.find('textarea');
     if (comment_textarea.val().trim() == '') {
-      discardComment.call(this);
+      discardComment(comment_block);
       return;
     }
     var line_id = comment_textarea.attr('data-comment-for');
     var line = $('#' + line_id)
     findCommentBlockFor(line).hide().after($('<div class="frozenComment"></div>').text(comment_textarea.val()));
-  });
+  }
 
   function focusOn(node) {
     $('.focused').removeClass('focused');
     selected.each(function() {
       addDataCommentBaseLine(this, comment_base_line);
     });
+
+    saveDraftComments();
   });
 
   function addDataCommentBaseLine(line, id) {
 
   $('#post_comments').live('click', function() {
     fillInReviewForm();
+    eraseDraftComments();
     $('#reviewform').contents().find('form').submit();
   });
 })();