Skip to content

GitLab

T
tangled
Commit a8332cc7 authored by Mike Dougherty's avatar Mike Dougherty Committed by Chromium LUCI CQ
Browse files
Options

Revert "[iOS] Fix Session.TotalDuration metric with multiwindow." 

This reverts commit 8904382c.

Reason for revert: This CL breaks AppStateTest.resumeSessionWithStartupParameters for iOS 12 devices. See https://ci.chromium.org/ui/p/chrome/builders/ci/iphone-device/15349/blamelist.

Original change's description:
> [iOS] Fix Session.TotalDuration metric with multiwindow.
>
> Track the session as open while there is at least one foreground window.
> Log this in a dedicated app agent to avoid polluting AppState.
> The same is done to IOSProfileSessionDurationsService's session
> start/end events.
>
> Bug: 1156586
> Change-Id: I37586f989039206fbaa90f7ec4046fbf9582e70a
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2599870
> Commit-Queue: Stepan Khapugin <stkhapugin@chromium.org>
> Auto-Submit: Stepan Khapugin <stkhapugin@chromium.org>
> Reviewed-by: Mark Pearson <mpearson@chromium.org>
> Reviewed-by: Mihai Sardarescu <msarda@chromium.org>
> Reviewed-by: Mark Cogan <marq@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#845653}

TBR=mpearson@chromium.org,marq@chromium.org,msarda@chromium.org,stkhapugin@chromium.org,chromium-scoped@luci-project-accounts.iam.gserviceaccount.com

