Bug 117220 – Allow for toggling fullscreen on <video> elements

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Ruth Fong]

Refactor code so that video fullscreen state can be toggled (much like play/pause is toggled).

[Ruth Fong]

<rdar://problem/14015839>

[Ruth Fong]

Created attachment 203747 [details]
Patch

[Early Warning System Bot]

Comment on attachment 203747 [details]
Patch

Attachment 203747 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/673630

[kov's GTK+ EWS bot]

Comment on attachment 203747 [details]
Patch

Attachment 203747 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/714621

[Early Warning System Bot]

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

[EFL EWS Bot]

Comment on attachment 203747 [details]
Patch

Attachment 203747 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/768278

[EFL EWS Bot]

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

[Eric Carlson]

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.

[Ruth Fong]

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?

[Jer Noble]

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.

[Jer Noble]

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.

[Ruth Fong]

Created attachment 203868 [details]
Patch

[WebKit Commit Bot]

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

[Early Warning System Bot]

Comment on attachment 203868 [details]
Patch

Attachment 203868 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/694411

[Geoffrey Garen]

> 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.

[EFL EWS Bot]

Comment on attachment 203868 [details]
Patch

Attachment 203868 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/689696

[Early Warning System Bot]

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

[EFL EWS Bot]

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

[Ruth Fong]

Created attachment 203887 [details]
Patch

[Early Warning System Bot]

Comment on attachment 203887 [details]
Patch

Attachment 203887 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/664384

[Early Warning System Bot]

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

[Ruth Fong]

Comment on attachment 203887 [details]
Patch

WIP - trying to get it building on all ports.

[Ruth Fong]

Created attachment 203980 [details]
Patch

[Jer Noble]

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.

[Ruth Fong]

Created attachment 203983 [details]
Patch

[WebKit Commit Bot]

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

[Ruth Fong]

Created attachment 203989 [details]
Patch

[Jer Noble]

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.

[Carlos Garcia Campos]

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.

[Carlos Garcia Campos]

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?

[Ruth Fong]

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?

[Carlos Garcia Campos]

(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.

[Gustavo Noronha (kov)]

(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.

[Darin Adler]

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?

[Gustavo Noronha (kov)]

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.

[Ruth Fong]

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.

[Ruth Fong]

(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.

[Ruth Fong]

Created attachment 204066 [details]
Patch

[Jer Noble]

Comment on attachment 204066 [details]
Patch

LGTM, but since this makes changes to WebKit2, it still needs sign off from a WebKit2 owner.

[Jon Lee]

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.

[Tim Horton]

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.

[Ruth Fong]

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?)

[Tim Horton]

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.

[Ruth Fong]

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.

[Ruth Fong]

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?

[Ruth Fong]

Created attachment 204076 [details]
Patch

[Darin Adler]

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?

[Early Warning System Bot]

Comment on attachment 204076 [details]
Patch

Attachment 204076 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/745777

[Early Warning System Bot]

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

[Ruth Fong]

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).

[EFL EWS Bot]

Comment on attachment 204076 [details]
Patch

Attachment 204076 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/697856

[Ruth Fong]

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).

[EFL EWS Bot]

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

[kov's GTK+ EWS bot]

Comment on attachment 204076 [details]
Patch

Attachment 204076 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/802001

[Ruth Fong]

Created attachment 204078 [details]
Patch

[Early Warning System Bot]

Comment on attachment 204078 [details]
Patch

Attachment 204078 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/746505

[Early Warning System Bot]

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

[EFL EWS Bot]

Comment on attachment 204078 [details]
Patch

Attachment 204078 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/760300

[EFL EWS Bot]

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

[Ruth Fong]

Created attachment 204080 [details]
Patch

[EFL EWS Bot]

Comment on attachment 204080 [details]
Patch

Attachment 204080 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/791216

[Ruth Fong]

Created attachment 204081 [details]
Patch

[Darin Adler]

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.

[Darin Adler]

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.

[Ruth Fong]

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".

[Ruth Fong]

Created attachment 204168 [details]
Patch

[Dean Jackson]

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...)

[Ruth Fong]

Created attachment 204221 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 204221 [details]
Patch

Clearing flags on attachment: 204221

Committed r151512: <http://trac.webkit.org/changeset/151512>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.