Bug 64742 - Expose WebPreferences for media playback requiring user gestures and inline playback
Summary: Expose WebPreferences for media playback requiring user gestures and inline p...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dean Jackson
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2011-07-18 12:11 PDT by Dean Jackson
Modified: 2011-07-18 19:22 PDT (History)
1 user (show)

See Also:


Attachments
Patch (20.01 KB, patch)
2011-07-18 12:34 PDT, Dean Jackson
dino: review+
Details | Formatted Diff | Diff
Patch (20.84 KB, patch)
2011-07-18 15:45 PDT, Dean Jackson
no flags Details | Formatted Diff | Diff
Patch (20.85 KB, patch)
2011-07-18 17:24 PDT, Dean Jackson
no flags Details | Formatted Diff | Diff
Patch (20.85 KB, patch)
2011-07-18 17:46 PDT, Dean Jackson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dean Jackson 2011-07-18 12:11:37 PDT
Expose a WebPreference and Setting for two media playback cases:

- media needing a user gesture for playback (this is already tested in HTMLMediaElement, but there is no way to set it)
- media allowing inline playback (some ports restrict playback to full screen only)
Comment 1 Dean Jackson 2011-07-18 12:34:19 PDT
Created attachment 101190 [details]
Patch
Comment 2 Dean Jackson 2011-07-18 12:37:25 PDT
Typo in ChangeLog already fixed.
Comment 3 Dean Jackson 2011-07-18 12:43:00 PDT
<rdar://problem/9794233>
Comment 4 Simon Fraser (smfr) 2011-07-18 13:19:53 PDT
Comment on attachment 101190 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        Media playback already tested for if it should require user gestures, but

" for if it should require" is awkward.

> Source/WebCore/html/HTMLMediaElement.cpp:189
> +        m_restrictions |= RequireUserGestureForRateChangeRestriction;

The "Restriction" suffix makes RequireUserGestureForRateChangeRestriction confusing to read. It could be removed.

> Source/WebKit/win/WebPreferences.h:445
> +    virtual HRESULT STDMETHODCALLTYPE mediaPlaybackRequiresUserGesture(BOOL*);
> +    virtual HRESULT STDMETHODCALLTYPE setMediaPlaybackRequiresUserGesture(BOOL);
> +
> +    virtual HRESULT STDMETHODCALLTYPE mediaPlaybackAllowsInline(BOOL*);
> +    virtual HRESULT STDMETHODCALLTYPE setMediaPlaybackAllowsInline(BOOL);

Please check to see whether these should be at the end to avoid changing the ABI

> Source/WebKit2/UIProcess/API/C/WKPreferencesPrivate.h:142
> +// Defaults to false.
> +WK_EXPORT void WKPreferencesSetMediaPlaybackRequiresUserGesture(WKPreferencesRef preferencesRef, bool flag);
> +WK_EXPORT bool WKPreferencesGetMediaPlaybackRequiresUserGesture(WKPreferencesRef preferencesRef);
> +
> +// Defaults to true.
> +WK_EXPORT void WKPreferencesSetMediaPlaybackAllowsInline(WKPreferencesRef preferencesRef, bool flag);
> +WK_EXPORT bool WKPreferencesGetMediaPlaybackAllowsInline(WKPreferencesRef preferencesRef);
> +

The parameter names seem redundant.
Comment 5 Dean Jackson 2011-07-18 15:29:06 PDT
(In reply to comment #4)
> (From update of attachment 101190 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=101190&action=review
> 
> > Source/WebCore/ChangeLog:8
> > +        Media playback already tested for if it should require user gestures, but
> 
> " for if it should require" is awkward.

Yeah, it was a typo.

> 
> > Source/WebCore/html/HTMLMediaElement.cpp:189
> > +        m_restrictions |= RequireUserGestureForRateChangeRestriction;
> 
> The "Restriction" suffix makes RequireUserGestureForRateChangeRestriction confusing to read. It could be removed.

OK. It's an enum with a few values. I'll change them all.

> 
> > Source/WebKit/win/WebPreferences.h:445
> > +    virtual HRESULT STDMETHODCALLTYPE mediaPlaybackRequiresUserGesture(BOOL*);
> > +    virtual HRESULT STDMETHODCALLTYPE setMediaPlaybackRequiresUserGesture(BOOL);
> > +
> > +    virtual HRESULT STDMETHODCALLTYPE mediaPlaybackAllowsInline(BOOL*);
> > +    virtual HRESULT STDMETHODCALLTYPE setMediaPlaybackAllowsInline(BOOL);
> 
> Please check to see whether these should be at the end to avoid changing the ABI

I'm told they should go at the bottom, so cool.

> 
> > Source/WebKit2/UIProcess/API/C/WKPreferencesPrivate.h:142
> > +// Defaults to false.
> > +WK_EXPORT void WKPreferencesSetMediaPlaybackRequiresUserGesture(WKPreferencesRef preferencesRef, bool flag);
> > +WK_EXPORT bool WKPreferencesGetMediaPlaybackRequiresUserGesture(WKPreferencesRef preferencesRef);
> > +
> > +// Defaults to true.
> > +WK_EXPORT void WKPreferencesSetMediaPlaybackAllowsInline(WKPreferencesRef preferencesRef, bool flag);
> > +WK_EXPORT bool WKPreferencesGetMediaPlaybackAllowsInline(WKPreferencesRef preferencesRef);
> > +
> 
> The parameter names seem redundant.

I was copying existing style, but I agree.
Comment 6 Dean Jackson 2011-07-18 15:45:33 PDT
Created attachment 101227 [details]
Patch

Testing windows compilation
Comment 7 Dean Jackson 2011-07-18 17:24:59 PDT
Created attachment 101241 [details]
Patch

Testing windows compilation again
Comment 8 Dean Jackson 2011-07-18 17:46:06 PDT
Created attachment 101244 [details]
Patch

Worked out where the typo was. Sad face.
Comment 9 Dean Jackson 2011-07-18 19:22:09 PDT
Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	Source/WebCore/ChangeLog
	M	Source/WebCore/html/HTMLMediaElement.cpp
	M	Source/WebCore/page/Settings.cpp
	M	Source/WebCore/page/Settings.h
	M	Source/WebKit/mac/ChangeLog
	M	Source/WebKit/mac/WebView/WebPreferenceKeysPrivate.h
	M	Source/WebKit/mac/WebView/WebPreferences.mm
	M	Source/WebKit/mac/WebView/WebPreferencesPrivate.h
	M	Source/WebKit/mac/WebView/WebView.mm
	M	Source/WebKit/win/ChangeLog
	M	Source/WebKit/win/Interfaces/IWebPreferencesPrivate.idl
	M	Source/WebKit/win/WebPreferenceKeysPrivate.h
	M	Source/WebKit/win/WebPreferences.cpp
	M	Source/WebKit/win/WebPreferences.h
	M	Source/WebKit/win/WebView.cpp
	M	Source/WebKit2/ChangeLog
	M	Source/WebKit2/Shared/WebPreferencesStore.h
	M	Source/WebKit2/UIProcess/API/C/WKPreferences.cpp
	M	Source/WebKit2/UIProcess/API/C/WKPreferencesPrivate.h
	M	Source/WebKit2/WebProcess/WebPage/WebPage.cpp
Committed r91232
Comment 10 Dean Jackson 2011-07-18 19:22:36 PDT
http://trac.webkit.org/changeset/91232