Bug 123041 – Limit use of display sleep assertion when <video> element is off-screen.

• Component: Media
• Status: RESOLVED FIXED

[Jer Noble]

Limit use of display sleep assertion when <video> element is off-screen.

[Jer Noble]

Created attachment 214607 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 214607 [details]
Patch

Why can't we just use Page Visibility to determine whether a <video> can prevent display sleep?

[EFL EWS Bot]

Comment on attachment 214607 [details]
Patch

Attachment 214607 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/6428014

[Jer Noble]

PageVisibility doesn't have a notification system outside dispatchEvent(), but it could be added to Page::setVisibilityState() pretty easily.  I'll modify my patch.

[Build Bot]

Comment on attachment 214607 [details]
Patch

Attachment 214607 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/6478015

[Jer Noble]

Created attachment 214618 [details]
Patch

[WebKit Commit Bot]

Attachment 214618 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/dom/Document.cpp', u'Source/WebCore/dom/Document.h', u'Source/WebCore/dom/Element.h', u'Source/WebCore/html/HTMLMediaElement.cpp', u'Source/WebCore/html/HTMLMediaElement.h', u'Source/WebCore/page/Page.cpp']" exit_code: 1
Source/WebCore/html/HTMLMediaElement.cpp:5060:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Total errors found: 1 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jer Noble]

Created attachment 214624 [details]
Patch

Protected updateDisableSleep() with a #ifdef guard

[WebKit Commit Bot]

Attachment 214624 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/dom/Document.cpp', u'Source/WebCore/dom/Document.h', u'Source/WebCore/dom/Element.h', u'Source/WebCore/html/HTMLMediaElement.cpp', u'Source/WebCore/html/HTMLMediaElement.h', u'Source/WebCore/page/Page.cpp']" exit_code: 1
Source/WebCore/html/HTMLMediaElement.cpp:5062:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Total errors found: 1 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 214624 [details]
Patch

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

> Source/WebCore/dom/Document.cpp:1530
> +void Document::registerForPageVisibilityStateCallbacks(Element* e)

Could you use the word "element" instead of the letter "e" for this?

> Source/WebCore/dom/Document.cpp:1535
> +void Document::unregisterForPageVisibilityStateCallbacks(Element* e)

Ditto.

> Source/WebCore/dom/Document.cpp:1543
> +    HashSet<Element*>::iterator end = m_pageVisibilityStateCallbackElements.end();

Should use auto for this too, like in the for loop, and also if you are willing put the end and begin both inside the for loop.

> Source/WebCore/dom/Document.h:420
> +    void pageVisibilityStateChanged();

Not sure the word “State” is helpful in this function name.

> Source/WebCore/dom/Document.h:973
> +    void registerForPageVisibilityStateCallbacks(Element*);
> +    void unregisterForPageVisibilityStateCallbacks(Element*);

Not sure the word “State” is helpful in these function names.

> Source/WebCore/dom/Document.h:1449
> +    HashSet<Element*> m_pageVisibilityStateCallbackElements;

Not sure the word “State” is helpful in this data member name.

> Source/WebCore/dom/Element.h:423
> +    virtual void pageVisibilityStateChanged() { }

Not sure the word “State” is helpful in this function name.

> Source/WebCore/html/HTMLMediaElement.cpp:4285
> +    m_displaySleepDisablingSuspended = document().hidden();

It’s inconsistent that the Document::hidden function establishes the concept of a “hidden document”, yet the callback for changes to whether it’s hidden calls it “page visibility state”. It seems like we could just call this “visibility” from the point of view of the document rather than calling it “page visibility state”.

> Source/WebCore/html/HTMLMediaElement.cpp:4288
> +#if PLATFORM(MAC)
> +    updateDisableSleep();
> +#endif

This pre-existing function has an unpleasantly ungrammatical name. “Update disable sleep” should instead be “update sleep disabling” or something else that makes sense with English grammar. Also, it would be nicer to have this factored so it did not require an #if at the call site. Maybe an inline empty version of the function for non-Mac platforms?

> Source/WebCore/html/HTMLMediaElement.cpp:5063
> +#if ENABLE(PAGE_VISIBILITY_API)
> +        !m_displaySleepDisablingSuspended &&
> +#endif

A nice way to avoid this awkward construction is to just add an early return.

#if ENABLE(PAGE_VISIBILITY_API)
    if (m_displaySleepDisablingSuspended)
        return false;
