[webkit-reviews] review granted: [Bug 36681] "new Audio()" doesn't work with plug-in backed media engine : [Attachment 51790] Revised patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Eric Carlson
<eric.carlson at apple.com>'s request for review:
Bug 36681: "new Audio()" doesn't work with plug-in backed media engine
https://bugs.webkit.org/show_bug.cgi?id=36681

Attachment 51790: Revised patch
https://bugs.webkit.org/attachment.cgi?id=51790&action=review

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>



> 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