Improve the efficiency and cleanliness of diff comment fragment loading.

Review Request #9135 — Created Aug. 14, 2017 and submitted

Information

Review Board
release-3.0.x
aa63a16...

Reviewers

Diff comment fragment loading used to depend on server-side templates
having knowledge of some of the internals of how fragments should be
rendered. The URL for fetching fragments would take in the name of the
loading queue and the prefix for diff containers, and the resulting HTML
would inject the HTML for the fragments into the DOM, followed by
triggering the next item in the queue. Since this was driven by
JavaScript, we had to load the fragments by injecting a <script> tag
and listening for the loaded event. We also had to make the HTML
JavaScript-safe, escaping much of the content and increasing the size of
the data.

This change reworks the loading to drive more of the process
client-side, and to reduce the amount of work needed to get fragments to
the client. The fragment content is no longer sent over via
server-generated JavaScript, but rather in a text format similar to that
used by the new review request updates payloads. The format encodes the
comment ID, HTML length, and raw HTML content in a streamable format,
which can be fetched by the client and quickly processed.

Since the server-generated JavaScript has been removed and the
load/render logic is now completely controlled client-side, the client
now has complete control over the queue, making that an implementation
detail. Details like the queue name and container prefix also no longer
need to be passed in.

The view serving up the fragments is a bit simpler now as well, since it
no longer needs to care about queue names or the container prefix.
Instead, it takes a new option specifying whether expansion is allowed,
which controls whether the expansion controls are returned in the HTML.
The logic for permitting expansion only for published comments (which is
there because of rendering issues in the Review Dialog) is now
controlled client-side, and allowed server-side.

The URL for the view has also been marked as internal by prefixing with
an underscore.

The result of all this is much cleaner code and logic that isn't driven
partially by the server-side results, with a decent bandwidth reduction
(often ~30% in my tests for real-world reviews, with ~20ms in time
savings).

Python and JavaScript unit tests pass.

Tested with review requests containing various diffs. Verified the correct
fragments were loading for the correct comments.

Tested viewing diff fragments for comments in the review dialog.

Simulated errors and saw that they were showing up properly.

Description From Last Updated

These two lines could be combined.

daviddavid

The performance benefit to pulling length out into a variable is zero on the vast majority of browsers and < …

daviddavid

It's probably more correct to use content_type='text/plain; charset=utf-8'

daviddavid

Just in case, how about _.isFunction(options.onDone)?

daviddavid
chipx86
david
  1. 
      
  2. reviewboard/reviews/tests/test_views.py (Diff revision 1)
     
     
     
    Show all issues

    These two lines could be combined.

  3. Show all issues

    The performance benefit to pulling length out into a variable is zero on the vast majority of browsers and < 1% overhead on a no-op loop. Doesn't seem worth it here.

  4. 
      
chipx86
chipx86
david
  1. 
      
  2. reviewboard/reviews/views.py (Diff revision 3)
     
     
    Show all issues

    It's probably more correct to use content_type='text/plain; charset=utf-8'

  3. Show all issues

    Just in case, how about _.isFunction(options.onDone)?

  4. 
      
chipx86
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-3.0.x (4e58aee)
Loading...