Bug 85194 – [GTK] Missing WebPreferences for media playback requiring user gestures and inline playback

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

[Simon Pena]

When the WebPreferences for media playback requiring user gestures or
supporting inline playback were added in bug #64742, GTK+ support wasn't.

Later, when bug #83620 (Media engine should not be told to prepare for playback
if media loading is not allowed) was fixed, some unit tests started being flaky.
This was reported as bug #83874. Although I proposed there a fix involving
adding the missing WebPreferences to the GTK+ API, that wasn't the cause for
flakyness (Philippe solved that fixing an existing error in the tests). 

The WebPreferences weren't finally added there, so this bug is meant for that.

[Simon Pena]

Created attachment 139466 [details]
Patch

[Simon Pena]

(In reply to comment #1)
> Created an attachment (id=139466) [details]
> Patch

I'm not sure if this patch deserves new tests: in bug #64742, no tests were added, and they were similar changes.

I noticed an almost-flaky test (media/track/track-cues-pause-on-exit.html). It's been failing sometimes (timing out) after applying this test, but not always. I don't think it is related to this change, but anyway.

[Philippe Normand]

Comment on attachment 139466 [details]
Patch

Alright, looks good to me but we'd need another reviewer to +1. Will you provide a similar patch but for WebKit2?

[Simon Pena]

(In reply to comment #3)
> (From update of attachment 139466 [details])
> Alright, looks good to me but we'd need another reviewer to +1. Will you provide a similar patch but for WebKit2?

Yes: I'll report a new bug with a patch for the WebKit2 part tomorrow.

[Philippe Normand]

Martin can you please have a look at this patch? I did a first review but we need another +1 for the added API. Thanks!

[Martin Robinson]

Comment on attachment 139466 [details]
Patch

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

Looks good. r- only because you don't have commit access yet the documentation isn't quite clear.

> Source/WebKit/gtk/webkit/webkitwebsettings.cpp:985
> +    * Whether an user gesture would be required to start media playback or load
> +    * media. This is off by default, so media playback could start
> +    * automatically. Setting it on requires a gesture by the user to start
> +    * playback, or to load the media.
> +    *

Weird English rules exception nit: Whether an user -> Whether a user 

I think this documentation could be slightly more clear about what kind of gesture is required to start playback. For instance, if I turn this on, what will I need to do to trigger playback? Mouse over the page? Mouse over the element itself? Click on the element?

[Simon Pena]

(In reply to comment #6)
> (From update of attachment 139466 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=139466&action=review
> 
> Looks good. r- only because you don't have commit access yet the documentation isn't quite clear.
> 
> > Source/WebKit/gtk/webkit/webkitwebsettings.cpp:985
> > +    * Whether an user gesture would be required to start media playback or load
> > +    * media. This is off by default, so media playback could start
> > +    * automatically. Setting it on requires a gesture by the user to start
> > +    * playback, or to load the media.
> > +    *
> 
> Weird English rules exception nit: Whether an user -> Whether a user 

Thanks for catching, I'll fix this.
 
> I think this documentation could be slightly more clear about what kind of gesture is required to start playback. For instance, if I turn this on, what will I need to do to trigger playback? Mouse over the page? Mouse over the element itself? Click on the element?

From « LayoutTests/media/video-play-require-user-gesture.html », I understand it typically involves clicking on the Play button, but probably other gestures could be required.

I guess I can make the documentation read something like:

« Whether a user gesture (typically clicking on the play button) would be required to start media playback or load media... »

What do you think? I'll use the same description for the WK2 patch.

[Martin Robinson]

Sounds good to me!

[Simon Pena]

Created attachment 140951 [details]
Patch

[WebKit Review Bot]

Comment on attachment 140951 [details]
Patch

Clearing flags on attachment: 140951

Committed r116541: <http://trac.webkit.org/changeset/116541>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.