Bug 85194 - [GTK] Missing WebPreferences for media playback requiring user gestures and inline playback
Summary: [GTK] Missing WebPreferences for media playback requiring user gestures and i...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-04-30 07:48 PDT by Simon Pena
Modified: 2012-05-09 10:22 PDT (History)
4 users (show)

See Also:


Attachments
Patch (7.74 KB, patch)
2012-04-30 09:40 PDT, Simon Pena
no flags Details | Formatted Diff | Diff
Patch (7.78 KB, patch)
2012-05-09 08:30 PDT, Simon Pena
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Pena 2012-04-30 07:48:44 PDT
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.
Comment 1 Simon Pena 2012-04-30 09:40:53 PDT
Created attachment 139466 [details]
Patch
Comment 2 Simon Pena 2012-04-30 09:46:23 PDT
(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.
Comment 3 Philippe Normand 2012-05-01 01:01:43 PDT
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?
Comment 4 Simon Pena 2012-05-02 13:58:08 PDT
(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.
Comment 5 Philippe Normand 2012-05-03 06:39:28 PDT
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!
Comment 6 Martin Robinson 2012-05-03 08:41:00 PDT
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?
Comment 7 Simon Pena 2012-05-09 07:55:04 PDT
(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.
Comment 8 Martin Robinson 2012-05-09 08:17:43 PDT
Sounds good to me!
Comment 9 Simon Pena 2012-05-09 08:30:16 PDT
Created attachment 140951 [details]
Patch
Comment 10 WebKit Review Bot 2012-05-09 10:22:40 PDT
Comment on attachment 140951 [details]
Patch

Clearing flags on attachment: 140951

Committed r116541: <http://trac.webkit.org/changeset/116541>
Comment 11 WebKit Review Bot 2012-05-09 10:22:45 PDT
All reviewed patches have been landed.  Closing bug.