Implement MediaController.
Created attachment 113339 [details] Add Clock, PlatformClockCA, and PlatformClockPOSIX
Created attachment 113364 [details] MediaController and HTMLMediaElement changes.
Comment on attachment 113339 [details] Add Clock, PlatformClockCA, and PlatformClockPOSIX View in context: https://bugs.webkit.org/attachment.cgi?id=113339&action=review We should be able to do this without virtual functions. > Source/WebCore/platform/Clock.cpp:44 > +PassRefPtr<Clock> Clock::create() > +{ > +#if USE(COREAUDIO) > + return adoptRef(new PlatformClockCA()); > +#else > + return adoptRef(new PlatformClockPOSIX()); > +#endif > +} Would it make sense to move Clock::create to PlatformClockCA.cpp and PlatformClockPOSIX.cpp, respectively? > Source/WebCore/platform/Clock.h:47 > + virtual void setCurrentTime(float) = 0; > + virtual float currentTime() const = 0; > + > + virtual void setPlayRate(float) = 0; > + virtual float playRate() const = 0; > + > + virtual void start() = 0; > + virtual void stop() = 0; > + virtual bool isRunning() const = 0; Why virtual functions? If only one implementation will be linked in at a time, there's no need for virtual functions.
Comment on attachment 113364 [details] MediaController and HTMLMediaElement changes. This patch is based on an old version of WebKit. Can you merge this to top-of-tree so we can see what the patch will look like when landed?
Also, we prefer to have one patch per bug. If you'd like to land these separately (which I think makes sense) please consider using multiple bugs.
(In reply to comment #3) > (From update of attachment 113339 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=113339&action=review > > We should be able to do this without virtual functions. > Would it make sense to move Clock::create to PlatformClockCA.cpp and PlatformClockPOSIX.cpp, respectively? See below. :) > Why virtual functions? If only one implementation will be linked in at a time, there's no need for virtual functions. There may be more than one implementation at a time. We are working on a clock subclass which can synchronize AVFoundation-based media elements, but would still need a CA or POSIX clock to synchronize between QTKit- and AVFoundation-based elements.
(In reply to comment #5) > Also, we prefer to have one patch per bug. If you'd like to land these separately (which I think makes sense) please consider using multiple bugs. Sure thing. I'll update the title of this bug, obsolete the MediaController patch, and file new bugs for the subsequent patches.
Filed bug #71408 and bug #71410 and marked as depending on this bug. I'll rebase my changes and get the updated.
Thanks.
Created attachment 113396 [details] Source/JavaScriptCore: Implement MediaController.
Created attachment 113397 [details] Source/WebCore: Implement MediaController.
Created attachment 113399 [details] Patch After a disasterous use of webkit-patch post-commit, webkit-patch upload is back to save the day...
Created attachment 113401 [details] Patch
Sigh. webkit-patch upload is still broken. (Adds the wrong patch to the wrong bug.)
<rdar://problem/10387316>
Comment on attachment 113399 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=113399&action=review PlatformClockPOSIX looks fine. I'm not sure I fully understand the role of m_offset. I don't know enough about CoreAudio to review that implementation. > Source/WebCore/platform/posix/PlatformClockPOSIX.cpp:35 > + return val.tv_sec + (val.tv_usec / 1000000.0f); I would have named this constant, but I'm not sure that's a big deal. > Source/WebCore/platform/posix/PlatformClockPOSIX.cpp:43 > + if (delta.tv_usec < 0) { Does this need to be a loop? I'm not sure how much error checking you want to do here.
(In reply to comment #16) > (From update of attachment 113399 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=113399&action=review > > PlatformClockPOSIX looks fine. I'm not sure I fully understand the role of m_offset. I don't know enough about CoreAudio to review that implementation. Sure thing. The general idea is that: current time = (now - start time) * rate + offset, and the offset gets reset whenever the clock is paused, played, or the rate changes. > > Source/WebCore/platform/posix/PlatformClockPOSIX.cpp:35 > > + return val.tv_sec + (val.tv_usec / 1000000.0f); > > I would have named this constant, but I'm not sure that's a big deal. I can add that. usecPerSec or similar. > > Source/WebCore/platform/posix/PlatformClockPOSIX.cpp:43 > > + if (delta.tv_usec < 0) { > > Does this need to be a loop? I'm not sure how much error checking you want to do here. Unless something goes seriously wrong with gettimeofday, this will ever only need to be a single pass. I could add an ASSERT(delta.tv_usec >= 0) after the if.
Committed r99222: <http://trac.webkit.org/changeset/99222>