Summary: | Close leak of PausedTimeouts, and clean up ownership | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Eric Seidel (no email) <eric> | ||||||||
Component: | New Bugs | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | ap, sam | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Mac | ||||||||||
OS: | OS X 10.5 | ||||||||||
Attachments: |
|
Description
Eric Seidel (no email)
2008-08-20 16:54:36 PDT
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!
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.
|