Limit use of display sleep assertion when <video> element is off-screen.
Created attachment 214607 [details] Patch
Comment on attachment 214607 [details] Patch Why can't we just use Page Visibility to determine whether a <video> can prevent display sleep?
Comment on attachment 214607 [details] Patch Attachment 214607 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/6428014
PageVisibility doesn't have a notification system outside dispatchEvent(), but it could be added to Page::setVisibilityState() pretty easily. I'll modify my patch.
Comment on attachment 214607 [details] Patch Attachment 214607 [details] did not pass win-ews (win): Output: http://webkit-queues.appspot.com/results/6478015
Created attachment 214618 [details] Patch
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.
Created attachment 214624 [details] Patch Protected updateDisableSleep() with a #ifdef guard
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.
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.
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.
(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!
(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.
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.
(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".
Committed r157735: <http://trac.webkit.org/changeset/157735>