On Mon, Nov 23, 2009 at 02:22, Marc-Antoine Ruel <mar...@google.com> wrote:
> It's really about the unit tests, not in chromium.
>
> I guess we could probably leak the singleton in the unit tests too on
> each reset. Pawel, what do you think?
>
> Note to all the static local makers: you create an implicit atexit()
> each time... Use a leaky singleton instead.
That was my first though as well, but it won't fix the problem really. If a
Singleton touches a MessageLoop, we're going to have bad problems in tests
(because we create and re-instantiate it for different tests, and some tests
don't have MessageLoop at all). And SystemMonitor does, and has to.
SystemMonitor makes one assumption that we can't really where it is
currently: that one MessageLoop is always running through its lifetime. I
have a patch ready (have to submit it yet) that moves SystemMonitor to
chrome/common, as well as HighResolutionTimerManager, and de-Singletonizes
them. They have to be instantiated where currently SystemMonitor::Enable is
called. I verified that we don't get the bad problems that we currently have
with that.
If it doesn't convince you... SystemMonitor was a LeakySingleton. And it
still caused problems because of its assumption that the same MessageLoop
exists through its lifetime. I think that moving it to a place where we can
make that assumption valid is the proper fix.
Another (slightly weird maybe) solution would be to have a per-thread
SystemMonitor, lazily created. Then we could just use ObserverList, which
doesn't use MessageLoop. But IMHO it is more complex than the proposed
solution above, and may have its own problems.
What do you think?
--
Chromium Developers mailing list: chromium-dev@googlegroups.com
View archives, change email options, or unsubscribe:
http://groups.google.com/group/chromium-dev
