Bug 36681 – "new Audio()" doesn't work with plug-in backed media engine

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Eric Carlson]

<audio> and <video> elements in WebKit with plug-in backed media engine (PLUGIN_PROXY_FOR_VIDEO) don't work until the element has been inserted into the DOM, because plug-ins don't work when not in the DOM. This means that "new Audio()" will create an audio element, but the plug-in that implements the media engine won't be instantiated unless the element is inserted into the DOM.

[Eric Carlson]

Created attachment 51786 [details]
proposed patch

[WebKit Review Bot]

Attachment 51786 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/rendering/RenderLayerCompositor.cpp:50:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/html/HTMLMediaElement.cpp:1965:  One space before end of line comments  [whitespace/comments] [5]
WebCore/loader/FrameLoader.cpp:1434:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 3 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Eric Carlson]

Created attachment 51790 [details]
Revised patch

[Simon Fraser (smfr)]

Comment on attachment 51790 [details]
Revised patch




> Index: WebCore/html/HTMLMediaElement.cpp
> ===================================================================

> +    if (m_proxyWidget)
> +        mediaRenderer->setWidget(m_proxyWidget);

I'd like to see a comment that explains why you hold onto the m_proxyWidget like this.

> +void HTMLMediaElement::allocateMediaPlayerIfNecessary()

I'd prefer a name like "ensureMediaPlayer()" for a method like this.

> +void HTMLMediaElement::getPluginProxyParams(KURL& url, Vector<String>& names, Vector<String>& values)
>  {
> -    KURL initialSrc = document()->completeURL(getAttribute(srcAttr));
> -    
> -    if (!initialSrc.isValid())
> -        initialSrc = selectNextSourceChild(0, DoNothing);
> +    Frame* frame = document()->frame();
> +    FrameLoader* loader = frame ? frame->loader() : 0;
>  
> -    m_currentSrc = initialSrc.string();
> +    if (isVideo()) {
> +        String poster = static_cast<HTMLVideoElement*>(this)->poster();

I don't really like seeing casts to the subclass here; it suggests that the code would be better in a virtual method.

> +                names.append("_media_element_poster_");

Are these magic strings documented anywhere, or could they be shared in a header? Who reads them?

> Index: WebCore/html/HTMLMediaElement.h
> ===================================================================

> +    void deliverNotification(MediaPlayerProxyNotificationType notification);
> +    void setMediaPlayerProxy(WebMediaPlayerProxy* proxy);

No need for the parameter names here.

> +    void getPluginProxyParams(KURL& url, Vector<String>& names, Vector<String>& values);

'url' is not necessary.

> Index: WebCore/rendering/RenderVideo.h
> ===================================================================
> --- WebCore/rendering/RenderVideo.h	(revision 56397)
> +++ WebCore/rendering/RenderVideo.h	(working copy)
> @@ -42,7 +42,9 @@ public:
>  
>      void videoSizeChanged();
>      IntRect videoBox() const;
> -    
> +
> +    static IntSize defaultSize();

Can this method be private?

r=me

[Eric Carlson]

(In reply to comment #4)
> > +
> > +    static IntSize defaultSize();
> 
> Can this method be private?
> 
No, it is called by FrameLoader::loadMediaPlayerProxyPlugin.

> r=me
Thanks!

[Eric Carlson]

http://trac.webkit.org/changeset/56650