[GTK] enhanced context menu for media elements

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Philippe Normand]

It'd be nice to have entries for:

- copy media url to clipboard
- download media
- open in new window (?)
- controls display control
- switch to fullscreen
- loop playback control

[Philippe Normand]

Created attachment 67017 [details]
proposed patch (test missing)

[Philippe Normand]

The test doesn't seem very easy to write, I can send a contextClick() event on the media element but then, how do i know at which position to move the mouse to click on a specific item? :(

[Philippe Normand]

Created attachment 67035 [details]
proposed patch (test missing)

[WebKit Review Bot]

Attachment 67035 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/platform/gtk/ContextMenuItemGtk.cpp:108:  Use 0 instead of NULL.  [readability/null] [5]
WebCore/rendering/HitTestResult.cpp:297:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Total errors found: 2 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Philippe Normand]

(In reply to comment #4)
> Attachment 67035 [details] did not pass style-queue:
> 
> Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
> WebCore/platform/gtk/ContextMenuItemGtk.cpp:108:  Use 0 instead of NULL.  [readability/null] [5]
> WebCore/rendering/HitTestResult.cpp:297:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
> Total errors found: 2 in 16 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

This version sports play/pause mute/unmute too

[Philippe Normand]

Created attachment 67650 [details]
proposed patch

[Philippe Normand]

This patch is on top of the one in bug 39102 which hasn't landed yet. That's while the style-bot fails to check it.

[Eric Carlson]

Comment on attachment 67650 [details]
proposed patch

> +            function start()
> +            {
> +                findMediaElement();
> +                waitForEvent('play', canplaythrough);
> +                run("video.src = '" + findMediaFile("video", "content/test") + "'");
> +            }
"canplaythrough" is an odd name for a test that triggers on the 'play' event ;-)

> +            if (result.mediaSupportsFullscreen())
> +                appendItem(EnterVideoFullscreen);
> +
I wonder if it would make more sense to always append the fullcreen menu 
item for a <video> element and enable/disable it based on supportsFullscreen().
It will be somewhat confusing to some that the menu item isn't always enabled,
but it will also be consusing that the menu item is sometimes there and sometimes
not for the same element.

I am not positive which is the correct behavior for this menu item...


> +        case ContextMenuItemTagMediaMute:
> +            if (m_hitTestResult.mediaMuted())
> +                item.setTitle(contextMenuItemTagMediaUnMute());
> +            else
> +                item.setTitle(contextMenuItemTagMediaMute());
You should disable the mute menu item and not toggle the title if the element does
not have an audio track:

    case ContextMenuItemTagMediaMute:
        shouldEnable = m_hitTestResult.mediaHasAudio();
        if (!shouldEnable || !m_hitTestResult.mediaMuted())
            item.setTitle(contextMenuItemTagMediaMute());
        else
            item.setTitle(contextMenuItemTagMediaUnMute());
        break;

> +    HTMLMediaElement* mediaElt(mediaElement());
> +    if (mediaElt)
> +        return m_innerNonSharedNode->document()->completeURL(deprecatedParseURL(mediaElt->currentSrc()));
I think something like "if (HTMLMediaElement* mediaElt = mediaElement())" would be
more in keeping with the current C++ style for this type of test.

> +    HTMLMediaElement* mediaElt(mediaElement());
> +    if (mediaElt)
> +        mediaElt->setControls(!mediaElt->controls());
Ditto.

> +    HTMLMediaElement* mediaElt(mediaElement());
> +    if (mediaElt)
> +        mediaElt->setLoop(!mediaElt->loop());
Ditto.

> +    HTMLMediaElement* mediaElt(mediaElement());
> +    if (mediaElt)
> +        return mediaElt->controls();
Ditto.

> +    HTMLMediaElement* mediaElt(mediaElement());
> +    if (mediaElt)
> +        return mediaElt->loop();
Ditto.

> +    HTMLMediaElement* mediaElt(mediaElement());
> +    if (mediaElt)
> +        return !mediaElt->paused();
Ditto.


> +    HTMLMediaElement* mediaElt(mediaElement());
> +    if (mediaElt)
> +        mediaElt->togglePlayState();
Ditto.

> +    HTMLMediaElement* mediaElt(mediaElement());
> +    if (mediaElt)
> +        return mediaElt->muted();
Ditto.

> +    HTMLMediaElement* mediaElt(mediaElement());
> +    if (mediaElt)
> +        mediaElt->setMuted(!mediaElt->muted());
Ditto.

It would be nice to have a menu item for saving the movie file. HTMLMediaElement has a
supportsSave() method that should be used to enable/disable (or add/remove) the menu item
because not all movies will be savable. Please file a bug for this if you don't want to
add it to this patch.

Nice stuff, it will be great to finally have this feature!

r=me with these suggestions.

[Philippe Normand]

Thanks for the review Eric!

I applied the changes requested in my branch.

About the fullscreen/save menus I think it depends on the UI design guidelines of the platform.

