[webkit-reviews] review denied: [Bug 24719] QTMovieWinTimer logic inversion : [Attachment 28789] proposed patch

[bugzilla-daemon at webkit.org]

Adam Roben (aroben) <aroben at apple.com> has denied Eric Carlson
<eric.carlson at apple.com>'s request for review:
Bug 24719: QTMovieWinTimer logic inversion
https://bugs.webkit.org/show_bug.cgi?id=24719

Attachment 28789: proposed patch
https://bugs.webkit.org/attachment.cgi?id=28789&action=review

------- Additional Comments from Adam Roben (aroben) <aroben at apple.com>
> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog (revision 41862)
> +++ WebCore/ChangeLog (working copy)
> @@ -1,3 +1,25 @@
> +2009-03-20  Eric Carlson  <eric.carlson at apple.com>
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   <rdar://problem/6704282>
> +	   https://bugs.webkit.org/show_bug.cgi?id=24719
> +	   QTMovieWinTimer logic inversion
> +
> +	   Fix logic inversion in the Win32 timer used by QTMovieWin that
caused it to always
> +	   use SetTimer, even when the intervals was below USER_TIMER_MINIMUM.
A side effect of
> +	   this is that the movie timer is sometimes blocked for significant
amounts of time 
> +	   because WM_TIMER messages are not processed when the thread's
message queue has any
> +	   higher priority messages, and WebCore/Win's timer uses PostMessage
for low interval
> +	   timers.

I'm not sure from this comment whether the movie timer is now blocked due to
your change, or whether it was getting blocked before your change and no longer
will.

> +	   Not possible to make a test for this because it is so timeing
dependant.

Typo: timeing

> @@ -51,7 +51,9 @@ static LRESULT CALLBACK TimerWindowWndPr
>	   processingCustomTimerMessage = true;
>	   sharedTimerFiredFunction();
>	   processingCustomTimerMessage = false;
> -    } else
> +    } else if (message == WM_TIMER) {
> +	   sharedTimerFiredFunction();
> +	} else

Should we call KillTimer here? WebCore does that. If we don't do that, WM_TIMER
will continue being sent to this window forever.

The braces around the body of the else if case should be removed. It looks like
you have a tab on the last modified line here.

> @@ -99,23 +96,20 @@ void setSharedTimerFireDelay(double inte
>	       intervalInMS = (unsigned)interval;
>      }
>  
> -    if (timerID) {
> -	   KillTimer(0, timerID);
> -	   timerID = 0;
> -    }

Why don't we need this KillTimer call anymore?

>      // We don't allow nested PostMessages, since the custom messages will
effectively starve
>      // painting and user input. (Win32 has a tri-level queue with
application messages > 
>      // user input > WM_PAINT/WM_TIMER.)
>      // In addition, if the queue contains input events that have been there
since the last call to
>      // GetQueueStatus, PeekMessage or GetMessage we favor timers.

> -    if (intervalInMS < USER_TIMER_MINIMUM && processingCustomTimerMessage &&

> -	   !LOWORD(::GetQueueStatus(QS_ALLINPUT))) {
> +    if (intervalInMS < USER_TIMER_MINIMUM 
> +		&& !processingCustomTimerMessage 
> +	   && !LOWORD(::GetQueueStatus(QS_ALLINPUT))) {

You have some tabs here.

> -    } else
> -	   timerID = SetTimer(0, 0, intervalInMS, timerFired);
> +	} else {
> +	   timerID = SetTimer(timerWindowHandle, timerFiredMessage,
intervalInMS, 0);
> +	}

It seems a little strange to use timerFiredMessage as the ID of the timer.

You have some tabs here. The braces around the body of the else should be
removed.

r- for now until you convince me we really don't need to call KillTimer ;-)