Skip to content

Commit

Permalink
[ios] Remove SnapshotDrawView experiment
Browse files Browse the repository at this point in the history
This experiment could not be rolled out because it caused a flicker
in the UI.

Bug: 945811
Change-Id: I843bdf4666e0af17e31e9f27bb8aaf8433cb6274
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2765246
Auto-Submit: edchin <edchin@chromium.org>
Commit-Queue: Rohit Rao <rohitrao@chromium.org>
Reviewed-by: Rohit Rao <rohitrao@chromium.org>
Cr-Commit-Position: refs/heads/master@{#863766}
  • Loading branch information
edx246 authored and Chromium LUCI CQ committed Mar 17, 2021
1 parent d586af0 commit 8964a56
Show file tree
Hide file tree
Showing 6 changed files with 7 additions and 42 deletions.
3 changes: 0 additions & 3 deletions ios/chrome/browser/flags/about_flags.mm
Original file line number Diff line number Diff line change
Expand Up @@ -384,9 +384,6 @@
flag_descriptions::kOmniboxLocalHistoryZeroSuggestName,
flag_descriptions::kOmniboxLocalHistoryZeroSuggestDescription,
flags_ui::kOsIos, FEATURE_VALUE_TYPE(omnibox::kLocalHistoryZeroSuggest)},
{"snapshot-draw-view", flag_descriptions::kSnapshotDrawViewName,
flag_descriptions::kSnapshotDrawViewDescription, flags_ui::kOsIos,
FEATURE_VALUE_TYPE(kSnapshotDrawView)},
#if defined(DCHECK_IS_CONFIGURABLE)
{"dcheck-is-fatal", flag_descriptions::kDcheckIsFatalName,
flag_descriptions::kDcheckIsFatalDescription, flags_ui::kOsIos,
Expand Down
4 changes: 0 additions & 4 deletions ios/chrome/browser/flags/ios_chrome_flag_descriptions.cc
Original file line number Diff line number Diff line change
Expand Up @@ -406,10 +406,6 @@ const char kShowAutofillTypePredictionsDescription[] =
"Annotates web forms with Autofill field type predictions as placeholder "
"text.";

const char kSnapshotDrawViewName[] = "Use DrawViewHierarchy for Snapshots";
const char kSnapshotDrawViewDescription[] =
"When enabled, snapshots will be taken using |-drawViewHierarchy:|.";

const char kSimplifySignOutIOSName[] = "Simplify sign-out";
const char kSimplifySignOutIOSDescription[] =
"When enabled, sign-out UI in the account table view is simplified.";
Expand Down
5 changes: 0 additions & 5 deletions ios/chrome/browser/flags/ios_chrome_flag_descriptions.h
Original file line number Diff line number Diff line change
Expand Up @@ -345,11 +345,6 @@ extern const char kShowAutofillTypePredictionsDescription[];
extern const char kSimplifySignOutIOSName[];
extern const char kSimplifySignOutIOSDescription[];

// Title and description for the flag to use |-drawViewHierarchy:| for taking
// snapshots.
extern const char kSnapshotDrawViewName[];
extern const char kSnapshotDrawViewDescription[];

// Title and description for the flag to enable the Start Surface.
extern const char kStartSurfaceName[];
extern const char kStartSurfaceDescription[];
Expand Down
28 changes: 7 additions & 21 deletions ios/chrome/browser/snapshots/snapshot_generator.mm
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@
#include "base/task/post_task.h"
#import "ios/chrome/browser/snapshots/snapshot_cache.h"
#import "ios/chrome/browser/snapshots/snapshot_generator_delegate.h"
#include "ios/chrome/browser/ui/ui_feature_flags.h"
#import "ios/chrome/browser/ui/util/uikit_ui_util.h"
#include "ios/web/public/thread/web_task_traits.h"
#include "ios/web/public/thread/web_thread.h"
Expand Down Expand Up @@ -215,24 +214,20 @@ - (UIImage*)snapshotBaseView:(UIView*)baseView
-frameInBaseView.origin.y);
BOOL snapshotSuccess = YES;

