[Webkit-unassigned] [Bug 20469] Close leak of PausedTimeouts, and clean up ownership

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=20469





------- Comment #5 from eric at webkit.org  2008-08-28 04:18 PDT -------
(In reply to comment #4)
> (From update of attachment 22908 [edit])
> Good change!
> 
> +    void resumeTimeouts(OwnPtr<PausedTimeouts>&);
> 
> You're declaring this in ScriptController.h, but I don't see an implemention in
> ScriptController.cpp.

Yeah, it was missing.  Oops!

> -    size_t count = m_deferredFrames.size();
> -    for (size_t i = 0; i < count; ++i)
> +    for (size_t i = 0; i < m_deferredFrames.size(); ++i)
> 
> Why this change? It makes the loop potentially slower by putting the calls to
> size() inside the loop instead of outside. Is there a benefit to the change?

It made the code smaller, and I know that Vector is a template, and thus all
methods must be inlined, so the count is just a member access thus there is no
function call overhead.  Perhaps that's cheating?  I can change it back if you
feel strongly.

> +        OwnPtr<PausedTimeouts> timeouts = m_pausedTimeouts[i].second;
> 
> +            OwnPtr<PausedTimeouts> timeouts(m_pausedTimeouts.take(frame));
> 
> Why did you use assignment syntax in the first case, but not the second.
> Consistent would be better.

Fixed.

> I'm going to say review- because of the resumeTimeouts issue. I think the patch
> won't build!

Yup.  Builds and runs fine on Mac, but wouldn't have on Windows!  Fixed.


-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.