Also update SharedPreferences with background media capturing enabled [chromium/src : main]
0 views
Skip to first unread message
Sean Li (Gerrit)
10:49 AM (3 hours ago) 10:49 AM
to Grace Cham, chromium...@chromium.org, feature-me...@chromium.org
Sean Li has uploaded the change for review![]( "Open in Gerrit")
Commit message
Also update SharedPreferences with background media capturing enabled
We use SharedPreferences to determine whether to enable the media
notification service. Therefore, we should also update it when
background media capturing isn enabled.
Bug: 487025385, 489516324, 489653509
Test: Test notification with desktop/non-desktop builds
Change-Id: I22678a32febe7600014dc84512e123f1f60bc5e1
Change diff
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/media/MediaCaptureNotificationServiceImpl.java b/chrome/android/java/src/org/chromium/chrome/browser/media/MediaCaptureNotificationServiceImpl.java
index 060c323..7fa35910 100644
--- a/chrome/android/java/src/org/chromium/chrome/browser/media/MediaCaptureNotificationServiceImpl.java
+++ b/chrome/android/java/src/org/chromium/chrome/browser/media/MediaCaptureNotificationServiceImpl.java
@@ -152,6 +152,10 @@
if (notificationIds == null) return;
Iterator<String> iterator = notificationIds.iterator();
while (iterator.hasNext()) {
+ // When background media capturing is enabled, this operation is a no-op because a
+ // foreground service handles notification updates. We avoid calling
+ // isBackgroundMediaCapturingEnabled() here because this code may execute during early
+ // startup before the JNI library is initialized.
mNotificationManager.cancel(NOTIFICATION_NAMESPACE, Integer.parseInt(iterator.next()));
}
mSharedPreferences.removeKey(ChromePreferenceKeys.MEDIA_WEBRTC_NOTIFICATION_IDS);
@@ -246,8 +250,8 @@
// by the foreground service. If disabled, we have to cancel the notification
// manually.
mNotificationManager.cancel(NOTIFICATION_NAMESPACE, notificationId);
- updateSharedPreferencesEntry(notificationId, true);
}
+ updateSharedPreferencesEntry(notificationId, true);
}
}
@@ -327,8 +331,8 @@
mNotifications.add(new Pair<>(notificationId, notification));
} else {
mNotificationManager.notify(notification);
- updateSharedPreferencesEntry(notificationId, false);
}
+ updateSharedPreferencesEntry(notificationId, false);
NotificationUmaTracker.getInstance()
.onNotificationShown(
NotificationUmaTracker.SystemNotificationType.MEDIA_CAPTURE,
Change information
Files:
- M chrome/android/java/src/org/chromium/chrome/browser/media/MediaCaptureNotificationServiceImpl.java
Change size: XS
Delta: 1 file changed, 6 insertions(+), 2 deletions(-)
Related details
Attention set is empty
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sean Li (Gerrit)
11:02 AM (3 hours ago) 11:02 AM
to chromium...@chromium.org, feature-me...@chromium.org
Sean Li has uploaded the change for review![]( "Open in Gerrit")
Commit message
Also update SharedPreferences with background media capturing enabled
We use SharedPreferences to determine whether to enable the media
notification service. Therefore, we should also update it when
background media capturing is enabled.
Bug: 487025385, 489516324, 489653509
Test: Test notification with desktop/non-desktop builds
Change-Id: I7268f7e773c5429572870105bab57e4b0abbc1f0
Sean Li (Gerrit)
11:09 AM (3 hours ago) 11:09 AM
to Colin Blundell, Grace Cham, chromium...@chromium.org, feature-me...@chromium.org
Attention needed from Colin Blundell and Grace Cham
Sean Li added 1 comment![]( "Open in Gerrit")
Patchset-level comments
File-level comment, Patchset 2 (Latest):
Sean Li . resolved
Hi Grace and Colin,
I made a mistake, we should always update shard preference since we use it to determine whether to start service. We can just remove `isBackgroundMediaCapturingEnabled()` from `cancelPreviousWebRtcNotifications()` to avoid crash. It is ok since `mNotificationManager.cancel()` will be non-op because we use foreground service to handle notification.
Sorry again for the inconvenience.
Related details
Attention is currently required from:
- Colin Blundell
- Grace Cham
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Colin Blundell (Gerrit)
12:49 PM (1 hour ago) 12:49 PM
to Sean Li, Colin Blundell, Chromium LUCI CQ, Grace Cham, chromium...@chromium.org, feature-me...@chromium.org
Attention needed from Grace Cham and Sean Li
Colin Blundell voted and added 1 comment![]( "Open in Gerrit")
Votes added by Colin Blundell
| Code-Review | +1 |
1 comment
Patchset-level comments
Attention is currently required from:
- Grace Cham
- Sean Li
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Colin Blundell (Gerrit)
12:49 PM (1 hour ago) 12:49 PM
to Sean Li, Colin Blundell, Chromium LUCI CQ, Grace Cham, chromium...@chromium.org, feature-me...@chromium.org
Attention needed from Grace Cham and Sean Li
Colin Blundell added 1 comment![]( "Open in Gerrit")
Patchset-level comments
Colin Blundell . resolved
Also - is there anyone else who has or can grow expertise in this code? I don't really have any familiarity here, and it sounds like Grace doesn't have much either IIUC.
Reply all
Reply to author
Forward
0 new messages