[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


darin at apple.com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #22908|review?                     |review-
               Flag|                            |




------- Comment #3 from darin at apple.com  2008-08-22 10:47 PDT -------
(From update of attachment 22908)
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 #4 from darin at apple.com  2008-08-22 10:47 PDT -------
(From update of attachment 22908)
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!


-- 
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.