[webkit-reviews] review granted: [Bug 86052] split MediaPlayer::enterFullscreen into 2 seperate functions : [Attachment 141074] Patch

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has granted Min Qin
<qinmin at chromium.org>'s request for review:
Bug 86052: split MediaPlayer::enterFullscreen into 2 seperate functions
https://bugs.webkit.org/show_bug.cgi?id=86052

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

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=141074&action=review


Looks good.

> Source/WebCore/ChangeLog:9
> +	   not do the same. And ios does not need the return value.

This has nothing to do with iOS, it was a general design issue.

> Source/WebKit/chromium/ChangeLog:12
> +	   It is confusing that enterFullscreen returns a boolean while
exitFullscreen does
> +	   not do the same. And ios does not need the return value.
> +	   So remove the return value on enterFullscreen and make a seperate
canEnterFullscreen()
> +	   function for android.
> +	   No behavior change, just refactoring.

You don't have to copy the text in each ChangeLog. The description is supposed
to be related to the part including the ChangeLog (so in this case only about
the Chromium changes).

> Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:359
> +    if (m_webMediaPlayer)
> +	   return m_webMediaPlayer->canEnterFullscreen();
> +    return false;

This could simply be written
return m_webMediaPlayer && m_webMediaPlayer->canEnterFullscreen();