[webkit-reviews] review denied: [Bug 196705] Background tabs are not fully reactivated after a link is opened from an external application : [Attachment 366979] Patch

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has denied Brady Eidson <beidson at apple.com>'s
request for review:
Bug 196705: Background tabs are not fully reactivated after a link is opened
from an external application
https://bugs.webkit.org/show_bug.cgi?id=196705

Attachment 366979: Patch

https://bugs.webkit.org/attachment.cgi?id=366979&action=review




--- Comment #4 from Geoffrey Garen <ggaren at apple.com> ---
Comment on attachment 366979
  --> https://bugs.webkit.org/attachment.cgi?id=366979
Patch

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

Seems like we need to pick one of these ways to fix the "move to window while
in the background" logic error.

>>> Source/WebKit/UIProcess/ios/WKApplicationStateTrackingView.mm:79
>>> +	     [self _applicationWillEnterForeground];
>> 
>> What happens if we enter a window while the application is background?
(Seems wrong to unconditionally foreground the WebView in that case.)
> 
> Maybe we should also make sure that the UIApplication's applicationState is
not UIApplicationStateBackground?

I guess this needs to be...

    if (_lastTrackedApplicationState == LastTrackedApplicationState::Background
&& !_applicationStateTracker->isInBackground())
	[self _applicationWillEnterForeground];

One bummer about this solution is that ApplicationStateTracker no longer
encapsulates the logic for tracking application state. It's partially
encapsulated in this view now, even to the point where the view calls its own
notification selector on itself. 

Another option is to change the strategy here. There's no need to delete
_applicationStateTracker when we exit the window. Instead, you can keep the
_applicationStateTracker, and simply return early from
_applicationDidEnterBackground,
_applicationDidFinishSnapshottingAfterEnteringBackground, and
_applicationWillEnterForeground if the view is not in window. The nice thing
about this new strategy is that you avoid splitting application state tracking
across two classes.