// TODO(crbug.com/636188): |-drawViewHierarchyInRect:afterScreenUpdates:| is
// buggy on iOS 8/9/10 (and state is unknown for iOS 11) causing GPU glitches,
// screen redraws during animations, broken pinch to dismiss on tablet, etc.
// Ensure iOS 11 is not affected by these issues before turning on
// |kSnapshotDrawView| experiment. On the other hand, |-renderInContext:| is
// buggy for WKWebView, which is used for some Chromium pages such as "No
// internet" or "Site can't be reached".
BOOL useDrawViewHierarchy = ViewHierarchyContainsWKWebView(baseView) ||
base::FeatureList::IsEnabled(kSnapshotDrawView);
// |drawViewHierarchyInRect:| has undefined behavior when the view is not
// in the visible view hierarchy. In practice, when this method is called
// on a view that is part of view controller containment and not in the view
// hierarchy, an UIViewControllerHierarchyInconsistency exception will be
// thrown.
if (useDrawViewHierarchy && baseView.window) {
if (baseView.window && ViewHierarchyContainsWKWebView(baseView)) {
// TODO(crbug.com/636188): |-drawViewHierarchyInRect:afterScreenUpdates:| is
// buggy causing GPU glitches, screen redraws during animations, broken
// pinch to dismiss on tablet, etc.
snapshotSuccess = [baseView drawViewHierarchyInRect:baseView.bounds
afterScreenUpdates:YES];
} else {
// |-renderInContext:| is buggy for WKWebView, which is used for some
// Chromium pages such as "No internet" or "Site can't be reached".
[[baseView layer] renderInContext:context];
}
UIImage* image = nil;
Expand Down Expand Up @@ -293,16 +288,7 @@ - (void)drawOverlays:(NSArray<UIView*>*)overlays context:(CGContext*)context {
// This shifts the context so that drawing starts at the overlay's offset.
CGContextTranslateCTM(context, frameInWindow.origin.x,
frameInWindow.origin.y);
// |drawViewHierarchyInRect:| has undefined behavior when the view is not
// in the visible view hierarchy. In practice, when this method is called
// on a view that is part of view controller containment, an
// UIViewControllerHierarchyInconsistency exception will be thrown.
if (base::FeatureList::IsEnabled(kSnapshotDrawView) && overlay.window) {
// The rect's origin is ignored. Only size is used.
[overlay drawViewHierarchyInRect:overlay.bounds afterScreenUpdates:YES];
} else {
[[overlay layer] renderInContext:context];
}
[[overlay layer] renderInContext:context];
CGContextRestoreGState(context);
}
}
Expand Down
6 changes: 0 additions & 6 deletions ios/chrome/browser/ui/ui_feature_flags.cc
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,6 @@
const base::Feature kExpandedTabStrip{"ExpandedTabStrip",
base::FEATURE_DISABLED_BY_DEFAULT};

// TODO(crbug.com/945811): Using |-drawViewHierarchyInRect:afterScreenUpdates:|
// has adverse flickering when taking a snapshot of the NTP while in the app
// switcher.
const base::Feature kSnapshotDrawView{"SnapshotDrawView",
base::FEATURE_DISABLED_BY_DEFAULT};

const base::Feature kSettingsRefresh{"SettingsRefresh",
base::FEATURE_DISABLED_BY_DEFAULT};

Expand Down
3 changes: 0 additions & 3 deletions ios/chrome/browser/ui/ui_feature_flags.h
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,6 @@
// Feature to open tab switcher after sliding down the toolbar.
extern const base::Feature kExpandedTabStrip;

// Feature to take snapshots using |-drawViewHierarchy:|.
extern const base::Feature kSnapshotDrawView;

// Feature to apply UI Refresh theme to the settings.
extern const base::Feature kSettingsRefresh;

Expand Down

0 comments on commit 8964a56

Please sign in to comment.