[webkit-reviews] review denied: [Bug 41295] [Chromium] Add chromium WebMediaPlayer to PlatformMedia : [Attachment 61276] Patch

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied Bo Liu
<boliu at chromium.org>'s request for review:
Bug 41295: [Chromium] Add chromium WebMediaPlayer to PlatformMedia
https://bugs.webkit.org/show_bug.cgi?id=41295

Attachment 61276: Patch
https://bugs.webkit.org/attachment.cgi?id=61276&action=review

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
WebKit/chromium/public/WebMediaElement.h:46
 +	WEBKIT_API WebMediaPlayer* getWebMediaPlayer() const;
webkit style is to avoid the "get" prefix for simple getter functions.
how about just calling this method mediaPlayer()?  we drop the "web"
prefix for method names that return a Web* type.

WebKit/chromium/src/WebMediaElement.cpp:46
 +	PlatformMedia pm = constUnwrap<HTMLMediaElement>()->platformMedia();
I think it would be better to create a method named fromMediaElement on
WebMediaPlayerClientImpl.  That hides the details of how a
WebMediaPlayerClientImpl
can be extracted from a HTMLMediaElement in the WebMediaPlayerClientImpl class.


You should also create a public accessor on WebMediaPlayerClientImpl named
mediaPlayer()
that returns m_webMediaPlayer.get().  That way you do not need to use friend to
access
private data.

So, for this method you should end up with:

WebMediaPlayer* WebMediaElement::getWebMediaPlayer() const
{
    return WebMediaPlayerClientImpl::fromMediaElement(this)->mediaPlayer();
}