• 
      

    Use binary payloads for diff fragments and review request page updates.

    Review Request #9461 — Created Dec. 26, 2017 and submitted

    Information

    Review Board
    release-3.0.x
    923ac71...

    Reviewers

    Review Board 3.0 shipped with a faster, more condensed method for
    streaming diff fragments for comments, which stored information on the
    comment and the payload of the text. It also introduced a similar format
    for sending updates to the review request page. These payloads sent
    Unicode strings, prefixing them with byte lengths, so that the client
    could read out the appropriate number of bytes for each string. However,
    this didn't actually work in JavaScript, since the entire payload was
    interpreted as a JavaScript string, meaning that byte counts didn't
    matter. A follow-up change attempted to fix this, but didn't support
    Emoji characters.

    This redoes the entire mechanism to fetch actual binary data and to
    parse that data properly, only converting data to strings when needed.
    To do this, a jQuery Ajax transport was added that enables use of
    XMLHttpRequest2's arraybuffer and blob types. This transport was
    designed to follow the basic logic of the standard transport, but
    doesn't include some of the more specialized features that we don't
    need. This effectively limits IE support to 10 and higher, but IE9
    already fails many tests for Review Board 3.0.

    Some of the data that was previously sent over as strings containing an
    integer and a newline delimiter has been changed to send packed 32-bit
    integers instead, reducing both the size of the data and the work
    required by the parser.

    Down the road, these improvements will let us send other forms of binary
    data, such as images or documents, in other similar transfer systems.

    Unit tests pass in Chrome, Firefox, Safari, IE10/11, and Edge.

    Manually tested both diff fragments and review request updates (with
    Unicode/Emoji).

    Description From Last Updated

    If you define this instead as const onUpdateLoaded = (metadata, html) => {}, you can then later call parsed.load(onUpdateLoaded), avoiding …

    david david

    Col: 17 Missing semicolon.

    reviewbot reviewbot

    ES6 offers a bit of syntactic sugar for this: ```javascript const result = { [options.dataType]: xhr.response, };

    david david

    Col: 27 Unnecessary semicolon.

    reviewbot reviewbot

    Might as well put all of these on one line.

    david david

    Similar to my comment above, if this is defined as a fat arrow function stored in a constant then we …

    david david
    Checks run (1 failed, 1 succeeded)
    flake8 passed.
    JSHint failed.

    JSHint

    david
    1. 
        
    2. Show all issues

      If you define this instead as const onUpdateLoaded = (metadata, html) => {}, you can then later call parsed.load(onUpdateLoaded), avoiding an extra bind per loop iteration.

    3. Show all issues

      ES6 offers a bit of syntactic sugar for this:

      ```javascript
      const result = {
      [options.dataType]: xhr.response,
      };

      1. I really want to use this feature, so badly, but it's not worth it in Babel today. It adds extra boilerplate per file (which means one Pipeline bundle may duplicate this a lot), and increases the number of operations needed to perform the assignment, including unnecessary lookups even if defining a brand new object. Just unnecessary overhead without any real benefit today. Shame it's not smart enough to just expand it to what I have in my change...

      1. Yep. Clearly wasn't correct. Didn't break anything before at least, since it was all synchronous.

    4. Show all issues

      Might as well put all of these on one line.

    5. Show all issues

      Similar to my comment above, if this is defined as a fat arrow function stored in a constant then we can avoid binding every fragment

    6. 
        
    chipx86
    david
    1. Ship It!
    2. 
        
    chipx86
    Review request changed
    Status:
    Completed
    Change Summary:
    Pushed to release-3.0.x (c96ab42)