Bug 103751 - Add support for the 'unpause()' method on MediaController.
Summary: Add support for the 'unpause()' method on MediaController.
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:
Depends on:
Blocks:
 
Reported: 2012-11-30 08:46 PST by Jer Noble
Modified: 2012-11-30 16:44 PST (History)
6 users (show)

See Also:


Attachments
Patch (8.92 KB, patch)
2012-11-30 09:47 PST, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (8.91 KB, patch)
2012-11-30 10:32 PST, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (9.59 KB, patch)
2012-11-30 10:39 PST, 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-11-30 08:46:43 PST
Add support for the 'unpause()' method on MediaController.
Comment 1 Jer Noble 2012-11-30 09:47:13 PST
Created attachment 176982 [details]
Patch
Comment 2 WebKit Review Bot 2012-11-30 09:52:30 PST
Attachment 176982 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/medi..." exit_code: 1
LayoutTests/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Jer Noble 2012-11-30 10:32:48 PST
Created attachment 176989 [details]
Patch

Now with 100% more bug numbers in the ChangeLog.
Comment 4 Jer Noble 2012-11-30 10:39:48 PST
Created attachment 176993 [details]
Patch

Updated media-controller-playback.txt to match new output.
Comment 5 Eric Carlson 2012-11-30 12:00:22 PST
Comment on attachment 176993 [details]
Patch

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

> Source/WebCore/html/MediaController.cpp:301
> +        return ASCIILiteral("waiting");
> +    case PLAYING:
> +        return ASCIILiteral("playing");
> +    case ENDED:
> +        return ASCIILiteral("ended");

It would be much more efficient to create these just once.

> Source/WebCore/html/MediaController.h:70
>      virtual bool paused() const { return m_paused; }
>      virtual void play();
>      virtual void pause();
> +    void unpause();

Why is this the only non-virtual function?

> LayoutTests/media/media-controller-unpause.html:11
> +        function start() {

Nit: the brace should be on a new line.

> LayoutTests/media/media-controller-unpause.html:22
> +        function canplaythrough() {

Ditto.

> LayoutTests/media/media-controller-unpause.html:29
> +        function pause() {

Ditto.

> LayoutTests/media/media-controller-unpause.html:36
> +        function play() {

Ditto.

> LayoutTests/media/media-controller-unpause.html:44
> +        function playing() {

Ditto.
Comment 6 Jer Noble 2012-11-30 12:05:34 PST
(In reply to comment #5)
> (From update of attachment 176993 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=176993&action=review
> 
> > Source/WebCore/html/MediaController.cpp:301
> > +        return ASCIILiteral("waiting");
> > +    case PLAYING:
> > +        return ASCIILiteral("playing");
> > +    case ENDED:
> > +        return ASCIILiteral("ended");
> 
> It would be much more efficient to create these just once.

Okay, I'll add some static functions.

> > Source/WebCore/html/MediaController.h:70
> >      virtual bool paused() const { return m_paused; }
> >      virtual void play();
> >      virtual void pause();
> > +    void unpause();
> 
> Why is this the only non-virtual function?

It's the only one which doesn't override something in MediaControllerInterface.

> > LayoutTests/media/media-controller-unpause.html:11
> > +        function start() {
> 
> Nit: the brace should be on a new line.

Will change (and all the dittos).
Comment 7 Jer Noble 2012-11-30 16:14:21 PST
Committed r136295: <http://trac.webkit.org/changeset/136295>
Comment 8 Roger Fong 2012-11-30 16:27:41 PST
Hi, I'm getting a windows build failure:

6>c:\cygwin\home\buildbot\slave\win-debug\build\source\webcore\html\mediacontroller.cpp(321) : error C2220: warning treated as error - no 'object' file generated
6>c:\cygwin\home\buildbot\slave\win-debug\build\source\webcore\html\mediacontroller.cpp(321) : warning C4715: 'WebCore::MediaController::playbackState' : not all control paths return a value
Comment 9 Jer Noble 2012-11-30 16:44:40 PST
Committed r136296: <http://trac.webkit.org/changeset/136296>