204005 - optimize caret timer usage

Status: RESOLVED FIXED

(Core :: DOM: Selection, defect)

Description

[leon.zhang]

Comment 1

[leon.zhang]

In caret process, timer always be created in PrimeTimer each time, to enhance
perf, we can reInit it and not create each time

Comment 2

[leon.zhang]

Attachment: proposed fix

Comment 3

[leon.zhang]

Attachment: a new patch

Comment 4

[Simon Fraser [no longer active]]

Comment on attachment 122165 [details] [diff] [review]
a new patch

I think StopBlinking should kill the timer.

Comment 5

[Simon Fraser [no longer active]]

The first patch looks good to me.

Comment 6

[Simon Fraser [no longer active]]

Oh, and the first patch seems OK on Mac OS X too.

Comment 7

[Boris Zbarsky [:bzbarsky]]

pav, is re-initing repeating timers supported?  If so, could we document that
clearly in nsITimer.idl?

Comment 8

[Jo Hermans]

I remember a very serious problem in the 1.3a and 1.3b builds on Mac OS X, with
timers that got re-inited : see bug 184363. The timer was the one used to show
tooltips for title-tags. We had to back out that change, which was a shame,
because it was a good performance speedup on pages with lots of title-tags on
their links. The problem was probably that we were firing and re-initing timers
in a very fast pace, which might have cuased multithreading problems.

Are you sure that it's safe to re-init timers ?

Comment 9

[Simon Fraser [no longer active]]

I was aware of that tooltip timer issue, which is why I tested the patch here on
OS X.

If we declare that it's safe to re-init timers, I'd like to see a new 'reinit'
method added to nsITimer, to make it clear to users of the API.

Comment 10

[leon.zhang]

Comment on attachment 122163 [details] [diff] [review]
proposed fix

bz, can give sr?

Comment 11

[Boris Zbarsky [:bzbarsky]]

I will not be able to sr this in the near future, since I haven't convinced
myself that it's safe to reinit the timer, and I do not have the time to read
the bugs and code necessary to do that.

"Near future" here is probably "until June 14".

Comment 12

[leon.zhang]

According to comments and patches of bug 181961,it seems that timer can be
ReInited in current trunk.

Comment 13

[Brendan Eich [:brendan]]

Comment on attachment 122163 [details] [diff] [review]
proposed fix

bz, I can sr this, but then I'm the one that made timer re-init work.  If
someone has doubts, please raise them with reference to specific code.

I don't think this should go into 1.4final.

I'm not planning to add a bunch of reinit methods to nsITimer, but I did add
comments when I was there last to say that re-init works, and that it can be
preferable to re-init, to save the overhead of destroying and recreating a
timer.

/be

Comment 14

[Simon Fraser [no longer active]]

fwiw, while experimenting with this, we did see odd results when reinitting
precise timers; making the timer a slack timer allows reinit to work OK.

Comment 15

[leon.zhang]

checked in