[webkit-reviews] review denied: [Bug 43394] [Chromium] Full screen media player interface between WebMediaPlayer and WebWidgetClient : [Attachment 63358] 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 43394: [Chromium] Full screen media player interface between WebMediaPlayer
and WebWidgetClient
https://bugs.webkit.org/show_bug.cgi?id=43394

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

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
WebKit/chromium/public/WebMediaPlayer.h:84
 +	virtual WebMediaPlayerClient* GetClient() const = 0;
use webkit style here.	this should be client() and setClient(),
but i have to ask:  why do you need the client to be accessible
like this, and why do you need it to be mutable?

we have so far tried to avoid exposing clients like this.  what
problem are you trying to solve?

WebKit/chromium/public/WebPopupType.h:40
 +	WebPopupTypeFullscreenMediaPlayer,
why does this popup type need to be media player specific?  in the future,
we may have other needs for a fullscreen popup, and i think the code you
are adding could be used for that too.	so i would drop the MediaPlayer
suffix.

WebKit/chromium/public/WebViewClient.h:91
 +	virtual WebWidget* createFullscreenWindow(WebPopupType) { return 0; }
i don't see why you need to pass WebPopupType to this method.

but instead of removing it, perhaps it would be better to combine
this function with the createPopupMenu(WebPopupType) method from
above into a single:

  virtual WebWidget* createPopup(WebPopupType);

then, deprecate the createPopupMenu(WebPopupType) method.
WebKit/chromium/public/WebFullscreenMediaPlayer.h:53
 +  class WebFullscreenMediaPlayer : public WebWidget,
implementation classes like this should go in chromium/src.  what you
should do in this case is make WebFullscreenMediaPlayer be an interface
with a static create function (see WebPopupMenu.h).  in chromium/src,
you can have an implementation of WebFullscreenMediaPlayer.