Bug 88555 - MediaController.currentTime should be kept stable during script execution.
Summary: MediaController.currentTime should be kept stable during script execution.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jer Noble
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2012-06-07 11:04 PDT by Jer Noble
Modified: 2012-07-23 14:40 PDT (History)
2 users (show)

See Also:


Attachments
Patch (7.67 KB, patch)
2012-06-07 11:12 PDT, Jer Noble
eric.carlson: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jer Noble 2012-06-07 11:04:19 PDT
MediaController.currentTime should be kept stable during script execution.
Comment 1 Jer Noble 2012-06-07 11:12:56 PDT
Created attachment 146332 [details]
Patch
Comment 2 Radar WebKit Bug Importer 2012-06-07 11:16:52 PDT
<rdar://problem/11616954>
Comment 3 Darin Adler 2012-06-07 11:20:04 PDT
Comment on attachment 146332 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=146332&action=review

> Source/WebCore/html/MediaController.cpp:50
> +    , m_position(-1)

To me, -1 does not seem like a great floating point value to mean "not set". I’d suggest a named constant or a separate boolean. I suppose the named constant could have the value -1. Although I would probably chose something else, like NaN.

> Source/WebCore/html/MediaController.h:152
> +    mutable Timer<MediaController> m_clearPositionTimer;

Having our own unique zero-duration timer seems a bit unpleasant. Is there any other good solution? Maybe there is a WebKit-wide concept of “exiting from script” that could work. I seem to recall some Chromium engineers proposing or perhaps even implementing something like that.
Comment 4 Jer Noble 2012-06-07 11:37:37 PDT
Comment on attachment 146332 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=146332&action=review

>> Source/WebCore/html/MediaController.cpp:50
>> +    , m_position(-1)
> 
> To me, -1 does not seem like a great floating point value to mean "not set". I’d suggest a named constant or a separate boolean. I suppose the named constant could have the value -1. Although I would probably chose something else, like NaN.

I agree, and I have a future patch which will add a new enum called InvalidMediaTime.  We already use a "static const float invalidMediaTime = -1;" in HTMLMediaElement for this purpose.  We just need to promote it to a header somewhere.

>> Source/WebCore/html/MediaController.h:152
>> +    mutable Timer<MediaController> m_clearPositionTimer;
> 
> Having our own unique zero-duration timer seems a bit unpleasant. Is there any other good solution? Maybe there is a WebKit-wide concept of “exiting from script” that could work. I seem to recall some Chromium engineers proposing or perhaps even implementing something like that.

:-D

I've got an upcoming patch that does that too.  I actually thought the reverse, however.  That seemed a bit heavy-handed for something that could be solved with a zero-duration timer.
Comment 5 Jer Noble 2012-06-07 14:24:25 PDT
(In reply to comment #4)
> (From update of attachment 146332 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=146332&action=review
> 
> >> Source/WebCore/html/MediaController.cpp:50
> >> +    , m_position(-1)
> > 
> > To me, -1 does not seem like a great floating point value to mean "not set". I’d suggest a named constant or a separate boolean. I suppose the named constant could have the value -1. Although I would probably chose something else, like NaN.
> 
> I agree, and I have a future patch which will add a new enum called InvalidMediaTime.  We already use a "static const float invalidMediaTime = -1;" in HTMLMediaElement for this purpose.  We just need to promote it to a header somewhere.

And here is that patch: https://bugs.webkit.org/show_bug.cgi?id=88572
Comment 6 Eric Carlson 2012-06-13 09:32:46 PDT
Comment on attachment 146332 [details]
Patch

r=me if you update to use MediaPlayer::invalidTime()
Comment 7 Jer Noble 2012-07-23 14:40:23 PDT
Committed r123386: <http://trac.webkit.org/changeset/123386>