[Webkit-unassigned] [Bug 45021] [GTK] enhanced context menu for media elements

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=45021


Eric Carlson <eric.carlson at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #67650|review?                     |review+
               Flag|                            |




--- Comment #8 from Eric Carlson <eric.carlson at apple.com>  2010-09-15 05:46:52 PST ---
(From update of attachment 67650)
> +            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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.