For instance the GNOME HIG recommends to show the items but make them inactive: http://library.gnome.org/devel/hig-book/stable/menus-design.html.en#menu-item-type-command

I think this is what I'll do... On mac it would probably be different, I don't know.

I will open a separate bug report for supportsSave, this patch is already quite big.

Also I'll wait the bug 39102 gets reviewed to land this patch ;)

[Philippe Normand]

Committed r67628: <http://trac.webkit.org/changeset/67628>

[WebKit Review Bot]

http://trac.webkit.org/changeset/67628 might have broken Qt Linux Release minimal, Qt Linux ARMv5 Release, Qt Linux ARMv7 Release, Qt Windows 32-bit Release, Qt Windows 32-bit Debug, Chromium Win Release, Chromium Mac Release, and Chromium Linux Release
The following changes are on the blame list:
http://trac.webkit.org/changeset/67626
http://trac.webkit.org/changeset/67627
http://trac.webkit.org/changeset/67628
http://trac.webkit.org/changeset/67629
http://trac.webkit.org/changeset/67630
http://trac.webkit.org/changeset/67631

[Philippe Normand]

the new ContextMenu::contextMenu* methods are not implemented in the mac, qt, chromium ports. Will send a fixed patch, sorry for the major breakage :/

[Philippe Normand]

Created attachment 67889 [details]
updated patch

[Philippe Normand]

Created attachment 67892 [details]
updated patch

[WebKit Review Bot]

Attachment 67892 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/platform/gtk/ContextMenuItemGtk.cpp:108:  Use 0 instead of NULL.  [readability/null] [5]
Total errors found: 1 in 36 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Philippe Normand]

Created attachment 67893 [details]
updated patch

[Eric Carlson]

Comment on attachment 67893 [details]
updated patch

This doesn't work on OS X and Windows because the localizable strings are missing. I will add them and update the patch.

[Philippe Normand]

Comment on attachment 67893 [details]
updated patch

>  WebKit/mac/WebCoreSupport/WebPlatformStrategies.h  |    9 ++
>  WebKit/mac/WebCoreSupport/WebPlatformStrategies.mm |   45 +++++++
>  .../win/WebCoreSupport/WebPlatformStrategies.cpp   |   45 +++++++
>  .../WebCoreSupport/WebPlatformStrategies.cpp       |   45 +++++++
>  .../WebCoreSupport/WebPlatformStrategies.h         |    9 ++

So what are those? Did I miss something?

[Eric Carlson]

(In reply to comment #18)
> (From update of attachment 67893 [details])
> >  WebKit/mac/WebCoreSupport/WebPlatformStrategies.h  |    9 ++
> >  WebKit/mac/WebCoreSupport/WebPlatformStrategies.mm |   45 +++++++
> >  .../win/WebCoreSupport/WebPlatformStrategies.cpp   |   45 +++++++
> >  .../WebCoreSupport/WebPlatformStrategies.cpp       |   45 +++++++
> >  .../WebCoreSupport/WebPlatformStrategies.h         |    9 ++
> 
> So what are those? Did I miss something?

You didn't add the actual strings to Localizable.strings or WebLocalizableStrings.cpp

[Eric Carlson]

(In reply to comment #19)

> You didn't add the actual strings to Localizable.strings or WebLocalizableStrings.cpp

No need to update WebLocalizableStrings.cpp as it doesn't exist any more ;-)

[Eric Carlson]

Created attachment 67917 [details]
Patch

This adds the missing Mac strings and changes the behavior of the "Mute" menu item from changing the text to adding and removing a check mark.

I think the patch should use "Audio" or "Video" instead of "Media" (depending on the element type and movie contents) in "Open Media in New Window" and "Copy Media Address".

[Ademar Reis]

Just a heads up:

WebCore/platform/qt/Localizations.cpp is no more after r67787 <http://trac.webkit.org/changeset/67787>, so you'll have to update your patch again to update the respective platform strategies file instead.

[Philippe Normand]

Created attachment 68097 [details]
updated patch

[Philippe Normand]

Created attachment 68100 [details]
updated patch probably more utf-16 friendly

[Eric Carlson]

Comment on attachment 68100 [details]
updated patch probably more utf-16 friendly

Thanks!

[Philippe Normand]

Committed r67928: <http://trac.webkit.org/changeset/67928>

[Simon Fraser (smfr)]

Comment on attachment 68097 [details]
updated patch

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

> WebCore/rendering/HitTestResult.h:97
> +    void toggleMediaControlsDisplay() const;
> +    void toggleMediaLoopPlayback() const;
> +    void enterFullscreenForVideo() const;
> +    bool mediaControlsEnabled() const;
> +    bool mediaLoopEnabled() const;
> +    bool mediaPlaying() const;
> +    bool mediaSupportsFullscreen() const;
> +    void toggleMediaPlayState() const;
> +    bool mediaHasAudio() const;
> +    bool mediaIsVideo() const;
> +    bool mediaMuted() const;
> +    void toggleMediaMuteState() const;

This stuff doesn't belong on rendering/HitTestResult.