Fix binary diff rendering.
Review Request #10766 — Created Oct. 26, 2019 and discarded
Follows up on previous binary file changes. This will allow the
frontend to render binary files for visual reviewing. This will
only display a diff if the binary file is a image. If the file is
a not a binary, exe for example, it will display this message
'This is a binary file. The content cannot be displayed.' This also
fixes problems where if the same commit with a binary is upload
multiple times the backend would hard fail. Now it returns without
doing anything.
Walked through a standard review process: posted changes,
interact with changes, commented, and posted changes. Also
covered by previouse tests written for image review UI.
| Summary | ID |
|---|---|
| cce36a415102bebecc26af8669378b2747212328 | |
| 39e05b50ef7f8ff2e758f83cfc282d4e820f0dab | |
| bb8bf7fbaa1c845b22044320c34aaf7797866003 | |
| ad1c108f3969d54da1abf31e8f507a54c4dd14fc | |
| 921855571820bd43a761197d2c8742390127ae5d |
| Description | From | Last Updated |
|---|---|---|
|
Since there isn't any unit tests for these changes, it would be helpful if listed the testing that you did … |
 amalik2
|
|
|
The title of the review request should also be more descriptive (it should be a complete sentence). I think there … |
amalik2
|
|
|
E501 line too long (97 > 79 characters) |
 reviewbot
|
|
|
E501 line too long (102 > 79 characters) |
reviewbot
|
|
|
E722 do not use bare except' |
reviewbot
|
|
|
E501 line too long (103 > 79 characters) |
reviewbot
|
|
|
E501 line too long (103 > 79 characters) |
reviewbot
|
|
|
Col: 55 Expected ')' to match '(' from line 381 and instead saw 's'. |
reviewbot
|
|
|
Col: 56 Expected '{' and instead saw '.'. |
reviewbot
|
|
|
Col: 56 Expected an identifier and instead saw '.'. |
reviewbot
|
|
|
Col: 56 Expected an assignment or function call and instead saw an expression. |
reviewbot
|
|
|
Col: 57 Missing semicolon. |
reviewbot
|
|
|
Col: 79 Missing semicolon. |
reviewbot
|
|
|
Col: 79 Expected an identifier and instead saw ')'. |
reviewbot
|
|
|
Col: 79 Expected an assignment or function call and instead saw an expression. |
reviewbot
|
|
|
Col: 80 Missing semicolon. |
reviewbot
|
|
|
Col: 27 Expected an identifier and instead saw 'else'. |
reviewbot
|
|
|
Col: 27 Expected an assignment or function call and instead saw an expression. |
reviewbot
|
|
|
Col: 31 Missing semicolon. |
reviewbot
|
|
|
E501 line too long (103 > 79 characters) |
reviewbot
|
|
|
Col: 55 Expected ')' to match '(' from line 381 and instead saw 's'. |
reviewbot
|
|
|
Col: 56 Expected '{' and instead saw '.'. |
reviewbot
|
|
|
Col: 56 Expected an identifier and instead saw '.'. |
reviewbot
|
|
|
Col: 56 Expected an assignment or function call and instead saw an expression. |
reviewbot
|
|
|
Col: 57 Missing semicolon. |
reviewbot
|
|
|
Col: 79 Missing semicolon. |
reviewbot
|
|
|
Col: 79 Expected an identifier and instead saw ')'. |
reviewbot
|
|
|
Col: 79 Expected an assignment or function call and instead saw an expression. |
reviewbot
|
|
|
Col: 80 Missing semicolon. |
reviewbot
|
|
|
Col: 27 Expected an identifier and instead saw 'else'. |
reviewbot
|
|
|
Col: 27 Expected an assignment or function call and instead saw an expression. |
reviewbot
|
|
|
Col: 31 Missing semicolon. |
reviewbot
|
|
|
This will block non-image review UIs, which will brick 3rd party extensions. |
 brennie
|
|
|
Since orig_review_ui is a local variable that does not actually get used by the returned renderer, it may make more … |
amalik2
|
|
|
Same thing as the above comment |
amalik2
|
|
|
$container.eq(0).html(xhr.responseText) |
brennie
|
- Summary:
-
WIP: Binary Diff RenderingBinary Diff Rendering
- Description:
-
~ Debug statements and some backend fixes
~ Follows up on previous binary file changes. This will allow the frontend to render binary files for visual reviewing. This will only display a diff if the binary file is a image. If the file is a not a binary, exe for example, it will display this message 'This is a binary file. The content cannot be displayed.' This also fixes problems where if the same commit with a binary is upload multiple times the backend would hard fail. Now it returns without doing anything.
- Commits:
-
Summary ID a8832f3835561f2b34701936e838dfb8c860581e 605b595db017e27f6e713ded54177462d06a8005
- Commits:
-
Summary ID 605b595db017e27f6e713ded54177462d06a8005 605b595db017e27f6e713ded54177462d06a8005 84368dc50c63ad8a3c23f757b1f65f51303cb1c7
Checks run (2 succeeded)
- Commits:
-
Summary ID 605b595db017e27f6e713ded54177462d06a8005 84368dc50c63ad8a3c23f757b1f65f51303cb1c7 cce36a415102bebecc26af8669378b2747212328 39e05b50ef7f8ff2e758f83cfc282d4e820f0dab
Checks run (2 failed)
flake8
JSHint
- Commits:
-
Summary ID cce36a415102bebecc26af8669378b2747212328 39e05b50ef7f8ff2e758f83cfc282d4e820f0dab cce36a415102bebecc26af8669378b2747212328 39e05b50ef7f8ff2e758f83cfc282d4e820f0dab bb8bf7fbaa1c845b22044320c34aaf7797866003
Checks run (1 failed, 1 succeeded)
JSHint
- Commits:
-
Summary ID cce36a415102bebecc26af8669378b2747212328 39e05b50ef7f8ff2e758f83cfc282d4e820f0dab bb8bf7fbaa1c845b22044320c34aaf7797866003 cce36a415102bebecc26af8669378b2747212328 39e05b50ef7f8ff2e758f83cfc282d4e820f0dab bb8bf7fbaa1c845b22044320c34aaf7797866003 ad1c108f3969d54da1abf31e8f507a54c4dd14fc
Checks run (2 succeeded)
-
-
Since there isn't any unit tests for these changes, it would be helpful if listed the testing that you did for this review request as well.
-
Since orig_review_ui is a local variable that does not actually get used by the returned renderer, it may make more sense to move this if statement to the beginning of this block
-
-
-
The title of the review request should also be more descriptive (it should be a complete sentence). I think there was a page on notion that discusses this but I can't remember what it is, but you can look at recent review requests that have landed for an example.
Each line in the description should be no more than 72 characters as well.
- Description:
-
~ Follows up on previous binary file changes. This will allow the frontend to render binary files for visual reviewing. This will only display a diff if the binary file is a image. If the file is a not a binary, exe for example, it will display this message 'This is a binary file. The content cannot be displayed.' This also fixes problems where if the same commit with a binary is upload multiple times the backend would hard fail. Now it returns without doing anything.
~ Follows up on previous binary file changes. This will allow the
+ frontend to render binary files for visual reviewing. This will + only display a diff if the binary file is a image. If the file is + a not a binary, exe for example, it will display this message + 'This is a binary file. The content cannot be displayed.' This also + fixes problems where if the same commit with a binary is upload + multiple times the backend would hard fail. Now it returns without + doing anything.
- Commits:
-
Summary ID cce36a415102bebecc26af8669378b2747212328 39e05b50ef7f8ff2e758f83cfc282d4e820f0dab bb8bf7fbaa1c845b22044320c34aaf7797866003 ad1c108f3969d54da1abf31e8f507a54c4dd14fc cce36a415102bebecc26af8669378b2747212328 39e05b50ef7f8ff2e758f83cfc282d4e820f0dab bb8bf7fbaa1c845b22044320c34aaf7797866003 ad1c108f3969d54da1abf31e8f507a54c4dd14fc 921855571820bd43a761197d2c8742390127ae5d