#endif

Then you could leave the boolean expression that follows it entirely alone.

> Source/WebCore/html/HTMLMediaElement.h:738
> +    bool m_displaySleepDisablingSuspended : 1;

This data member name sounds like a verb “display”. One way to fix that is to add an “is” prefix. There are probably other ways.

[Darin Adler]

Comment on attachment 214624 [details]
Patch

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

> Source/WebCore/dom/Document.cpp:1544
> +    for (auto i = m_pageVisibilityStateCallbackElements.begin(); i != end; ++i)

We normally use “it” rather than “i” when it’s an iterator instead of an index.

> Source/WebCore/html/HTMLMediaElement.cpp:349
>      document.registerForPrivateBrowsingStateChangedCallbacks(this);
> +#if ENABLE(PAGE_VISIBILITY_API)
> +    document.registerForPageVisibilityStateCallbacks(this);
> +#endif

I’d prefer to see a blank line before this.

> Source/WebCore/html/HTMLMediaElement.cpp:378
>      document().unregisterForPrivateBrowsingStateChangedCallbacks(this);
> +#if ENABLE(PAGE_VISIBILITY_API)
> +    document().unregisterForPageVisibilityStateCallbacks(this);
> +#endif

I’d prefer to see a blank line before this.

[Jer Noble]

