diff --git a/reviewboard/settings.py b/reviewboard/settings.py index c010494a3c0861ecb9cb544d86dba57c2f921963..bb3d98557cf3869ce13cb325de8acab836748447 100644 --- a/reviewboard/settings.py +++ b/reviewboard/settings.py @@ -311,6 +311,7 @@ PIPELINE_JS = { 'rb/js/utils/tests/linkifyUtilsTests.js', 'rb/js/utils/tests/propertyUtilsTests.js', 'rb/js/views/tests/commentDialogViewTests.js', + 'rb/js/views/tests/diffFragmentQueueViewTests.js', 'rb/js/views/tests/draftReviewBannerViewTests.js', 'rb/js/views/tests/fileAttachmentThumbnailViewTests.js', 'rb/js/views/tests/reviewBoxViewTests.js', @@ -372,6 +373,7 @@ PIPELINE_JS = { 'rb/js/views/abstractCommentBlockView.js', 'rb/js/views/abstractReviewableView.js', 'rb/js/views/commentDialogView.js', + 'rb/js/views/diffFragmentQueueView.js', 'rb/js/views/dndUploaderView.js', 'rb/js/views/draftReviewBannerView.js', 'rb/js/views/fileAttachmentCommentBlockView.js', diff --git a/reviewboard/static/rb/js/reviews.js b/reviewboard/static/rb/js/reviews.js index 1bf519302be47c198f597ec4b8128330fb169b0a..18e2a7b77694f7ebb411a6c9995083022e3a3f41 100644 --- a/reviewboard/static/rb/js/reviews.js +++ b/reviewboard/static/rb/js/reviews.js @@ -1,5 +1,4 @@ // State variables -var gPendingDiffFragments = {}; var gReviewBanner = $("#review-banner"); var issueSummaryTableManager; @@ -353,6 +352,13 @@ $.reviewForm = function(review) { function createForm(formHTML) { reviewRequestEditor.incr('editCount'); + /* XXX Remove this global when we can. */ + window.gReviewFormDiffQueue = new RB.DiffFragmentQueueView({ + containerPrefix: 'review_draft_comment_container', + reviewRequestPath: gReviewRequestPath, + queueName: 'review_draft_diff_comments' + }); + dlg = $("
") .attr("id", "review-form") .appendTo("body") // Needed for scripts embedded in the HTML @@ -424,8 +430,7 @@ $.reviewForm = function(review) { $("textarea:first", dlg).focus(); dlg.attr("scrollTop", 0); - loadDiffFragments("review_draft_diff_comments", - "review_draft_comment_container"); + gReviewFormDiffQueue.loadFragments(); } /* @@ -608,71 +613,6 @@ RB.registerForUpdates = function(lastTimestamp, type) { /* - * Queues the load of a diff fragment from the server. - * - * This will be added to a list, which will fetch the comments in batches - * based on file IDs. - * - * @param {string} queue_name The name for this load queue. - * @param {string} comment_id The ID of the comment. - * @param {string} key The key for this request, using the - * filediff and interfilediff. - */ -RB.queueLoadDiffFragment = function(queue_name, comment_id, key) { - if (!gPendingDiffFragments[queue_name]) { - gPendingDiffFragments[queue_name] = {}; - } - - if (!gPendingDiffFragments[queue_name][key]) { - gPendingDiffFragments[queue_name][key] = []; - } - - gPendingDiffFragments[queue_name][key].push(comment_id); -} - - -/* - * Begins the loading of all diff fragments on the page belonging to - * the specified queue and storing in containers with the specified - * prefix. - */ -function loadDiffFragments(queue_name, container_prefix) { - if (!gPendingDiffFragments[queue_name]) { - return; - } - - for (var key in gPendingDiffFragments[queue_name]) { - var comments = gPendingDiffFragments[queue_name][key]; - var url = gReviewRequestPath + "fragments/diff-comments/"; - - for (var i = 0; i < comments.length; i++) { - url += comments[i]; - - if (i != comments.length - 1) { - url += "," - } - } - - url += "/?queue=" + queue_name + - "&container_prefix=" + container_prefix + - "&" + AJAX_SERIAL; - - $.funcQueue("diff_comments").add(function(url) { - var e = document.createElement("script"); - e.type = "text/javascript"; - e.src = url; - document.body.appendChild(e); - }(url)); - } - - // Clear the list. - gPendingDiffFragments[queue_name] = {}; - - $.funcQueue(queue_name).start(); -} - - -/* * Initializes review request pages. * * XXX This is a temporary function that exists while we're transitioning @@ -717,8 +657,6 @@ RB.initReviewRequestPage = function() { model: gCommentIssueManager }); issueSummaryTableManager.render(); - - loadDiffFragments("diff_fragments", "comment_container"); } // vim: set et:sw=4: diff --git a/reviewboard/static/rb/js/views/diffFragmentQueueView.js b/reviewboard/static/rb/js/views/diffFragmentQueueView.js new file mode 100644 index 0000000000000000000000000000000000000000..51622d73c47fe4bd49efbb614a8cd0a3feb89ba0 --- /dev/null +++ b/reviewboard/static/rb/js/views/diffFragmentQueueView.js @@ -0,0 +1,71 @@ +/* + * Queues loading of diff fragments from a page. + * + * This is used to load diff fragments one-by-one, and to intelligently + * batch the loads to only fetch at most one set of fragments per file. + */ +RB.DiffFragmentQueueView = Backbone.View.extend({ + initialize: function() { + this._queue = {}; + }, + + /* + * Queues the load of a diff fragment from the server. + * + * This will be added to a list, which will fetch the comments in batches + * based on file IDs. + */ + queueLoad: function(comment_id, key) { + var queue = this._queue; + + if (!queue[key]) { + queue[key] = []; + } + + queue[key].push(comment_id); + }, + + /* + * Begins the loading of all diff fragments on the page belonging to + * the specified queue. + */ + loadFragments: function() { + var queueName = this.options.queueName, + urlPrefix, + urlSuffix; + + if (!this._queue) { + return; + } + + urlPrefix = this.options.reviewRequestPath + + 'fragments/diff-comments/'; + urlSuffix = '/?queue=' + queueName + + '&container_prefix=' + this.options.containerPrefix + + '&' + AJAX_SERIAL; + + _.each(this._queue, function(comments, key) { + var url = urlPrefix + comments.join(',') + urlSuffix; + + $.funcQueue(queueName).add(_.bind(function() { + this._addScript(url); + }, this)); + }, this); + + // Clear the list. + this._queue = {}; + + $.funcQueue(queueName).start(); + }, + + /* + * Adds a script tag for a diff fragment to the bottom of the page. + */ + _addScript: function(url) { + var e = document.createElement('script'); + + e.type = 'text/javascript'; + e.src = url; + document.body.appendChild(e); + } +}); diff --git a/reviewboard/static/rb/js/views/reviewBoxListView.js b/reviewboard/static/rb/js/views/reviewBoxListView.js index d4b48e511d27807b49e8cc3384aed3e4ae0aeaf6..93477d0cde959a135beb81193611ea363eb65e86 100644 --- a/reviewboard/static/rb/js/views/reviewBoxListView.js +++ b/reviewboard/static/rb/js/views/reviewBoxListView.js @@ -17,6 +17,12 @@ RB.ReviewBoxListView = Backbone.View.extend({ * Initializes the list. */ initialize: function() { + this.diffFragmentQueue = new RB.DiffFragmentQueueView({ + reviewRequestPath: gReviewRequestPath, + containerPrefix: 'comment_container', + queueName: 'diff_fragments' + }); + this._boxes = []; }, @@ -26,6 +32,9 @@ RB.ReviewBoxListView = Backbone.View.extend({ * Each review on the page will be scanned and a ReviewBoxView will * be created. Along with this, a Review model will be created with * the information contained on the page. + * + * Each diff fragment that a comment references will be loaded and + * rendered into the appropriate review boxes. */ render: function() { var pageEditState = this.options.pageEditState, @@ -54,6 +63,8 @@ RB.ReviewBoxListView = Backbone.View.extend({ this._boxes.push(box); }, this); + this.diffFragmentQueue.loadFragments(); + return this; }, diff --git a/reviewboard/static/rb/js/views/tests/diffFragmentQueueViewTests.js b/reviewboard/static/rb/js/views/tests/diffFragmentQueueViewTests.js new file mode 100644 index 0000000000000000000000000000000000000000..7af1db27a952ba9a411f797797627d6bdbef1c8d --- /dev/null +++ b/reviewboard/static/rb/js/views/tests/diffFragmentQueueViewTests.js @@ -0,0 +1,43 @@ +describe('views/DiffFragmentQueueView', function() { + var fragmentQueue; + + beforeEach(function() { + fragmentQueue = new RB.DiffFragmentQueueView({ + containerPrefix: 'container1', + reviewRequestPath: '/r/123/', + queueName: 'diff_fragments' + }); + }); + + describe('Diff fragment loading', function() { + beforeEach(function() { + fragmentQueue.queueLoad("123", 'key1'); + fragmentQueue.queueLoad("124", 'key1'); + fragmentQueue.queueLoad("125", 'key2'); + }); + + it('Fragment queueing', function() { + var queue = fragmentQueue._queue; + + expect(queue.length).not.toBe(0); + + expect(queue.key1.length).toBe(2); + expect(queue.key1).toContain('123'); + expect(queue.key1).toContain('124'); + expect(queue.key2.length).toBe(1); + expect(queue.key2).toContain('125'); + }); + + it('Batch loading', function() { + spyOn(fragmentQueue, '_addScript'); + + fragmentQueue.loadFragments(); + + expect(fragmentQueue._addScript).toHaveBeenCalledWith( + '/r/123/fragments/diff-comments/' + + '123,124/?queue=diff_fragments&' + + 'container_prefix=container1&' + AJAX_SERIAL + ); + }); + }); +}); diff --git a/reviewboard/templates/reviews/review_detail.html b/reviewboard/templates/reviews/review_detail.html index 5386aa407c77077f960305e8a27c4c03e77e1b2a..72a02a2c7afc8330bde206ca03c80f7b1fedde73 100644 --- a/reviewboard/templates/reviews/review_detail.html +++ b/reviewboard/templates/reviews/review_detail.html @@ -192,7 +192,8 @@