RESOLVED FIXED 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
Summary Background tabs are not fully reactivated after a link is opened from an exte...
Brady Eidson
Reported 2019-04-08 14:11:02 PDT
Background tabs are not fully reactivated after a link is opened from an external application <rdar://problem/49533278>
Attachments
Patch (4.00 KB, patch)
2019-04-08 14:17 PDT, Brady Eidson
no flags
Patch (5.74 KB, patch)
2019-04-09 11:57 PDT, Brady Eidson
no flags
Patch (3.74 KB, patch)
2019-04-09 17:14 PDT, Brady Eidson
no flags
Brady Eidson
Comment 1 2019-04-08 14:17:51 PDT
Geoffrey Garen
Comment 2 2019-04-08 14:52:24 PDT
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 3 2019-04-08 15:03:37 PDT
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 4 2019-04-08 15:56:43 PDT
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
Comment 5 2019-04-08 16:01:10 PDT
Hmmm... I guess either way you need to track the last state you were in or the last update you ignored.
Brady Eidson
Comment 6 2019-04-08 17:06:04 PDT
(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
Comment 7 2019-04-09 11:57:01 PDT
Chris Dumez
Comment 8 2019-04-09 15:00:00 PDT
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
Comment 9 2019-04-09 16:01:10 PDT
(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
Comment 10 2019-04-09 16:43:57 PDT
(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
Comment 11 2019-04-09 16:48:58 PDT
(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
Comment 12 2019-04-09 16:51:21 PDT
(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
Comment 13 2019-04-09 17:14:37 PDT
Brady Eidson
Comment 14 2019-04-10 09:11:12 PDT
(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 15 2019-04-10 09:32:36 PDT
Comment on attachment 367087 [details] Patch Clearing flags on attachment: 367087 Committed r244113: <https://trac.webkit.org/changeset/244113>
WebKit Commit Bot
Comment 16 2019-04-10 09:32:38 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.