Commit 28ee59cf authored by Gang Wu's avatar Gang Wu Committed by Commit Bot

Revert "[Instant Start] Avoid pre-native GURL usage"

This reverts commit cb2f5900.

Reason for revert: 
https://ci.chromium.org/p/chromium/builders/ci/Lollipop%20Phone%20Tester/27059


Original change's description:
> [Instant Start] Avoid pre-native GURL usage
> 
> Remove native dependency of
> ReturnToChromeExperimentsUtil.shouldShowStartSurfaceAsTheHomePage()
> and PseudoTab.readAllPseudoTabsFromStateFile() so that they can be
> called without leading to native initialization.
> 
> Benchmarking results using http://crrev.com/c/2373287:
> 
> 100 samples on Nexus 5X
>                                 Median  Diff with control   p-value
> FirstDrawCompletedTime         414.0ms   -29.0ms (-7.00%)  0.000000
> SingleTabTitleAvailableTime    287.0ms  -31.5ms (-10.98%)  0.000000
> FeedStreamCreatedTime          836.5ms   -39.5ms (-4.72%)  0.000030
> FeedContentFirstLoadedTime    1898.5ms    -0.5ms (-0.03%)  0.534189
> FeedsLoadingPlaceholderShown   234.0ms    -2.0ms (-0.85%)  0.629761
> 
> 100 samples on Pixel 2 XL
>                                 Median  Diff with control   p-value
> FirstDrawCompletedTime         176.0ms  -18.0ms (-10.23%)  0.000000
> SingleTabTitleAvailableTime    131.0ms  -14.0ms (-10.69%)  0.000000
> FeedStreamCreatedTime          424.0ms   -31.0ms (-7.31%)  0.000180
> FeedContentFirstLoadedTime    1137.0ms   -18.5ms (-1.63%)  0.075061
> FeedsLoadingPlaceholderShown   107.0ms    -1.0ms (-0.93%)  0.718766
> 
> 100 samples on Pixel 3 XL
>                                 Median  Diff with control   p-value
> FirstDrawCompletedTime         155.0ms   -13.5ms (-8.71%)  0.000000
> SingleTabTitleAvailableTime    117.0ms  -13.0ms (-11.11%)  0.000000
> FeedStreamCreatedTime          356.0ms   -35.5ms (-9.97%)  0.000001
> FeedsLoadingPlaceholderShown    94.5ms    -2.0ms (-2.12%)  0.007312
> FeedContentFirstLoadedTime     924.0ms    -6.5ms (-0.70%)  0.034100
> 
> Bug: 1041865
> Change-Id: I640aaeb6f2dae64b9152fd94d0cd354a487f5bf5
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2372900
> Reviewed-by: Andrew Grieve <agrieve@chromium.org>
> Reviewed-by: Michael Thiessen <mthiesse@chromium.org>
> Reviewed-by: Xi Han <hanxi@chromium.org>
> Commit-Queue: Wei-Yin Chen (陳威尹) <wychen@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#809226}

TBR=mthiesse@chromium.org,hanxi@chromium.org,agrieve@chromium.org,wychen@chromium.org

Change-Id: I7d864239a477c7875bc79f1e39be54d18a72c9ff
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 1041865,1131139
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2424983Reviewed-by: default avatarGang Wu <gangwu@chromium.org>
Commit-Queue: Gang Wu <gangwu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#809467}
parent ac41e4e0
...@@ -27,10 +27,10 @@ import org.chromium.base.NativeLibraryLoadedStatus; ...@@ -27,10 +27,10 @@ import org.chromium.base.NativeLibraryLoadedStatus;
import org.chromium.base.NativeLibraryLoadedStatus.NativeLibraryLoadedStatusProvider; import org.chromium.base.NativeLibraryLoadedStatus.NativeLibraryLoadedStatusProvider;
import org.chromium.base.StrictModeContext; import org.chromium.base.StrictModeContext;
import org.chromium.base.TraceEvent; import org.chromium.base.TraceEvent;
import org.chromium.base.annotations.CheckDiscard;
import org.chromium.base.annotations.JNINamespace; import org.chromium.base.annotations.JNINamespace;
import org.chromium.base.annotations.MainDex; import org.chromium.base.annotations.MainDex;
import org.chromium.base.annotations.NativeMethods; import org.chromium.base.annotations.NativeMethods;
import org.chromium.base.annotations.RemovableInRelease;
import org.chromium.base.compat.ApiHelperForM; import org.chromium.base.compat.ApiHelperForM;
import org.chromium.base.metrics.RecordHistogram; import org.chromium.base.metrics.RecordHistogram;
import org.chromium.base.metrics.UmaRecorderHolder; import org.chromium.base.metrics.UmaRecorderHolder;
...@@ -235,7 +235,7 @@ public class LibraryLoader { ...@@ -235,7 +235,7 @@ public class LibraryLoader {
return mUseModernLinker; return mUseModernLinker;
} }
@CheckDiscard("Can't use @RemovableInRelease because Release build with DCHECK_IS_ON needs it") @RemovableInRelease
public void enableJniChecks() { public void enableJniChecks() {
if (!BuildConfig.DCHECK_IS_ON) return; if (!BuildConfig.DCHECK_IS_ON) return;
......
...@@ -15,7 +15,6 @@ import static androidx.test.espresso.matcher.ViewMatchers.withText; ...@@ -15,7 +15,6 @@ import static androidx.test.espresso.matcher.ViewMatchers.withText;
import static com.google.common.truth.Truth.assertThat; import static com.google.common.truth.Truth.assertThat;
import static org.hamcrest.CoreMatchers.allOf; import static org.hamcrest.CoreMatchers.allOf;
import static org.hamcrest.CoreMatchers.is;
import static org.hamcrest.Matchers.greaterThan; import static org.hamcrest.Matchers.greaterThan;
import static org.hamcrest.Matchers.instanceOf; import static org.hamcrest.Matchers.instanceOf;
import static org.hamcrest.Matchers.notNullValue; import static org.hamcrest.Matchers.notNullValue;
...@@ -43,13 +42,10 @@ import org.hamcrest.core.AllOf; ...@@ -43,13 +42,10 @@ import org.hamcrest.core.AllOf;
import org.junit.Assert; import org.junit.Assert;
import org.junit.Rule; import org.junit.Rule;
import org.junit.Test; import org.junit.Test;
import org.junit.rules.ErrorCollector;
import org.junit.runner.RunWith; import org.junit.runner.RunWith;
import org.chromium.base.BuildConfig;
import org.chromium.base.Callback; import org.chromium.base.Callback;
import org.chromium.base.CommandLine; import org.chromium.base.CommandLine;
import org.chromium.base.NativeLibraryLoadedStatus;
import org.chromium.base.StreamUtil; import org.chromium.base.StreamUtil;
import org.chromium.base.library_loader.LibraryLoader; import org.chromium.base.library_loader.LibraryLoader;
import org.chromium.base.test.util.CommandLineFlags; import org.chromium.base.test.util.CommandLineFlags;
...@@ -128,9 +124,6 @@ public class InstantStartTest { ...@@ -128,9 +124,6 @@ public class InstantStartTest {
public ChromeRenderTestRule mRenderTestRule = public ChromeRenderTestRule mRenderTestRule =
ChromeRenderTestRule.Builder.withPublicCorpus().build(); ChromeRenderTestRule.Builder.withPublicCorpus().build();
@Rule
public ErrorCollector collector = new ErrorCollector();
/** /**
* Only launch Chrome without waiting for a current tab. * Only launch Chrome without waiting for a current tab.
* This test could not use {@link ChromeActivityTestRule#startMainActivityFromLauncher()} * This test could not use {@link ChromeActivityTestRule#startMainActivityFromLauncher()}
...@@ -818,30 +811,6 @@ public class InstantStartTest { ...@@ -818,30 +811,6 @@ public class InstantStartTest {
Assert.assertEquals(123, ((StaticLayout) activeLayout).getCurrentTabIdForTesting()); Assert.assertEquals(123, ((StaticLayout) activeLayout).getCurrentTabIdForTesting());
} }
@Test
@SmallTest
@Restriction({UiRestriction.RESTRICTION_TYPE_PHONE})
@EnableFeatures({ChromeFeatureList.START_SURFACE_ANDROID + "<Study"})
// clang-format off
@CommandLineFlags.Add({ChromeSwitches.DISABLE_NATIVE_INITIALIZATION,
"force-fieldtrials=Study/Group",
"force-fieldtrial-params=Study.Group:start_surface_variation/single"})
public void testNoGURLPreNative() {
// clang-format on
if (!BuildConfig.DCHECK_IS_ON) return;
collector.checkThat(StartSurfaceConfiguration.isStartSurfaceSinglePaneEnabled(), is(true));
collector.checkThat(TextUtils.isEmpty(HomepageManager.getHomepageUri()), is(false));
Assert.assertFalse(
NativeLibraryLoadedStatus.getProviderForTesting().areMainDexNativeMethodsReady());
ReturnToChromeExperimentsUtil.shouldShowStartSurfaceAsTheHomePage();
ReturnToChromeExperimentsUtil.shouldShowStartSurfaceAsTheHomePageNoTabs();
PseudoTab.getAllPseudoTabsFromStateFile();
Assert.assertFalse("There should be no GURL usages triggering native library loading",
NativeLibraryLoadedStatus.getProviderForTesting().areMainDexNativeMethodsReady());
}
/** /**
* Toggles the header and checks whether the header has the right status. * Toggles the header and checks whether the header has the right status.
* *
......
...@@ -14,6 +14,7 @@ import org.chromium.base.Log; ...@@ -14,6 +14,7 @@ import org.chromium.base.Log;
import org.chromium.base.StreamUtil; import org.chromium.base.StreamUtil;
import org.chromium.chrome.browser.flags.CachedFeatureFlags; import org.chromium.chrome.browser.flags.CachedFeatureFlags;
import org.chromium.chrome.browser.flags.ChromeFeatureList; import org.chromium.chrome.browser.flags.ChromeFeatureList;
import org.chromium.chrome.browser.ntp.NewTabPage;
import org.chromium.chrome.browser.tab.Tab; import org.chromium.chrome.browser.tab.Tab;
import org.chromium.chrome.browser.tab.state.CriticalPersistedTabData; import org.chromium.chrome.browser.tab.state.CriticalPersistedTabData;
import org.chromium.chrome.browser.tabmodel.TabList; import org.chromium.chrome.browser.tabmodel.TabList;
...@@ -23,7 +24,6 @@ import org.chromium.chrome.browser.tabmodel.TabModelSelector; ...@@ -23,7 +24,6 @@ import org.chromium.chrome.browser.tabmodel.TabModelSelector;
import org.chromium.chrome.browser.tabmodel.TabPersistentStore; import org.chromium.chrome.browser.tabmodel.TabPersistentStore;
import org.chromium.chrome.browser.tabmodel.TabbedModeTabPersistencePolicy; import org.chromium.chrome.browser.tabmodel.TabbedModeTabPersistencePolicy;
import org.chromium.chrome.browser.tabpersistence.TabStateDirectory; import org.chromium.chrome.browser.tabpersistence.TabStateDirectory;
import org.chromium.chrome.browser.tasks.ReturnToChromeExperimentsUtil;
import org.chromium.chrome.browser.tasks.tab_management.TabUiFeatureUtilities; import org.chromium.chrome.browser.tasks.tab_management.TabUiFeatureUtilities;
import java.io.ByteArrayInputStream; import java.io.ByteArrayInputStream;
...@@ -347,10 +347,7 @@ public class PseudoTab { ...@@ -347,10 +347,7 @@ public class PseudoTab {
(index, id, url, isIncognito, isStandardActiveIndex, isIncognitoActiveIndex) (index, id, url, isIncognito, isStandardActiveIndex, isIncognitoActiveIndex)
-> { -> {
// Skip restoring of non-selected NTP to match the real restoration logic. // Skip restoring of non-selected NTP to match the real restoration logic.
if (ReturnToChromeExperimentsUtil.isCanonicalizedNTPUrl(url) if (NewTabPage.isNTPUrl(url) && !isStandardActiveIndex) return;
&& !isStandardActiveIndex) {
return;
}
PseudoTab tab = PseudoTab.fromTabId(id); PseudoTab tab = PseudoTab.fromTabId(id);
if (isStandardActiveIndex) { if (isStandardActiveIndex) {
assert sActiveTabFromStateFile == null; assert sActiveTabFromStateFile == null;
......
...@@ -22,6 +22,7 @@ import org.chromium.chrome.browser.flags.ChromeFeatureList; ...@@ -22,6 +22,7 @@ import org.chromium.chrome.browser.flags.ChromeFeatureList;
import org.chromium.chrome.browser.flags.IntCachedFieldTrialParameter; import org.chromium.chrome.browser.flags.IntCachedFieldTrialParameter;
import org.chromium.chrome.browser.homepage.HomepageManager; import org.chromium.chrome.browser.homepage.HomepageManager;
import org.chromium.chrome.browser.locale.LocaleManager; import org.chromium.chrome.browser.locale.LocaleManager;
import org.chromium.chrome.browser.ntp.NewTabPage;
import org.chromium.chrome.browser.tab.TabLaunchType; import org.chromium.chrome.browser.tab.TabLaunchType;
import org.chromium.chrome.browser.tabmodel.TabModel; import org.chromium.chrome.browser.tabmodel.TabModel;
import org.chromium.chrome.browser.tabmodel.TabModelSelector; import org.chromium.chrome.browser.tabmodel.TabModelSelector;
...@@ -210,11 +211,17 @@ public final class ReturnToChromeExperimentsUtil { ...@@ -210,11 +211,17 @@ public final class ReturnToChromeExperimentsUtil {
return chromeActivity; return chromeActivity;
} }
public static boolean isCanonicalizedNTPUrl(String url) { /**
// Avoid loading native library due to GURL usage since * TODO(crbug/1041865): avoid using GURL since {@link #shouldShowStartSurfaceAsTheHomePage()}
// #shouldShowStartSurfaceAsTheHomePage() is in the critical path in Instant Start. * is in the critical path in Instant Start.
return url.equals("chrome://newtab/") || url.equals("chrome-native://newtab/") */
|| url.equals("about:newtab"); private static boolean isNTPUrl(String url) {
if (CachedFeatureFlags.isEnabled(ChromeFeatureList.INSTANT_START)) {
try (StrictModeContext ignored = StrictModeContext.allowDiskReads()) {
return NewTabPage.isNTPUrl(url);
}
}
return NewTabPage.isNTPUrl(url);
} }
/** /**
...@@ -263,7 +270,7 @@ public final class ReturnToChromeExperimentsUtil { ...@@ -263,7 +270,7 @@ public final class ReturnToChromeExperimentsUtil {
// accessibility is not enabled and not on tablet. // accessibility is not enabled and not on tablet.
String homePageUrl = HomepageManager.getHomepageUri(); String homePageUrl = HomepageManager.getHomepageUri();
return StartSurfaceConfiguration.isStartSurfaceSinglePaneEnabled() return StartSurfaceConfiguration.isStartSurfaceSinglePaneEnabled()
&& (TextUtils.isEmpty(homePageUrl) || isCanonicalizedNTPUrl(homePageUrl)) && (TextUtils.isEmpty(homePageUrl) || isNTPUrl(homePageUrl))
&& !ChromeAccessibilityUtil.get().isAccessibilityEnabled() && !ChromeAccessibilityUtil.get().isAccessibilityEnabled()
&& !DeviceFormFactor.isNonMultiDisplayContextOnTablet( && !DeviceFormFactor.isNonMultiDisplayContextOnTablet(
ContextUtils.getApplicationContext()); ContextUtils.getApplicationContext());
......
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