[Glic] Make notifications silent only if we are not in PiP or in CTA [chromium/src : main]
Bhuvana Betini (Gerrit)
Bhuvana Betini added 5 comments![]( "Open in Gerrit")
[Glic] Make notifications silent Only if we are not in PiP or in CTABhuvana Betininit: %s/Only/only
Done
* @return True if ChromeTabbedActivity is currently visible (Resumed, Paused or Started).Bhuvana Betininit : and is in foreground.
Done
if (!activity.getClass()
.getName()
.equals("org.chromium.chrome.browser.ChromeTabbedActivity")) {Bhuvana BetiniThis is bad in a lot of reasons (e.g. unreliable, and multi-instances) :/
I also don't know if there's a good place to check this as a static method. I think we should make the caller of this method accept an activity object, and check if we should make this silence. Maybe having `isSilent` as a input to `buildNotification`. It will still require decent amount of wiring, though.
Addressed this by restructuring a bit
- moved the visibility and silence logic out of the static ActorNotificationFactory and into a non-static implementation (ActorForegroundServiceControllerImpl).
- uses TabModelSelectorSupplier to verify if the specific tabs associated with a task are actually present in the current activity instance to address multi-instance concerns
if (!activity.getClass()
.getName()
.equals("org.chromium.chrome.browser.ChromeTabbedActivity")) {
return false;
}Bhuvana Betininit : instanceof should be better here?
if (!(activity instanceof ChromeTabbedActivity)) {
return false;
}
Done
public void testIsChromeTabbedActivityVisible_NoActivities() {Bhuvana BetiniPlease fix failing test please.
Done
Related details
- Ritika Gupta
- Wenyu Fu
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Wenyu Fu (Gerrit)
Wenyu Fu added 11 comments![]( "Open in Gerrit")
for (Activity activity : ApplicationStatus.getRunningActivities()) {This might be ok assuming we won't have too many activty, but I still don't like the fact we have to loop through all activities.
Any chance we can just use a `ActivityStateListener` here? https://source.chromium.org/chromium/chromium/src/+/main:base/android/java/src/org/chromium/base/ApplicationStatus.java;l=154?q=-file:%5Eout%2F%20-file:%5Ethird_party%2F%20ApplicationStatus&ss=chromium%2Fchromium%2Fsrc
if (taskTabs.isEmpty()) {This seems not needed to live in the loop?
return true;I'm not sure how to link `taskTabs.isEmpty()` with `isActivityVisibleForTask = true`... Is this an inexplicit assumption that if there's no tab associate with task, task is finishing, so we can make the notification silence?
when(mChromeActivity.getTabModelSelector()).thenReturn(mTabModelSelector);This line doesn't appear to do anything, as the implementation under test uses `TabModelSelectorSupplier.getValueOrNullFrom()`. The test works due to `TabModelSelectorSupplier.setInstanceForTesting()` in `setUp()`.
To avoid confusion, it might be best to remove this line (and the similar one in `testIsActivityVisibleForTask_WithTabs_NoSilenceWhenTabNotInActivity`).
boolean isSilent = task != null && mServiceController.isActivityVisibleForTask(task);Need to think about this - if task is null, should we make it silent? Is `task == null` here a valid case?
.setSilent(isSilent);Sorry didn't flag in earlier review, but I think we should only make a subset of notification type non-silence.
ACTING / REFLECTING / PAUSED_BY_USER does not feel they should be non-silence; it is only the ones that needs user attention.
if (notification != null && notification.isSilent() != isSilent) {Im unsure about this... I think for a task, it's possible
public @Nullable ActorTask getTask(int taskId) {nit: Seems can be package private
ActorTask task = mKeyedService.getTask(taskId);Intuitively I thought we read from cache first before checking the service. Any reason we are going with the reverse order?
If it is indeed needed, a comment around would be helpful.
public class NotificationWrapperStandardBuilder implements NotificationWrapperBuilder {In fact, I can't seem to find usage of this class, maybe we can remove it
mIsSilent = silent;Since it's not supported, we should remove this so mIsSilent is always false.
Related details
- Bhuvana Betini
- Ritika Gupta
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Bhuvana Betini (Gerrit)
Bhuvana Betini added 11 comments![]( "Open in Gerrit")
for (Activity activity : ApplicationStatus.getRunningActivities()) {This might be ok assuming we won't have too many activty, but I still don't like the fact we have to loop through all activities.
Any chance we can just use a `ActivityStateListener` here? https://source.chromium.org/chromium/chromium/src/+/main:base/android/java/src/org/chromium/base/ApplicationStatus.java;l=154?q=-file:%5Eout%2F%20-file:%5Ethird_party%2F%20ApplicationStatus&ss=chromium%2Fchromium%2Fsrc
Done
This seems not needed to live in the loop?
Done
return true;I'm not sure how to link `taskTabs.isEmpty()` with `isActivityVisibleForTask = true`... Is this an inexplicit assumption that if there's no tab associate with task, task is finishing, so we can make the notification silence?
Yes, I was finding this to be an issue with manual testing. It seems to create a race condition between taskTabs and the task itself when finishing.
when(mChromeActivity.getTabModelSelector()).thenReturn(mTabModelSelector);This line doesn't appear to do anything, as the implementation under test uses `TabModelSelectorSupplier.getValueOrNullFrom()`. The test works due to `TabModelSelectorSupplier.setInstanceForTesting()` in `setUp()`.
To avoid confusion, it might be best to remove this line (and the similar one in `testIsActivityVisibleForTask_WithTabs_NoSilenceWhenTabNotInActivity`).
Done
boolean isSilent = task != null && mServiceController.isActivityVisibleForTask(task);Need to think about this - if task is null, should we make it silent? Is `task == null` here a valid case?
If task is null, we still proceed to updateNotificationForTask because that method handles the 'safe cleanup' (cancelling the notification and clearing internal data structures) for tasks that are no longer found in native or the cache.
Sorry didn't flag in earlier review, but I think we should only make a subset of notification type non-silence.
ACTING / REFLECTING / PAUSED_BY_USER does not feel they should be non-silence; it is only the ones that needs user attention.
Done
if (notification != null && notification.isSilent() != isSilent) {Im unsure about this... I think for a task, it's possible
Sorry, can you clarify which part you mean?
nit: Seems can be package private
Done
Intuitively I thought we read from cache first before checking the service. Any reason we are going with the reverse order?
If it is indeed needed, a comment around would be helpful.
Done
public class NotificationWrapperStandardBuilder implements NotificationWrapperBuilder {In fact, I can't seem to find usage of this class, maybe we can remove it
Done
Since it's not supported, we should remove this so mIsSilent is always false.
- Ritika Gupta
- Wenyu Fu
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Wenyu Fu (Gerrit)
Wenyu Fu added 3 comments![]( "Open in Gerrit")
this::onActivityStateChange);nit: Use a member variable to store this. We need to remove it when service unbind.
mVisibleActivities.add(asyncActivity);This feels scary we are keeping a list of activity reference (which can very much cause memory leaks).
When this method is called, can we just update a boolean `shouldSilenceNotification(task, activity)`? (this means we store ActorTask in this class too - maybe we do that using a weak reference)
if (notification != null && notification.isSilent() != isSilent) {Bhuvana BetiniIm unsure about this... I think for a task, it's possible
Sorry, can you clarify which part you mean?
Marked as resolved. I don't remember what I intend to say. Sorry for the churn
Related details
- Bhuvana Betini
- Ritika Gupta
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ritika Gupta (Gerrit)
Ritika Gupta added 4 comments![]( "Open in Gerrit")
public ActorForegroundServiceControllerImpl() {Why do we need to keep track of all the activities here? I dont think we should keep track of all the activities.
if (!(activity instanceof AsyncInitializationActivity asyncActivity)) return;Does this need to be AsyncInitializationActivity, can we check ChromeTabbedActivity here? @wen...@chromium.org
return true;Bhuvana BetiniI'm not sure how to link `taskTabs.isEmpty()` with `isActivityVisibleForTask = true`... Is this an inexplicit assumption that if there's no tab associate with task, task is finishing, so we can make the notification silence?
Yes, I was finding this to be an issue with manual testing. It seems to create a race condition between taskTabs and the task itself when finishing.
We figured out yesterday that as soon as task is completed/finished all the tabs related to actor task are cleaned up.
public boolean isActivityVisibleForTask(ActorTask task) {
ThreadUtils.assertOnUiThread();
Set<Integer> taskTabs = task.getTabs();
boolean hasVisibleActivityNotInPip = false;
for (AsyncInitializationActivity asyncActivity : mVisibleActivities) {
if (asyncActivity.isInPictureInPictureMode()) continue;
hasVisibleActivityNotInPip = true;
if (!taskTabs.isEmpty()) {
TabModelSelector selector =
TabModelSelectorSupplier.getValueOrNullFrom(
asyncActivity.getWindowAndroid());
if (selector != null) {
for (int tabId : taskTabs) {
if (selector.getTabById(tabId) != null) return true;
}
}
}
}
return taskTabs.isEmpty() && hasVisibleActivityNotInPip;
}
I think all we are trying to do here is that we are not in any of the tabs related to task.
I think all we should do in this class should be : Check if the current activity is an instance of CTA and if check if selector has tabs related to actor.
Related details
- Bhuvana Betini
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Bhuvana Betini (Gerrit)
Bhuvana Betini added 5 comments![]( "Open in Gerrit")
Why do we need to keep track of all the activities here? I dont think we should keep track of all the activities.
Acknowledged
nit: Use a member variable to store this. We need to remove it when service unbind.
Implemented fix Ritika mentioned so this should no longer be relevant, I believe.
if (!(activity instanceof AsyncInitializationActivity asyncActivity)) return;Does this need to be AsyncInitializationActivity, can we check ChromeTabbedActivity here? @wen...@chromium.org
CTA import is banned in chrome/android
This feels scary we are keeping a list of activity reference (which can very much cause memory leaks).
When this method is called, can we just update a boolean `shouldSilenceNotification(task, activity)`? (this means we store ActorTask in this class too - maybe we do that using a weak reference)
Implemented alternate fix :)
public boolean isActivityVisibleForTask(ActorTask task) {
ThreadUtils.assertOnUiThread();
Set<Integer> taskTabs = task.getTabs();
boolean hasVisibleActivityNotInPip = false;
for (AsyncInitializationActivity asyncActivity : mVisibleActivities) {
if (asyncActivity.isInPictureInPictureMode()) continue;
hasVisibleActivityNotInPip = true;
if (!taskTabs.isEmpty()) {
TabModelSelector selector =
TabModelSelectorSupplier.getValueOrNullFrom(
asyncActivity.getWindowAndroid());
if (selector != null) {
for (int tabId : taskTabs) {
if (selector.getTabById(tabId) != null) return true;
}
}
}
}
return taskTabs.isEmpty() && hasVisibleActivityNotInPip;
}
I think all we are trying to do here is that we are not in any of the tabs related to task.
I think all we should do in this class should be : Check if the current activity is an instance of CTA and if check if selector has tabs related to actor.
- Ritika Gupta
- Wenyu Fu
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Wenyu Fu (Gerrit)
Wenyu Fu voted and added 7 comments![]( "Open in Gerrit")
Votes added by Wenyu Fu
| Code-Review | +1 |
7 comments
public ActorForegroundServiceControllerImpl() {}nit: Seems redundant?
// An activity is considered visible for a task if it is a ChromeTabbedActivity that is not
// in PiP. Since we cannot directly reference ChromeTabbedActivity, we use theI don't like that this method adds the assumption of PiP :/ Might be ok because this comment, but we might want to rename this to something else
return true;Bhuvana BetiniI'm not sure how to link `taskTabs.isEmpty()` with `isActivityVisibleForTask = true`... Is this an inexplicit assumption that if there's no tab associate with task, task is finishing, so we can make the notification silence?
Ritika GuptaYes, I was finding this to be an issue with manual testing. It seems to create a race condition between taskTabs and the task itself when finishing.
We figured out yesterday that as soon as task is completed/finished all the tabs related to actor task are cleaned up.
In this case, it means this is a new notification sent while task is done. Should we send a non-silent notifications?
}
}
}
}
nit: Use early return to reduce indentation
boolean isSilent = task != null && mServiceController.isActivityVisibleForTask(task);Bhuvana BetiniNeed to think about this - if task is null, should we make it silent? Is `task == null` here a valid case?
If task is null, we still proceed to updateNotificationForTask because that method handles the 'safe cleanup' (cancelling the notification and clearing internal data structures) for tasks that are no longer found in native or the cache.
Acknowledged- thanks!
if (!activity.getClass()
.getName()
.equals("org.chromium.chrome.browser.ChromeTabbedActivity")) {This is bad in a lot of reasons (e.g. unreliable, and multi-instances) :/
I also don't know if there's a good place to check this as a static method. I think we should make the caller of this method accept an activity object, and check if we should make this silence. Maybe having `isSilent` as a input to `buildNotification`. It will still require decent amount of wiring, though.
Addressed this by restructuring a bit
- moved the visibility and silence logic out of the static ActorNotificationFactory and into a non-static implementation (ActorForegroundServiceControllerImpl).
- uses TabModelSelectorSupplier to verify if the specific tabs associated with a task are actually present in the current activity instance to address multi-instance concerns
- Bhuvana Betini
- Ritika Gupta
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Bhuvana Betini (Gerrit)
Bhuvana Betini added 4 comments![]( "Open in Gerrit")
public ActorForegroundServiceControllerImpl() {}Bhuvana Betininit: Seems redundant?
Done
if (!(activity instanceof AsyncInitializationActivity asyncActivity)) return;Bhuvana BetiniDoes this need to be AsyncInitializationActivity, can we check ChromeTabbedActivity here? @wen...@chromium.org
CTA import is banned in chrome/android
Marking as resolved.
return true;Bhuvana BetiniI'm not sure how to link `taskTabs.isEmpty()` with `isActivityVisibleForTask = true`... Is this an inexplicit assumption that if there's no tab associate with task, task is finishing, so we can make the notification silence?
Ritika GuptaYes, I was finding this to be an issue with manual testing. It seems to create a race condition between taskTabs and the task itself when finishing.
Wenyu FuWe figured out yesterday that as soon as task is completed/finished all the tabs related to actor task are cleaned up.
In this case, it means this is a new notification sent while task is done. Should we send a non-silent notifications?
Oh, thanks for catching that. Making it return false!
nit: Use early return to reduce indentation
- Ritika Gupta
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ritika Gupta (Gerrit)
Ritika Gupta added 14 comments![]( "Open in Gerrit")
I am going to send more comments,
for notificationService and ActorFGSManager.
if (!(activity instanceof AsyncInitializationActivity aia)) return false;aia looks strange lets call the variable asyncInitActivity.
ApplicationStatus.onStateChangeForTesting(mChromeActivity, ActivityState.CREATED);
ApplicationStatus.onStateChangeForTesting(mChromeActivity, ActivityState.RESUMED);Comment for overall tests methods for new behaviour.
Just to check, do we need both created and resumed states for the activity everywhere? I think setting the final state should be enough.
May be add test for Settings activity and see it is not silent.
* Returns true if there is a visible Chrome activity that is currently showing one of the tabs
* the given task is acting on.nit : that has one of the tabs, the given task is acting on.
boolean isSilent = task != null && mServiceController.isActivityVisibleForTask(task);Could we have a method in this class, saying:
```
public boolean isActivityVisibleForTask(int taskId){
ActorTask task = mNotificationService.getTask(taskId);
boolean isSilent = task != null && mServiceController.isActivityVisible(task.getTabs());
}
```
mServiceController doesnt need to know about the actor task.
ActorTask task = mNotificationService.getTask(taskId);
boolean isSilent = task != null && mServiceController.isActivityVisibleForTask(task);
mNotificationService.updateNotificationForTask(taskId, newState, isSilent);Simplifies to:
```
mNotificationService.updateNotificationForTask(taskId, newState, isActivityVisibleForTask(taskId));
```
Now even when we call updateNotificationForTask, we need to ensure that if the category of notification is same, we shouldnt popup notification.
ActorTask task = mNotificationService.getTask(mPinnedNotificationId);
boolean isSilent =
task != null && mServiceController.isActivityVisibleForTask(task);same as above
if (state == ActorTaskState.ACTING || state == ActorTaskState.REFLECTING) {add state == ActorTaskState.CREATED here.
if (state == ActorTaskState.ACTING || state == ActorTaskState.REFLECTING) {add state == ActorTaskState.CREATED here.
private static final String TAG = "ActNotification";nit : ActorNotification
unrelated to CL
* @param isSilent Whether the notification should be silent if it needs to be built.nit : or popup.
* @param isSilent Whether the notification should be silent if it needs to be built.or popup
if (notification != null && notification.isSilent() != isSilent) {
notification = null;
}what does this mean? that if cache have a notification but it is silent whereas it should be non-silent that set is null, so that the next block could execute.
This code logic is hard to read.
@Nullable ActorTask getTask(int taskId) {
ActorTask task = mTaskCache.get(taskId);
if (task == null) {
task = mKeyedService.getTask(taskId);
if (task != null) {
mTaskCache.put(taskId, task);
}
}
return task;
}Nit or directly at the calling site
```
@Nullable ActorTask getTask(int taskId) {
return mTaskCache.computeIfAbsent(taskId, mKeyedService::getTask);
}
```
Related details
- Bhuvana Betini
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ritika Gupta (Gerrit)
Ritika Gupta added 1 comment![]( "Open in Gerrit")
if (notification != null && notification.isSilent() != isSilent) {we shouldn't update the notification only because it differs from previous state in terms of silence.
Overall logic should be here as well :
If notification category is same, we shouldn't update the notification.
Also we somehow need some refactoring,
Like see in notification cache, task state cache and have a single method to simplify the logic for both.
and use it both for updateNotificationForTask and in getCachedNotification.
Bhuvana Betini (Gerrit)
Bhuvana Betini added 15 comments![]( "Open in Gerrit")
I am going to send more comments,
for notificationService and ActorFGSManager.
Acknowledged
if (!(activity instanceof AsyncInitializationActivity aia)) return false;aia looks strange lets call the variable asyncInitActivity.
Done
ApplicationStatus.onStateChangeForTesting(mChromeActivity, ActivityState.CREATED);
ApplicationStatus.onStateChangeForTesting(mChromeActivity, ActivityState.RESUMED);Comment for overall tests methods for new behaviour.
Just to check, do we need both created and resumed states for the activity everywhere? I think setting the final state should be enough.
May be add test for Settings activity and see it is not silent.
It seems that we do need to set CREATED first, so I kept only that state.
Test added.
* Returns true if there is a visible Chrome activity that is currently showing one of the tabs
* the given task is acting on.nit : that has one of the tabs, the given task is acting on.
Done
boolean isSilent = task != null && mServiceController.isActivityVisibleForTask(task);Could we have a method in this class, saying:
```
public boolean isActivityVisibleForTask(int taskId){
ActorTask task = mNotificationService.getTask(taskId);
boolean isSilent = task != null && mServiceController.isActivityVisible(task.getTabs());
}
```
mServiceController doesnt need to know about the actor task.
Done
ActorTask task = mNotificationService.getTask(taskId);
boolean isSilent = task != null && mServiceController.isActivityVisibleForTask(task);
mNotificationService.updateNotificationForTask(taskId, newState, isSilent);Simplifies to:
```
mNotificationService.updateNotificationForTask(taskId, newState, isActivityVisibleForTask(taskId));
```Now even when we call updateNotificationForTask, we need to ensure that if the category of notification is same, we shouldnt popup notification.
Done
ActorTask task = mNotificationService.getTask(mPinnedNotificationId);
boolean isSilent =
task != null && mServiceController.isActivityVisibleForTask(task);Bhuvana Betinisame as above
Done
if (state == ActorTaskState.ACTING || state == ActorTaskState.REFLECTING) {add state == ActorTaskState.CREATED here.
Done
if (state == ActorTaskState.ACTING || state == ActorTaskState.REFLECTING) {add state == ActorTaskState.CREATED here.
Done
nit : ActorNotification
unrelated to CL
Done
* @param isSilent Whether the notification should be silent if it needs to be built.Bhuvana Betininit : or popup.
Done
* @param isSilent Whether the notification should be silent if it needs to be built.Bhuvana Betinior popup
Done
if (notification != null && notification.isSilent() != isSilent) {we shouldn't update the notification only because it differs from previous state in terms of silence.
Overall logic should be here as well :If notification category is same, we shouldn't update the notification.
Also we somehow need some refactoring,Like see in notification cache, task state cache and have a single method to simplify the logic for both.
and use it both for updateNotificationForTask and in getCachedNotification.
Done
if (notification != null && notification.isSilent() != isSilent) {
notification = null;
}what does this mean? that if cache have a notification but it is silent whereas it should be non-silent that set is null, so that the next block could execute.
This code logic is hard to read.
Done
@Nullable ActorTask getTask(int taskId) {
ActorTask task = mTaskCache.get(taskId);
if (task == null) {
task = mKeyedService.getTask(taskId);
if (task != null) {
mTaskCache.put(taskId, task);
}
}
return task;
}Nit or directly at the calling site
```
@Nullable ActorTask getTask(int taskId) {
return mTaskCache.computeIfAbsent(taskId, mKeyedService::getTask);
}
```
- Ritika Gupta
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ritika Gupta (Gerrit)
Ritika Gupta added 2 comments![]( "Open in Gerrit")
NotificationWrapper cached = mNotificationCache.get(taskId);nit : cachedNotification
Related details
- Bhuvana Betini
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ritika Gupta (Gerrit)
Ritika Gupta voted and added 1 comment![]( "Open in Gerrit")
Votes added by Ritika Gupta
| Code-Review | +1 |
1 comment
Thanks, nits + fix tests. LGTM overall.
Please check the behaviour manually in all cases.
Bhuvana Betini (Gerrit)
Bhuvana Betini added 2 comments![]( "Open in Gerrit")
Thanks, nits + fix tests. LGTM overall.
Please check the behaviour manually in all cases.
Acknowledged
NotificationWrapper cached = mNotificationCache.get(taskId);Bhuvana Betininit : cachedNotification
Done
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Bhuvana Betini (Gerrit)
Bhuvana Betini voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
13 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: chrome/browser/actor/android/java/src/org/chromium/chrome/browser/actor/ActorNotificationService.java
Insertions: 22, Deletions: 5.
@@ -73,6 +73,13 @@
}
Integer oldState = mTaskStates.get(taskId);
+
+ // If the task appears to have regressed to CREATED from a terminal state, it's likely
+ // because the native task was destroyed. In this case, we trust our cached state.
+ if (oldState != null && isTerminalState(oldState) && newState == ActorTaskState.CREATED) {
+ newState = oldState;
+ }
+
if (oldState != null
&& !ActorNotificationFactory.shouldUpdateNotification(oldState, newState)) {
mTaskStates.put(taskId, newState);
@@ -104,17 +111,27 @@
if (task == null) return null;
@ActorTaskState int state = task.getState();
- NotificationWrapper cached = mNotificationCache.get(taskId);
+ NotificationWrapper cachedNotification = mNotificationCache.get(taskId);
Integer oldState = mTaskStates.get(taskId);
- if (cached == null
+ if (oldState != null && isTerminalState(oldState) && state == ActorTaskState.CREATED) {
+ state = oldState;
+ }
+
+ if (cachedNotification == null
|| oldState == null
|| ActorNotificationFactory.shouldUpdateNotification(oldState, state)) {
- cached = ActorNotificationFactory.buildNotification(task, state, isSilent);
- mNotificationCache.put(taskId, cached);
+ cachedNotification = ActorNotificationFactory.buildNotification(task, state, isSilent);
+ mNotificationCache.put(taskId, cachedNotification);
mTaskStates.put(taskId, state);
}
- return cached;
+ return cachedNotification;
+ }
+
+ private boolean isTerminalState(@ActorTaskState int state) {
+ return state == ActorTaskState.FINISHED
+ || state == ActorTaskState.FAILED
+ || state == ActorTaskState.CANCELLED;
}
/**
```
Change information
[Glic] Make notifications silent only if we are not in PiP or in CTA
- M chrome/android/java/src/org/chromium/chrome/browser/actor/ActorForegroundServiceControllerImpl.java
- M chrome/android/junit/src/org/chromium/chrome/browser/actor/ActorForegroundServiceControllerImplTest.java
- M chrome/browser/actor/android/java/src/org/chromium/chrome/browser/actor/ActorForegroundServiceController.java
- M chrome/browser/actor/android/java/src/org/chromium/chrome/browser/actor/ActorForegroundServiceManager.java
- M chrome/browser/actor/android/java/src/org/chromium/chrome/browser/actor/ActorForegroundServiceManagerTest.java
- M chrome/browser/actor/android/java/src/org/chromium/chrome/browser/actor/ActorNotificationFactory.java
- M chrome/browser/actor/android/java/src/org/chromium/chrome/browser/actor/ActorNotificationFactoryTest.java
- M chrome/browser/actor/android/java/src/org/chromium/chrome/browser/actor/ActorNotificationService.java
- M chrome/browser/actor/android/java/src/org/chromium/chrome/browser/actor/ActorNotificationServiceTest.java
- M chrome/browser/actor/android/java/src/org/chromium/chrome/browser/actor/NoOpActorForegroundServiceController.java
Code-Review: +1 by Wenyu Fu, +1 by Ritika Gupta
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |