Bug 86052

Summary: Split MediaPlayer::enterFullscreen into 2 separate functions
Product: WebKit Reporter: Min Qin <qinmin>
Component: New BugsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, benjamin, dglazkov, eric.carlson, feature-media-reviews, fishd, jamesr, jer.noble, tkent, tkent+wkapi, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 86181    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Min Qin 2012-05-09 19:23:57 PDT
split MediaPlayer::enterFullscreen into 2 seperate functions
Comment 1 Min Qin 2012-05-09 19:37:04 PDT
Created attachment 141074 [details]
Patch
Comment 2 WebKit Review Bot 2012-05-09 19:39:30 PDT
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment 3 Benjamin Poulain 2012-05-09 19:52:55 PDT
Comment on attachment 141074 [details]
Patch

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();
Comment 4 Min Qin 2012-05-10 14:45:22 PDT
Created attachment 141267 [details]
Patch
Comment 5 Min Qin 2012-05-10 14:46:10 PDT
(In reply to comment #3)
> (From update of attachment 141074 [details])
> 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();

Done, changed it in the new patch
Comment 6 Benjamin Poulain 2012-05-10 18:56:41 PDT
Comment on attachment 141267 [details]
Patch

You did not update the Changelogs... Let's land like this, no big deal for the copied text.
Comment 7 WebKit Review Bot 2012-05-10 21:26:35 PDT
Comment on attachment 141267 [details]
Patch

Clearing flags on attachment: 141267

Committed r116727: <http://trac.webkit.org/changeset/116727>
Comment 8 WebKit Review Bot 2012-05-10 21:26:40 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 WebKit Review Bot 2012-05-11 00:25:49 PDT
Re-opened since this is blocked by 86181
Comment 10 Kent Tamura 2012-05-11 00:31:48 PDT
(In reply to comment #7)
> (From update of attachment 141267 [details])
> Clearing flags on attachment: 141267
> 
> Committed r116727: <http://trac.webkit.org/changeset/116727>

I'm rolling this out because of a build failure on Android.

http://build.webkit.org/builders/Chromium%20Android%20Release/builds/17398/steps/compile-webkit/logs/stdio

Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:343: error: prototype for 'bool WebKit::WebMediaPlayerClientImpl::enterFullscreen()' does not match any in class 'WebKit::WebMediaPlayerClientImpl'
Source/WebKit/chromium/src/WebMediaPlayerClientImpl.h:138: error: candidate is: virtual void WebKit::WebMediaPlayerClientImpl::enterFullscreen()
Comment 11 Min Qin 2012-05-11 06:47:34 PDT
Created attachment 141404 [details]
Patch
Comment 12 Min Qin 2012-05-11 06:49:18 PDT
This should fix the break on the downstream android bot, and also updated the changeLog in chromium
Comment 13 Adam Barth 2012-05-11 09:51:16 PDT
Comment on attachment 141404 [details]
Patch

Forwarding benjamin's r+
Comment 14 WebKit Review Bot 2012-05-11 10:04:38 PDT
Comment on attachment 141404 [details]
Patch

Clearing flags on attachment: 141404

Committed r116782: <http://trac.webkit.org/changeset/116782>
Comment 15 WebKit Review Bot 2012-05-11 10:04:46 PDT
All reviewed patches have been landed.  Closing bug.
Comment 16 Alexey Proskuryakov 2012-05-11 10:36:36 PDT
Comment on attachment 141404 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=141404&action=review

> Source/WebCore/ChangeLog:3
> +        split MediaPlayer::enterFullscreen into 2 seperate functions

Ideally, this would have been updated to match updated bug title.
Comment 17 Benjamin Poulain 2012-05-11 19:26:03 PDT
I am a bit late to the party here. Thanks Adam for reviewing the update.
Comment 18 Adam Barth 2012-05-11 23:08:13 PDT
(In reply to comment #17)
> I am a bit late to the party here. Thanks Adam for reviewing the update.

Sorry, I didn't mean to jump in on your review.  It looked like Min had addressed all of your comments and the patch itself looked simple enough.
Comment 19 Benjamin Poulain 2012-05-12 14:42:44 PDT
 (In reply to comment #17)
> > I am a bit late to the party here. Thanks Adam for reviewing the update.
> 
> Sorry, I didn't mean to jump in on your review.  It looked like Min had addressed all of your comments and the patch itself looked simple enough.

I was not ironic in any way.
I was away for a little while that morning, I genuinely appreciate you took the time to review the build fix on this patch.