Bug 36681 - "new Audio()" doesn't work with plug-in backed media engine
Summary: "new Audio()" doesn't work with plug-in backed media engine
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.5
: P2 Normal
Assignee: Eric Carlson
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-03-26 15:18 PDT by Eric Carlson
Modified: 2010-03-26 18:22 PDT (History)
1 user (show)

See Also:


Attachments
proposed patch (20.11 KB, patch)
2010-03-26 15:24 PDT, Eric Carlson
no flags Details | Formatted Diff | Diff
Revised patch (20.16 KB, patch)
2010-03-26 15:57 PDT, Eric Carlson
simon.fraser: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Carlson 2010-03-26 15:18:52 PDT
<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.
Comment 1 Eric Carlson 2010-03-26 15:24:49 PDT
Created attachment 51786 [details]
proposed patch
Comment 2 WebKit Review Bot 2010-03-26 15:26:19 PDT
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.
Comment 3 Eric Carlson 2010-03-26 15:57:17 PDT
Created attachment 51790 [details]
Revised patch
Comment 4 Simon Fraser (smfr) 2010-03-26 16:07:19 PDT
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
Comment 5 Eric Carlson 2010-03-26 18:22:39 PDT
(In reply to comment #4)
> > +
> > +    static IntSize defaultSize();
> 
> Can this method be private?
> 
No, it is called by FrameLoader::loadMediaPlayerProxyPlugin.

> r=me
Thanks!
Comment 6 Eric Carlson 2010-03-26 18:22:54 PDT
http://trac.webkit.org/changeset/56650