182141 - '#define REPEATING_TIMERS 1' removed, but some code still tests #ifndef

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[John Morrison]

For bug 181961, I had put a printf in the constructor of nsTimerImpl.  I then
noticed that this was being called a couple of times per second, even though I
wasn't using the browser at all (e.g., it was minimized).  I broke in the
debugger and saw that this was nsCaret destroying and recreating the timer,
even though it was creating the timer as a repeating timer.

sfraser had a look, and noticed that it was executing code inside of '#ifndef
REPEATING_TIMERS' (which made it fall back to the old, non-repeating timer
code path).

So, it looks like we removed the define of REPEATING_TIMERS, but have left
several places in the code where we test ifndef. This applies both the branch
and the trunk:

On the trunk (unless I'm reading this wrong):

http://lxr.mozilla.org/seamonkey/search?string=REPEATING_TIMERS

/layout/base/src/nsCaret.cpp, line 1152 -- #ifndef REPEATING_TIMERS
/layout/html/base/src/nsTextFrame.cpp, line 330 -- #ifndef REPEATING_TIMERS
/webshell/tests/viewer/nsThrobber.cpp, line 333 -- #ifndef REPEATING_TIMERS
/xpfe/components/bookmarks/src/nsBookmarksService.cpp, line 2137 -- #ifndef
REPEATING_TIMERS
/xpfe/components/search/src/nsInternetSearchService.cpp, line 716 -- #ifndef
REPEATING_TIMERS

On the branch:

http://lxr.mozilla.org/mozilla1.0/search?string=REPEATING_TIMERS

/modules/plugin/base/src/nsPluginViewer.cpp, line 1376 -- #ifndef REPEATING_TIMERS
/modules/plugin/base/src/nsPluginViewer.cpp, line 1383 -- #endif // REPEATING_TIMERS
/xpcom/threads/nsITimer.h, line 106 -- #define REPEATING_TIMERS 1
/layout/base/src/nsCaret.cpp, line 1119 -- #ifndef REPEATING_TIMERS
/layout/html/base/src/nsObjectFrame.cpp, line 3691 -- #ifndef REPEATING_TIMERS
/layout/html/base/src/nsTextFrame.cpp, line 330 -- #ifndef REPEATING_TIMERS
/webshell/tests/viewer/nsThrobber.cpp, line 333 -- #ifndef REPEATING_TIMERS
/xpfe/components/bookmarks/src/nsBookmarksService.cpp, line 2132 -- #ifndef
REPEATING_TIMERS
/xpfe/components/search/src/nsInternetSearchService.cpp, line 712 -- #ifndef
REPEATING_TIMERS

Comment 1

[John Morrison]

Hmm, maybe I'm wrong about the branch although I can't figure out what 
bonsai is telling me, so I'm updating my branch to look at what's there.

Comment 2

[John Morrison]

So, I was wrong about the branch. No problem there.

Here's a patch to remove the code that was #ifndef REPEATING_TIMERS. I've 
checked that in each of the five classes, it was using a repeating timer
before the #define was removed.

In the case of nsThrobber.cpp, this file is a mess and there is no throbber
in viewer since imglib2 landed. In fact, none of the timer code is called
in that file, but if it were turned back on, the timer would work as it 
originally (abusively) did. Nothing else about the throbber would work, but 
anyways.

I've tested that the <blink> timer, the caret timer and the bookmark service
timers all work as intended. For the search service timer, I know the repeating
timer is firing, but I don't know how to trigger an actual search update (i.e.,
how is this feature supposed to work normally).

Comment 3

[John Morrison]

Attachment: patch to remove the code that was #ifndef REPEATING_TIMERS

Comment 4

[Doug Turner (:dougt)]

Comment on attachment 107675 [details] [diff] [review]
patch to remove the code that was #ifndef REPEATING_TIMERS

we always have cvs for history.  r=dougt.

before you commit this, you should post something to the newsgroup.  the fear
here is that someone has this defined in their build.  if they do, we want to
know why.

Comment 5

[Doug Turner (:dougt)]

jrgm is doing all of the work.  over to him. :-)

Comment 6

[Doug Turner (:dougt)]

Comment on attachment 107675 [details] [diff] [review]
patch to remove the code that was #ifndef REPEATING_TIMERS

damn.  i misread this patch.  i thought that we were removing dead code.  I am
removing my review until I have more time to review this (tomorrow).

Comment 7

[John Morrison]

paging dougt, smfr

Yes, it's not removing dead code, it's removing formerly-unused-but-now-live
code. But I've checked the 5 uses of the repeating timers (win32) and they
are all firing as intended.

Comment 8

[Randell Jesup [:jesup] (needinfo me)]

jrgm: if this doesn't affect the 1.0 branch, please remove mozilla1.0.2

Comment 9

[John Morrison]

I checked this in yesterday evening.