Use binary payloads for diff fragments and review request page updates.
Review Request #9461 — Created Dec. 26, 2017 and submitted
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 | |
Col: 17 Missing semicolon. |
reviewbot | |
ES6 offers a bit of syntactic sugar for this: ```javascript const result = { [options.dataType]: xhr.response, }; |
david | |
Col: 27 Unnecessary semicolon. |
reviewbot | |
Might as well put all of these on one line. |
david | |
Similar to my comment above, if this is defined as a fat arrow function stored in a constant then we … |
david |
-
-
If you define this instead as
const onUpdateLoaded = (metadata, html) => {}
, you can then later callparsed.load(onUpdateLoaded)
, avoiding an extra bind per loop iteration. -
ES6 offers a bit of syntactic sugar for this:
```javascript
const result = {
[options.dataType]: xhr.response,
}; -
-
-
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
- Change Summary:
-
- Condensed some lines.
- Changed some functions to pre-bind.
- Commit:
-
88d16f8ff7706c022bd82fef3d211662edfd1f4f923ac71a9450db95e3a58ac71b82cdf4afd933d7
- Diff:
-
Revision 2 (+859 -394)