[webkit-reviews] review granted: [Bug 45021] [GTK] enhanced context menu for media elements : [Attachment 67650] proposed patch

[bugzilla-daemon at webkit.org]

Eric Carlson <eric.carlson at apple.com> has granted Philippe Normand
<pnormand at igalia.com>'s request for review:
Bug 45021: [GTK] enhanced context menu for media elements
https://bugs.webkit.org/show_bug.cgi?id=45021

Attachment 67650: proposed patch
https://bugs.webkit.org/attachment.cgi?id=67650&action=review

------- Additional Comments from Eric Carlson <eric.carlson at apple.com>
> +	       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->curr
entSrc()));
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.