Add window occlusion criteria to determine page visibility on Mac

• Component: WebKit2
• Status: RESOLVED FIXED

[Kiran Muppala]

Use window occlusion notifications on Mac to determine when the window corresponding to a WKView is occluded and notify the Page that it's visibility has changed.  This allows even the foreground tabs in a browser window to be treated like a background tab when the window is occluded and hence any power saving optimizations such as throttling timers or suspending animations can be applied to it as well.

[Kiran Muppala]

Created attachment 183876 [details]
Patch

[Build Bot]

Comment on attachment 183876 [details]
Patch

Attachment 183876 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16031412

[Build Bot]

Comment on attachment 183876 [details]
Patch

Attachment 183876 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16045591

[Simon Fraser (smfr)]

Comment on attachment 183876 [details]
Patch

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

r- pending some discussion of the implementation

> Source/WebKit2/UIProcess/API/mac/WKView.mm:1881
> +    if (context == &windowNumberPropertyObserverContext) {
> +        id isPriorChange = [change objectForKey:NSKeyValueChangeNotificationIsPriorKey];
> +        if (isPriorChange && [isPriorChange isKindOfClass:[NSNumber class]] && ([isPriorChange boolValue] == YES))
> +            [self _removeFromWindowViewsMap];
> +        else
> +            [self _addToWindowViewsMap];
> +    }

I don't understand why you have to observe the NSWindows' windowNumber property. Does this ever change after window creation? Can't you just update the window view's map in viewWillMoveToWindow/viewDidMoveToWindow?

> Source/WebKit2/UIProcess/API/mac/WKView.mm:2341
> +    WindowViewsMap *windowViewMap = [WKView _windowViewsMap];
> +    WindowViewsMap::iterator it = windowViewMap->find(windowID);
> +    if (it != windowViewMap->end()) {
> +        HashSet<WKView *>& views = it->value;
> +        for(HashSet<WKView *>::iterator view_it = views.begin(), view_end = views.end(); view_it != view_end; ++view_it) {
> +            WKView *view = *view_it;
> +            view->_data->_page->viewStateDidChange(WebPageProxy::ViewIsVisible);
> +        }
> +    }

So another way to do this would be to keep track of all WKViews, and just go through them all asking which window they are in.

[Kiran Muppala]

Created attachment 184623 [details]
Patch

[Kiran Muppala]

