Background tabs are not fully reactivated after a link is opened from an external application

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Brady Eidson]

Background tabs are not fully reactivated after a link is opened from an external application

<rdar://problem/49533278>

[Brady Eidson]

Created attachment 366979 [details]
Patch

[Geoffrey Garen]

Comment on attachment 366979 [details]
Patch

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

> Source/WebKit/UIProcess/ios/WKApplicationStateTrackingView.mm:79
> +    if (_lastTrackedApplicationState == LastTrackedApplicationState::Background)
> +        [self _applicationWillEnterForeground];

What happens if we enter a window while the application is background? (Seems wrong to unconditionally foreground the WebView in that case.)

[Chris Dumez]

Comment on attachment 366979 [details]
Patch

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

>> 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?

[Geoffrey Garen]

Comment on attachment 366979 [details]
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.

[Geoffrey Garen]

Hmmm... I guess either way you need to track the last state you were in or the last update you ignored.

[Brady Eidson]

(In reply to Geoffrey Garen from comment #4)
> Comment on attachment 366979 [details]
> 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.

The changes suggested here break things in seemingly subtle ways. E.g. switching between Mail and Safari, now Mail is broken.

I'll have to re-add logging and step through carefully to continue pursuing these changes.

[Brady Eidson]

Created attachment 367058 [details]
Patch

[Chris Dumez]

Comment on attachment 367058 [details]
Patch

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

> Source/WebKit/UIProcess/ios/WKApplicationStateTrackingView.mm:49
> +    _applicationStateTracker = std::make_unique<WebKit::ApplicationStateTracker>(self, @selector(_applicationDidEnterBackground), @selector(_applicationDidFinishSnapshottingAfterEnteringBackground), @selector(_applicationWillEnterForeground));

I do not understand why we're making this particular change. It used to be that only parent views would have an ApplicationStateTracker. Now all views get one and get notified whenever the app goes to background / foreground. Aren't we doing a lot of unnecessary work?
Also, even though those unparented views have an applicationState tracker, they seem to return early whenever they get notifications, this seems unfortunate.

[Brady Eidson]

(In reply to Chris Dumez from comment #8)
> Comment on attachment 367058 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=367058&action=review
> 
> > Source/WebKit/UIProcess/ios/WKApplicationStateTrackingView.mm:49
> > +    _applicationStateTracker = std::make_unique<WebKit::ApplicationStateTracker>(self, @selector(_applicationDidEnterBackground), @selector(_applicationDidFinishSnapshottingAfterEnteringBackground), @selector(_applicationWillEnterForeground));
> 
> I do not understand why we're making this particular change. It used to be
> that only parent views would have an ApplicationStateTracker. Now all views
> get one and get notified whenever the app goes to background / foreground.
> Aren't we doing a lot of unnecessary work?

They are getting notified, but they aren't doing any work.

> Also, even though those unparented views have an applicationState tracker,
> they seem to return early whenever they get notifications, this seems
> unfortunate.

That's literally the whole point of Geoff's suggestion (and that's how - like I mention above - they aren't doing any work)

[Chris Dumez]

(In reply to Brady Eidson from comment #9)
> (In reply to Chris Dumez from comment #8)
> > Comment on attachment 367058 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=367058&action=review
> > 
> > > Source/WebKit/UIProcess/ios/WKApplicationStateTrackingView.mm:49
> > > +    _applicationStateTracker = std::make_unique<WebKit::ApplicationStateTracker>(self, @selector(_applicationDidEnterBackground), @selector(_applicationDidFinishSnapshottingAfterEnteringBackground), @selector(_applicationWillEnterForeground));
> > 
> > I do not understand why we're making this particular change. It used to be
> > that only parent views would have an ApplicationStateTracker. Now all views
> > get one and get notified whenever the app goes to background / foreground.
> > Aren't we doing a lot of unnecessary work?
> 
> They are getting notified, but they aren't doing any work.
> 
> > Also, even though those unparented views have an applicationState tracker,
> > they seem to return early whenever they get notifications, this seems
> > unfortunate.
> 
> That's literally the whole point of Geoff's suggestion (and that's how -
> like I mention above - they aren't doing any work)

Just because they are returning early does not mean we're not doing any work. There is certainly a cost memory wise and computation wise to having many clients registered for notifications that they actually do not care about.

[Brady Eidson]

(In reply to Chris Dumez from comment #10)
> (In reply to Brady Eidson from comment #9)
> > (In reply to Chris Dumez from comment #8)
> > > Comment on attachment 367058 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=367058&action=review
> > > 
> > > > Source/WebKit/UIProcess/ios/WKApplicationStateTrackingView.mm:49
> > > > +    _applicationStateTracker = std::make_unique<WebKit::ApplicationStateTracker>(self, @selector(_applicationDidEnterBackground), @selector(_applicationDidFinishSnapshottingAfterEnteringBackground), @selector(_applicationWillEnterForeground));
> > > 
> > > I do not understand why we're making this particular change. It used to be
> > > that only parent views would have an ApplicationStateTracker. Now all views
> > > get one and get notified whenever the app goes to background / foreground.
> > > Aren't we doing a lot of unnecessary work?
> > 
> > They are getting notified, but they aren't doing any work.
> > 
> > > Also, even though those unparented views have an applicationState tracker,
> > > they seem to return early whenever they get notifications, this seems
> > > unfortunate.
> > 
> > That's literally the whole point of Geoff's suggestion (and that's how -
> > like I mention above - they aren't doing any work)
> 
> Just because they are returning early does not mean we're not doing any
> work. There is certainly a cost memory wise and computation wise to having
> many clients registered for notifications that they actually do not care
> about.

So it sounds to me like you are suggesting that the original patch is a better approach and that Geoff's suggestion that we always keep the notifier around is a bad one?

[Chris Dumez]

(In reply to Brady Eidson from comment #11)
> (In reply to Chris Dumez from comment #10)
> > (In reply to Brady Eidson from comment #9)
> > > (In reply to Chris Dumez from comment #8)
> > > > Comment on attachment 367058 [details]
> > > > Patch
> > > > 
> > > > View in context:
> > > > https://bugs.webkit.org/attachment.cgi?id=367058&action=review
> > > > 
> > > > > Source/WebKit/UIProcess/ios/WKApplicationStateTrackingView.mm:49
> > > > > +    _applicationStateTracker = std::make_unique<WebKit::ApplicationStateTracker>(self, @selector(_applicationDidEnterBackground), @selector(_applicationDidFinishSnapshottingAfterEnteringBackground), @selector(_applicationWillEnterForeground));
> > > > 
> > > > I do not understand why we're making this particular change. It used to be
> > > > that only parent views would have an ApplicationStateTracker. Now all views
> > > > get one and get notified whenever the app goes to background / foreground.
> > > > Aren't we doing a lot of unnecessary work?
> > > 
> > > They are getting notified, but they aren't doing any work.
> > > 
> > > > Also, even though those unparented views have an applicationState tracker,
> > > > they seem to return early whenever they get notifications, this seems
> > > > unfortunate.
> > > 
> > > That's literally the whole point of Geoff's suggestion (and that's how -
> > > like I mention above - they aren't doing any work)
> > 
> > Just because they are returning early does not mean we're not doing any
> > work. There is certainly a cost memory wise and computation wise to having
> > many clients registered for notifications that they actually do not care
> > about.
> 
> So it sounds to me like you are suggesting that the original patch is a
> better approach and that Geoff's suggestion that we always keep the notifier
> around is a bad one?

I definitely liked the original patch better, yes. If Geoff suggested this then I'll let him review.

[Brady Eidson]

Created attachment 367087 [details]
Patch

[Brady Eidson]

(I believe the Geoff r- on the first patch was do to the not-paying-attn-to-background state, which I've resolved here and Chris r+'ed)

[WebKit Commit Bot]

Comment on attachment 367087 [details]
Patch

Clearing flags on attachment: 367087

Committed r244113: <https://trac.webkit.org/changeset/244113>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.