Skip to content

Commit

Permalink
Add enable_feed_v1 compile flag
Browse files Browse the repository at this point in the history
And restructure feed code so that we can use it.

This flag pulls Feed v1 out of the build.
It's a bit different that enable_feed_v2. enable_feed_v2
retains most of the v2 code in chromium -- only the internal
code is actually removed when enable_feed_v2 is off.
If both flags are ON (the default), we use the InterestFeedV2
feature to select which version to use. To facilitate this,
only feed_feature_list.cc and FeedFeatures.java should inspect
the state of this feature directly.

If both build flags are OFF, we use the V2 UI classes as
essentially a pass-through to a recycler view, but will
not request any Feed content.

The compile flag enable_feed_in_chrome was removed.

Other changes:
* I've moved some files around to make it more clear what code
  is for v1 only.
* I've added FeedV1 and FeedV2 java classes to wrap access to
  version specific things, so that FeedV1 can be removed using
  the compile flag.

Bug: 1129187
Change-Id: I19b1b8deeafae51b00e6b2c23373da787dc7c93d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2417533
Commit-Queue: Dan H <harringtond@chromium.org>
Reviewed-by: Carlos Knippschild <carlosk@chromium.org>
Reviewed-by: Mustafa Emre Acer <meacer@chromium.org>
Reviewed-by: Ted Choc <tedchoc@chromium.org>
Reviewed-by: Mohamed Heikal <mheikal@chromium.org>
Cr-Commit-Position: refs/heads/master@{#812249}
  • Loading branch information
Dan Harrington authored and Commit Bot committed Sep 30, 2020
1 parent 0d21648 commit e274ab4
Show file tree
Hide file tree
Showing 113 changed files with 937 additions and 668 deletions.
28 changes: 13 additions & 15 deletions chrome/android/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -1193,9 +1193,7 @@ android_library("chrome_test_java") {
"//url/mojom:url_mojom_gurl_java",
]

if (enable_feed_in_chrome) {
deps += feed_test_deps
}
deps += feed_test_deps

data = [
"//chrome/test/data/android/",
Expand Down Expand Up @@ -3017,6 +3015,9 @@ generate_jni("chrome_jni_headers") {
sources = [
# Files under a feature's public/ dir are included in chrome_java's source
# files, so include these files in chrome_jni_headers.
"feed/core/java/src/org/chromium/chrome/browser/feed/v2/FeedImageFetchClient.java",
"feed/core/java/src/org/chromium/chrome/browser/feed/v2/FeedServiceBridge.java",
"feed/core/java/src/org/chromium/chrome/browser/feed/v2/FeedStreamSurface.java",
"java/src/org/chromium/chrome/browser/AfterStartupTaskUtils.java",
"java/src/org/chromium/chrome/browser/AppHooks.java",
"java/src/org/chromium/chrome/browser/ApplicationLifetime.java",
Expand Down Expand Up @@ -3282,19 +3283,16 @@ generate_jni("chrome_jni_headers") {
sources += [ "java/src/org/chromium/chrome/browser/offlinepages/evaluation/OfflinePageEvaluationBridge.java" ]
}

if (enable_feed_in_chrome) {
if (enable_feed_v1) {
sources += [
"feed/core/java/src/org/chromium/chrome/browser/feed/FeedContentBridge.java",
"feed/core/java/src/org/chromium/chrome/browser/feed/FeedDebuggingBridge.java",
"feed/core/java/src/org/chromium/chrome/browser/feed/FeedJournalBridge.java",
"feed/core/java/src/org/chromium/chrome/browser/feed/FeedLifecycleBridge.java",
"feed/core/java/src/org/chromium/chrome/browser/feed/FeedLoggingBridge.java",
"feed/core/java/src/org/chromium/chrome/browser/feed/FeedNetworkBridge.java",
"feed/core/java/src/org/chromium/chrome/browser/feed/FeedOfflineBridge.java",
"feed/core/java/src/org/chromium/chrome/browser/feed/FeedSchedulerBridge.java",
"feed/core/java/src/org/chromium/chrome/browser/feed/v2/FeedImageFetchClient.java",
"feed/core/java/src/org/chromium/chrome/browser/feed/v2/FeedServiceBridge.java",
"feed/core/java/src/org/chromium/chrome/browser/feed/v2/FeedStreamSurface.java",
"feed/core/java/src/org/chromium/chrome/browser/feed/v1/FeedContentBridge.java",
"feed/core/java/src/org/chromium/chrome/browser/feed/v1/FeedDebuggingBridge.java",
"feed/core/java/src/org/chromium/chrome/browser/feed/v1/FeedJournalBridge.java",
"feed/core/java/src/org/chromium/chrome/browser/feed/v1/FeedLifecycleBridge.java",
"feed/core/java/src/org/chromium/chrome/browser/feed/v1/FeedLoggingBridge.java",
"feed/core/java/src/org/chromium/chrome/browser/feed/v1/FeedNetworkBridge.java",
"feed/core/java/src/org/chromium/chrome/browser/feed/v1/FeedOfflineBridge.java",
"feed/core/java/src/org/chromium/chrome/browser/feed/v1/FeedSchedulerBridge.java",
]
}

Expand Down
7 changes: 6 additions & 1 deletion chrome/android/chrome_test_java_sources.gni
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
# Use of this source code is governed by a BSD-style license that can be
# found in the LICENSE file.

import("//components/feed/features.gni")

# Java test file for clank components. all test sources that are not
# conditionally added. See java_source.gni for conditionally added files.

Expand Down Expand Up @@ -314,7 +316,6 @@ chrome_test_java_sources = [
"javatests/src/org/chromium/chrome/browser/offlinepages/indicator/OfflineIndicatorControllerTest.java",
"javatests/src/org/chromium/chrome/browser/offlinepages/prefetch/PrefetchBackgroundTaskTest.java",
"javatests/src/org/chromium/chrome/browser/offlinepages/prefetch/PrefetchConfigurationTest.java",
"javatests/src/org/chromium/chrome/browser/offlinepages/prefetch/PrefetchFeedFlowTest.java",
"javatests/src/org/chromium/chrome/browser/offlinepages/prefetch/TestOfflinePageService.java",
"javatests/src/org/chromium/chrome/browser/offlinepages/prefetch/TestSuggestionsService.java",
"javatests/src/org/chromium/chrome/browser/omaha/ExponentialBackoffSchedulerTest.java",
Expand Down Expand Up @@ -625,3 +626,7 @@ chrome_test_java_sources = [
"javatests/src/org/chromium/chrome/browser/widget/bottomsheet/TestBottomSheetContent.java",
"javatests/src/org/chromium/chrome/test/crash/IntentionalCrashTest.java",
]

if (enable_feed_v1) {
chrome_test_java_sources += [ "javatests/src/org/chromium/chrome/browser/offlinepages/prefetch/PrefetchFeedFlowTest.java" ]
}
2 changes: 1 addition & 1 deletion chrome/android/expectations/lint-suppressions.xml
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ Still reading?
<issue id="LongLogTag" severity="ignore"/>
<issue id="MergeRootFrame">
<!-- TODO(crbug.com/1039415): Remove suppression after fixing bug. -->
<ignore regexp="chrome/android/feed/core/java/res/layout/feed_more_button.xml"/>
<ignore regexp="chrome/android/feed/core/java/resv1/layout/feed_more_button.xml"/>
</issue>
<issue id="MissingClass" severity="ignore"/>
<issue id="MissingDefaultResource">
Expand Down
38 changes: 13 additions & 25 deletions chrome/android/features/start_surface/internal/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ java_strings_grd("java_strings_grd") {

android_resources("java_resources") {
sources = [
"java/explore_res/layout/ss_feed_header.xml",
"java/res/drawable-hdpi/ic_explore.png",
"java/res/drawable-hdpi/ic_home.png",
"java/res/drawable-mdpi/ic_explore.png",
Expand All @@ -36,9 +37,6 @@ android_resources("java_resources") {
"java/res/values/dimens.xml",
"java/res/values/ids.xml",
]
if (enable_feed_in_chrome) {
sources += [ "java/explore_res/layout/ss_feed_header.xml" ]
}
deps = [
":java_strings_grd",
"//chrome/android:chrome_app_java_resources",
Expand All @@ -50,6 +48,12 @@ android_library("java") {
"java/src/org/chromium/chrome/features/start_surface/BottomBarCoordinator.java",
"java/src/org/chromium/chrome/features/start_surface/BottomBarView.java",
"java/src/org/chromium/chrome/features/start_surface/BottomBarViewBinder.java",
"java/src/org/chromium/chrome/features/start_surface/ExploreSurfaceCoordinator.java",
"java/src/org/chromium/chrome/features/start_surface/ExploreSurfaceNavigationDelegate.java",
"java/src/org/chromium/chrome/features/start_surface/ExploreSurfaceStreamLifecycleManager.java",
"java/src/org/chromium/chrome/features/start_surface/ExploreSurfaceViewBinder.java",
"java/src/org/chromium/chrome/features/start_surface/FeedLoadingCoordinator.java",
"java/src/org/chromium/chrome/features/start_surface/FeedLoadingLayout.java",
"java/src/org/chromium/chrome/features/start_surface/ReturnToStartSurfaceUtil.java",
"java/src/org/chromium/chrome/features/start_surface/SecondaryTasksSurfaceViewBinder.java",
"java/src/org/chromium/chrome/features/start_surface/StartSurfaceCoordinator.java",
Expand All @@ -65,10 +69,12 @@ android_library("java") {
":java_resources",
"//base:base_java",
"//chrome/android:chrome_java",
"//chrome/android/feed:chrome_feed_java_resources",
"//chrome/android/third_party/compositor_animator:compositor_animator_java",
"//chrome/browser/browser_controls/android:java",
"//chrome/browser/flags:java",
"//chrome/browser/preferences:java",
"//chrome/browser/profiles/android:java",
"//chrome/browser/tab:java",
"//chrome/browser/tabmodel:java",
"//chrome/browser/ui/messages/android:java",
Expand All @@ -77,34 +83,16 @@ android_library("java") {
"//components/browser_ui/widget/android:java",
"//components/prefs/android:java",
"//components/user_prefs/android:java",
"//content/public/android:content_java",
"//third_party/android_deps:android_support_v7_appcompat_java",
"//third_party/android_deps:androidx_annotation_annotation_java",
"//third_party/android_deps:material_design_java",
"//third_party/android_sdk/androidx_browser:androidx_browser_java",
"//ui/android:ui_full_java",
"//ui/android:ui_utils_java",
"//ui/base/mojom:mojom_java",
]

if (enable_feed_in_chrome) {
sources += [
"java/src/org/chromium/chrome/features/start_surface/ExploreSurfaceCoordinator.java",
"java/src/org/chromium/chrome/features/start_surface/ExploreSurfaceNavigationDelegate.java",
"java/src/org/chromium/chrome/features/start_surface/ExploreSurfaceStreamLifecycleManager.java",
"java/src/org/chromium/chrome/features/start_surface/ExploreSurfaceViewBinder.java",
"java/src/org/chromium/chrome/features/start_surface/FeedLoadingCoordinator.java",
"java/src/org/chromium/chrome/features/start_surface/FeedLoadingLayout.java",
]

deps += [
"//chrome/android/feed:chrome_feed_java_resources",
"//chrome/browser/profiles/android:java",
"//content/public/android:content_java",
"//third_party/android_sdk/androidx_browser:androidx_browser_java",
"//ui/android:ui_utils_java",
"//ui/base/mojom:mojom_java",
]
} else {
sources += [ "dummy/java/src/org/chromium/chrome/features/start_surface/ExploreSurfaceCoordinator.java" ]
}

if (!is_java_debug) {
if (!defined(proguard_configs)) {
proguard_configs = []
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,8 @@

import org.chromium.chrome.browser.app.ChromeActivity;
import org.chromium.chrome.browser.feed.FeedSurfaceCoordinator;
import org.chromium.chrome.browser.feed.FeedV1ActionOptions;
import org.chromium.chrome.browser.feed.StreamLifecycleManager;
import org.chromium.chrome.browser.feed.action.FeedActionHandler;
import org.chromium.chrome.browser.feed.shared.FeedFeatures;
import org.chromium.chrome.browser.feed.shared.FeedSurfaceDelegate;
import org.chromium.chrome.browser.feed.shared.stream.Stream;
Expand Down Expand Up @@ -102,7 +102,7 @@ private FeedSurfaceCoordinator internalCreateFeedSurfaceCoordinator(boolean hasH
}
}

FeedActionHandler.Options feedActionOptions = new FeedActionHandler.Options();
FeedV1ActionOptions feedActionOptions = new FeedV1ActionOptions();
feedActionOptions.inhibitDownload = true;
feedActionOptions.inhibitOpenInIncognito = true;
feedActionOptions.inhibitOpenInNewTab = true;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@
import org.chromium.chrome.browser.browser_controls.BrowserControlsStateProvider;
import org.chromium.chrome.browser.compositor.layouts.OverviewModeState;
import org.chromium.chrome.browser.feed.FeedSurfaceCoordinator;
import org.chromium.chrome.browser.feed.shared.FeedFeatures;
import org.chromium.chrome.browser.feed.shared.stream.Stream;
import org.chromium.chrome.browser.flags.CachedFeatureFlags;
import org.chromium.chrome.browser.flags.ChromeFeatureList;
Expand Down Expand Up @@ -700,7 +701,7 @@ public boolean shouldShowFeedPlaceholder() {
return mSurfaceMode == SurfaceMode.SINGLE_PANE
&& CachedFeatureFlags.isEnabled(ChromeFeatureList.INSTANT_START)
&& StartSurfaceConfiguration.getFeedArticlesVisibility()
&& !CachedFeatureFlags.isEnabled(ChromeFeatureList.INTEREST_FEED_V2);
&& !FeedFeatures.cachedIsV2Enabled();
}

/** This interface builds the feed surface coordinator when showing if needed. */
Expand Down
69 changes: 39 additions & 30 deletions chrome/android/feed/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -6,36 +6,45 @@ import("//build/config/android/rules.gni")
import("//chrome/common/features.gni")
import("//components/feed/features.gni")

if (enable_feed_in_chrome) {
android_resources("chrome_feed_java_resources") {
sources = [
"core/java/res/drawable-hdpi/ic_amp_24dp.png",
"core/java/res/drawable-mdpi/ic_amp_24dp.png",
"core/java/res/drawable-xhdpi/ic_amp_24dp.png",
"core/java/res/drawable-xxhdpi/ic_amp_24dp.png",
"core/java/res/drawable-xxxhdpi/ic_amp_24dp.png",
"core/java/res/drawable/feed_text_ripple_drawable_dark.xml",
"core/java/res/drawable/feed_text_ripple_drawable_light.xml",
"core/java/res/drawable/hairline_border_card_background_with_inset.xml",
"core/java/res/layout/feed_more_button.xml",
"core/java/res/layout/feed_simple_list_item.xml",
"core/java/res/layout/feed_spinner.xml",
"core/java/res/layout/feed_spinner_gone.xml",
"core/java/res/layout/no_connection.xml",
"core/java/res/layout/no_content.xml",
"core/java/res/layout/no_content_v2.xml",
"core/java/res/layout/zero_state.xml",
"core/java/res/values-v16/styles.xml",
"core/java/res/values-v17/styles.xml",
"core/java/res/values/attrs.xml",
"core/java/res/values/colors.xml",
"core/java/res/values/dimens.xml",
"core/java/res/values/ids.xml",
"core/java/res/values/styles.xml",
]
deps = [
"//chrome/android:chrome_app_java_resources",
"//ui/android:ui_java_resources",
android_resources("chrome_feed_java_resources") {
# TODO(crbug.com/1129187): Determine if more resources can be moved to v1-only.
sources = [
"core/java/res/drawable/feed_text_ripple_drawable_dark.xml",
"core/java/res/drawable/feed_text_ripple_drawable_light.xml",
"core/java/res/drawable/hairline_border_card_background_with_inset.xml",
"core/java/res/layout/feed_spinner.xml",
"core/java/res/layout/no_connection.xml",
"core/java/res/layout/no_content_v2.xml",
"core/java/res/values-v16/styles.xml",
"core/java/res/values-v17/styles.xml",
"core/java/res/values/attrs.xml",
"core/java/res/values/colors.xml",
"core/java/res/values/dimens.xml",
"core/java/res/values/ids.xml",
"core/java/res/values/styles.xml",
]

if (enable_feed_v1) {
# Note that we had to move v1 resources to a new directory because the
# android_resources rule ensures that you include all files in your resource
# directories.
sources += [
"core/java/resv1/drawable-hdpi/ic_amp_24dp.png",
"core/java/resv1/drawable-mdpi/ic_amp_24dp.png",
"core/java/resv1/drawable-xhdpi/ic_amp_24dp.png",
"core/java/resv1/drawable-xxhdpi/ic_amp_24dp.png",
"core/java/resv1/drawable-xxxhdpi/ic_amp_24dp.png",
"core/java/resv1/layout/feed_more_button.xml",
"core/java/resv1/layout/feed_simple_list_item.xml",
"core/java/resv1/layout/feed_spinner_gone.xml",
"core/java/resv1/layout/no_content.xml",
"core/java/resv1/layout/zero_state.xml",
]
}

deps = [
"//chrome/android:chrome_app_java_resources",
"//ui/android:ui_java_resources",
]
create_srcjar = false
}
6 changes: 2 additions & 4 deletions chrome/android/feed/DEPS
Original file line number Diff line number Diff line change
@@ -1,14 +1,12 @@
include_rules = [
"+chrome/browser/android/lifecycle",
"+chrome/browser/image_fetcher",
"+chrome/browser/profiles/android/java",
"+chrome/browser/tab/java",
"+chrome/browser/ui/messages/android/java",
"+chrome/browser/xsurface/android",
"+components/background_task_scheduler",
"+components/feature_engagement",
"+components/feed",

"-mojo", # See crbug.com/1108983
"-content",
"+content/public/android/java/src/org/chromium/content_public",
"+content/public/android/java/src/org/chromium/content_public/browser",
]
5 changes: 5 additions & 0 deletions chrome/android/feed/core/java/DEPS
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
include_rules = [
"-mojo", # See crbug.com/1108983
"-content",
"+content/public/android/java/src/org/chromium/content_public",
]
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
include_rules = [
"-chrome/android/feed/core/java/src/org/chromium/chrome/browser/feed/v1",
"-chrome/android/feed/core/java/src/org/chromium/chrome/browser/feed/v2",
"-chrome/android/feed/core/java/src/org/chromium/chrome/browser/feed/library",
]

specific_include_rules = {
"FeedV1\.java": [
"+chrome/android/feed/core/java/src/org/chromium/chrome/browser/feed/v1",
"+chrome/android/feed/core/java/src/org/chromium/chrome/browser/feed/library",
],
"FeedV2\.java": [
"+chrome/android/feed/core/java/src/org/chromium/chrome/browser/feed/v2",
],
}
Loading

0 comments on commit e274ab4

Please sign in to comment.