Bug 84414 - Expose a flag so that fullscreen video on android can work with FULLSCREEN_API
Summary: Expose a flag so that fullscreen video on android can work with FULLSCREEN_API
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-04-19 19:45 PDT by Min Qin
Modified: 2012-05-09 15:57 PDT (History)
9 users (show)

See Also:


Attachments
Patch (10.03 KB, patch)
2012-04-19 19:52 PDT, Min Qin
no flags Details | Formatted Diff | Diff
Patch (10.08 KB, patch)
2012-04-19 20:27 PDT, Min Qin
no flags Details | Formatted Diff | Diff
Patch (9.68 KB, patch)
2012-04-20 11:24 PDT, Min Qin
no flags Details | Formatted Diff | Diff
Patch (9.80 KB, patch)
2012-04-20 12:01 PDT, Min Qin
no flags Details | Formatted Diff | Diff
Patch (10.11 KB, patch)
2012-04-30 09:46 PDT, Min Qin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Min Qin 2012-04-19 19:45:43 PDT
Expose a flag so that fullscreen video on android can work with FULLSCREEN_API
Comment 1 Min Qin 2012-04-19 19:52:33 PDT
Created attachment 138032 [details]
Patch
Comment 2 WebKit Review Bot 2012-04-19 19:54:46 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 WebKit Review Bot 2012-04-19 19:55:10 PDT
Attachment 138032 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Min Qin 2012-04-19 20:27:14 PDT
Created attachment 138034 [details]
Patch
Comment 5 Eric Carlson 2012-04-20 10:46:38 PDT
Comment on attachment 138034 [details]
Patch

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

The WebCore part of this looks fine to me modulo the simplification comment.

> Source/WebCore/platform/graphics/MediaPlayer.cpp:768
> +#if ENABLE(PLUGIN_PROXY_FOR_VIDEO)
>  void MediaPlayer::enterFullscreen()
>  {
>      m_private->enterFullscreen();
> -}    
> +}
> +#elif ENABLE(NATIVE_FULLSCREEN_VIDEO)
> +bool MediaPlayer::enterFullscreen() const
> +{
> +    return m_private->enterFullscreen();
> +}

I think you might as well just change m_private->enterFullscreen to return a bool so you can combine these.
Comment 6 Min Qin 2012-04-20 11:24:36 PDT
Created attachment 138125 [details]
Patch
Comment 7 Eric Carlson 2012-04-20 11:34:24 PDT
This looks fine to me, but I am not marking r+ pending approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org.
Comment 8 James Robinson 2012-04-20 11:42:37 PDT
Comment on attachment 138125 [details]
Patch

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

> Source/WebKit/chromium/public/WebMediaPlayer.h:212
> +    virtual bool enterFullscreen() { return false; }

could you document what this does (i'm guessing tell this media player to go fullscreen) and what the bool return value means?
Comment 9 Min Qin 2012-04-20 12:01:31 PDT
Created attachment 138137 [details]
Patch
Comment 10 Min Qin 2012-04-20 12:03:26 PDT
Comments added for enter and exit fullscreen
Comment 11 Min Qin 2012-04-24 15:07:11 PDT
Ping, anyone from the chromium side can review this?
Comment 12 Darin Fisher (:fishd, Google) 2012-04-27 09:45:53 PDT
Comment on attachment 138137 [details]
Patch

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

> Source/WebKit/chromium/src/WebViewImpl.cpp:1592
>      if (m_client && m_client->enterFullScreen())

so then on the chromium side you will make this enterFullScreen()
method always return false?  or, will you try to make fullscreen
mode work for non-video elements too?
Comment 13 Min Qin 2012-04-27 09:51:59 PDT
On chromium side, the enterFullScreen should only work for video element. For audio element, we will return false.  There is no current plan for MediaSource element yet. All the logic should be in webmediaplayer_android.cc.
Comment 14 WebKit Review Bot 2012-04-27 15:03:20 PDT
Comment on attachment 138137 [details]
Patch

Rejecting attachment 138137 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
Source/WebKit/chromium/public/WebMediaPlayer.h.rej
patching file Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp
patching file Source/WebKit/chromium/src/WebMediaPlayerClientImpl.h
patching file Source/WebKit/chromium/src/WebViewImpl.cpp
Hunk #1 succeeded at 1584 (offset 5 lines).
Hunk #2 succeeded at 1603 (offset 5 lines).

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Darin Fish..." exit_code: 1 cwd: /mnt/git/webkit-commit-queue/

Full output: http://queues.webkit.org/results/12555355
Comment 15 Min Qin 2012-04-30 09:46:59 PDT
Created attachment 139467 [details]
Patch
Comment 16 Min Qin 2012-04-30 09:49:03 PDT
Sorry, there is a merge error during the previous commit process. Resolved the conflicts now
Comment 17 WebKit Review Bot 2012-04-30 13:26:52 PDT
Comment on attachment 139467 [details]
Patch

Clearing flags on attachment: 139467

Committed r115661: <http://trac.webkit.org/changeset/115661>
Comment 18 WebKit Review Bot 2012-04-30 13:26:59 PDT
All reviewed patches have been landed.  Closing bug.
Comment 19 Kent Tamura 2012-04-30 19:25:52 PDT
Comment on attachment 139467 [details]
Patch

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

> Source/WebCore/platform/graphics/MediaPlayer.cpp:772
> +#if ENABLE(PLUGIN_PROXY_FOR_VIDEO) || ENABLE(NATIVE_FULLSCREEN_VIDEO)

Would you explain what is NATIVE_FULLSCREEN_VIDEO?
Shouldn't it be a USE() flag instead of an ENABEL() flag?

http://trac.webkit.org/wiki/Porting%20Macros%20plan
* USE() - use a particular third-party library or optional OS service
* ENABLE() - turn on a specific feature of WebKit
Comment 20 Min Qin 2012-04-30 19:38:12 PDT
For android, fullscreen video is implemented by passing a java video surfaceView object to the webmediaplayer. As a consequence, NATIVE_FULLSCREEN_VIDEO means the fullscreen video is implemented by the native system view,  and is not implemented by any of the webkit code.

Probably USE makes more sense here, I will have another change to fix this,

(In reply to comment #19)
> (From update of attachment 139467 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=139467&action=review
> 
> > Source/WebCore/platform/graphics/MediaPlayer.cpp:772
> > +#if ENABLE(PLUGIN_PROXY_FOR_VIDEO) || ENABLE(NATIVE_FULLSCREEN_VIDEO)
> 
> Would you explain what is NATIVE_FULLSCREEN_VIDEO?
> Shouldn't it be a USE() flag instead of an ENABEL() flag?
> 
> http://trac.webkit.org/wiki/Porting%20Macros%20plan
> * USE() - use a particular third-party library or optional OS service
> * ENABLE() - turn on a specific feature of WebKit
Comment 21 Benjamin Poulain 2012-05-09 12:46:58 PDT
Comment on attachment 139467 [details]
Patch

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

> Source/WebCore/platform/graphics/MediaPlayer.h:330
> +    bool enterFullscreen() const;

Why would you change this to be const?? That is not a method free of side effects on the state of the media player.
Comment 22 Benjamin Poulain 2012-05-09 12:47:00 PDT
Comment on attachment 139467 [details]
Patch

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

> Source/WebCore/platform/graphics/MediaPlayer.h:330
> +    bool enterFullscreen() const;

Why would you change this to be const?? That is not a method free of side effects on the state of the media player.
Comment 23 Alexis Menard (darktears) 2012-05-09 13:01:25 PDT
Comment on attachment 139467 [details]
Patch

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

> Source/WebKit/chromium/ChangeLog:11
> +        This change makes it possble for WebViewImpl::enterFullScreenForElement()

Typo here, be careful next time.

> Source/WebKit/chromium/ChangeLog:14
> +        Sorry, there is a merge error during the previous commit, resolved now

This sentence doesn't make sense to me. Be careful next time.

>>> Source/WebCore/platform/graphics/MediaPlayer.cpp:772
>>> +#if ENABLE(PLUGIN_PROXY_FOR_VIDEO) || ENABLE(NATIVE_FULLSCREEN_VIDEO)
>> 
>> Would you explain what is NATIVE_FULLSCREEN_VIDEO?
>> Shouldn't it be a USE() flag instead of an ENABEL() flag?
>> 
>> http://trac.webkit.org/wiki/Porting%20Macros%20plan
>> * USE() - use a particular third-party library or optional OS service
>> * ENABLE() - turn on a specific feature of WebKit
> 
> 

I would like to know too.

>>> Source/WebCore/platform/graphics/MediaPlayer.h:330
>>> +    bool enterFullscreen() const;
>> 
>> Why would you change this to be const?? That is not a method free of side effects on the state of the media player.
> 
> Why would you change this to be const?? That is not a method free of side effects on the state of the media player.

In addition of the const I really dislike the asymmetry with exitFullscreen();
Other question : Why the bool as a return value? Could you elaborate when and which scenario it would return false?

> Source/WebKit/chromium/public/WebMediaPlayer.h:187
> +    // Instuct WebMediaPlayer to enter/exit fullscreen.

Typo here.
Comment 24 Benjamin Poulain 2012-05-09 13:07:06 PDT
> > Why would you change this to be const?? That is not a method free of side effects on the state of the media player.
> 
> In addition of the const I really dislike the asymmetry with exitFullscreen();
> Other question : Why the bool as a return value? Could you elaborate when and which scenario it would return false?

That is a good point. With all the existing code ignoring the return value, this is weird.
Comment 25 Min Qin 2012-05-09 13:25:54 PDT
(In reply to comment #22)
> (From update of attachment 139467 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=139467&action=review
> 
> > Source/WebCore/platform/graphics/MediaPlayer.h:330
> > +    bool enterFullscreen() const;
> 
> Why would you change this to be const?? That is not a method free of side effects on the state of the media player.

So this is what could happen for chrome on android, when calling enterFullscreen() it could actually fail for the following 2 reasons:
1. if the MediaPlayerPrivate object is gone.
2. the native media player is already in fullscreen mode.

Since this return value is not used when ENABLE(PLUGIN_PROXY_FOR_VIDEO) is set, returning true should just be fine.
Comment 26 Min Qin 2012-05-09 13:31:21 PDT
(In reply to comment #23)
> (From update of attachment 139467 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=139467&action=review
> 
> > Source/WebKit/chromium/ChangeLog:11
> > +        This change makes it possble for WebViewImpl::enterFullScreenForElement()
> 
> Typo here, be careful next time.
> 
> > Source/WebKit/chromium/ChangeLog:14
> > +        Sorry, there is a merge error during the previous commit, resolved now
> 
> This sentence doesn't make sense to me. Be careful next time.
> 
> >>> Source/WebCore/platform/graphics/MediaPlayer.cpp:772
> >>> +#if ENABLE(PLUGIN_PROXY_FOR_VIDEO) || ENABLE(NATIVE_FULLSCREEN_VIDEO)
> >> 
> >> Would you explain what is NATIVE_FULLSCREEN_VIDEO?
> >> Shouldn't it be a USE() flag instead of an ENABEL() flag?
> >> 
> >> http://trac.webkit.org/wiki/Porting%20Macros%20plan
> >> * USE() - use a particular third-party library or optional OS service
> >> * ENABLE() - turn on a specific feature of WebKit
> > 
> > 
> 
> I would like to know too.

This has been fixed in https://bugs.webkit.org/show_bug.cgi?id=85316

> 
> >>> Source/WebCore/platform/graphics/MediaPlayer.h:330
> >>> +    bool enterFullscreen() const;
> >> 
> >> Why would you change this to be const?? That is not a method free of side effects on the state of the media player.
> > 
> > Why would you change this to be const?? That is not a method free of side effects on the state of the media player.
> 
> In addition of the const I really dislike the asymmetry with exitFullscreen();
> Other question : Why the bool as a return value? Could you elaborate when and which scenario it would return false?
> 

So if you take a look at the change I made in WebViewImpl::enterFullScreenForElement(WebCore::Element* element) in this patch, you will see this return value got used. We need the return value to know whether enterfullscreen actually succeeded, and whether we should set the current m_provisionalFullScreenElement element to that media element.


> > Source/WebKit/chromium/public/WebMediaPlayer.h:187
> > +    // Instuct WebMediaPlayer to enter/exit fullscreen.
> 
> Typo here.

ok, will fix the typo later
Comment 27 Benjamin Poulain 2012-05-09 13:33:56 PDT
> > Why would you change this to be const?? That is not a method free of side effects on the state of the media player.

You have not explained why this is valid regarding const-correctness.

> So this is what could happen for chrome on android, when calling enterFullscreen() it could actually fail for the following 2 reasons:
> 1. if the MediaPlayerPrivate object is gone.
> 2. the native media player is already in fullscreen mode.
> 
> Since this return value is not used when ENABLE(PLUGIN_PROXY_FOR_VIDEO) is set, returning true should just be fine.

This sounds like reasons to have a "canEnterFullScreen()" method or something similar. Now you changed the API so that enterFullScreen() can fail. The fact that the existing code ignore this is no excuse.
Comment 28 Min Qin 2012-05-09 13:54:19 PDT
(In reply to comment #27)
> > > Why would you change this to be const?? That is not a method free of side effects on the state of the media player.
> 
> You have not explained why this is valid regarding const-correctness.

Because when we call this, it won't change the object. If the const-correctness really affects the const-correctness in your implementation, we can remove that const away.  
> 
> > So this is what could happen for chrome on android, when calling enterFullscreen() it could actually fail for the following 2 reasons:
> > 1. if the MediaPlayerPrivate object is gone.
> > 2. the native media player is already in fullscreen mode.
> > 
> > Since this return value is not used when ENABLE(PLUGIN_PROXY_FOR_VIDEO) is set, returning true should just be fine.
> 
> This sounds like reasons to have a "canEnterFullScreen()" method or something similar. Now you changed the API so that enterFullScreen() can fail. The fact that the existing code ignore this is no excuse.

(In reply to comment #27)
> > > Why would you change this to be const?? That is not a method free of side effects on the state of the media player.
> 
> You have not explained why this is valid regarding const-correctness.
> 
> > So this is what could happen for chrome on android, when calling enterFullscreen() it could actually fail for the following 2 reasons:
> > 1. if the MediaPlayerPrivate object is gone.
> > 2. the native media player is already in fullscreen mode.
> > 
> > Since this return value is not used when ENABLE(PLUGIN_PROXY_FOR_VIDEO) is set, returning true should just be fine.
> 
> This sounds like reasons to have a "canEnterFullScreen()" method or something similar. Now you changed the API so that enterFullScreen() can fail. The fact that the existing code ignore this is no excuse.

So in the NATIVE_FULLSCREEN_VIDEO case, there is nothing in the DOM that blocks the media element from going fullscreen. Only the native mediaplayer knows whether it can enter fullscreen or not. And also because that the MediaPlayerPrivate can get deleted when this is called, so enterFullscreen() is not guaranteed to be successful.
Comment 29 Benjamin Poulain 2012-05-09 15:15:25 PDT
> > You have not explained why this is valid regarding const-correctness.
> 
> Because when we call this, it won't change the object. If the const-correctness really affects the const-correctness in your implementation, we can remove that const away.  

In WebKit, we consider the semantic const correctness. not the C++ type checking.

Yes, I think the "const" should not be there.


> > > Since this return value is not used when ENABLE(PLUGIN_PROXY_FOR_VIDEO) is set, returning true should just be fine.
> > 
> > This sounds like reasons to have a "canEnterFullScreen()" method or something similar. Now you changed the API so that enterFullScreen() can fail. The fact that the existing code ignore this is no excuse.
> 
> So in the NATIVE_FULLSCREEN_VIDEO case, there is nothing in the DOM that blocks the media element from going fullscreen. Only the native mediaplayer knows whether it can enter fullscreen or not. And also because that the MediaPlayerPrivate can get deleted when this is called, so enterFullscreen() is not guaranteed to be successful.

If you chance the contract of an API, you have to do it all the way.

Please explain how can the MediaPlayerPrivate get deleted in MediaPlayer::enterFullScreen() because that is relevant to the other use of this API.
Comment 30 Min Qin 2012-05-09 15:26:53 PDT
(In reply to comment #29)
> > > You have not explained why this is valid regarding const-correctness.
> > 
> > Because when we call this, it won't change the object. If the const-correctness really affects the const-correctness in your implementation, we can remove that const away.  
> 
> In WebKit, we consider the semantic const correctness. not the C++ type checking.
> 
> Yes, I think the "const" should not be there.
> 
> 
> > > > Since this return value is not used when ENABLE(PLUGIN_PROXY_FOR_VIDEO) is set, returning true should just be fine.
> > > 
> > > This sounds like reasons to have a "canEnterFullScreen()" method or something similar. Now you changed the API so that enterFullScreen() can fail. The fact that the existing code ignore this is no excuse.
> > 
> > So in the NATIVE_FULLSCREEN_VIDEO case, there is nothing in the DOM that blocks the media element from going fullscreen. Only the native mediaplayer knows whether it can enter fullscreen or not. And also because that the MediaPlayerPrivate can get deleted when this is called, so enterFullscreen() is not guaranteed to be successful.
> 
> If you chance the contract of an API, you have to do it all the way.
>

ok, I will add a canEnterFullscreen() call in another change, and revert the changes to the enterFullscreen() call.

> Please explain how can the MediaPlayerPrivate get deleted in MediaPlayer::enterFullScreen() because that is relevant to the other use of this API.

I am not sure why this happens, but this did happen once a while in our stacktrace. Could be a bug in javascript engine, or could be something else.
Comment 31 Benjamin Poulain 2012-05-09 15:57:23 PDT
> ok, I will add a canEnterFullscreen() call in another change, and revert the changes to the enterFullscreen() call.

Great, thanks for following up on this. Feel free to add me in CC if you need reviews.

> > Please explain how can the MediaPlayerPrivate get deleted in MediaPlayer::enterFullScreen() because that is relevant to the other use of this API.
> 
> I am not sure why this happens, but this did happen once a while in our stacktrace. Could be a bug in javascript engine, or could be something else.

This could be important. Please file a bug next time you encounter this issue.