Bug 117220 - Allow for toggling fullscreen on <video> elements
Summary: Allow for toggling fullscreen on <video> elements
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: InRadar
Depends on:
Blocks:
 
Reported: 2013-06-04 18:49 PDT by Ruth Fong
Modified: 2013-06-12 10:33 PDT (History)
23 users (show)

See Also:


Attachments
Patch (11.85 KB, patch)
2013-06-04 19:05 PDT, Ruth Fong
no flags Details | Formatted Diff | Diff
Patch (25.61 KB, patch)
2013-06-05 11:24 PDT, Ruth Fong
no flags Details | Formatted Diff | Diff
Patch (28.45 KB, patch)
2013-06-05 15:26 PDT, Ruth Fong
no flags Details | Formatted Diff | Diff
Patch (27.00 KB, patch)
2013-06-06 17:22 PDT, Ruth Fong
no flags Details | Formatted Diff | Diff
Patch (27.03 KB, patch)
2013-06-06 17:53 PDT, Ruth Fong
no flags Details | Formatted Diff | Diff
Patch (27.16 KB, patch)
2013-06-06 18:36 PDT, Ruth Fong
no flags Details | Formatted Diff | Diff
Patch (12.84 KB, patch)
2013-06-07 14:13 PDT, Ruth Fong
no flags Details | Formatted Diff | Diff
Patch (13.93 KB, patch)
2013-06-07 17:42 PDT, Ruth Fong
no flags Details | Formatted Diff | Diff
Patch (12.11 KB, patch)
2013-06-07 18:44 PDT, Ruth Fong
no flags Details | Formatted Diff | Diff
Patch (12.14 KB, patch)
2013-06-07 19:01 PDT, Ruth Fong
no flags Details | Formatted Diff | Diff
Patch (12.14 KB, patch)
2013-06-07 19:20 PDT, Ruth Fong
no flags Details | Formatted Diff | Diff
Patch (12.71 KB, patch)
2013-06-10 09:16 PDT, Ruth Fong
no flags Details | Formatted Diff | Diff
Patch (12.61 KB, patch)
2013-06-10 15:52 PDT, Ruth Fong
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ruth Fong 2013-06-04 18:49:25 PDT
Refactor code so that video fullscreen state can be toggled (much like play/pause is toggled).
Comment 1 Ruth Fong 2013-06-04 18:50:39 PDT
<rdar://problem/14015839>
Comment 2 Ruth Fong 2013-06-04 19:05:04 PDT
Created attachment 203747 [details]
Patch
Comment 3 Early Warning System Bot 2013-06-04 19:11:49 PDT
Comment on attachment 203747 [details]
Patch

Attachment 203747 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/673630
Comment 4 kov's GTK+ EWS bot 2013-06-04 19:12:36 PDT
Comment on attachment 203747 [details]
Patch

Attachment 203747 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/714621
Comment 5 Early Warning System Bot 2013-06-04 19:13:59 PDT
Comment on attachment 203747 [details]
Patch

Attachment 203747 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/780052
Comment 6 EFL EWS Bot 2013-06-04 19:15:13 PDT
Comment on attachment 203747 [details]
Patch

Attachment 203747 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/768278
Comment 7 EFL EWS Bot 2013-06-04 19:19:10 PDT
Comment on attachment 203747 [details]
Patch

Attachment 203747 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/778144
Comment 8 Eric Carlson 2013-06-05 07:55:07 PDT
Comment on attachment 203747 [details]
Patch

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

> Source/WebCore/rendering/HitTestResult.cpp:405
> +    HTMLMediaElement* mediaElt = mediaElement();

Nit: "mediaElt" seems odd and letters are cheap, I would spell out the variable name.

> Source/WebCore/rendering/HitTestResult.cpp:415
> +    HTMLMediaElement* mediaElt(mediaElement());

Ditto. 

We usually use "=" for an assignment like this.
Comment 9 Ruth Fong 2013-06-05 10:25:04 PDT
Comment on attachment 203747 [details]
Patch

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

>> Source/WebCore/rendering/HitTestResult.cpp:405
>> +    HTMLMediaElement* mediaElt = mediaElement();
> 
> Nit: "mediaElt" seems odd and letters are cheap, I would spell out the variable name.

HTMLMediaElement* mediaElement = mediaElement(); yields an error that the "called object type 'WebCore::HTMLMediaElement *" is not a function or function pointer". Would this be because I'm naming the variable the same thing as a function?
Comment 10 Jer Noble 2013-06-05 10:30:12 PDT
Comment on attachment 203747 [details]
Patch

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

>>> Source/WebCore/rendering/HitTestResult.cpp:405
>>> +    HTMLMediaElement* mediaElt = mediaElement();
>> 
>> Nit: "mediaElt" seems odd and letters are cheap, I would spell out the variable name.
> 
> HTMLMediaElement* mediaElement = mediaElement(); yields an error that the "called object type 'WebCore::HTMLMediaElement *" is not a function or function pointer". Would this be because I'm naming the variable the same thing as a function?

Yes, and your local variable overrides the function of the same name, so the compiler thinks you're trying to call the local variable like a function pointer.

I think 'element' would be fine.

> Source/WebCore/rendering/HitTestResult.cpp:406
> +    if (mediaElt && mediaElt->hasTagName(HTMLNames::videoTag))

You should use "element->isVideo()" here instead.
Comment 11 Jer Noble 2013-06-05 10:30:13 PDT
Comment on attachment 203747 [details]
Patch

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

>>> Source/WebCore/rendering/HitTestResult.cpp:405
>>> +    HTMLMediaElement* mediaElt = mediaElement();
>> 
>> Nit: "mediaElt" seems odd and letters are cheap, I would spell out the variable name.
> 
> HTMLMediaElement* mediaElement = mediaElement(); yields an error that the "called object type 'WebCore::HTMLMediaElement *" is not a function or function pointer". Would this be because I'm naming the variable the same thing as a function?

Yes, and your local variable overrides the function of the same name, so the compiler thinks you're trying to call the local variable like a function pointer.

I think 'element' would be fine.

> Source/WebCore/rendering/HitTestResult.cpp:406
> +    if (mediaElt && mediaElt->hasTagName(HTMLNames::videoTag))

You should use "element->isVideo()" here instead.
Comment 12 Ruth Fong 2013-06-05 11:24:05 PDT
Created attachment 203868 [details]
Patch
Comment 13 WebKit Commit Bot 2013-06-05 11:25:14 PDT
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
Comment 14 Early Warning System Bot 2013-06-05 11:30:37 PDT
Comment on attachment 203868 [details]
Patch

Attachment 203868 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/694411
Comment 15 Geoffrey Garen 2013-06-05 11:35:00 PDT
> HTMLMediaElement* mediaElement = mediaElement(); yields an error that the "called object type 'WebCore::HTMLMediaElement *" is not a function or function pointer".

Minor comment:

It's a pet peeve of mine when we give two names to the same thing -- especially if it's just because the compiler is being a bully.

My preferred way to preserve the preferred name is either

    HTMLMediaElement* mediaElement = this->mediaElement();

or

    HTMLMediaElement* mediaElement = HitTestResult::mediaElement();

That way, the name of the thing is always "media element", which is our preferred name.
Comment 16 EFL EWS Bot 2013-06-05 11:35:52 PDT
Comment on attachment 203868 [details]
Patch

Attachment 203868 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/689696
Comment 17 Early Warning System Bot 2013-06-05 11:37:29 PDT
Comment on attachment 203868 [details]
Patch

Attachment 203868 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/780095
Comment 18 EFL EWS Bot 2013-06-05 11:43:32 PDT
Comment on attachment 203868 [details]
Patch

Attachment 203868 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/768335
Comment 19 Ruth Fong 2013-06-05 15:26:00 PDT
Created attachment 203887 [details]
Patch
Comment 20 Early Warning System Bot 2013-06-05 15:33:38 PDT
Comment on attachment 203887 [details]
Patch

Attachment 203887 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/664384
Comment 21 Early Warning System Bot 2013-06-05 15:35:29 PDT
Comment on attachment 203887 [details]
Patch

Attachment 203887 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/763337
Comment 22 Ruth Fong 2013-06-05 15:48:07 PDT
Comment on attachment 203887 [details]
Patch

WIP - trying to get it building on all ports.
Comment 23 Ruth Fong 2013-06-06 17:22:12 PDT
Created attachment 203980 [details]
Patch
Comment 24 Jer Noble 2013-06-06 17:49:16 PDT
Comment on attachment 203980 [details]
Patch

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

> Source/WebCore/rendering/HitTestResult.cpp:421
> +    HTMLMediaElement* mediaElt = mediaElement();
> +    if (mediaElt && mediaElt->hasTagName(HTMLNames::videoTag)) {
> +        HTMLVideoElement* videoElt = static_cast<HTMLVideoElement*>(mediaElt);
> +        if (mediaElt->supportsFullscreen()) {
> +            UserGestureIndicator indicator(DefinitelyProcessingNewUserGesture);
> +            videoElt->toggleFullscreenState();
> +        }

Re-picking Geoff's nit: "mediaElt" => "mediaElement", "videoElt" => "videoElement", and solve the name collision by calling "this->mediaElement()"

Apart from that, r=me.
Comment 25 Ruth Fong 2013-06-06 17:53:18 PDT
Created attachment 203983 [details]
Patch
Comment 26 WebKit Commit Bot 2013-06-06 17:56:46 PDT
Comment on attachment 203983 [details]
Patch

Rejecting attachment 203983 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-01', 'validate-changelog', '--check-oops', '--non-interactive', 203983, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in Source/WebCore/ChangeLog contains OOPS!.

Full output: http://webkit-queues.appspot.com/results/790105
Comment 27 Ruth Fong 2013-06-06 18:36:00 PDT
Created attachment 203989 [details]
Patch
Comment 28 Jer Noble 2013-06-06 18:37:26 PDT
Comment on attachment 203989 [details]
Patch

r=me, but lets let the EWS bots chew on this change; I suspect there may be a switch statement somewhere that will complain about a missing case.
Comment 29 Carlos Garcia Campos 2013-06-07 00:10:45 PDT
Comment on attachment 203989 [details]
Patch

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

Thanks for fixing the GTK+ bits, but unfortunately it breaks WebKit1 GTK+ API and adds new API to WebKit2 GTK+. This should be reviewed by at least 2 GTK+ reviewers. r- because of the API break.

> Source/WebKit2/UIProcess/API/gtk/WebKitContextMenuActions.h:72
> + * @WEBKIT_CONTEXT_MENU_ACTION_EXIT_VIDEO_FULLSCREEN: Exit current video element in fullscreen mode.

This is new API, could you please move this to the end of the enum and append Since 2.2 before landing?

@WEBKIT_CONTEXT_MENU_ACTION_EXIT_VIDEO_FULLSCREEN: Exit current video element in fullscreen mode. Since 2.2

> Source/WebKit2/UIProcess/API/gtk/WebKitContextMenuActions.h:122
> +    WEBKIT_CONTEXT_MENU_ACTION_EXIT_VIDEO_FULLSCREEN,

Move this to the end please.

> Source/WebKit/gtk/webkit/webkitglobals.h:95
> - * @WEBKIT_CONTEXT_MENU_ACTION_ENTER_VIDEO_FULLSCREEN: Show current video element in fullscreen mode.
> + * @WEBKIT_CONTEXT_MENU_ACTION_TOGGLE_VIDEO_FULLSCREEN: Show current video element in fullscreen mode.

This is an API break.

> Source/WebKit/gtk/webkit/webkitglobals.h:140
> -    WEBKIT_CONTEXT_MENU_ACTION_ENTER_VIDEO_FULLSCREEN,
> +    WEBKIT_CONTEXT_MENU_ACTION_TOGGLE_VIDEO_FULLSCREEN,

Isn't it possible to do the same than in wk2, instead of renaming? Otherwise we need to deprecate the ENTER one and add TOGGLE at the end.
Comment 30 Carlos Garcia Campos 2013-06-07 00:25:16 PDT
Comment on attachment 203989 [details]
Patch

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

>> Source/WebKit/gtk/webkit/webkitglobals.h:140
>> +    WEBKIT_CONTEXT_MENU_ACTION_TOGGLE_VIDEO_FULLSCREEN,
> 
> Isn't it possible to do the same than in wk2, instead of renaming? Otherwise we need to deprecate the ENTER one and add TOGGLE at the end.

I wonder how this could build in EWS gtk, since we are using WEBKIT_CONTEXT_MENU_ACTION_ENTER_VIDEO_FULLSCREEN in webkitglobals.cpp, are we building with CONTEXT_MENUS disabled in EWS? phil?
Comment 31 Ruth Fong 2013-06-07 00:26:59 PDT
Comment on attachment 203989 [details]
Patch

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

>> Source/WebKit/gtk/webkit/webkitglobals.h:140
>> +    WEBKIT_CONTEXT_MENU_ACTION_TOGGLE_VIDEO_FULLSCREEN,
> 
> Isn't it possible to do the same than in wk2, instead of renaming? Otherwise we need to deprecate the ENTER one and add TOGGLE at the end.

I think the tag should be renamed in order to reflect the behavior appropriately; what if we introduced TOGGLE without deleting ENTER for now and file a bug for ENTER to be removed once gtk is stable and transitions to TOGGLE?
Comment 32 Carlos Garcia Campos 2013-06-07 00:37:05 PDT
(In reply to comment #31)
> (From update of attachment 203989 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=203989&action=review
> 
> >> Source/WebKit/gtk/webkit/webkitglobals.h:140
> >> +    WEBKIT_CONTEXT_MENU_ACTION_TOGGLE_VIDEO_FULLSCREEN,
> > 
> > Isn't it possible to do the same than in wk2, instead of renaming? Otherwise we need to deprecate the ENTER one and add TOGGLE at the end.
> 
> I think the tag should be renamed in order to reflect the behavior appropriately; what if we introduced TOGGLE without deleting ENTER for now and file a bug for ENTER to be removed once gtk is stable and transitions to TOGGLE?

Yes, that's the other alternative, marking ENTER as deprecated and add TOGGLE at the end as a new symbol. Why is not consistent with WebKit2 API then? This patch introduces EXIT instead of TOGGLE. I would like to discuss this API with the other GTK+ reviewers (patches adding new GTK+ API must be approved by at least 2 GTK+ reviewers), and then I can update the patch with the appropriate GTK+ changes to make it easier.
Comment 33 Gustavo Noronha (kov) 2013-06-07 07:02:01 PDT
(In reply to comment #30)
> I wonder how this could build in EWS gtk, since we are using WEBKIT_CONTEXT_MENU_ACTION_ENTER_VIDEO_FULLSCREEN in webkitglobals.cpp, are we building with CONTEXT_MENUS disabled in EWS? phil?

The EWS does a regular build, it does not change any (build-webkit) defaults, in terms of features. It looks like for GTK CONTEXT_MENUS is harcoded to enabled in Source/WTF/wtf/FeatureDefines.h, I can't see it in any of the feature configuration infrastructure.
Comment 34 Darin Adler 2013-06-07 07:19:52 PDT
Comment on attachment 203989 [details]
Patch

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

I think you should be leaving the GTK API change to people who work directly on GTK. There’s no rush to add this at the same time you add the underlying capability to WebCore.

> Source/WebKit2/Shared/API/c/WKSharedAPICast.h:472
> -    case WebCore::ContextMenuItemTagEnterVideoFullscreen:
> -        return kWKContextMenuItemTagEnterVideoFullscreen;
> +    case WebCore::ContextMenuItemTagToggleVideoFullscreen:
> +        return kWKContextMenuItemTagToggleVideoFullscreen;

I don’t understand why we are removing the enter fullscreen menu item while adding the toggle fullscreen one. Who suggested the removal? What’s the rationale?
Comment 35 Gustavo Noronha (kov) 2013-06-07 07:39:52 PDT
Comment on attachment 203989 [details]
Patch

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

>>>>> Source/WebKit/gtk/webkit/webkitglobals.h:140
>>>>> +    WEBKIT_CONTEXT_MENU_ACTION_TOGGLE_VIDEO_FULLSCREEN,
>>>> 
>>>> Isn't it possible to do the same than in wk2, instead of renaming? Otherwise we need to deprecate the ENTER one and add TOGGLE at the end.
>>> 
>>> I wonder how this could build in EWS gtk, since we are using WEBKIT_CONTEXT_MENU_ACTION_ENTER_VIDEO_FULLSCREEN in webkitglobals.cpp, are we building with CONTEXT_MENUS disabled in EWS? phil?
>> 
>> I think the tag should be renamed in order to reflect the behavior appropriately; what if we introduced TOGGLE without deleting ENTER for now and file a bug for ENTER to be removed once gtk is stable and transitions to TOGGLE?
> 
> Yes, that's the other alternative, marking ENTER as deprecated and add TOGGLE at the end as a new symbol. Why is not consistent with WebKit2 API then? This patch introduces EXIT instead of TOGGLE. I would like to discuss this API with the other GTK+ reviewers (patches adding new GTK+ API must be approved by at least 2 GTK+ reviewers), and then I can update the patch with the appropriate GTK+ changes to make it easier.

If you look at the hunk just above this one, WEBKIT_CONTEXT_MENU_ACTION_ENTER_VIDEO_FULLSCREEN has also been changed to WEBKIT_CONTEXT_MENU_ACTION_TOGGLE_VIDEO_FULLSCREEN in webkitglobals.cpp, that's why it builds.
Comment 36 Ruth Fong 2013-06-07 09:22:39 PDT
Comment on attachment 203989 [details]
Patch

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

>> Source/WebKit2/Shared/API/c/WKSharedAPICast.h:472
>> +        return kWKContextMenuItemTagToggleVideoFullscreen;
> 
> I don’t understand why we are removing the enter fullscreen menu item while adding the toggle fullscreen one. Who suggested the removal? What’s the rationale?

The patch changes fullscreen so that it works like the play/pause context menu item, i.e. when the video is paused, the "Play" item appears and vice versa. 

Previously, after entering fullscreen from the context menu, if you right-clicked the video to show the context menu while in fullscreen, you'd still see the "Enter Fullscreen" item, which when clicked in fullscreen, would do nothing.

The change from "Enter" to "Toggle" was meant to reflect that change in how the fullscreen context menu item would work.
Comment 37 Ruth Fong 2013-06-07 09:26:32 PDT
(In reply to comment #34)
> (From update of attachment 203989 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=203989&action=review
> 
> I think you should be leaving the GTK API change to people who work directly on GTK. There’s no rush to add this at the same time you add the underlying capability to WebCore.
> 
> > Source/WebKit2/Shared/API/c/WKSharedAPICast.h:472
> > -    case WebCore::ContextMenuItemTagEnterVideoFullscreen:
> > -        return kWKContextMenuItemTagEnterVideoFullscreen;
> > +    case WebCore::ContextMenuItemTagToggleVideoFullscreen:
> > +        return kWKContextMenuItemTagToggleVideoFullscreen;
> 
> I don’t understand why we are removing the enter fullscreen menu item while adding the toggle fullscreen one. Who suggested the removal? What’s the rationale?

I can continue to support "Enter" tags (and simply treat them) on GTK (and other ports as well) and leave the transition to port APIs to "Toggle" tags as another bug.
Comment 38 Ruth Fong 2013-06-07 14:13:19 PDT
Created attachment 204066 [details]
Patch
Comment 39 Jer Noble 2013-06-07 14:26:12 PDT
Comment on attachment 204066 [details]
Patch

LGTM, but since this makes changes to WebKit2, it still needs sign off from a WebKit2 owner.
Comment 40 Jon Lee 2013-06-07 14:58:06 PDT
Comment on attachment 204066 [details]
Patch

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

> Source/WebCore/ChangeLog:13
> +        without mac-specific changes to DumpRenderTree.

reference to bug?

> Source/WebCore/ChangeLog:23
> +        Updated to reame variables more appropriately to reflect the toggle-ability of video fullscreen.

sp: rename

> Source/WebCore/rendering/HitTestResult.cpp:407
> +        return mediaElement->isFullscreen();

I'd suggest containing the element within the var scope to reduce the code a bit, and improve readability:

if (HTMLMediaElement* mediaElement = this->mediaElement())
    return mediaElement->isVideo() && mediaElement->isFullscreen();

> Source/WebCore/rendering/HitTestResult.cpp:418
> +        HTMLVideoElement* videoElement = static_cast<HTMLVideoElement*>(mediaElement);

Is this local var necessary?

I think you can apply the above pattern here also.
Comment 41 Tim Horton 2013-06-07 15:05:12 PDT
Comment on attachment 204066 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        For mac ports only, this patch allows the fullscreen 

Mac.

> Source/WebCore/html/HTMLMediaElement.cpp:4365
> +#if PLATFORM(MAC)

Wonder if you want an #if ENABLE(TOGGLE_FULLSCREEN) or something (ugh, that seems terrible too, though). Something that other ports can turn on as they change their API instead of adding themselves to this list of (currently one) platform(s).

> Source/WebKit2/Shared/API/c/WKContextMenuItemTypes.h:113
> +#if PLATFORM(MAC)

I don't think we can add things in the middle of an API enum.
Comment 42 Ruth Fong 2013-06-07 16:32:12 PDT
Comment on attachment 204066 [details]
Patch

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

>> Source/WebCore/html/HTMLMediaElement.cpp:4365
>> +#if PLATFORM(MAC)
> 
> Wonder if you want an #if ENABLE(TOGGLE_FULLSCREEN) or something (ugh, that seems terrible too, though). Something that other ports can turn on as they change their API instead of adding themselves to this list of (currently one) platform(s).

Yeah, this is probably a more robust implementation.

>> Source/WebKit2/Shared/API/c/WKContextMenuItemTypes.h:113
>> +#if PLATFORM(MAC)
> 
> I don't think we can add things in the middle of an API enum.

Wouldn't "Enter" and "Toggle" effectively be the same tag? (since only one will ever be enumerated, they'd share the same enum value?)
Comment 43 Tim Horton 2013-06-07 16:42:18 PDT
Comment on attachment 204066 [details]
Patch

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

>>> Source/WebKit2/Shared/API/c/WKContextMenuItemTypes.h:113
>>> +#if PLATFORM(MAC)
>> 
>> I don't think we can add things in the middle of an API enum.
> 
> Wouldn't "Enter" and "Toggle" effectively be the same tag? (since only one will ever be enumerated, they'd share the same enum value?)

You're right, ignore me.
Comment 44 Ruth Fong 2013-06-07 16:54:28 PDT
Comment on attachment 204066 [details]
Patch

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

>>>> Source/WebKit2/Shared/API/c/WKContextMenuItemTypes.h:113
>>>> +#if PLATFORM(MAC)
>>> 
>>> I don't think we can add things in the middle of an API enum.
>> 
>> Wouldn't "Enter" and "Toggle" effectively be the same tag? (since only one will ever be enumerated, they'd share the same enum value?)
> 
> You're right, ignore me.

Are you sure? Alex just persuaded me that you're right. I was going to eliminate the preprocessing conditions and just add kWKContextMenuItemTagToggleVideoFullscreen at the end of the list.
Comment 45 Ruth Fong 2013-06-07 17:05:27 PDT
Comment on attachment 204066 [details]
Patch

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

>>> Source/WebCore/html/HTMLMediaElement.cpp:4365
>>> +#if PLATFORM(MAC)
>> 
>> Wonder if you want an #if ENABLE(TOGGLE_FULLSCREEN) or something (ugh, that seems terrible too, though). Something that other ports can turn on as they change their API instead of adding themselves to this list of (currently one) platform(s).
> 
> Yeah, this is probably a more robust implementation.

Is there a specific feature defines file that I can define TOGGLE_FULLSCREEN in?
Comment 46 Ruth Fong 2013-06-07 17:42:41 PDT
Created attachment 204076 [details]
Patch
Comment 47 Darin Adler 2013-06-07 17:47:05 PDT
Comment on attachment 204076 [details]
Patch

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

> Source/WTF/wtf/FeatureDefines.h:757
> +#if !defined(ENABLE_TOGGLE_VIDEO_FULLSCREEN)
> +#define ENABLE_TOGGLE_VIDEO_FULLSCREEN 1
> +#endif

Does not seem right to have this be a feature flag.

> Source/WebCore/html/HTMLMediaElement.cpp:4375
> +#if ENABLE(TOGGLE_VIDEO_FULLSCREEN)
> +void HTMLMediaElement::toggleFullscreenState()
> +{
> +    LOG(Media, "HTMLMediaElement::toggleFullscreenState - isFullscreen() is %s", boolString(isFullscreen()));
> +    
> +    if (isFullscreen())
> +        exitFullscreen();
> +    else
> +        enterFullscreen();
> +}
> +#endif

This should be in there unconditionally. Whether someone wants this command in the context menu or not should not control whether we even compile this code.

> Source/WebCore/platform/ContextMenuItem.h:163
> +#if ENABLE(TOGGLE_VIDEO_FULLSCREEN)
> +        ContextMenuItemTagToggleVideoFullscreen,
> +#else
>          ContextMenuItemTagEnterVideoFullscreen,
> +#endif

I really don’t understand this. Why is the toggle video and enter video an either/or choice? Why can’t both be possible context menu items?
Comment 48 Early Warning System Bot 2013-06-07 17:56:06 PDT
Comment on attachment 204076 [details]
Patch

Attachment 204076 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/745777
Comment 49 Early Warning System Bot 2013-06-07 17:59:13 PDT
Comment on attachment 204076 [details]
Patch

Attachment 204076 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/697855
Comment 50 Ruth Fong 2013-06-07 17:59:54 PDT
Comment on attachment 204076 [details]
Patch

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

>> Source/WTF/wtf/FeatureDefines.h:757
>> +#endif
> 
> Does not seem right to have this be a feature flag.

There should be some sort of pre-processing checking for parts of this patch (for ports that do want to be able to toggle fullscreen, other parts need to be removed) and defining it as a feature flag seems to be the best way to minimize the amount of code change needed to allow other ports to upgrade their APIs to support toggling fullscreen video.

>> Source/WebCore/html/HTMLMediaElement.cpp:4375
>> +#endif
> 
> This should be in there unconditionally. Whether someone wants this command in the context menu or not should not control whether we even compile this code.

You're right.

>> Source/WebCore/platform/ContextMenuItem.h:163
>> +#endif
> 
> I really don’t understand this. Why is the toggle video and enter video an either/or choice? Why can’t both be possible context menu items?

The toggle video item also encompasses entering into fullscreen; the either/or choice was made to encourage either supporting the existing implementation of fullscreen context menu item or switching to a toggling implementation but not to support both (that would create the side effect of possibly having two fullscreen context menu items).
Comment 51 EFL EWS Bot 2013-06-07 18:00:14 PDT
Comment on attachment 204076 [details]
Patch

Attachment 204076 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/697856
Comment 52 Ruth Fong 2013-06-07 18:06:54 PDT
Comment on attachment 204076 [details]
Patch

Just talked to Sam. Will be adding toggling while still fully supporting enter (i.e. no ifdefs/feature flags).
Comment 53 EFL EWS Bot 2013-06-07 18:07:31 PDT
Comment on attachment 204076 [details]
Patch

Attachment 204076 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/752273
Comment 54 kov's GTK+ EWS bot 2013-06-07 18:19:32 PDT
Comment on attachment 204076 [details]
Patch

Attachment 204076 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/802001
Comment 55 Ruth Fong 2013-06-07 18:44:40 PDT
Created attachment 204078 [details]
Patch
Comment 56 Early Warning System Bot 2013-06-07 18:52:27 PDT
Comment on attachment 204078 [details]
Patch

Attachment 204078 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/746505
Comment 57 Early Warning System Bot 2013-06-07 18:53:06 PDT
Comment on attachment 204078 [details]
Patch

Attachment 204078 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/805004
Comment 58 EFL EWS Bot 2013-06-07 18:53:29 PDT
Comment on attachment 204078 [details]
Patch

Attachment 204078 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/760300
Comment 59 EFL EWS Bot 2013-06-07 18:57:21 PDT
Comment on attachment 204078 [details]
Patch

Attachment 204078 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/791211
Comment 60 Ruth Fong 2013-06-07 19:01:22 PDT
Created attachment 204080 [details]
Patch
Comment 61 EFL EWS Bot 2013-06-07 19:06:54 PDT
Comment on attachment 204080 [details]
Patch

Attachment 204080 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/791216
Comment 62 Ruth Fong 2013-06-07 19:20:14 PDT
Created attachment 204081 [details]
Patch
Comment 63 Darin Adler 2013-06-08 18:42:34 PDT
Comment on attachment 204078 [details]
Patch

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

> Source/WebCore/page/ContextMenuController.cpp:854
> +            appendItem(ToggleVideoFullscreen, m_contextMenu.get());
>              appendItem(EnterVideoFullscreen, m_contextMenu.get());

This is the one place where we should not keep both items, right? Because this is defining what we actually see in the menu. For the Mac platform I assume we now want the toggle menu item, not the enter one.

Otherwise, this patch looks like a step in the right direction.
Comment 64 Darin Adler 2013-06-08 18:43:37 PDT
Comment on attachment 204081 [details]
Patch

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

> Source/WebCore/page/ContextMenuController.cpp:854
> +            appendItem(ToggleVideoFullscreen, m_contextMenu.get());
>              appendItem(EnterVideoFullscreen, m_contextMenu.get());

Here is the place where we want only one or the other. I think this is part that needs to be in an #if.
Comment 65 Ruth Fong 2013-06-09 19:45:24 PDT
Comment on attachment 204081 [details]
Patch

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

>> Source/WebCore/page/ContextMenuController.cpp:854
>>              appendItem(EnterVideoFullscreen, m_contextMenu.get());
> 
> Here is the place where we want only one or the other. I think this is part that needs to be in an #if.

Yeah, we should probably put an #if here (though each port should take the populated list of context menu items and make final edits beforehand).

> Source/WebCore/page/ContextMenuController.cpp:1352
> +#if PLATFORM(MAC)

Here's another place we need the #if (since contextMenuItemTagExitVideoFullscreen() is not supported on all ports)

Since the two places we need #if statements is in ContextMenuController; perhaps I can define a SUPPORTS_TOGGLE_VIDEO_FULLSCREEN feature in the header or create a static helper function that checks whether the given port supports "toggle".
Comment 66 Ruth Fong 2013-06-10 09:16:07 PDT
Created attachment 204168 [details]
Patch
Comment 67 Dean Jackson 2013-06-10 15:13:31 PDT
Comment on attachment 204168 [details]
Patch

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

> Source/WebCore/page/ContextMenuController.cpp:1366
> +        case ContextMenuItemTagToggleVideoFullscreen:
> +#if SUPPORTS_TOGGLE_VIDEO_FULLSCREEN
> +                if (!m_hitTestResult.mediaIsInFullscreen())
> +                    item.setTitle(contextMenuItemTagEnterVideoFullscreen());
> +                else 
> +                    item.setTitle(contextMenuItemTagExitVideoFullscreen());
> +                break;
> +#endif

Nit: indent here is one level too many.

Also, I suggest rewriting this as item.setTitle(m_hitTestResult.mediaIsInFullscreen() ? contextMen...)
Comment 68 Ruth Fong 2013-06-10 15:52:24 PDT
Created attachment 204221 [details]
Patch
Comment 69 WebKit Commit Bot 2013-06-12 10:33:40 PDT
Comment on attachment 204221 [details]
Patch

Clearing flags on attachment: 204221

Committed r151512: <http://trac.webkit.org/changeset/151512>
Comment 70 WebKit Commit Bot 2013-06-12 10:33:48 PDT
All reviewed patches have been landed.  Closing bug.