Revert "flags: remove page-visibility-page-content-annotations" This reverts commit 385a71875e0a9fd510c9a7f35a139c035e8dddf6. Reason for revert: Failing PageContentAnnotationsServiceTest Original change's description: > flags: remove page-visibility-page-content-annotations > > This removes the flag and retains the default behavior. > > Fixed: 361124410 > Change-Id: I2c6347303afadc839ace7cfdb411641f63e7035c > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7234456 > Reviewed-by: Zekun Jiang <zekunjiang@google.com> > Auto-Submit: Mike Wittman <wittman@chromium.org> > Commit-Queue: Zekun Jiang <zekunjiang@google.com> > Cr-Commit-Position: refs/heads/main@{#1557732} Bug: 361124410, 468402904 Change-Id: I611e9ba9db67d92f2fc370a9467ced8e85bda934 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7255406 Owners-Override: Joel Tan-Aristy <jtanaristy@google.com> Commit-Queue: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com> Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com> Auto-Submit: Joel Tan-Aristy <jtanaristy@google.com> Cr-Commit-Position: refs/heads/main@{#1558334}
diff --git a/chrome/browser/about_flags.cc b/chrome/browser/about_flags.cc index f15c27411..33a0173 100644 --- a/chrome/browser/about_flags.cc +++ b/chrome/browser/about_flags.cc
@@ -7607,6 +7607,13 @@ flag_descriptions::kPageContentCacheDescription, kOsAndroid, FEATURE_VALUE_TYPE(page_content_annotations::features::kPageContentCache)}, + {"page-visibility-page-content-annotations", + flag_descriptions::kPageVisibilityPageContentAnnotationsName, + flag_descriptions::kPageVisibilityPageContentAnnotationsDescription, + kOsDesktop | kOsAndroid, + FEATURE_VALUE_TYPE(page_content_annotations::features:: + kPageVisibilityPageContentAnnotations)}, + #if BUILDFLAG(IS_CHROMEOS) {"language-packs-in-settings", flag_descriptions::kLanguagePacksInSettingsName,
diff --git a/chrome/browser/flag_descriptions.h b/chrome/browser/flag_descriptions.h index c7ee4aa..8660e1f 100644 --- a/chrome/browser/flag_descriptions.h +++ b/chrome/browser/flag_descriptions.h
@@ -3351,6 +3351,12 @@ "Enables the Geolocation Permission Control feature, which allows the " "use of the HTML 'geolocation' element."; +inline constexpr char kPageVisibilityPageContentAnnotationsName[] = + "Page visibility content annotations"; +inline constexpr char kPageVisibilityPageContentAnnotationsDescription[] = + "Enables annotating the page visibility model for each page load " + "on-device."; + inline constexpr char kParallelDownloadingName[] = "Parallel downloading"; inline constexpr char kParallelDownloadingDescription[] = "Enable parallel downloading to accelerate download speed.";
diff --git a/chrome/browser/page_content_annotations/page_content_annotations_service_browsertest.cc b/chrome/browser/page_content_annotations/page_content_annotations_service_browsertest.cc index c49af4229..328ee9bc 100644 --- a/chrome/browser/page_content_annotations/page_content_annotations_service_browsertest.cc +++ b/chrome/browser/page_content_annotations/page_content_annotations_service_browsertest.cc
@@ -237,6 +237,7 @@ { {"write_to_history_service", "true"}, }}, + {features::kPageVisibilityPageContentAnnotations, {}}, {history::kVisitedLinksOn404, {}}}, /*disabled_features=*/{ optimization_guide::features::kPreventLongRunningPredictionModels}); @@ -922,7 +923,8 @@ {{features::kPageContentAnnotations, { {"write_to_history_service", "false"}, - }}}, + }}, + {features::kPageVisibilityPageContentAnnotations, {}}}, /*disabled_features=*/{}); } ~PageContentAnnotationsServiceNoHistoryTest() override = default; @@ -1018,7 +1020,8 @@ scoped_feature_list_.InitWithFeaturesAndParameters( {{features::kPageContentAnnotations, {{"write_to_history_service", "false"}, - {"annotate_visit_batch_size", "2"}}}}, + {"annotate_visit_batch_size", "2"}}}, + {features::kPageVisibilityPageContentAnnotations, {}}}, /*disabled_features=*/{}); } ~PageContentAnnotationsServiceBatchVisitTest() override = default; @@ -1099,7 +1102,8 @@ scoped_feature_list_.InitWithFeaturesAndParameters( {{features::kPageContentAnnotations, {{"write_to_history_service", "false"}, - {"annotate_visit_batch_size", "1"}}}}, + {"annotate_visit_batch_size", "1"}}}, + {features::kPageVisibilityPageContentAnnotations, {}}}, /*disabled_features=*/{}); } ~PageContentAnnotationsServiceBatchVisitNoAnnotateTest() override = default;
diff --git a/components/page_content_annotations/core/page_content_annotations_features.cc b/components/page_content_annotations/core/page_content_annotations_features.cc index 0632760..364697b0 100644 --- a/components/page_content_annotations/core/page_content_annotations_features.cc +++ b/components/page_content_annotations/core/page_content_annotations_features.cc
@@ -31,6 +31,13 @@ base::FEATURE_ENABLED_BY_DEFAULT; #endif +constexpr auto enabled_by_default_non_arm32 = +#if defined(ARCH_CPU_ARMEL) + base::FEATURE_DISABLED_BY_DEFAULT; +#else + base::FEATURE_ENABLED_BY_DEFAULT; +#endif + constexpr char enabled_all_mobile_locales_en_us_desktop_only[] = #if BUILDFLAG(IS_ANDROID) || BUILDFLAG(IS_IOS) "*"; @@ -105,6 +112,10 @@ // Enables page content to be annotated. BASE_FEATURE(kPageContentAnnotations, base::FEATURE_ENABLED_BY_DEFAULT); +// Enables the page visibility model to be annotated on every page load. +BASE_FEATURE(kPageVisibilityPageContentAnnotations, + enabled_by_default_non_arm32); + BASE_FEATURE(kPageContentAnnotationsValidation, base::FEATURE_DISABLED_BY_DEFAULT); @@ -168,13 +179,10 @@ } bool ShouldExecutePageVisibilityModelOnPageContent(const std::string& locale) { -#if defined(ARCH_CPU_ARMEL) - return false; -#else - return IsSupportedLocale( - locale, - /*default_value=*/"ar,en,es,fa,fr,hi,id,pl,pt,tr,vi"); -#endif + return base::FeatureList::IsEnabled(kPageVisibilityPageContentAnnotations) && + IsSupportedLocaleForFeature( + locale, kPageVisibilityPageContentAnnotations, + /*default_value=*/"ar,en,es,fa,fr,hi,id,pl,pt,tr,vi"); } bool RemotePageMetadataEnabled(const std::string& locale,
diff --git a/components/page_content_annotations/core/page_content_annotations_features.h b/components/page_content_annotations/core/page_content_annotations_features.h index 2e4dd84..0dc9134 100644 --- a/components/page_content_annotations/core/page_content_annotations_features.h +++ b/components/page_content_annotations/core/page_content_annotations_features.h
@@ -15,6 +15,8 @@ COMPONENT_EXPORT(PAGE_CONTENT_ANNOTATIONS_FEATURES) BASE_DECLARE_FEATURE(kPageContentAnnotations); COMPONENT_EXPORT(PAGE_CONTENT_ANNOTATIONS_FEATURES) +BASE_DECLARE_FEATURE(kPageVisibilityPageContentAnnotations); +COMPONENT_EXPORT(PAGE_CONTENT_ANNOTATIONS_FEATURES) BASE_DECLARE_FEATURE(kPageContentAnnotationsValidation); COMPONENT_EXPORT(PAGE_CONTENT_ANNOTATIONS_FEATURES) BASE_DECLARE_FEATURE(kRemotePageMetadata);
diff --git a/components/page_content_annotations/core/page_content_annotations_features_unittest.cc b/components/page_content_annotations/core/page_content_annotations_features_unittest.cc index 8d973ae..dae6efea 100644 --- a/components/page_content_annotations/core/page_content_annotations_features_unittest.cc +++ b/components/page_content_annotations/core/page_content_annotations_features_unittest.cc
@@ -42,26 +42,30 @@ } TEST(PageContentAnnotationsFeaturesTest, - ShouldExecutePageVisibilityModelOnPageContent) { -#if defined(ARCH_CPU_ARMEL) - constexpr bool expected_enabled = false; -#else - constexpr bool expected_enabled = true; -#endif + ShouldExecutePageVisibilityModelOnPageContentDisabled) { + base::test::ScopedFeatureList scoped_feature_list; + + scoped_feature_list.InitAndDisableFeature( + features::kPageVisibilityPageContentAnnotations); + + EXPECT_FALSE( + features::ShouldExecutePageVisibilityModelOnPageContent("en-US")); +} + +TEST(PageContentAnnotationsFeaturesTest, + ShouldExecutePageVisibilityModelOnPageContentEmptyAllowlist) { + base::test::ScopedFeatureList scoped_feature_list; + + scoped_feature_list.InitAndEnableFeature( + features::kPageVisibilityPageContentAnnotations); // These are default enabled values. - EXPECT_EQ(expected_enabled, - features::ShouldExecutePageVisibilityModelOnPageContent("en")); - EXPECT_EQ(expected_enabled, - features::ShouldExecutePageVisibilityModelOnPageContent("en-AU")); - EXPECT_EQ(expected_enabled, - features::ShouldExecutePageVisibilityModelOnPageContent("en-CA")); - EXPECT_EQ(expected_enabled, - features::ShouldExecutePageVisibilityModelOnPageContent("en-GB")); - EXPECT_EQ(expected_enabled, - features::ShouldExecutePageVisibilityModelOnPageContent("en-US")); - EXPECT_EQ(expected_enabled, - features::ShouldExecutePageVisibilityModelOnPageContent("fr")); + EXPECT_TRUE(features::ShouldExecutePageVisibilityModelOnPageContent("en")); + EXPECT_TRUE(features::ShouldExecutePageVisibilityModelOnPageContent("en-AU")); + EXPECT_TRUE(features::ShouldExecutePageVisibilityModelOnPageContent("en-CA")); + EXPECT_TRUE(features::ShouldExecutePageVisibilityModelOnPageContent("en-GB")); + EXPECT_TRUE(features::ShouldExecutePageVisibilityModelOnPageContent("en-US")); + EXPECT_TRUE(features::ShouldExecutePageVisibilityModelOnPageContent("fr")); EXPECT_FALSE( features::ShouldExecutePageVisibilityModelOnPageContent("zh-CN")); @@ -95,6 +99,20 @@ EXPECT_TRUE(features::RemotePageMetadataEnabled("badlocale", "US")); } +TEST(PageContentAnnotationsFeaturesTest, + ShouldExecutePageVisibilityModelOnPageContentWithAllowlist) { + base::test::ScopedFeatureList scoped_feature_list; + + scoped_feature_list.InitAndEnableFeatureWithParameters( + features::kPageVisibilityPageContentAnnotations, + {{"supported_locales", "en,zh-TW"}}); + + EXPECT_TRUE(features::ShouldExecutePageVisibilityModelOnPageContent("en-US")); + EXPECT_FALSE(features::ShouldExecutePageVisibilityModelOnPageContent("")); + EXPECT_FALSE( + features::ShouldExecutePageVisibilityModelOnPageContent("zh-CN")); +} + TEST(PageContentAnnotationsFeaturesTest, ShouldPersistSalientImageMetadata) { #if BUILDFLAG(IS_ANDROID) || BUILDFLAG(IS_IOS) // Mobile should accept all locales and countries.
diff --git a/components/page_content_annotations/core/page_content_annotations_model_manager_unittest.cc b/components/page_content_annotations/core/page_content_annotations_model_manager_unittest.cc index 0e5658b..1661fec 100644 --- a/components/page_content_annotations/core/page_content_annotations_model_manager_unittest.cc +++ b/components/page_content_annotations/core/page_content_annotations_model_manager_unittest.cc
@@ -58,9 +58,10 @@ class PageContentAnnotationsModelManagerTest : public testing::Test { public: PageContentAnnotationsModelManagerTest() { - // Disable Entities. - scoped_feature_list_.InitAndDisableFeature( - optimization_guide::features::kPreventLongRunningPredictionModels); + // Enable Visibility but disable Entities. + scoped_feature_list_.InitWithFeatures( + {features::kPageVisibilityPageContentAnnotations}, + {optimization_guide::features::kPreventLongRunningPredictionModels}); } ~PageContentAnnotationsModelManagerTest() override = default;
diff --git a/components/page_content_annotations/core/page_content_annotations_service_unittest.cc b/components/page_content_annotations/core/page_content_annotations_service_unittest.cc index 5f42029..ad5dd929 100644 --- a/components/page_content_annotations/core/page_content_annotations_service_unittest.cc +++ b/components/page_content_annotations/core/page_content_annotations_service_unittest.cc
@@ -145,7 +145,8 @@ {"write_to_history_service", "true"}, {"pca_service_wait_for_title_delay_in_milliseconds", "4999"}, {"annotate_visit_batch_size", "1"}, - }}}, + }}, + {features::kPageVisibilityPageContentAnnotations, {}}}, /*disabled_features=*/{ optimization_guide::features::kPreventLongRunningPredictionModels}); }
diff --git a/ios/chrome/browser/flags/about_flags.mm b/ios/chrome/browser/flags/about_flags.mm index cb2d4aa..da7d2b8 100644 --- a/ios/chrome/browser/flags/about_flags.mm +++ b/ios/chrome/browser/flags/about_flags.mm
@@ -1946,6 +1946,12 @@ flags_ui::kOsIos, FEATURE_VALUE_TYPE( page_content_annotations::features::kRemotePageMetadata)}, + {"page-visibility-page-content-annotations", + flag_descriptions::kPageVisibilityPageContentAnnotationsName, + flag_descriptions::kPageVisibilityPageContentAnnotationsDescription, + flags_ui::kOsIos, + FEATURE_VALUE_TYPE(page_content_annotations::features:: + kPageVisibilityPageContentAnnotations)}, {"enhanced-safe-browsing-promo", flag_descriptions::kEnhancedSafeBrowsingPromoName, flag_descriptions::kEnhancedSafeBrowsingPromoDescription, flags_ui::kOsIos,