(In reply to comment #4)
> (From update of attachment 183876 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=183876&action=review
> 
> r- pending some discussion of the implementation
> 
> > Source/WebKit2/UIProcess/API/mac/WKView.mm:1881
> > +    if (context == &windowNumberPropertyObserverContext) {
> > +        id isPriorChange = [change objectForKey:NSKeyValueChangeNotificationIsPriorKey];
> > +        if (isPriorChange && [isPriorChange isKindOfClass:[NSNumber class]] && ([isPriorChange boolValue] == YES))
> > +            [self _removeFromWindowViewsMap];
> > +        else
> > +            [self _addToWindowViewsMap];
> > +    }
> 
> I don't understand why you have to observe the NSWindows' windowNumber property. Does this ever change after window creation? Can't you just update the window view's map in viewWillMoveToWindow/viewDidMoveToWindow?
> 

I initially did this to handle the case of a NSWindow object not having a windowNumber assigned yet, such as during application launch.  But, after looking at the API documentation this property is not declared as being KVO observable.  So, I removed this observer and replaced it with willOrderOffScreen and didOrderOnScreen notifications.

> > Source/WebKit2/UIProcess/API/mac/WKView.mm:2341
> > +    WindowViewsMap *windowViewMap = [WKView _windowViewsMap];
> > +    WindowViewsMap::iterator it = windowViewMap->find(windowID);
> > +    if (it != windowViewMap->end()) {
> > +        HashSet<WKView *>& views = it->value;
> > +        for(HashSet<WKView *>::iterator view_it = views.begin(), view_end = views.end(); view_it != view_end; ++view_it) {
> > +            WKView *view = *view_it;
> > +            view->_data->_page->viewStateDidChange(WebPageProxy::ViewIsVisible);
> > +        }
> > +    }
> 
> So another way to do this would be to keep track of all WKViews, and just go through them all asking which window they are in.

I removed the map and notifying views was easy enough but I ran into a problem with removing window ids from occluded windows set.  With the map, I can tell if there are no more views in a window and remove it from the occluded window set.  Without it, periodic pruning of the occluded window set becomes necessary.  Given that I find using the map easier.

Also fixed the build failure in latest patch due to missing #ifdef.

[Kiran Muppala]

Created attachment 184649 [details]
Patch

[Kiran Muppala]

(In reply to comment #7)
> Created an attachment (id=184649) [details]
> Patch

Re-uploading previous patch since EWS didn't seem to run with the old one.

[Kiran Muppala]

Created attachment 184668 [details]
Patch

[Kiran Muppala]

> > > Source/WebKit2/UIProcess/API/mac/WKView.mm:2341
> > > +    WindowViewsMap *windowViewMap = [WKView _windowViewsMap];
> > > +    WindowViewsMap::iterator it = windowViewMap->find(windowID);
> > > +    if (it != windowViewMap->end()) {
> > > +        HashSet<WKView *>& views = it->value;
> > > +        for(HashSet<WKView *>::iterator view_it = views.begin(), view_end = views.end(); view_it != view_end; ++view_it) {
> > > +            WKView *view = *view_it;
> > > +            view->_data->_page->viewStateDidChange(WebPageProxy::ViewIsVisible);
> > > +        }
> > > +    }
> > 
> > So another way to do this would be to keep track of all WKViews, and just go through them all asking which window they are in.
> 
> I removed the map and notifying views was easy enough but I ran into a problem with removing window ids from occluded windows set.  With the map, I can tell if there are no more views in a window and remove it from the occluded window set.  Without it, periodic pruning of the occluded window set becomes necessary.  Given that I find using the map easier.
> 

After some thought I realized I can eliminate the occluded window set if I add a boolean field to WKViewData for remember if it's window is occluded.  That would allow me to use the simpler loop through all WKViews when handling occlusion notifications as you suggested.

I used this approach in the latest patch and the code looks much simpler to me compared to using the map.  But, I am not sure if the extra boolean is acceptable.  I would like to choose a patch based on your feedback.

[Simon Fraser (smfr)]

> After some thought I realized I can eliminate the occluded window set if I add a boolean field to WKViewData for remember if it's window is occluded.  That would allow me to use the simpler loop through all WKViews when handling occlusion notifications as you suggested.
> 
> I used this approach in the latest patch and the code looks much simpler to me compared to using the map.  But, I am not sure if the extra boolean is acceptable.  I would like to choose a patch based on your feedback.

I think the boolean is fine; one per view is trivial overhead.

[Kiran Muppala]

Created attachment 184794 [details]
Patch

[Kiran Muppala]

(In reply to comment #12)
> Created an attachment (id=184794) [details]
> Patch

Only one change from previous patch, instead of directly setting _isWindowOccluded to "NO" from _disableWindowOcclusionNotifications, added a call to _setIsWindowOccluded: so that the associated page is made aware of a visibility change if necessary.

[Simon Fraser (smfr)]

Comment on attachment 184794 [details]
Patch

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

> Source/WebKit2/UIProcess/API/mac/WKView.mm:276
> +    [[WKView _allViews] removeObject:self];

This will never get hit. -allViews retains, so you'll never hit this dealloc.

[Kiran Muppala]

Created attachment 184823 [details]
Patch

[Kiran Muppala]

(In reply to comment #14)
> (From update of attachment 184794 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=184794&action=review
> 
> > Source/WebKit2/UIProcess/API/mac/WKView.mm:276
> > +    [[WKView _allViews] removeObject:self];
> 
> This will never get hit. -allViews retains, so you'll never hit this dealloc.

Yeah, a bad bug I overlooked.  Fixed by replacing the NSMutableArray with a Vector and verified that dealloc was being called after the fix.

[Kiran Muppala]

Created attachment 185057 [details]
Patch

[Kiran Muppala]

(In reply to comment #17)
> Created an attachment (id=185057) [details]
> Patch

Modified _enableWindowOcclusionNotifications to only enable and use the current window occlusion state if the notification handlers were successfully registered.  The registration is unlikely to fail, but if it does, it is better to not mark any window as occluded.

[Simon Fraser (smfr)]

Comment on attachment 185057 [details]
Patch

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

> Source/WebKit2/UIProcess/API/mac/WKView.mm:2277
> +    // Occlusiont notifications for a given window might also be used else where in the

Occlusiont

[Kiran Muppala]

Created attachment 185074 [details]
Patch

[Kiran Muppala]

(In reply to comment #19)
> (From update of attachment 185057 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=185057&action=review
> 
> > Source/WebKit2/UIProcess/API/mac/WKView.mm:2277
> > +    // Occlusiont notifications for a given window might also be used else where in the
> 
> Occlusiont

Fixed typo, no other changes.

[Build Bot]

Comment on attachment 185074 [details]
Patch

Attachment 185074 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16155847

New failing tests:
fast/workers/worker-lifecycle.html

[Kiran Muppala]

(In reply to comment #22)
> (From update of attachment 185074 [details])
> Attachment 185074 [details] did not pass mac-wk2-ews (mac-wk2):
> Output: http://queues.webkit.org/results/16155847
> 
> New failing tests:
> fast/workers/worker-lifecycle.html

This test seems to be flaky and is failing even without my patch.
webkit-ews-11 shows "Unable to pass tests without patch (tree is red?)"
http://queues.webkit.org/queue-status/mac-wk2-ews/bots/webkit-ews-11
http://queues.webkit.org/results/16155803

[WebKit Review Bot]

Comment on attachment 185074 [details]
Patch

Clearing flags on attachment: 185074

Committed r141045: <http://trac.webkit.org/changeset/141045>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Kiran Muppala]

<rdar://problem/12960662>