Change-Id: Icd927a3252658933c02c8101067657f3ac7365b2
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 1156586
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2643264Reviewed-by: default avatarMike Dougherty <michaeldo@chromium.org>
Commit-Queue: Mike Dougherty <michaeldo@chromium.org>
Cr-Commit-Position: refs/heads/master@{#845730}
parent 26897571
Show whitespace changes
Inline Side-by-side
Showing with 45 additions and 176 deletions
ios/chrome/app/BUILD.gn
View file @ a8332cc7
......@@ -148,20 +148,6 @@ source_set("content_suggestions_scheduler_app_state_agent") {
]
}
source_set("app_metrics_app_state_agent") {
configs += [ "//build/config/compiler:enable_arc" ]
sources = [
"app_metrics_app_state_agent.h",
"app_metrics_app_state_agent.mm",
]
deps = [
"//base",
"//ios/chrome/app/application_delegate:app_state_header",
"//ios/chrome/browser/metrics",
"//ios/chrome/browser/ui/main:scene_state_header",
]
}
source_set("app_internal") {
configs += [ "//build/config/compiler:enable_arc" ]
sources = [
......@@ -181,7 +167,6 @@ source_set("app_internal") {
deps = [
":app",
":app_metrics_app_state_agent",
":blocking_scene_commands",
":content_suggestions_scheduler_app_state_agent",
":mode",
......
ios/chrome/app/app_metrics_app_state_agent.h deleted 100644 → 0
View file @ 26897571
// Copyright 2020 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
#ifndef IOS_CHROME_APP_APP_METRICS_APP_STATE_AGENT_H_
#define IOS_CHROME_APP_APP_METRICS_APP_STATE_AGENT_H_
#import <Foundation/Foundation.h>
#import "ios/chrome/app/application_delegate/app_state_agent.h"
// The agent that logs app-scoped metrics.
@interface AppMetricsAppStateAgent : NSObject <AppStateAgent>
@end
#endif // IOS_CHROME_APP_APP_METRICS_APP_STATE_AGENT_H_
ios/chrome/app/app_metrics_app_state_agent.mm deleted 100644 → 0
View file @ 26897571
// Copyright 2020 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
#import "ios/chrome/app/app_metrics_app_state_agent.h"
#include "base/metrics/histogram_macros.h"
#include "base/time/time.h"
#import "ios/chrome/app/application_delegate/app_state.h"
#import "ios/chrome/browser/metrics/ios_profile_session_durations_service.h"
#import "ios/chrome/browser/metrics/ios_profile_session_durations_service_factory.h"
#import "ios/chrome/browser/ui/main/scene_state.h"
#if !defined(__has_feature) || !__has_feature(objc_arc)
#error "This file requires ARC support."
#endif
@interface AppMetricsAppStateAgent () <AppStateObserver, SceneStateObserver>
// Observed app state.
@property(nonatomic, weak) AppState* appState;
@end
@implementation AppMetricsAppStateAgent
#pragma mark - AppStateAgent
- (void)setAppState:(AppState*)appState {
// This should only be called once!
DCHECK(!_appState);
_appState = appState;
[appState addObserver:self];
}
#pragma mark - AppStateObserver
- (void)appState:(AppState*)appState sceneConnected:(SceneState*)sceneState {
[sceneState addObserver:self];
}
- (void)appStateDidExitSafeMode:(AppState*)appState {
DCHECK(self.appState.lastTimeInForeground.is_null());
// Log session start. This normally happens in
// sceneState:transitionedToActivationLevel:, but is skipped in safe mode.
[self handleSessionStart];
}
#pragma mark - SceneStateObserver
- (void)sceneState:(SceneState*)sceneState
transitionedToActivationLevel:(SceneActivationLevel)level {
if (self.appState.isInSafeMode) {
// Don't log any metrics at safe mode. Wait for AppStateObserver's
// -appStateDidExitSafeMode to log session start.
return;
}
if (level >= SceneActivationLevelForegroundInactive &&
self.appState.lastTimeInForeground.is_null()) {
[self handleSessionStart];
} else if (level <= SceneActivationLevelBackground) {
for (SceneState* scene in self.appState.connectedScenes) {
if (scene.activationLevel > SceneActivationLevelBackground) {
// One scene has gone background, but at least one other is still
// foreground. Consider the session ongoing.
return;
}
}
if (self.appState.lastTimeInForeground.is_null()) {
// This method will be called multiple times, once per scene, if multiple
// scenes go background simulatneously (for example, if two windows were
// in split screen and the user swiped to go home). Only log the session
// duration once.
return;
}
[self handleSessionEnd];
DCHECK(self.appState.lastTimeInForeground.is_null());
}
}
#pragma mark - private
- (void)handleSessionStart {
self.appState.lastTimeInForeground = base::TimeTicks::Now();
IOSProfileSessionDurationsService* psdService = [self psdService];
if (psdService) {
psdService->OnSessionStarted(self.appState.lastTimeInForeground);
}
}
- (void)handleSessionEnd {
DCHECK(!self.appState.lastTimeInForeground.is_null());
base::TimeDelta duration =
base::TimeTicks::Now() - self.appState.lastTimeInForeground;
UMA_HISTOGRAM_LONG_TIMES("Session.TotalDuration", duration);
UMA_HISTOGRAM_CUSTOM_TIMES("Session.TotalDurationMax1Day", duration,
base::TimeDelta::FromMilliseconds(1),
base::TimeDelta::FromHours(24), 50);
IOSProfileSessionDurationsService* psdService = [self psdService];
if (psdService) {
psdService->OnSessionEnded(duration);
}
self.appState.lastTimeInForeground = base::TimeTicks();
}
- (IOSProfileSessionDurationsService*)psdService {
if (!self.appState.mainBrowserState) {
return nil;
}
return IOSProfileSessionDurationsServiceFactory::GetForBrowserState(
self.appState.mainBrowserState);
}
@end
ios/chrome/app/application_delegate/app_state.h
View file @ a8332cc7
......@@ -7,8 +7,6 @@
#import <UIKit/UIKit.h>
#include <memory>
#import "ios/chrome/app/application_delegate/app_state_agent.h"
#import "ios/chrome/browser/ui/main/scene_state_observer.h"
#import "ios/chrome/browser/ui/scoped_ui_blocker/ui_blocker_manager.h"
......@@ -26,10 +24,6 @@ class ChromeBrowserState;
@protocol TabOpening;
@protocol TabSwitching;
namespace base {
class TimeTicks;
}
@protocol AppStateObserver <NSObject>
@optional
......@@ -100,9 +94,6 @@ initWithBrowserLauncher:(id<BrowserLauncher>)browserLauncher
// multiple windows.
@property(nonatomic, strong) NSString* previousSingleWindowSessionID;
// Timestamp of when a scene was last becoming active. Can be null.
@property(nonatomic, assign) base::TimeTicks lastTimeInForeground;
// Saves the launchOptions to be used from -newTabFromLaunchOptions. If the
// application is in background, initialize the browser to basic. If not, launch
// the browser.
......
ios/chrome/app/application_delegate/app_state.mm
View file @ a8332cc7
......@@ -38,6 +38,8 @@
#include "ios/chrome/browser/feature_engagement/tracker_factory.h"
#import "ios/chrome/browser/geolocation/omnibox_geolocation_config.h"
#import "ios/chrome/browser/main/browser.h"
#import "ios/chrome/browser/metrics/ios_profile_session_durations_service.h"
#import "ios/chrome/browser/metrics/ios_profile_session_durations_service_factory.h"
#import "ios/chrome/browser/signin/authentication_service.h"
#import "ios/chrome/browser/signin/authentication_service_factory.h"
#import "ios/chrome/browser/ui/authentication/signed_in_accounts_view_controller.h"
......@@ -98,6 +100,8 @@ const NSTimeInterval kMemoryFootprintRecordingTimeInterval = 5;
// Variables backing properties of same name.
SafeModeCoordinator* _safeModeCoordinator;
// Start of the current session, used for UMA.
base::TimeTicks _sessionStartTime;
// YES if the app is currently in the process of terminating.
BOOL _appIsTerminating;
// Whether the application is currently in the background.
......@@ -383,6 +387,8 @@ initWithBrowserLauncher:(id<BrowserLauncher>)browserLauncher
DCHECK([_browserLauncher browserInitializationStage] ==
INITIALIZATION_STAGE_FOREGROUND);
_sessionStartTime = base::TimeTicks::Now();
id<BrowserInterface> currentInterface =
_browserLauncher.interfaceProvider.currentInterface;
CommandDispatcher* dispatcher =
......@@ -409,6 +415,12 @@ initWithBrowserLauncher:(id<BrowserLauncher>)browserLauncher
[HandlerForProtocol(dispatcher, HelpCommands) showHelpBubbleIfEligible];
}
IOSProfileSessionDurationsService* psdService =
IOSProfileSessionDurationsServiceFactory::GetForBrowserState(
currentInterface.browserState);
if (psdService)
psdService->OnSessionStarted(_sessionStartTime);
[MetricsMediator logStartupDuration:self.startupInformation
connectionInformation:connectionInformation];
}
......@@ -495,6 +507,11 @@ initWithBrowserLauncher:(id<BrowserLauncher>)browserLauncher
id<BrowserInterface> currentInterface =
_browserLauncher.interfaceProvider.currentInterface;
base::TimeDelta duration = base::TimeTicks::Now() - _sessionStartTime;
UMA_HISTOGRAM_LONG_TIMES("Session.TotalDuration", duration);
UMA_HISTOGRAM_CUSTOM_TIMES("Session.TotalDurationMax1Day", duration,
base::TimeDelta::FromMilliseconds(1),
base::TimeDelta::FromHours(24), 50);
// Record session metrics (currentInterface.browserState may be null during
// tests).
......@@ -516,6 +533,14 @@ initWithBrowserLauncher:(id<BrowserLauncher>)browserLauncher
->RecordAndClearSessionMetrics(MetricsToRecordFlags::kNoMetrics);
}
}
if (currentInterface.browserState) {
IOSProfileSessionDurationsService* psdService =
IOSProfileSessionDurationsServiceFactory::GetForBrowserState(
currentInterface.browserState);
if (psdService)
psdService->OnSessionEnded(duration);
}
}
- (BOOL)requiresHandlingAfterLaunchWithOptions:(NSDictionary*)launchOptions
......
ios/chrome/app/main_controller.mm
View file @ a8332cc7
......@@ -26,7 +26,6 @@
#import "components/previous_session_info/previous_session_info.h"
#include "components/ukm/ios/features.h"
#include "components/web_resource/web_resource_pref_names.h"
#include "ios/chrome/app/app_metrics_app_state_agent.h"
#import "ios/chrome/app/application_delegate/metrics_mediator.h"
#import "ios/chrome/app/blocking_scene_commands.h"
#import "ios/chrome/app/content_suggestions_scheduler_app_state_agent.h"
......@@ -605,7 +604,6 @@ void MainControllerAuthenticationServiceDelegate::ClearBrowsingData(
[appState addObserver:self];
// Create app state agents.
[appState addAgent:[[AppMetricsAppStateAgent alloc] init]];
[appState addAgent:[[ContentSuggestionsSchedulerAppAgent alloc] init]];
[appState addAgent:[[IncognitoUsageAppStateAgent alloc] init]];
......
tools/metrics/histograms/histograms_xml/session/histograms.xml
View file @ a8332cc7
......@@ -135,16 +135,26 @@ reviews. Googlers can read more about this at go/gwsq-gerrit.
when Chrome leaves the screen. This is generally straightforward. Chrome
leaves the screen when the screen goes blank or shows the lock screen, when
Chrome clicks a link that opens in another app, or when the user switches to
another app in the app switcher. Note that, unlike on desktop, even if
Chrome is playing media in the background or in picture-in-picture mode, the
session is still terminated when Chrome leaves the screen. Also note that if
Chrome starts in Safe Mode, the session will only start when the user quits
Safe Mode. When multiple windows are used, the session is considered ongoing
as long as there is at least one foreground Chrome window. This was revised
in M-89 to support multiple windows. In M-87 and M-88, nothing was logged
due to a bug. Before M-86, a similar metric was recorded, with some
differences in edge cases. See the old histogram description for more
details.
the app switcher. Note that, unlike on desktop, even if Chrome is playing
media in the background, the session is still terminated when Chrome leaves
the screen.
iOS continued: These are some edge cases to be aware of: (i) OS overlays.
These can appear, for examples, when a link is clicked that could go to a
non-Google app that has not yet been set to default and the OS asks whether
to open the link in the app by default, or when a user has set the OS to
block certain sites or types of sites by default and require authentication
to access them. This authentication prompt is an OS overlay. At the time of
writing this description, we're not sure if displaying an overlay will end
the session. (ii) full-screen media playing. Because media is played through
iOS's technology stack, if the user takes media that's playing within Chrome
and displayed it full-screen, the session ends because no part of Chrome is
on the screen anymore. (iii) multi-window support. Not yet launched as of
M-85. Sessions end and immediately restart when a user switches from a
single-window to a multi-window view and vice versa, and also ends when
resizing the Chrome window. The end is because Chrome is considered no
longer active during the time the OS animates the windows to change their
sizes.
This histogram is of special interest to the chrome-analysis-team@. Do not
change its semantics or retire it without talking to them first.
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment