Bug 124983 - Nix Upstream: Adding MediaControlsNix to WebCore
Summary: Nix Upstream: Adding MediaControlsNix to WebCore
Status: RESOLVED INVALID
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Hugo Parente Lima
URL:
Keywords:
Depends on:
Blocks: 124950
  Show dependency treegraph
 
Reported: 2013-11-28 10:45 PST by Thiago de Barros Lacerda
Modified: 2014-01-16 13:41 PST (History)
8 users (show)

See Also:


Attachments
Patch (10.47 KB, patch)
2013-11-28 10:47 PST, Thiago de Barros Lacerda
no flags Details | Formatted Diff | Diff
Patch (10.48 KB, patch)
2013-11-28 10:51 PST, Thiago de Barros Lacerda
benjamin: review-
benjamin: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Thiago de Barros Lacerda 2013-11-28 10:45:39 PST
Nix Upstream: Adding MediaControlsNix to WebCore
Comment 1 Thiago de Barros Lacerda 2013-11-28 10:47:02 PST
Created attachment 218015 [details]
Patch
Comment 2 Thiago de Barros Lacerda 2013-11-28 10:51:38 PST
Created attachment 218016 [details]
Patch
Comment 3 Eric Carlson 2013-12-02 07:55:12 PST
Have you seen Source/WebCore/Modules/mediacontrols/? The Apple ports use this to implement media controls with JavaScript. The WinCairo port is looking at implementing media controls this was as well [1].

That approach is *much* simpler in the long run. 

[1] https://bugs.webkit.org/show_bug.cgi?id=124868
Comment 4 Benjamin Poulain 2013-12-02 14:34:52 PST
Comment on attachment 218016 [details]
Patch

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

> Source/WebCore/html/shadow/MediaControlsNix.cpp:60
> +    return 0;

return nullptr.

> Source/WebCore/html/shadow/MediaControlsNix.cpp:145
> +    Page* page = document().page();
> +    if (!page)

if (!document().page())
    return

> Source/WebCore/html/shadow/MediaControlsNix.cpp:166
> +    double now = m_mediaController->currentTime();
> +    double duration = m_mediaController->duration();

This should be done _after_ the early return.

> Source/WebCore/html/shadow/MediaControlsNix.cpp:170
> +
> +    Page* page = document().page();
> +    if (!page)
> +        return;

ditto

> Source/WebCore/html/shadow/MediaControlsNix.h:45
> +    void changedMute() OVERRIDE;

Missing virtual.
Comment 5 Thiago de Barros Lacerda 2013-12-02 14:48:20 PST
(In reply to comment #3)
> Have you seen Source/WebCore/Modules/mediacontrols/? The Apple ports use this to implement media controls with JavaScript. The WinCairo port is looking at implementing media controls this was as well [1].
> 
> That approach is *much* simpler in the long run. 
> 
> [1] https://bugs.webkit.org/show_bug.cgi?id=124868

Thanks Eric, I will take a look on that
Comment 6 Thiago de Barros Lacerda 2013-12-02 14:48:55 PST
(In reply to comment #4)
> (From update of attachment 218016 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=218016&action=review
> 
> > Source/WebCore/html/shadow/MediaControlsNix.cpp:60
> > +    return 0;
> 
> return nullptr.
> 
> > Source/WebCore/html/shadow/MediaControlsNix.cpp:145
> > +    Page* page = document().page();
> > +    if (!page)
> 
> if (!document().page())
>     return
> 
> > Source/WebCore/html/shadow/MediaControlsNix.cpp:166
> > +    double now = m_mediaController->currentTime();
> > +    double duration = m_mediaController->duration();
> 
> This should be done _after_ the early return.
> 
> > Source/WebCore/html/shadow/MediaControlsNix.cpp:170
> > +
> > +    Page* page = document().page();
> > +    if (!page)
> > +        return;
> 
> ditto
> 
> > Source/WebCore/html/shadow/MediaControlsNix.h:45
> > +    void changedMute() OVERRIDE;
> 
> Missing virtual.

Thanks Benjamin, I will fix those