• 
      

    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:
    Completed
    Change Summary:
    Pushed to release-3.0.x (4e58aee)