[webkit-reviews] review granted: [Bug 64742] Expose WebPreferences for media playback requiring user gestures and inline playback : [Attachment 101190] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Dean Jackson
<dino at apple.com>'s request for review:
Bug 64742: Expose WebPreferences for media playback requiring user gestures and
inline playback
https://bugs.webkit.org/show_bug.cgi?id=64742

Attachment 101190: Patch
https://bugs.webkit.org/attachment.cgi?id=101190&action=review

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
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.