Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

Issue 4806142987337728: Issue 2215 - Prevent page from freezing when highlighting a lot of elements (Closed)

Created:
April 8, 2015, 10:25 a.m. by Sebastian Noack
Modified:
April 20, 2015, 11:02 a.m.
Reviewers:
kzar, Wladimir Palant
Visibility:
Public.

Description

Issue 2215 - Prevent page from freezing when highlighting a lot of elements

Patch Set 1 : #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+24 lines, -3 lines) Patch
M include.postload.js View 2 chunks +24 lines, -3 lines 2 comments Download

Messages

Total messages: 4
Sebastian Noack
April 8, 2015, 10:27 a.m. (2015-04-08 10:27:07 UTC) #1
kzar
LGTM
April 8, 2015, 10:53 a.m. (2015-04-08 10:53:30 UTC) #2
Wladimir Palant
http://codereview.adblockplus.org/4806142987337728/diff/5741031244955648/include.postload.js File include.postload.js (right): http://codereview.adblockplus.org/4806142987337728/diff/5741031244955648/include.postload.js#newcode118 include.postload.js:118: }, 0); Using setInterval() instead of setTimeout() here has ...
April 14, 2015, 4:06 p.m. (2015-04-14 16:06:16 UTC) #3
Sebastian Noack
April 20, 2015, 11:02 a.m. (2015-04-20 11:02:21 UTC) #4
http://codereview.adblockplus.org/4806142987337728/diff/5741031244955648/incl...
File include.postload.js (right):

http://codereview.adblockplus.org/4806142987337728/diff/5741031244955648/incl...
include.postload.js:118: }, 0);
On 2015/04/14 16:06:17, Wladimir Palant wrote:
> Using setInterval() instead of setTimeout() here has a disadvantage: if you
> error out in the function body and don't call clearInterval(), it will keep
> running forever. If you call setTimeout() at the end of the function then it
> will terminate automatically.
> 
> In terms of statements it's exactly the same - you need
> |highlightedElementsInterval = setTimeout(arguments.callee, 0)| inside the
> function but you don't need to call clearInteval() any more.
> 
> Up to you whether you want to address this issue.

I see your point, but IMO not worth a follow up commit, in particular since the
current implementation has already been tested and approved by QA.

Powered by Google App Engine
This is Rietveld