[webkit-reviews] review granted: [Bug 66978] <video> playlist can not advance when playing in background tab : [Attachment 105256] Proposed patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Eric Carlson
<eric.carlson at apple.com>'s request for review:
Bug 66978: <video> playlist can not advance when playing in background tab
https://bugs.webkit.org/show_bug.cgi?id=66978

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

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=105256&action=review


> Source/WebCore/ChangeLog:4
> +	   <video> playlist can not advance when playing in background tab
> +	   https://bugs.webkit.org/show_bug.cgi?id=66978

Change log should give the reason there is no accompanying regression test.

> Source/WebCore/html/HTMLMediaElement.cpp:496
> -    if (m_restrictions & RequireUserGestureForLoadRestriction &&
!ScriptController::processingUserGesture())
> +    if (requireUserGestureForLoad() &&
!ScriptController::processingUserGesture())

Seems this improvement, not needed to fix the bug, could be done in a separate
patch.

> Source/WebCore/html/HTMLMediaElement.cpp:595
> +    // Once the page has allowed an element load media, it is free to load
at will. This allows a 

Should read: "an element *to* load media".

> Source/WebCore/html/HTMLMediaElement.cpp:1501
> -    if (m_restrictions & RequireUserGestureForRateChangeRestriction &&
!ScriptController::processingUserGesture())
> +    if (requireUserGestureForRateChange() &&
!ScriptController::processingUserGesture())

Seems this improvement, not needed to fix the bug, could be done in a separate
patch.

> Source/WebCore/html/HTMLMediaElement.cpp:1549
> -    if (m_restrictions & RequireUserGestureForRateChangeRestriction &&
!ScriptController::processingUserGesture())
> +    if (requireUserGestureForRateChange() &&
!ScriptController::processingUserGesture())

Seems this improvement, not needed to fix the bug, could be done in a separate
patch.

> Source/WebCore/html/HTMLMediaElement.h:195
>      bool requireUserGestureForLoad() const { return m_restrictions &
RequireUserGestureForLoadRestriction; }
>      bool requireUserGestureForRateChange() const { return m_restrictions &
RequireUserGestureForRateChangeRestriction; }
>      bool requireUserGestureForFullScreen() const { return m_restrictions &
RequireUserGestureForFullScreenRestriction; }
> +    bool requirePageConsentToLoadMedia() const { return m_restrictions &
RequirePageConsentToLoadMedia; }

For future consideration: These function names have verbs in them and can be
interpreted as calls to change behavior restrictions rather than to give the
state of those restrictions. It would be best to rename them to start with the
word “should” so it’s clear they are boolean getters.

Since these are used only in the class it seems they could start as private. We
could make them public later if needed.

> Source/WebCore/html/HTMLMediaElement.h:197
> +    void removeBehaviorRestriction(BehaviorRestrictions restriction) {
m_restrictions &= ~restriction; }

Since this is used only in the class, we could start it as private. We could
make it public later if needed.