Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(286)

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 years, 4 months ago by Sebastian Noack
Modified:
4 years, 4 months ago
Reviewers:
Wladimir Palant, kzar
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
4 years, 4 months ago (2015-04-08 10:27:07 UTC) #1
kzar
LGTM
4 years, 4 months ago (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 ...
4 years, 4 months ago (2015-04-14 16:06:16 UTC) #3
Sebastian Noack
4 years, 4 months ago (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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5