(In reply to comment #10)
> (From update of attachment 214624 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=214624&action=review
> 
> > Source/WebCore/dom/Document.cpp:1530
> > +void Document::registerForPageVisibilityStateCallbacks(Element* e)
> 
> Could you use the word "element" instead of the letter "e" for this?

Absolutely.

> > Source/WebCore/dom/Document.cpp:1535
> > +void Document::unregisterForPageVisibilityStateCallbacks(Element* e)
> 
> Ditto.

Ditto.

> > Source/WebCore/dom/Document.cpp:1543
> > +    HashSet<Element*>::iterator end = m_pageVisibilityStateCallbackElements.end();
> 
> Should use auto for this too, like in the for loop, and also if you are willing put the end and begin both inside the for loop.

Will do.

> > Source/WebCore/dom/Document.h:420
> > +    void pageVisibilityStateChanged();
> 
> Not sure the word “State” is helpful in this function name.

Agreed.  This will become pageVisibilityChanged().

> > Source/WebCore/dom/Document.h:973
> > +    void registerForPageVisibilityStateCallbacks(Element*);
> > +    void unregisterForPageVisibilityStateCallbacks(Element*);
> 
> Not sure the word “State” is helpful in these function names.
> 
> > Source/WebCore/dom/Document.h:1449
> > +    HashSet<Element*> m_pageVisibilityStateCallbackElements;
> 
> Not sure the word “State” is helpful in this data member name.
> 
> > Source/WebCore/dom/Element.h:423
> > +    virtual void pageVisibilityStateChanged() { }
> 
> Not sure the word “State” is helpful in this function name.

Ditto to all.

> > Source/WebCore/html/HTMLMediaElement.cpp:4285
> > +    m_displaySleepDisablingSuspended = document().hidden();
> 
> It’s inconsistent that the Document::hidden function establishes the concept of a “hidden document”, yet the callback for changes to whether it’s hidden calls it “page visibility state”. It seems like we could just call this “visibility” from the point of view of the document rather than calling it “page visibility state”.

The justification is that hidden() is an easy, public alias for pageVisibilityState() != PageVisibilityStateVisible, where the pageVisibilityState() method is private.

I'm not sure if you're suggesting we change the "isHidden()" method to "isVisible()", or whether we should be calling pageVisibilityState() (and making that method public), or something else.

> > Source/WebCore/html/HTMLMediaElement.cpp:4288
> > +#if PLATFORM(MAC)
> > +    updateDisableSleep();
> > +#endif
> 
> This pre-existing function has an unpleasantly ungrammatical name. “Update disable sleep” should instead be “update sleep disabling” or something else that makes sense with English grammar. Also, it would be nicer to have this factored so it did not require an #if at the call site. Maybe an inline empty version of the function for non-Mac platforms?

Sure thing; that would clean up a lot of call sites in this class.

> > Source/WebCore/html/HTMLMediaElement.cpp:5063
> > +#if ENABLE(PAGE_VISIBILITY_API)
> > +        !m_displaySleepDisablingSuspended &&
> > +#endif
> 
> A nice way to avoid this awkward construction is to just add an early return.
> 
> #if ENABLE(PAGE_VISIBILITY_API)
>     if (m_displaySleepDisablingSuspended)
>         return false;
> #endif
> 
> Then you could leave the boolean expression that follows it entirely alone.

Indeed, great suggestion.

> > Source/WebCore/html/HTMLMediaElement.h:738
> > +    bool m_displaySleepDisablingSuspended : 1;
> 
> This data member name sounds like a verb “display”. One way to fix that is to add an “is” prefix. There are probably other ways.

But that one will do, thanks!

[Jer Noble]

(In reply to comment #11)
> (From update of attachment 214624 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=214624&action=review
> 
> > Source/WebCore/dom/Document.cpp:1544
> > +    for (auto i = m_pageVisibilityStateCallbackElements.begin(); i != end; ++i)
> 
> We normally use “it” rather than “i” when it’s an iterator instead of an index.

Changed.

> > Source/WebCore/html/HTMLMediaElement.cpp:349
> >      document.registerForPrivateBrowsingStateChangedCallbacks(this);
> > +#if ENABLE(PAGE_VISIBILITY_API)
> > +    document.registerForPageVisibilityStateCallbacks(this);
> > +#endif
> 
> I’d prefer to see a blank line before this.

Added.

> > Source/WebCore/html/HTMLMediaElement.cpp:378
> >      document().unregisterForPrivateBrowsingStateChangedCallbacks(this);
> > +#if ENABLE(PAGE_VISIBILITY_API)
> > +    document().unregisterForPageVisibilityStateCallbacks(this);
> > +#endif
> 
> I’d prefer to see a blank line before this.

Ditto.

[Darin Adler]

Comment on attachment 214624 [details]
Patch

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

>>> Source/WebCore/html/HTMLMediaElement.cpp:4285
>>> +    m_displaySleepDisablingSuspended = document().hidden();
>> 
>> It’s inconsistent that the Document::hidden function establishes the concept of a “hidden document”, yet the callback for changes to whether it’s hidden calls it “page visibility state”. It seems like we could just call this “visibility” from the point of view of the document rather than calling it “page visibility state”.
> 
> The justification is that hidden() is an easy, public alias for pageVisibilityState() != PageVisibilityStateVisible, where the pageVisibilityState() method is private.
> 
> I'm not sure if you're suggesting we change the "isHidden()" method to "isVisible()", or whether we should be calling pageVisibilityState() (and making that method public), or something else.

If we are comfortable with just calling this “hidden” or “isHidden”, then I suggest taking the word “page” out of all the other function names too. They can just be called things like “visibility changed”. Which I think would be great. If there is more than one kind of visibility a document might be talking about, then I suggest that the hidden function be renamed so it has the disambiguating word “page” in it.

[Jer Noble]

(In reply to comment #14)
> (From update of attachment 214624 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=214624&action=review
> 
> >>> Source/WebCore/html/HTMLMediaElement.cpp:4285
> >>> +    m_displaySleepDisablingSuspended = document().hidden();
> >> 
> >> It’s inconsistent that the Document::hidden function establishes the concept of a “hidden document”, yet the callback for changes to whether it’s hidden calls it “page visibility state”. It seems like we could just call this “visibility” from the point of view of the document rather than calling it “page visibility state”.
> > 
> > The justification is that hidden() is an easy, public alias for pageVisibilityState() != PageVisibilityStateVisible, where the pageVisibilityState() method is private.
> > 
> > I'm not sure if you're suggesting we change the "isHidden()" method to "isVisible()", or whether we should be calling pageVisibilityState() (and making that method public), or something else.
> 
> If we are comfortable with just calling this “hidden” or “isHidden”, then I suggest taking the word “page” out of all the other function names too. They can just be called things like “visibility changed”. Which I think would be great. If there is more than one kind of visibility a document might be talking about, then I suggest that the hidden function be renamed so it has the disambiguating word “page” in it.

It looks like "Document::hidden()" is a requirement of the Page Visibility API: <https://dvcs.w3.org/hg/webperf/raw-file/tip/specs/PageVisibility/Overview.html#sec-document-interface>, which also requires visibilityState().  So I'll update these new APIs and callbacks to drop the "page".

[Jer Noble]

Committed r157735: <http://trac.webkit.org/changeset/157735>