Skip to content

Commit

Permalink
Revert "Componentize ChannelsInitializer"
Browse files Browse the repository at this point in the history
This reverts commit c71eaf3.

Reason for revert: CL breaks chrome perf builder

Original change's description:
> Componentize ChannelsInitializer
> 
> ChannelDefinitions is made into an abstract class that exposes
> functionality to ChannelsInitializer, and defines useful types like
> PredefinedChannel. The channel definitions themselves live in the
> Chrome subclass, ChromeChannelDefinitions. Some of these definitions
> will be duplicated later for WebLayer.
> 
> ChannelsInitializerTest uses ChromeChannelDefinitions so it's left
> in //chrome. (We could replace ChromeChannelDefinitions with a
> TestChannelDefinitions, but that would reduce coverage of production
> code.)
> 
> Both the channel IDs and the channel group IDs will have to be different
> for WebLayer. It would be possible to account for this in code by
> judiciously prepending "org.chromium.weblayer.", and then keeping
> the channel definitions together, but that was both more complicated
> and seemed more error prone, as it would mean that the
> ChannelDefinitions.ChannelId type would no longer represent the Android
> system notification channel identifier. For example, code that used
> NotificationManagerProxy.getNotificationChannel() would have to be
> careful *not* to pass ChannelDefinitions.ChannelId without adding a
> prefix. In Chrome, which has no prefix, this would succeed, but in
> to-be-shared code, such as NotificationCompatBuilder, this would succeed
> for Chrome and fail for WebLayer.
> 
> Bug: 1069895
> Change-Id: I55890466797aafc34ef8a821a1cb43de8e3d03ab
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2150066
> Commit-Queue: Evan Stade <estade@chromium.org>
> Auto-Submit: Evan Stade <estade@chromium.org>
> Reviewed-by: David Trainor <dtrainor@chromium.org>
> Reviewed-by: Peter Beverloo <peter@chromium.org>
> Reviewed-by: Xing Liu <xingliu@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#761922}

TBR=peter@chromium.org,dtrainor@chromium.org,estade@chromium.org,xingliu@chromium.org,knollr@chromium.org

Change-Id: I1e0af9d688d5c7a23b2977f0ef7471716c07c8ff
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 1069895
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2162771
Reviewed-by: Maksim Moskvitin <mmoskvitin@google.com>
Commit-Queue: Maksim Moskvitin <mmoskvitin@google.com>
Cr-Commit-Position: refs/heads/master@{#761950}
  • Loading branch information
Maksim Moskvitin authored and Commit Bot committed Apr 23, 2020
1 parent 38b52e4 commit eb35df1
Show file tree
Hide file tree
Showing 50 changed files with 349 additions and 424 deletions.
3 changes: 2 additions & 1 deletion chrome/android/chrome_java_sources.gni
Original file line number Diff line number Diff line change
Expand Up @@ -996,8 +996,9 @@ chrome_java_sources = [
"java/src/org/chromium/chrome/browser/notifications/NotificationUmaTracker.java",
"java/src/org/chromium/chrome/browser/notifications/PendingIntentProvider.java",
"java/src/org/chromium/chrome/browser/notifications/StandardNotificationBuilder.java",
"java/src/org/chromium/chrome/browser/notifications/channels/ChannelDefinitions.java",
"java/src/org/chromium/chrome/browser/notifications/channels/ChannelsInitializer.java",
"java/src/org/chromium/chrome/browser/notifications/channels/ChannelsUpdater.java",
"java/src/org/chromium/chrome/browser/notifications/channels/ChromeChannelDefinitions.java",
"java/src/org/chromium/chrome/browser/notifications/channels/SiteChannelsManager.java",
"java/src/org/chromium/chrome/browser/notifications/scheduler/DisplayAgent.java",
"java/src/org/chromium/chrome/browser/notifications/scheduler/NotificationSchedulerTask.java",
Expand Down
2 changes: 1 addition & 1 deletion chrome/android/chrome_junit_test_java_sources.gni
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ chrome_junit_test_java_sources = [
"junit/src/org/chromium/chrome/browser/notifications/NotificationSystemStatusUtilUnitTest.java",
"junit/src/org/chromium/chrome/browser/notifications/NotificationTriggerBackgroundTaskTest.java",
"junit/src/org/chromium/chrome/browser/notifications/NotificationTriggerSchedulerTest.java",
"junit/src/org/chromium/chrome/browser/notifications/channels/ChromeChannelDefinitionsTest.java",
"junit/src/org/chromium/chrome/browser/notifications/channels/ChannelDefinitionsTest.java",
"junit/src/org/chromium/chrome/browser/ntp/TitleUtilTest.java",
"junit/src/org/chromium/chrome/browser/ntp/cards/ContentSuggestionsUnitTestUtils.java",
"junit/src/org/chromium/chrome/browser/ntp/cards/InnerNodeTest.java",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
import org.chromium.chrome.R;
import org.chromium.chrome.browser.notifications.NotificationBuilderFactory;
import org.chromium.chrome.browser.notifications.NotificationConstants;
import org.chromium.chrome.browser.notifications.channels.ChromeChannelDefinitions;
import org.chromium.chrome.browser.notifications.channels.ChannelDefinitions;
import org.chromium.components.browser_ui.notifications.NotificationManagerProxy;
import org.chromium.components.browser_ui.notifications.NotificationManagerProxyImpl;
import org.chromium.content_public.browser.UiThreadTaskTraits;
Expand All @@ -30,8 +30,7 @@ public static void showFailureNotification(Context context) {
new NotificationManagerProxyImpl(context);
Notification notification =
NotificationBuilderFactory
.createChromeNotificationBuilder(
true, ChromeChannelDefinitions.ChannelId.VR)
.createChromeNotificationBuilder(true, ChannelDefinitions.ChannelId.VR)
.setContentTitle(context.getResources().getString(
R.string.vr_preparing_vr_notification_title))
.setContentText(context.getResources().getString(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@
import org.chromium.chrome.browser.notifications.NotificationBuilderFactory;
import org.chromium.chrome.browser.notifications.NotificationUmaTracker;
import org.chromium.chrome.browser.notifications.PendingIntentProvider;
import org.chromium.chrome.browser.notifications.channels.ChromeChannelDefinitions;
import org.chromium.chrome.browser.notifications.channels.ChannelDefinitions;
import org.chromium.components.browser_ui.notifications.ChromeNotification;
import org.chromium.components.browser_ui.notifications.NotificationManagerProxy;
import org.chromium.components.browser_ui.notifications.NotificationManagerProxyImpl;
Expand Down Expand Up @@ -129,7 +129,7 @@ private static void showNotification(String url) {
ChromeNotificationBuilder builder =
NotificationBuilderFactory
.createChromeNotificationBuilder(true /* preferCompat */,
ChromeChannelDefinitions.ChannelId.ANNOUNCEMENT,
ChannelDefinitions.ChannelId.ANNOUNCEMENT,
null /* remoteAppPackageName */,
new NotificationMetadata(
NotificationUmaTracker.SystemNotificationType.ANNOUNCEMENT,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
import android.os.Build;

import org.chromium.chrome.browser.notifications.NotificationChannelStatus;
import org.chromium.chrome.browser.notifications.channels.ChromeChannelDefinitions;
import org.chromium.chrome.browser.notifications.channels.ChannelDefinitions;
import org.chromium.chrome.browser.notifications.channels.SiteChannelsManager;
import org.chromium.components.embedder_support.util.Origin;

Expand Down Expand Up @@ -48,7 +48,7 @@ void deleteChannel(Origin origin) {
if (beforeAndroidO()) return;

String channelId = mSiteChannelsManager.getChannelIdForOrigin(origin.toString());
if (ChromeChannelDefinitions.ChannelId.SITES.equals(channelId)) {
if (ChannelDefinitions.ChannelId.SITES.equals(channelId)) {
// If we were given the generic "sites" channel that meant no origin-specific channel
// existed. We don't need to do anything.
return;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@
import org.chromium.chrome.browser.notifications.NotificationConstants;
import org.chromium.chrome.browser.notifications.NotificationUmaTracker;
import org.chromium.chrome.browser.notifications.PendingIntentProvider;
import org.chromium.chrome.browser.notifications.channels.ChromeChannelDefinitions;
import org.chromium.chrome.browser.notifications.channels.ChannelDefinitions;
import org.chromium.components.browser_ui.notifications.NotificationMetadata;
import org.chromium.components.embedder_support.util.UrlUtilities;
import org.chromium.components.offline_items_collection.ContentId;
Expand Down Expand Up @@ -77,11 +77,11 @@ public final class DownloadNotificationFactory {
public static Notification buildNotification(Context context,
@DownloadNotificationService.DownloadStatus int downloadStatus,
DownloadUpdate downloadUpdate, int notificationId) {
String channelId = ChromeChannelDefinitions.ChannelId.DOWNLOADS;
String channelId = ChannelDefinitions.ChannelId.DOWNLOADS;
if (LegacyHelpers.isLegacyDownload(downloadUpdate.getContentId())
&& downloadStatus == DownloadNotificationService.DownloadStatus.COMPLETED
&& ChromeFeatureList.isEnabled(ChromeFeatureList.DOWNLOAD_NOTIFICATION_BADGE)) {
channelId = ChromeChannelDefinitions.ChannelId.COMPLETED_DOWNLOADS;
channelId = ChannelDefinitions.ChannelId.COMPLETED_DOWNLOADS;
}
ChromeNotificationBuilder builder =
NotificationBuilderFactory
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
import org.chromium.chrome.browser.notifications.NotificationBuilderFactory;
import org.chromium.chrome.browser.notifications.NotificationConstants;
import org.chromium.chrome.browser.notifications.NotificationUmaTracker;
import org.chromium.chrome.browser.notifications.channels.ChromeChannelDefinitions;
import org.chromium.chrome.browser.notifications.channels.ChannelDefinitions;
import org.chromium.components.browser_ui.notifications.ChromeNotification;
import org.chromium.components.browser_ui.notifications.NotificationManagerProxy;
import org.chromium.components.browser_ui.notifications.NotificationManagerProxyImpl;
Expand Down Expand Up @@ -45,7 +45,7 @@ public static void showIncognitoNotification() {
ChromeNotificationBuilder builder =
NotificationBuilderFactory
.createChromeNotificationBuilder(true /* preferCompat */,
ChromeChannelDefinitions.ChannelId.INCOGNITO,
ChannelDefinitions.ChannelId.INCOGNITO,
null /* remoteAppPackageName */,
new NotificationMetadata(
NotificationUmaTracker.SystemNotificationType
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@
import org.chromium.chrome.browser.notifications.NotificationBuilderFactory;
import org.chromium.chrome.browser.notifications.NotificationUmaTracker;
import org.chromium.chrome.browser.notifications.PendingIntentProvider;
import org.chromium.chrome.browser.notifications.channels.ChromeChannelDefinitions;
import org.chromium.chrome.browser.notifications.channels.ChannelDefinitions;
import org.chromium.chrome.browser.preferences.ChromePreferenceKeys;
import org.chromium.chrome.browser.preferences.SharedPreferencesManager;
import org.chromium.chrome.browser.tab.Tab;
Expand Down Expand Up @@ -185,8 +185,8 @@ private static boolean isRunningAtLeastN() {
private void createNotification(
int notificationId, @MediaType int mediaType, String url, boolean isIncognito) {
final String channelId = mediaType == MediaType.SCREEN_CAPTURE
? ChromeChannelDefinitions.ChannelId.SCREEN_CAPTURE
: ChromeChannelDefinitions.ChannelId.MEDIA;
? ChannelDefinitions.ChannelId.SCREEN_CAPTURE
: ChannelDefinitions.ChannelId.MEDIA;

ChromeNotificationBuilder builder =
NotificationBuilderFactory
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@
import org.chromium.chrome.browser.notifications.NotificationBuilderFactory;
import org.chromium.chrome.browser.notifications.NotificationConstants;
import org.chromium.chrome.browser.notifications.NotificationUmaTracker;
import org.chromium.chrome.browser.notifications.channels.ChromeChannelDefinitions;
import org.chromium.chrome.browser.notifications.channels.ChannelDefinitions;
import org.chromium.components.browser_ui.notifications.ChromeNotification;
import org.chromium.components.browser_ui.notifications.NotificationManagerProxy;
import org.chromium.components.browser_ui.notifications.NotificationManagerProxyImpl;
Expand Down Expand Up @@ -296,7 +296,7 @@ private static void finishStartingForegroundService(ListenerService s) {
null /* notificationTag */, s.getNotificationId());
ChromeNotificationBuilder builder =
NotificationBuilderFactory.createChromeNotificationBuilder(true /* preferCompat */,
ChromeChannelDefinitions.ChannelId.MEDIA, null /* remoteAppPackageName */,
ChannelDefinitions.ChannelId.MEDIA, null /* remoteAppPackageName */,
metadata);
ForegroundServiceUtils.getInstance().startForeground(s, s.getNotificationId(),
builder.buildChromeNotification().getNotification(), 0 /* foregroundServiceType */);
Expand Down Expand Up @@ -988,7 +988,7 @@ void updateNotificationBuilder() {
new NotificationMetadata(NotificationUmaTracker.SystemNotificationType.MEDIA,
null /* notificationTag */, mMediaNotificationInfo.id);
mNotificationBuilder = NotificationBuilderFactory.createChromeNotificationBuilder(
true /* preferCompat */, ChromeChannelDefinitions.ChannelId.MEDIA,
true /* preferCompat */, ChannelDefinitions.ChannelId.MEDIA,
null /* remoteAppPackageName*/, metadata);
setMediaStyleLayoutForNotificationBuilder(mNotificationBuilder);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,9 @@
import android.support.v4.media.session.MediaSessionCompat;
import android.widget.RemoteViews;

import org.chromium.chrome.browser.notifications.channels.ChannelsInitializer;
import org.chromium.components.browser_ui.notifications.ChromeNotification;
import org.chromium.components.browser_ui.notifications.NotificationMetadata;
import org.chromium.components.browser_ui.notifications.channels.ChannelsInitializer;

/**
* Wraps a Notification.Builder object.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,9 @@

import org.chromium.base.ContextUtils;
import org.chromium.chrome.browser.flags.ChromeFeatureList;
import org.chromium.chrome.browser.notifications.channels.ChromeChannelDefinitions;
import org.chromium.chrome.browser.notifications.channels.ChannelsInitializer;
import org.chromium.components.browser_ui.notifications.NotificationManagerProxyImpl;
import org.chromium.components.browser_ui.notifications.NotificationMetadata;
import org.chromium.components.browser_ui.notifications.channels.ChannelsInitializer;

/**
* Factory which supplies the appropriate type of notification builder based on Android version.
Expand Down Expand Up @@ -72,8 +71,8 @@ public static ChromeNotificationBuilder createChromeNotificationBuilder(boolean
NotificationManagerProxyImpl notificationManagerProxy =
new NotificationManagerProxyImpl(context);

ChannelsInitializer channelsInitializer = new ChannelsInitializer(notificationManagerProxy,
ChromeChannelDefinitions.getInstance(), context.getResources());
ChannelsInitializer channelsInitializer =
new ChannelsInitializer(notificationManagerProxy, context.getResources());

return preferCompat
? new NotificationCompatBuilder(context, channelId, channelsInitializer, metadata)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,9 @@
import androidx.core.app.NotificationCompat;

import org.chromium.base.Log;
import org.chromium.chrome.browser.notifications.channels.ChannelsInitializer;
import org.chromium.components.browser_ui.notifications.ChromeNotification;
import org.chromium.components.browser_ui.notifications.NotificationMetadata;
import org.chromium.components.browser_ui.notifications.channels.ChannelsInitializer;

/**
* Wraps a NotificationCompat.Builder object.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
import org.chromium.chrome.browser.ChromeApplication;
import org.chromium.chrome.browser.browserservices.TrustedWebActivityClient;
import org.chromium.chrome.browser.init.ChromeBrowserInitializer;
import org.chromium.chrome.browser.notifications.channels.ChannelDefinitions;
import org.chromium.chrome.browser.notifications.channels.SiteChannelsManager;
import org.chromium.chrome.browser.preferences.Pref;
import org.chromium.chrome.browser.preferences.PrefServiceBridge;
Expand Down Expand Up @@ -411,7 +412,8 @@ public static String getOriginFromNotificationTag(@Nullable String tag) {
@Nullable
@VisibleForTesting
static String getOriginFromChannelId(@Nullable String channelId) {
if (channelId == null || !SiteChannelsManager.isValidSiteChannelId(channelId)) {
if (channelId == null
|| !channelId.startsWith(ChannelDefinitions.CHANNEL_ID_PREFIX_SITES)) {
return null;
}
return SiteChannelsManager.toSiteOrigin(channelId);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
import android.os.Build;

import org.chromium.base.annotations.CalledByNative;
import org.chromium.chrome.browser.notifications.channels.ChromeChannelDefinitions;
import org.chromium.chrome.browser.notifications.channels.ChannelDefinitions;
import org.chromium.chrome.browser.notifications.channels.SiteChannelsManager;
import org.chromium.components.url_formatter.SchemeDisplay;
import org.chromium.components.url_formatter.UrlFormatter;
Expand Down Expand Up @@ -98,7 +98,7 @@ public NotificationChannel toChannel() {
mStatus == NotificationChannelStatus.BLOCKED
? NotificationManager.IMPORTANCE_NONE
: NotificationManager.IMPORTANCE_DEFAULT);
channel.setGroup(ChromeChannelDefinitions.ChannelGroupId.SITES);
channel.setGroup(ChannelDefinitions.ChannelGroupId.SITES);
return channel;
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
import org.chromium.base.MathUtils;
import org.chromium.base.library_loader.LibraryLoader;
import org.chromium.base.metrics.RecordHistogram;
import org.chromium.chrome.browser.notifications.channels.ChromeChannelDefinitions;
import org.chromium.chrome.browser.notifications.channels.ChannelDefinitions;
import org.chromium.chrome.browser.preferences.ChromePreferenceKeys;
import org.chromium.chrome.browser.preferences.SharedPreferencesManager;

Expand Down Expand Up @@ -269,8 +269,8 @@ public static void onNotificationFailedToCreate(@SystemNotificationType int type
recordHistogram("Mobile.SystemNotification.CreationFailure", type);
}

private void logNotificationShown(@SystemNotificationType int type,
@ChromeChannelDefinitions.ChannelId String channelId) {
private void logNotificationShown(
@SystemNotificationType int type, @ChannelDefinitions.ChannelId String channelId) {
if (!mNotificationManager.areNotificationsEnabled()) {
logPotentialBlockedCause();
recordHistogram("Mobile.SystemNotification.Blocked", type);
Expand All @@ -286,7 +286,7 @@ && isChannelBlocked(channelId)) {
}

@TargetApi(26)
private boolean isChannelBlocked(@ChromeChannelDefinitions.ChannelId String channelId) {
private boolean isChannelBlocked(@ChannelDefinitions.ChannelId String channelId) {
// Use non-compat notification manager as compat does not have getNotificationChannel (yet).
NotificationManager notificationManager =
ContextUtils.getApplicationContext().getSystemService(NotificationManager.class);
Expand Down
Loading

0 comments on commit eb35df1

Please sign in to comment.