[Glic] TabModel integration and unit tests [chromium/src : main]
Hailey Wang (Gerrit)
Hailey Wang added 1 comment![]( "Open in Gerrit")
Related details
- Wenyu Fu
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Wenyu Fu (Gerrit)
Wenyu Fu added 1 comment![]( "Open in Gerrit")
+Calder to help review - Im not as familiar with tab model stuff yet, but I'll also use this CL for learning :)
Related details
- Calder Kitagawa
- Hailey Wang
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Calder Kitagawa (Gerrit)
Calder Kitagawa added 4 comments![]( "Open in Gerrit")
List<Integer> getOnGoingActorTasks();Nit: `Ongoing`
case IMMEDIATE_CONTINUE: // fallthroughI assume immediate continue won't be supported? i.e. it isn't possible to disable the dialog. If that's the case, this should fall through to the not reached.
assert !allowDialog : "removeTab does not support allowDialog.";Do we need the `removeTab` change? `removeTab` is primarily for reparenting tabs so the actor task should/will continue to exist, just perhaps in a different context.
We don't throw up a dialog for these operations because the tab continues to exist and it is generally in the middle of a sensitive flow that if we crash or get interrupted inside we could end up with some weird disk state. I'd rather we not show a dialog on remove and either try to handle this gracefully or just cancel the task.
@Override
public List<Integer> getOnGoingActorTasks() {
return ActorServiceTabUtils.getOnGoingActorTasks(
mTabGroupModelFilter.getTabModel(), mTabsToUngroup);
}
Do we actually care about showing a dialog for ungroup? I think we could just not return anything here.
Related details
- Hailey Wang
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 2 comments![]( "Open in Gerrit")
List<Integer> onGoingActorTasks = handler.getOnGoingActorTasks();I guess theoretically we can have a list of tasks here, since this class handles tab closure when there are multiple tabs, right?
@Nullable ActorKeyedService actorKeyedService = getActorService();
if (actorKeyedService == null) return;
for (Integer taskId : handler.getOnGoingActorTasks()) {
actorKeyedService.stopTask(taskId, StoppedReason.STOPPED_BY_USER);
}nit: Maybe we can move this into a util method
Related details
- Hailey Wang
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. |
Calder Kitagawa (Gerrit)
Calder Kitagawa added 1 comment![]( "Open in Gerrit")
List<Integer> onGoingActorTasks = handler.getOnGoingActorTasks();I guess theoretically we can have a list of tasks here, since this class handles tab closure when there are multiple tabs, right?
Yes, it handles single or multiple tab closure.
Related details
- Hailey Wang
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. |
Hailey Wang (Gerrit)
Hailey Wang voted and added 7 comments![]( "Open in Gerrit")
Votes added by Hailey Wang
| Commit-Queue | +1 |
7 comments
List<Integer> getOnGoingActorTasks();Hailey WangNit: `Ongoing`
Done
List<Integer> onGoingActorTasks = handler.getOnGoingActorTasks();Calder KitagawaI guess theoretically we can have a list of tasks here, since this class handles tab closure when there are multiple tabs, right?
Yes, it handles single or multiple tab closure.
Done
I assume immediate continue won't be supported? i.e. it isn't possible to disable the dialog. If that's the case, this should fall through to the not reached.
it isn't possible to disable the dialog
I wasn't sure about this, but how does the other closure dialog get disabled?
Made the change to not support this case. Thanks!
@Nullable ActorKeyedService actorKeyedService = getActorService();
if (actorKeyedService == null) return;
for (Integer taskId : handler.getOnGoingActorTasks()) {
actorKeyedService.stopTask(taskId, StoppedReason.STOPPED_BY_USER);
}nit: Maybe we can move this into a util method
Done
assert !allowDialog : "removeTab does not support allowDialog.";Do we need the `removeTab` change? `removeTab` is primarily for reparenting tabs so the actor task should/will continue to exist, just perhaps in a different context.
We don't throw up a dialog for these operations because the tab continues to exist and it is generally in the middle of a sensitive flow that if we crash or get interrupted inside we could end up with some weird disk state. I'd rather we not show a dialog on remove and either try to handle this gracefully or just cancel the task.
Got it, I was under the impression that removeTab was used for other purposes than reparenting. Removing, thanks!
@Override
public List<Integer> getOnGoingActorTasks() {
return ActorServiceTabUtils.getOnGoingActorTasks(
mTabGroupModelFilter.getTabModel(), mTabsToUngroup);
}
Do we actually care about showing a dialog for ungroup? I think we could just not return anything here.
- Calder Kitagawa
- Wenyu Fu
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hailey Wang (Gerrit)
Hailey Wang voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Calder Kitagawa
- Wenyu Fu
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Calder Kitagawa (Gerrit)
Calder Kitagawa voted and added 4 comments![]( "Open in Gerrit")
Votes added by Calder Kitagawa
| Code-Review | +1 |
4 comments
case IMMEDIATE_CONTINUE: // fallthroughHailey WangI assume immediate continue won't be supported? i.e. it isn't possible to disable the dialog. If that's the case, this should fall through to the not reached.
it isn't possible to disable the dialog
I wasn't sure about this, but how does the other closure dialog get disabled?
Made the change to not support this case. Thanks!
You can set a `supportStopShowing` flag in the `ActionConfirmationManager`. If that flag is set a checkbox shows up to optionally save a shared pref that prevents the dialog from showing again. If the pref is set we do an `IMMEDIATE_CONTINUE` I see from your other CL you didn't set that so I think that excluding `IMMEDIATE_CONTINUE` in this callback is correct.
return Collections.emptyList();Add a comment that returning empty is intentional to skip the check?
return Collections.emptyList();Add a comment that returning empty is intentional to skip the check?
Related details
- Hailey Wang
- 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 Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Hailey Wang
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. |
Hailey Wang (Gerrit)
Hailey Wang voted and added 4 comments![]( "Open in Gerrit")
Votes added by Hailey Wang
| Commit-Queue | +1 |
4 comments
case IMMEDIATE_CONTINUE: // fallthroughHailey WangI assume immediate continue won't be supported? i.e. it isn't possible to disable the dialog. If that's the case, this should fall through to the not reached.
Calder Kitagawait isn't possible to disable the dialog
I wasn't sure about this, but how does the other closure dialog get disabled?
Made the change to not support this case. Thanks!
You can set a `supportStopShowing` flag in the `ActionConfirmationManager`. If that flag is set a checkbox shows up to optionally save a shared pref that prevents the dialog from showing again. If the pref is set we do an `IMMEDIATE_CONTINUE` I see from your other CL you didn't set that so I think that excluding `IMMEDIATE_CONTINUE` in this callback is correct.
Sounds good, thanks!
Add a comment that returning empty is intentional to skip the check?
Done
Add a comment that returning empty is intentional to skip the check?
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Wenyu Fu (Gerrit)
Wenyu Fu voted Code-Review+1![]( "Open in Gerrit")
- Calder Kitagawa
- Hailey Wang
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hailey Wang (Gerrit)
Hailey Wang voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
- Calder Kitagawa
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Wenyu Fu (Gerrit)
Wenyu Fu added 1 comment![]( "Open in Gerrit")
I thnk the code make sense to me but I might missed cases where it matters. Can you please get Calder's +1 as well?
Related details
- Calder Kitagawa
- Hailey Wang
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. |
Hailey Wang (Gerrit)
Hailey Wang voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |