Close leak of PausedTimeouts, and clean up ownership See attached patch.
Created attachment 22907 [details] First attempt at PausedTimeouts cleanup WebCore/ChangeLog | 31 ++++++++++++++++++++++++++++++ WebCore/bindings/js/JSDOMWindowBase.cpp | 21 ++++++++++--------- WebCore/bindings/js/JSDOMWindowBase.h | 5 ++- WebCore/bindings/js/ScriptController.cpp | 11 ++++++++++ WebCore/bindings/js/ScriptController.h | 3 ++ WebCore/history/CachedPage.cpp | 4 +- WebCore/page/Chrome.cpp | 24 ++++++++-------------- WebCore/page/JavaScriptDebugServer.cpp | 14 +++++++++--- 8 files changed, 80 insertions(+), 33 deletions(-)
Created attachment 22908 [details] First attempt at PausedTimeouts cleanup WebCore/ChangeLog | 34 ++++++++++++++++++++++++++++++ WebCore/bindings/js/JSDOMWindowBase.cpp | 21 +++++++++-------- WebCore/bindings/js/JSDOMWindowBase.h | 5 ++- WebCore/bindings/js/ScriptController.cpp | 11 +++++++++ WebCore/bindings/js/ScriptController.h | 3 ++ WebCore/history/CachedPage.cpp | 4 +- WebCore/page/Chrome.cpp | 24 ++++++++------------- WebCore/page/JavaScriptDebugServer.cpp | 14 ++++++++--- 8 files changed, 83 insertions(+), 33 deletions(-)
Comment on attachment 22908 [details] First attempt at PausedTimeouts cleanup Good change! + void resumeTimeouts(OwnPtr<PausedTimeouts>&); You're declaring this in ScriptController.h, but I don't see an implemention in ScriptController.cpp. - 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? + 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. I'm going to say review- because of the resumeTimeouts issue. I think the patch won't build!
(In reply to comment #4) > (From update of attachment 22908 [details] [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.
Created attachment 23052 [details] Updated patch with darin's suggested changes WebCore/ChangeLog | 35 ++++++++++++++++++++++++++++++ WebCore/bindings/js/JSDOMWindowBase.cpp | 21 +++++++++-------- WebCore/bindings/js/JSDOMWindowBase.h | 5 ++- WebCore/bindings/js/ScriptController.cpp | 22 ++++++++++++++++++ WebCore/bindings/js/ScriptController.h | 3 ++ WebCore/history/CachedPage.cpp | 4 +- WebCore/page/Chrome.cpp | 26 ++++++++------------- WebCore/page/JavaScriptDebugServer.cpp | 14 ++++++++--- 8 files changed, 96 insertions(+), 34 deletions(-)
(In reply to comment #5) > (In reply to comment #4) > ... thus all > methods must be inlined, so the count is just a member access thus there is no > function call overhead. I meant that all methods must be declared in a header, and thus highly likely to be inlined. I guess I'm choosing more and more code size and cleanliness over possible speed optimizations these days.
Comment on attachment 23052 [details] Updated patch with darin's suggested changes r=me, based on Darin's earlier review.
Thanks to both of you! r35964