Summary: | "new Audio()" doesn't work with plug-in backed media engine | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Eric Carlson <eric.carlson> | ||||||
Component: | Media | Assignee: | Eric Carlson <eric.carlson> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | webkit.review.bot | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Mac | ||||||||
OS: | OS X 10.5 | ||||||||
Attachments: |
|
Description
Eric Carlson
2010-03-26 15:18:52 PDT
Created attachment 51786 [details]
proposed patch
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.
Created attachment 51790 [details]
Revised patch
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 (In reply to comment #4) > > + > > + static IntSize defaultSize(); > > Can this method be private? > No, it is called by FrameLoader::loadMediaPlayerProxyPlugin. > r=me Thanks! |