JSHint
passed.
PEP8 Style Checker
passed.
Pyflakes
passed.
chipx86 got a fish trophy!
Improve performance and visuals when resizing the diff viewer window.
Review Request #8888 — Created April 7, 2017 and submitted
Resizing the diff viewer was not very smooth, and some UI elements would stutter. The active chunk highlighter would sort of jump around a little bit or lag behind when the chunks changed position or height. There was also a horizontal scrollbar that would flicker on and off when resizing the window smaller. Along with this, it was just a bit too slow to resize. Many of these problems were ordering issues in event processing and style issues. We also just had some less-than-optimal logic in resize handlers. Resize handling is now driven primarily by the diff viewer page, allowing some coordination to take place. Upon receiving a resize event (or a layout is otherwise scheduled), the new _.throttleLayout() is called. This makes use of requestAnimationFrame() to schedule the function used to compute the new layout. If there are subsequent calls to this before the new layout has been computed (which can happen during resizes with very heavy pages), they'll be ignored, keeping us from swamping the browser. The layout handler tells each diff to update its layout, and then tells the chunk highlighter to update. This ensures that any layout changes from the diffs can be immediately reflected in the highlighter, and prevents all this from constantly happening while resizing the page (which caused resizes to be slow and events to accumulate). The chunk highlighter's been optimized quite a bit. Some jQuery calls, which are more expensive than needed for this work, have been replaced with element attribute lookups, and the logic for what CSS to apply and when has been reworked. The width is no longer set on resize. Instead, we simply dock to the left and right of the container, allowing the browser to handle that and preventing the horizontal scrollbars from appearing. Resize handlers are also more aware of vertical vs. horizontal window sizes, preventing any work from occurring if the resize wouldn't result in any changes. Now that the orders are guaranteed, we can also remove the defer() for the chunk highlighter, which allows the highlighter to smoothly appear bound to the chunk as its position or height changes. The result is much smoother resizing, improved logic, and simplified code that's easier to reason about.
Tested on Chrome, Firefox, and Safari. Resizing was noticeably improved
on all browsers, particularly on Firefox and Safari.Profiled resizing diffs and switching chunks before and after.
Unit tests pass.
| Description | From | Last Updated |
|---|---|---|
|
I think we should use _.throttle instead of _.debounce. Debounce will wait at least the delay before the first call, … |
 david
|
|
|
Same thing here: adding in a mandatory 300ms delay is going to make things perceive as slower for the general … |
david
|
|
|
This can just be done with the variable definition. |
david
|
|
|
requestAnimationFrame is available in all the browsers we support. |
david
|
-
-
I think we should use
_.throttleinstead of_.debounce. Debounce will wait at least the delay before the first call, whereas throttle will call it immediately.How did you come up with 300ms?
-
Same thing here: adding in a mandatory 300ms delay is going to make things perceive as slower for the general case. I think throttle was correct.
-
- Change Summary:
-
- Switched to using
requestAnimationFrame, to let the browser help manage its own update schedule, instead of debouncing/throttling with a fixed time interval. - Fixed initial sizing of comment flags and size jumps on window resize.
- Moved a variable initialization inline into the declaration list.
- Switched to using
- Description:
-
Resizing the diff viewer was not very smooth, and some UI elements would
stutter. The active chunk highlighter would sort of jump around a little bit or lag behind when the chunks changed position or height. There was also a horizontal scrollbar that would flicker on and off when resizing the window smaller. Along with this, it was just a bit too slow to resize. Many of these problems were ordering issues in event processing and
style issues. We also just had some less-than-optimal logic in resize handlers. Resize handling is now driven primarily by the diff viewer page,
~ allowing some coordination to take place. The page listens for resize ~ events and debounces, waiting 300ms for resize events to stop coming in. ~ It then tells each diff to update its layout, and then tells the chunk ~ highlighter to update. This ensures that any layout changes from the ~ diffs can be immediately reflected in the highlighter, and prevents all ~ this from constantly happening while resizing the page (which caused ~ resizes to be slow and events to accumulate). ~ allowing some coordination to take place. Upon receiving a resize event ~ (or a layout is otherwise scheduled), the new _.throttleLayout() is ~ called. This makes use of requestAnimationFrame() (if available) to ~ schedule the function used to compute the new layout. If there are ~ subsequent calls to this before the new layout has been computed (which ~ can happen during resizes with very heavy pages), they'll be ignored, ~ keeping us from swamping the browser. Older browsers that don't have + requestAnimationFrame() will fall back to a 100ms timer. + + The layout handler tells each diff to update its layout, and then tells
+ the chunk highlighter to update. This ensures that any layout changes + from the diffs can be immediately reflected in the highlighter, and + prevents all this from constantly happening while resizing the page + (which caused resizes to be slow and events to accumulate). The chunk highlighter's been optimized quite a bit. Some jQuery calls,
which are more expensive than needed for this work, have been replaced with element attribute lookups, and the logic for what CSS to apply and when has been reworked. The width is no longer set on resize. Instead, we simply dock to the left and right of the container, allowing the browser to handle that and preventing the horizontal scrollbars from appearing. Resize handlers are also more aware of vertical vs. horizontal window
sizes, preventing any work from occurring if the resize wouldn't result in any changes. Now that the orders are guaranteed, we can also remove the defer() for
the chunk highlighter, which allows the highlighter to smoothly appear bound to the chunk as its position or height changes. The result is much smoother resizing, improved logic, and simplified
code that's easier to reason about. - Commit:
-
15adbff9ca5134dd1f2115275b783dd8aaddc7e4e61c07b9b13046a04a53bda61f108f62dd14db6b
- Diff:
-
Revision 2 (+231 -118)
Checks run (3 succeeded)
JSHint
passed.
PEP8 Style Checker
passed.
Pyflakes
passed.
- Change Summary:
-
requestAnimationFrameis now a requirement.- Removed an unintentional initial call to the callback that was left in for a test.
- Description:
-
Resizing the diff viewer was not very smooth, and some UI elements would
stutter. The active chunk highlighter would sort of jump around a little bit or lag behind when the chunks changed position or height. There was also a horizontal scrollbar that would flicker on and off when resizing the window smaller. Along with this, it was just a bit too slow to resize. Many of these problems were ordering issues in event processing and
style issues. We also just had some less-than-optimal logic in resize handlers. Resize handling is now driven primarily by the diff viewer page,
allowing some coordination to take place. Upon receiving a resize event (or a layout is otherwise scheduled), the new _.throttleLayout() is ~ called. This makes use of requestAnimationFrame() (if available) to ~ schedule the function used to compute the new layout. If there are ~ subsequent calls to this before the new layout has been computed (which ~ can happen during resizes with very heavy pages), they'll be ignored, ~ keeping us from swamping the browser. Older browsers that don't have ~ called. This makes use of requestAnimationFrame() to schedule the ~ function used to compute the new layout. If there are subsequent calls ~ to this before the new layout has been computed (which can happen during ~ resizes with very heavy pages), they'll be ignored, keeping us from ~ swamping the browser. - requestAnimationFrame() will fall back to a 100ms timer. The layout handler tells each diff to update its layout, and then tells
the chunk highlighter to update. This ensures that any layout changes from the diffs can be immediately reflected in the highlighter, and prevents all this from constantly happening while resizing the page (which caused resizes to be slow and events to accumulate). The chunk highlighter's been optimized quite a bit. Some jQuery calls,
which are more expensive than needed for this work, have been replaced with element attribute lookups, and the logic for what CSS to apply and when has been reworked. The width is no longer set on resize. Instead, we simply dock to the left and right of the container, allowing the browser to handle that and preventing the horizontal scrollbars from appearing. Resize handlers are also more aware of vertical vs. horizontal window
sizes, preventing any work from occurring if the resize wouldn't result in any changes. Now that the orders are guaranteed, we can also remove the defer() for
the chunk highlighter, which allows the highlighter to smoothly appear bound to the chunk as its position or height changes. The result is much smoother resizing, improved logic, and simplified
code that's easier to reason about. - Commit:
-
e61c07b9b13046a04a53bda61f108f62dd14db6be27eb384ca6f360947c0978183e71cf441aff6f1
- Diff:
-
Revision 3 (+216 -118)
Checks run (3 succeeded)
JSHint
passed.
PEP8 Style Checker
passed.
Pyflakes
passed.