Firefox performance improved with highlight feature #352
Conversation
|
I can't reproduce this bug in the Can you think of what might be different in your environment versus mine? I'd like to be able to reproduce this locally to be able to properly review the PR |
|
using firefox and old experience, looks like the element target can be reached if the pr is small, when is bigger, some elements gets removed (diff tab) by bitbucket when in the commit tab. this does happen when developping, firefox has some memory management issues when reloading, it cannot remove unused memory correctly, resulting in unmanaged memory. and the webpage gets slow and the error pops, at this time, firefox takes around 6gb ram space. so maybe an issue when the selector is triggered at the same time it gets removed by the DOM. Slow computers might have the same issue. I can't reproduce this bug as well, this morning, but I remember it was coming from |
- valid autocollapse first and return if successful to kill the propagation - added promises to prism highlight - lines over the arbitrary max length of 9999 will be considered as minified
| '#pr-tab-content', | ||
| '#commit', | ||
| '#diff', | ||
| ] | ||
| const diffSelector = 'section.bb-udiff' |
There was a problem hiding this comment.
added back the previous selector but kept validation hidden/visible line 197:202
jwallet
changed the title
| @@ -39,10 +39,10 @@ export default function syntaxHighlight(diff, afterWordDiff) { | |||
| } | |||
|
|
|||
| const $diff = $(diff) | |||
| syntaxHighlightSourceCodeLines($diff) | |||
| syntaxHighlightSourceCodeLines($diff, 'pre.source:not([class*=language])') | |||
|
|
|||
There was a problem hiding this comment.
first time we highlight we never have class language
|
|
||
| afterWordDiff(() => { | ||
| syntaxHighlightSourceCodeLines($diff) | ||
| syntaxHighlightSourceCodeLines($diff, '.addition pre, .deletion pre') |
There was a problem hiding this comment.
second time, we will want to look for tags pre:has(ins) pre:has(del) but the word-diff block only have <ins> elements in the addition block, the deletion block will be re-rendered as well, so we have to re-highlight there too. so the selector is .addition pre, .deletion pre and anyway the mutation-observer will not ask to re-render a diff block that as no word-diff ins/del
| return | ||
| } | ||
|
|
||
| Prism.highlightElement(preElement) |
There was a problem hiding this comment.
Maybe we can use web worker, but Prism warn about using async highlight... web workers are slower and just limiting the text to a max length of 9999 is quite enough. Sure it will skip this line, but the ui does not freezes anymore
Prims.highlightElement(element, true, () => {}); // async with web worker
| const { classList, firstChild, innerText } = preElement | ||
|
|
||
| if (firstChild.$$rbb_isSyntaxHighlighted) { | ||
| reject(new Error('Already highlighted')) |
There was a problem hiding this comment.
Do we have to reject with an Error here? Can't we just ignore this line, resolve, and skip to the next promise? Worried that we will polluting the console and stack with unhandled exceptions that are unactionable and don't really represent an error conceptually
There was a problem hiding this comment.
no no need to add an error. you can just reject. sorry I lately left github for other stuff I do not think I will give more time to projects outside of those I am maintening.
There was a problem hiding this comment.
No problem @jwallet ! Thanks a lot for all you've contributed so far! Greatly appreciated ♥
I got the same issue as the issues mentionned here, I did not added my build index.js at this time in Options - Collapsable files / Delete files. So highlighter was trying to colorize 6 minified lines:
the last 4 were causing the script to crash, they resolved after 9secs each, but even then firefox was not responding, just scrolling up and down, I had to kill the tab after some minutes to kill Prism from keep trying to highlight the whole lines
firefox performance improvements
<add><del>tags)some tweak for diff selector
console.errornew Setfor selectors to make sure no duplicates are therearia-hiddenordisplay:noneclose #348
close #351
close #266