Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2782713002:
Correct UIApplicationState check for sad tab test. (Closed)
|
Created:
3 years, 8 months ago by justincohen Modified:
3 years, 8 months ago CC:
chromium-reviews, ios-reviews+chrome_chromium.org, ios-reviews_chromium.org, pkl (ping after 24h if needed), noyau+watch_chromium.org, marq+watch_chromium.org, sdefresne+watch_chromium.org, pink (ping after 24hrs) Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionCorrect UIApplicationState check for sad tab test.
Checking for the backgroundState isn't enough -- the app can be in the inactive
state. Instead, just check for !active.
BUG=701583
Review-Url: https://codereview.chromium.org/2782713002
Cr-Commit-Position: refs/heads/master@{#461453}
Committed: https://chromium.googlesource.com/chromium/src/+/e888fae6c93c60d14163109930ade47a0fec644a
Patch Set 1 #
Total comments: 2
Patch Set 2 : Nit applicationIsNotActive #Patch Set 3 : Use whitelist instead of blacklist #Patch Set 4 : nit #Messages
Total messages: 25 (15 generated)
Description was changed from ========== 16# Enter a description of the change. Correct UIApplicationState check for sad tab test. Checking for the backgroundState isn't enough -- the app can be in the inactive state. Instead, just check for !active. BUG=701583 ========== to ========== Correct UIApplicationState check for sad tab test. Checking for the backgroundState isn't enough -- the app can be in the inactive state. Instead, just check for !active. BUG=701583 ==========
justincohen@chromium.org changed reviewers: + gchatz@chromium.org, rohitrao@chromium.org
PTAL!
On 2017/03/28 20:04:06, justincohen wrote: > PTAL! lgtm
pkl@chromium.org changed reviewers: + pkl@chromium.org
LGTM. I don't think the code has changed, so iOS 10 must have changed the meaning of Inactive. https://codereview.chromium.org/2782713002/diff/1/ios/chrome/browser/tabs/tab.mm File ios/chrome/browser/tabs/tab.mm (right): https://codereview.chromium.org/2782713002/diff/1/ios/chrome/browser/tabs/tab... ios/chrome/browser/tabs/tab.mm:1930: BOOL applicationIsBackgrounded = nit: Can we rename this var to isNotActive or appIsNotActive?
Do we auto-reload if we don't show a sad tab? I'm worried about using a blacklist instead of a whitelist if the default behavior is to auto-reload. What happens if we inadvertently trigger the reload behavior on a page that crashes on load? Also, what state is the app in when you slide over another app on top of chrome? If the foreground tab crashes in this state, will it auto-reload? On Mar 28, 2017 1:04 PM, <justincohen@chromium.org> wrote: > Reviewers: gchatz, rohitrao (OOO until 3-31) > CL: https://codereview.chromium.org/2782713002/ > > Message: > PTAL! > > Description: > Correct UIApplicationState check for sad tab test. > > Checking for the backgroundState isn't enough -- the app can be in the > inactive > state. Instead, just check for !active. > > BUG=701583 > > Affected files (+4, -4 lines): > M ios/chrome/browser/tabs/tab.mm > > > Index: ios/chrome/browser/tabs/tab.mm > diff --git a/ios/chrome/browser/tabs/tab.mm b/ios/chrome/browser/tabs/tab. > mm > index b5e12d3765c2add415dff86d912e2a3beaf136a6.. > ec5ea753207215d46c425e232eb606079ff31195 100644 > --- a/ios/chrome/browser/tabs/tab.mm > +++ b/ios/chrome/browser/tabs/tab.mm > @@ -1913,8 +1913,8 @@ void TabInfoBarObserver:: > OnInfoBarReplaced(infobars::InfoBar* old_infobar, > RendererTerminationTabState tab_state = > visible_ ? RendererTerminationTabState::FOREGROUND_TAB_FOREGROUND_APP > : RendererTerminationTabState::BACKGROUND_TAB_FOREGROUND_APP; > - if ([UIApplication sharedApplication].applicationState == > - UIApplicationStateBackground) { > + if ([UIApplication sharedApplication].applicationState != > + UIApplicationStateActive) { > tab_state = > visible_ ? RendererTerminationTabState::FOREGROUND_TAB_BACKGROUND_APP > : RendererTerminationTabState::BACKGROUND_TAB_BACKGROUND_APP; > @@ -1928,8 +1928,8 @@ void TabInfoBarObserver:: > OnInfoBarReplaced(infobars::InfoBar* old_infobar, > } > > BOOL applicationIsBackgrounded = > - [UIApplication sharedApplication].applicationState == > - UIApplicationStateBackground; > + [UIApplication sharedApplication].applicationState != > + UIApplicationStateActive; > if (visible_) { > if (!applicationIsBackgrounded) { > base::WeakNSObject<Tab> weakSelf(self); > > > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
rohitrao@ Re: crashes on load, it depends on what UIApplicationState is. If the crash happens quickly, while the app is still backgrounded or inactive, we will reload it. During initial load background and inactive states are temporary, so after the first reload the app should be active. If the app is in slide over the app is inactive, so this will cause a reload instead of a sad tab. UIApplicationState doesn't differentiate between 'the app is being restored but still inactive' (we want to reload, not show sad tab), and 'the app is in slide over mode, and inactive' (no idea, do we show reload or sad tab?). I don't think it matters if we use a whitelist or a blacklist here, the question is can we use UIApplicationState, or do we have to build our own flags for tracking when to show sad tab.
The CQ bit was checked by justincohen@chromium.org to run a CQ dry run
pklpkl@ I suspect this never worked correctly during the inactive state. I'd guess com.apple.WebKit.WebContent is just more crashy -- and perhaps we are tickling something to make this happen? https://codereview.chromium.org/2782713002/diff/1/ios/chrome/browser/tabs/tab.mm File ios/chrome/browser/tabs/tab.mm (right): https://codereview.chromium.org/2782713002/diff/1/ios/chrome/browser/tabs/tab... ios/chrome/browser/tabs/tab.mm:1930: BOOL applicationIsBackgrounded = On 2017/03/29 14:28:01, pklpkl wrote: > nit: Can we rename this var to isNotActive or appIsNotActive? Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
rohitrao@ ptal, updated to whitelist.
The CQ bit was checked by justincohen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
LGTM We should test to see what happens when a tab dies in slideover mode. Do we automatically reload when the app becomes active again?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by justincohen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkl@chromium.org, gchatz@chromium.org Link to the patchset: https://codereview.chromium.org/2782713002/#ps60001 (title: "nit")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1491237570698100,
"parent_rev": "e8b152d84d7771f2874718112edfb7fba4668d0c", "commit_rev":
"e888fae6c93c60d14163109930ade47a0fec644a"}
Message was sent while issue was closed.
Description was changed from ========== Correct UIApplicationState check for sad tab test. Checking for the backgroundState isn't enough -- the app can be in the inactive state. Instead, just check for !active. BUG=701583 ========== to ========== Correct UIApplicationState check for sad tab test. Checking for the backgroundState isn't enough -- the app can be in the inactive state. Instead, just check for !active. BUG=701583 Review-Url: https://codereview.chromium.org/2782713002 Cr-Commit-Position: refs/heads/master@{#461453} Committed: https://chromium.googlesource.com/chromium/src/+/e888fae6c93c60d14163109930ad... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/e888fae6c93c60d14163109930ad... |
