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 …

daviddavid

Col: 17 Missing semicolon.

reviewbotreviewbot

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

daviddavid

Col: 27 Unnecessary semicolon.

reviewbotreviewbot

Might as well put all of these on one line.

daviddavid

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

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

JSHint

david
  1. 
      
  2. 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. 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. Might as well put all of these on one line.

  5. 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: Closed (submitted)

Change Summary:

Pushed to release-3.0.x (c96ab42)
Loading...