[STTS] Add direct-send pipeline for Send Tab to Self [chromium/src : main]
Marc Treib (Gerrit)
Marc Treib added 4 comments![]( "Open in Gerrit")
[_sendTabToSelfCoordinator sendTabToTargetDeviceCacheGUID:cacheGUIDThe coordinator is a UI concept, so going through it if we're not actually showing any UI seems a bit weird. But maybe showing the confirmation toast is "enough UI" to warrant going through the coordinator?
- (void)sendTabToTargetDeviceCacheGUID:(NSString*)cacheGUIDExposing this feels odd. AFAIK usually the pattern of using a coordinator is: Create it via init..., then call start.
So maybe instead of exposing this, we should have another init variant, and then call start to actually kick off the process?
deviceName:(NSString*)deviceName {
// Avoid performing any UI updates or delegate callbacks if the coordinator
// has already been stopped.
if (self.stopped) {
return;
}Is this change related to the direct-send flow somehow?
- (void)sendTabToSelfWithoutTargetPickerUI:(const GURL&)urlThis is a bit of an odd name - can we find a name that describes what this does, rather than what it does not do? Something like `sendTabToSelfToSpecificDevice`?
Related details
- Michael Tatarski
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. |
Michael Tatarski (Gerrit)
Michael Tatarski added 5 comments![]( "Open in Gerrit")
[_sendTabToSelfCoordinator sendTabToTargetDeviceCacheGUID:cacheGUIDThe coordinator is a UI concept, so going through it if we're not actually showing any UI seems a bit weird. But maybe showing the confirmation toast is "enough UI" to warrant going through the coordinator?
Yeah, that's a fair point. Coordinators are usually UI-only concepts. However, going through SendTabToSelfCoordinator here is the cleanest way IMHO.
It prevents us from bloating the fairly generic `BrowserCoordinator` with feature-specific logic like sync model listeners, error handling, and toast triggering.
The toast can be considered the UI representation for this command, and keeping it encapsulated in the coordinator keeps everything modular.
- (void)sendTabToTargetDeviceCacheGUID:(NSString*)cacheGUIDExposing this feels odd. AFAIK usually the pattern of using a coordinator is: Create it via init..., then call start.
So maybe instead of exposing this, we should have another init variant, and then call start to actually kick off the process?
Good point. I will refactor the coordinator to expose a new initializer that accepts the target device's cacheGUID and deviceName. If a target device is specified, the coordinator will execute the direct-send command.
deviceName:(NSString*)deviceName {
// Avoid performing any UI updates or delegate callbacks if the coordinator
// has already been stopped.
if (self.stopped) {
return;
}Is this change related to the direct-send flow somehow?
Yes. In the direct-send flow, no view controller is created (`self.sendTabToSelfViewController` is nil). This guard is necessary to prevent attempting to dismiss a non-existent view controller.
- (void)sendTabToSelfWithoutTargetPickerUI:(const GURL&)urlThis is a bit of an odd name - can we find a name that describes what this does, rather than what it does not do? Something like `sendTabToSelfToSpecificDevice`?
Agreed completely. I will rename the command to `sendTabToSelfToSpecificDevice`.
Related details
- Marc Treib
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. |
Marc Treib (Gerrit)
Marc Treib added 11 comments![]( "Open in Gerrit")
[_sendTabToSelfCoordinator sendTabToTargetDeviceCacheGUID:cacheGUIDMichael TatarskiThe coordinator is a UI concept, so going through it if we're not actually showing any UI seems a bit weird. But maybe showing the confirmation toast is "enough UI" to warrant going through the coordinator?
Yeah, that's a fair point. Coordinators are usually UI-only concepts. However, going through SendTabToSelfCoordinator here is the cleanest way IMHO.
It prevents us from bloating the fairly generic `BrowserCoordinator` with feature-specific logic like sync model listeners, error handling, and toast triggering.
The toast can be considered the UI representation for this command, and keeping it encapsulated in the coordinator keeps everything modular.
It prevents us from bloating the fairly generic BrowserCoordinator with feature-specific logic
Well, that's not the *only* alternative to the current approach. The relevant parts could also be pulled into some other (non-coordinator) helper class. But I guess if there's *some* UI involved, then using the same coordinator probably makes sense.
But then by that argument, it's probably best to go through `ExecuteWhenTransitionsComplete` after all. I'm not *sure* it's required in this case, but it can't hurt, and it's more consistent.
Also add a comment for this one? Or add an "otherwise" to the comment above.
send_tab_to_self::ShareEntryPoint _entryPoint;Also add a comment here, since otherwise these fields (or what it means when they're nil) aren't really obvious
_title = title;
_targetDeviceCacheGUID = [targetDeviceCacheGUID copy];
_targetDeviceName = [targetDeviceName copy];Why copy? The title is not copied.
deviceName:(NSString*)deviceName {
// Avoid performing any UI updates or delegate callbacks if the coordinator
// has already been stopped.
if (self.stopped) {
return;
}Michael TatarskiIs this change related to the direct-send flow somehow?
Yes. In the direct-send flow, no view controller is created (`self.sendTabToSelfViewController` is nil). This guard is necessary to prevent attempting to dismiss a non-existent view controller.
Sorry, this comment makes no sense to me. Why does that influence whether the coordinator is stopped?
if (self.sendTabToSelfViewController &&I find this kinda hard to read. What does it mean if there's no view controller?
Would it be equivalent to check for a non-nil _targetDeviceCacheGUID? (If so, should there be a helper like isDirectSend?)
deviceName:(NSString*)deviceName {
// Avoid performing any UI updates or delegate callbacks if the coordinator
// has already been stopped.
if (self.stopped) {
return;
}I still don't understand this. Why can the coordinator *now* be stopped already at this point?
SendTabToSelfCoordinator* CreateDirectSendCoordinator(NSString* cacheGUID,
NSString* deviceName) {Misaligned - run `git cl format`?
// Enable post-send toast flag for toast behavior tests.Even if the flag is somewhat-required for the test, this comment is just confusing - this is not a "toast behavior test".
task_environment_.RunUntilIdle();RunUntilIdle is an antipattern. Can we wait for something more specific?
}Why was this moved here? Having a separate build file made more sense to me.
Related details
- Mark Cogan
- Michael Tatarski
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. |
Michael Tatarski (Gerrit)
Michael Tatarski added 11 comments![]( "Open in Gerrit")
[_sendTabToSelfCoordinator sendTabToTargetDeviceCacheGUID:cacheGUIDMichael TatarskiThe coordinator is a UI concept, so going through it if we're not actually showing any UI seems a bit weird. But maybe showing the confirmation toast is "enough UI" to warrant going through the coordinator?
Marc TreibYeah, that's a fair point. Coordinators are usually UI-only concepts. However, going through SendTabToSelfCoordinator here is the cleanest way IMHO.
It prevents us from bloating the fairly generic `BrowserCoordinator` with feature-specific logic like sync model listeners, error handling, and toast triggering.
The toast can be considered the UI representation for this command, and keeping it encapsulated in the coordinator keeps everything modular.
It prevents us from bloating the fairly generic BrowserCoordinator with feature-specific logic
Well, that's not the *only* alternative to the current approach. The relevant parts could also be pulled into some other (non-coordinator) helper class. But I guess if there's *some* UI involved, then using the same coordinator probably makes sense.
But then by that argument, it's probably best to go through `ExecuteWhenTransitionsComplete` after all. I'm not *sure* it's required in this case, but it can't hurt, and it's more consistent.
SG, I have wrapped the direct-send start call inside `ExecuteWhenTransitionsComplete`
Also add a comment for this one? Or add an "otherwise" to the comment above.
Added a comment above the initializer to document that it runs in the standard picker mode.
send_tab_to_self::ShareEntryPoint _entryPoint;Also add a comment here, since otherwise these fields (or what it means when they're nil) aren't really obvious
Done
_title = title;
_targetDeviceCacheGUID = [targetDeviceCacheGUID copy];
_targetDeviceName = [targetDeviceName copy];Why copy? The title is not copied.
Removed the copy calls and assigned the strings directly for consistency with _title and other properties.
deviceName:(NSString*)deviceName {
// Avoid performing any UI updates or delegate callbacks if the coordinator
// has already been stopped.
if (self.stopped) {
return;
}Michael TatarskiIs this change related to the direct-send flow somehow?
Marc TreibYes. In the direct-send flow, no view controller is created (`self.sendTabToSelfViewController` is nil). This guard is necessary to prevent attempting to dismiss a non-existent view controller.
Sorry, this comment makes no sense to me. Why does that influence whether the coordinator is stopped?
In the standard picker flow, if the enhanced bottom sheet is disabled, the picker is dismissed synchronously on tap, so the coordinator requests immediate teardown.
In the direct-send flow, there is obvisouly no picker UI. Thus, we must keep the coordinator active to await the asynchronous background transaction before showing the snackbar and stopping.
I added a detailed comment explaining the lifecycle.
if (self.sendTabToSelfViewController &&I find this kinda hard to read. What does it mean if there's no view controller?
Would it be equivalent to check for a non-nil _targetDeviceCacheGUID? (If so, should there be a helper like isDirectSend?)
Okay, good suggestion.
deviceName:(NSString*)deviceName {
// Avoid performing any UI updates or delegate callbacks if the coordinator
// has already been stopped.
if (self.stopped) {
return;
}I still don't understand this. Why can the coordinator *now* be stopped already at this point?
This guard is a safety check for the asynchronous callback. Because SendEntry executes an asynchronously, there is a delay before handleSendResult is called. During this asynchronous window, it is possible for the parent coordinator to stop and tear down this coordinator (for example, if the user closes the tab, navigates away, or logs out).
If the coordinator has already been stopped by its parent (self.stopped is YES), this check prevents us from performing any subsequent UI updates (like displaying the snackbar) or invoking delegate callbacks on a coordinator that is already dead, avoiding crashes.
SendTabToSelfCoordinator* CreateDirectSendCoordinator(NSString* cacheGUID,
NSString* deviceName) {Misaligned - run `git cl format`?
Done
// Enable post-send toast flag for toast behavior tests.Even if the flag is somewhat-required for the test, this comment is just confusing - this is not a "toast behavior test".
Rewrote the comment.
task_environment_.RunUntilIdle();RunUntilIdle is an antipattern. Can we wait for something more specific?
Used a future.
Why was this moved here? Having a separate build file made more sense to me.
- Marc Treib
- Mark Cogan
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. |
Marc Treib (Gerrit)
Marc Treib added 4 comments![]( "Open in Gerrit")
[_sendTabToSelfCoordinator sendTabToTargetDeviceCacheGUID:cacheGUIDMichael TatarskiThe coordinator is a UI concept, so going through it if we're not actually showing any UI seems a bit weird. But maybe showing the confirmation toast is "enough UI" to warrant going through the coordinator?
Marc TreibYeah, that's a fair point. Coordinators are usually UI-only concepts. However, going through SendTabToSelfCoordinator here is the cleanest way IMHO.
It prevents us from bloating the fairly generic `BrowserCoordinator` with feature-specific logic like sync model listeners, error handling, and toast triggering.
The toast can be considered the UI representation for this command, and keeping it encapsulated in the coordinator keeps everything modular.
Michael TatarskiIt prevents us from bloating the fairly generic BrowserCoordinator with feature-specific logic
Well, that's not the *only* alternative to the current approach. The relevant parts could also be pulled into some other (non-coordinator) helper class. But I guess if there's *some* UI involved, then using the same coordinator probably makes sense.
But then by that argument, it's probably best to go through `ExecuteWhenTransitionsComplete` after all. I'm not *sure* it's required in this case, but it can't hurt, and it's more consistent.
SG, I have wrapped the direct-send start call inside `ExecuteWhenTransitionsComplete`
Acknowledged
base::BindRepeating(
[](ProfileIOS* profile) -> std::unique_ptr<KeyedService> {
return std::make_unique<syncer::TestSyncService>();
})); // Enable post-send toast flag for toast behavior tests.Michael TatarskiEven if the flag is somewhat-required for the test, this comment is just confusing - this is not a "toast behavior test".
Rewrote the comment.
No you didn't
Michael TatarskiWhy was this moved here? Having a separate build file made more sense to me.
Done
- Mark Cogan
- Michael Tatarski
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. |
Michael Tatarski (Gerrit)
Michael Tatarski added 4 comments![]( "Open in Gerrit")
deviceName:(NSString*)deviceName {
// Avoid performing any UI updates or delegate callbacks if the coordinator
// has already been stopped.
if (self.stopped) {
return;
}Michael TatarskiI still don't understand this. Why can the coordinator *now* be stopped already at this point?
This guard is a safety check for the asynchronous callback. Because SendEntry executes an asynchronously, there is a delay before handleSendResult is called. During this asynchronous window, it is possible for the parent coordinator to stop and tear down this coordinator (for example, if the user closes the tab, navigates away, or logs out).
If the coordinator has already been stopped by its parent (self.stopped is YES), this check prevents us from performing any subsequent UI updates (like displaying the snackbar) or invoking delegate callbacks on a coordinator that is already dead, avoiding crashes.
Done
base::BindRepeating(
[](ProfileIOS* profile) -> std::unique_ptr<KeyedService> {
return std::make_unique<syncer::TestSyncService>();
}));Cool, thanks for the work.
// Enable post-send toast flag for toast behavior tests.Michael TatarskiEven if the flag is somewhat-required for the test, this comment is just confusing - this is not a "toast behavior test".
Marc TreibRewrote the comment.
No you didn't
Done
Michael TatarskiWhy was this moved here? Having a separate build file made more sense to me.
Marc TreibDone
Not actually done
- Marc Treib
- Mark Cogan
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Marc Treib (Gerrit)
Marc Treib voted and added 2 comments![]( "Open in Gerrit")
Votes added by Marc Treib
| Code-Review | +1 |
2 comments
"//ui/base",Isn't this one needed for `l10n_util`? (I haven't checked all of the other ones, but `gn gen --check out/myoutdir` should tell you if anything's missing.)
if (self.sendTabToSelfViewController &&`!self.isDirectSend`?
Related details
- Mark Cogan
- Michael Tatarski
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. |
Mark Cogan (Gerrit)
Mark Cogan added 1 comment![]( "Open in Gerrit")
It introduces the command to execute the background send action and
display the post-send snackbar upon completion.So the intent behind UI commands is that they are used to (in a decoupled way) induce UI changes. But this sounds like a model-layer action which shows a snackbar afterwards, so it's not really a UI change.
The usual structure for handling model-layer actions like this is a browser agent, which has a lifetime unrelated to any UI objects and is simpler to unit test.
I don't know the details of the feature well enough to judge how much of the logic this CL adds to the coordinator could be moved to a browser agent.
Related details
- Michael Tatarski
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. |