[webkit-reviews] review denied: [Bug 88691] [BlackBerry] Fix two problems introduced by the userDrivenSeekTimer in MediaPlayerPrivate : [Attachment 146656] Patch

[bugzilla-daemon at webkit.org]

Antonio Gomes <tonikitoo at webkit.org> has denied Max Feil <mfeil at rim.com>'s
request for review:
Bug 88691: [BlackBerry] Fix two problems introduced by the userDrivenSeekTimer
in MediaPlayerPrivate
https://bugs.webkit.org/show_bug.cgi?id=88691

Attachment 146656: Patch
https://bugs.webkit.org/attachment.cgi?id=146656&action=review

------- Additional Comments from Antonio Gomes <tonikitoo at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=146656&action=review


Code looks ok, I have some questions, and one request: split it up in two
patches/bugs, since we are dealing with two unrelated bugs here.

> Source/WebCore/ChangeLog:19
> +	   is a non-issue for such short media.
> +
> +
> +	   Test: platform/blackberry/media/short-media-repeats-correctly.html

nit: extra line here

>
Source/WebCore/platform/graphics/blackberry/MediaPlayerPrivateBlackBerry.cpp:23
7
> +    return m_userDrivenSeekTimer.isActive() && !shortMedia ? m_lastSeekTime:
m_platformPlayer->currentTime();

nit: needs a space before :

>
Source/WebCore/platform/graphics/blackberry/MediaPlayerPrivateBlackBerry.cpp:25
4
> +    if (m_lastSeekTimePending) {
> +	   m_platformPlayer->seek(m_lastSeekTime);
> +	   m_lastSeekTimePending = false;
> +	   m_userDrivenSeekTimer.startOneShot(SeekSubmissionDelay);
> +    }

could you explain why m_lastSeekTimePending is needed? I take it that it is
because you are restarting the timer from within line 253, but why do we need
it?

Also Max, I think we should do two patches for these two issues.