WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
86052
Split MediaPlayer::enterFullscreen into 2 separate functions
https://bugs.webkit.org/show_bug.cgi?id=86052
Summary
Split MediaPlayer::enterFullscreen into 2 separate functions
Min Qin
Reported
2012-05-09 19:23:57 PDT
split MediaPlayer::enterFullscreen into 2 seperate functions
Attachments
Patch
(9.18 KB, patch)
2012-05-09 19:37 PDT
,
Min Qin
no flags
Details
Formatted Diff
Diff
Patch
(9.15 KB, patch)
2012-05-10 14:45 PDT
,
Min Qin
no flags
Details
Formatted Diff
Diff
Patch
(9.11 KB, patch)
2012-05-11 06:47 PDT
,
Min Qin
no flags
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Min Qin
Comment 1
2012-05-09 19:37:04 PDT
Created
attachment 141074
[details]
Patch
WebKit Review Bot
Comment 2
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
.
Benjamin Poulain
Comment 3
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();
Min Qin
Comment 4
2012-05-10 14:45:22 PDT
Created
attachment 141267
[details]
Patch
Min Qin
Comment 5
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
Benjamin Poulain
Comment 6
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.
WebKit Review Bot
Comment 7
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
>
WebKit Review Bot
Comment 8
2012-05-10 21:26:40 PDT
All reviewed patches have been landed. Closing bug.
WebKit Review Bot
Comment 9
2012-05-11 00:25:49 PDT
Re-opened since this is blocked by 86181
Kent Tamura
Comment 10
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()
Min Qin
Comment 11
2012-05-11 06:47:34 PDT
Created
attachment 141404
[details]
Patch
Min Qin
Comment 12
2012-05-11 06:49:18 PDT
This should fix the break on the downstream android bot, and also updated the changeLog in chromium
Adam Barth
Comment 13
2012-05-11 09:51:16 PDT
Comment on
attachment 141404
[details]
Patch Forwarding benjamin's r+
WebKit Review Bot
Comment 14
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
>
WebKit Review Bot
Comment 15
2012-05-11 10:04:46 PDT
All reviewed patches have been landed. Closing bug.
Alexey Proskuryakov
Comment 16
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.
Benjamin Poulain
Comment 17
2012-05-11 19:26:03 PDT
I am a bit late to the party here. Thanks Adam for reviewing the update.
Adam Barth
Comment 18
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.
Benjamin Poulain
Comment 19
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.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug