[webkit-reviews] review canceled: [Bug 81154] [WK2] Add Page Visibility API support : [Attachment 134868] Patch

[bugzilla-daemon at webkit.org]

Jesus Sanchez-Palencia <jesus at webkit.org> has canceled Jesus Sanchez-Palencia
<jesus at webkit.org>'s request for review:
Bug 81154: [WK2] Add Page Visibility API support
https://bugs.webkit.org/show_bug.cgi?id=81154

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

------- Additional Comments from Jesus Sanchez-Palencia <jesus at webkit.org>
I will comment on all comments here so we can discuss the raised points and so
I'm clearing the review flag for now.


(In reply to comment #8)
> Should the initial state be set after we crash? Couldn't this be done more
"automatically". Why does it matter anyway?

I guess Page is already setting the initial state anyway, so I will have a
better look at this. IMHO, it's not needed.


(In reply to comment #9)
> When setting the page visibility, please also use take advantage of the new
visibility info to set the FrameView visibility (inherited from Widget).

Perhaps we should first land this more "raw" implementation for Page Visibility
in WK2, which is pretty close to how it's is implemented in WK1. After that we
can have a follow-up patch that can play around with the FrameView visibility,
Page::willMoveOffscreen, ScrollView::hide(), etc, as they all need to be
investigated and are actually improvements that will take advantage of the
visibility state of WebPage after we have settled down what is needed for the
Page Visibility API implementation itself. We can leave suspend/resume of
activeDOMObjects out for now as well.


(In reply to comment #10)
> I don't think suspendActiveDOMObjectsAndAnimations makes sense in this
context. The use case for page visibility API is specifically that web-pages
can adjust their timers to make fewer updates.
> 
> Instead use Page::willMoveOffscreen which only disables animations but not
active DOM objects.
> I also think it makes sense to calls ScrollView::hide() which tells the
rendering logic that the page is no longer being rendered.

Ditto.


What do you guys think?