WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch
(5.74 KB, patch)
2019-04-09 11:57 PDT
,
Brady Eidson
no flags
Details
Formatted Diff
Diff
Patch
(3.74 KB, patch)
2019-04-09 17:14 PDT
,
Brady Eidson
no flags
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Brady Eidson
Comment 1
2019-04-08 14:17:51 PDT
Created
attachment 366979
[details]
Patch
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
Created
attachment 367058
[details]
Patch
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
Created
attachment 367087
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug