ui::SelectFile: deprecate params [chromium/src : main]
Elly FJ (Gerrit)
Elly FJ added 1 comment![]( "Open in Gerrit")
sky: please have a look? if this looks good to you I'll ask an LSC reviewer to look at it because I had to touch a gazillion files.
Related details
- Scott Violet
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Scott Violet (Gerrit)
Scott Violet voted Code-Review+1![]( "Open in Gerrit")
- Elly FJ
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Elly FJ (Gerrit)
Elly FJ added 1 comment![]( "Open in Gerrit")
avi: please have a look as a //content owner?
joedow: please have a look as a //remoting owner?
thanks both!
Related details
- Avi Drissman
- Joe Downing
- Scott Violet
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joe Downing (Gerrit)
Joe Downing voted and added 1 comment![]( "Open in Gerrit")
Votes added by Joe Downing
| Code-Review | +1 |
1 comment
- Avi Drissman
- Elly FJ
- Scott Violet
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joe Downing (Gerrit)
Joe Downing voted and added 1 comment![]( "Open in Gerrit")
Votes added by Joe Downing
| Code-Review | +1 |
1 comment
Related details
- Avi Drissman
- Elly FJ
- Scott Violet
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Avi Drissman (Gerrit)
Avi Drissman added 1 comment![]( "Open in Gerrit")
virtual void FileSelected(const SelectedFileInfo& file, int index) {}The `= 0` for this and for `FileSelectionCanceled` below were deliberate. If you wanted to do a file picker, you had to provide a success callback and a failure callback. If callers wanted to do nothing, they had to explicitly do nothing. This also made it harder to use the class wrong.
Related details
- Elly FJ
- Scott Violet
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Elly FJ (Gerrit)
Elly FJ added 1 comment![]( "Open in Gerrit")
virtual void FileSelected(const SelectedFileInfo& file, int index) {}The `= 0` for this and for `FileSelectionCanceled` below were deliberate. If you wanted to do a file picker, you had to provide a success callback and a failure callback. If callers wanted to do nothing, they had to explicitly do nothing. This also made it harder to use the class wrong.
Because it makes it harder to forget to handle cancellation, you mean? That makes sense. I can put the `= 0` back, but it makes this CL uglier, and opens up a design problem: if I make `FileSelectionCanceled(void* params)` unimplemented, then every subclass will continue to have to implement it, even those that don't use params (which is most of them). If I make `FileSelectionCanceled()` unimplemented, then subclasses that *do* use `params` and have a `FileSelectionCanceled(void* params)` implementation, which presumably doesn't call `FileSelectionCanceled()`, will have to have an empty implementation of `FileSelectionCanceled()` which is never called.
So, because of that, I prefer not to `= 0` either of these functions. Some client classes also genuinely don't care about cancellation (eg `FolderSelectionDialogController`), and it seems unlikely that someone will accidentally not implement `FileSelected()` and not notice, so I don't know how much the harder-to-misuse concern bothers me.
What do you think?
Related details
- Avi Drissman
- Scott Violet
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Avi Drissman (Gerrit)
Avi Drissman voted and added 1 comment![]( "Open in Gerrit")
Votes added by Avi Drissman
| Code-Review | +1 |
1 comment
virtual void FileSelected(const SelectedFileInfo& file, int index) {}Elly FJThe `= 0` for this and for `FileSelectionCanceled` below were deliberate. If you wanted to do a file picker, you had to provide a success callback and a failure callback. If callers wanted to do nothing, they had to explicitly do nothing. This also made it harder to use the class wrong.
Because it makes it harder to forget to handle cancellation, you mean? That makes sense. I can put the `= 0` back, but it makes this CL uglier, and opens up a design problem: if I make `FileSelectionCanceled(void* params)` unimplemented, then every subclass will continue to have to implement it, even those that don't use params (which is most of them). If I make `FileSelectionCanceled()` unimplemented, then subclasses that *do* use `params` and have a `FileSelectionCanceled(void* params)` implementation, which presumably doesn't call `FileSelectionCanceled()`, will have to have an empty implementation of `FileSelectionCanceled()` which is never called.
So, because of that, I prefer not to `= 0` either of these functions. Some client classes also genuinely don't care about cancellation (eg `FolderSelectionDialogController`), and it seems unlikely that someone will accidentally not implement `FileSelected()` and not notice, so I don't know how much the harder-to-misuse concern bothers me.
What do you think?
Fair points. Some of that seems to be unfortunate consequences of handling the migration, and I don’t want to press there. You are right in that this is straightforward enough for callers to probably get right.
I thought there was a point in my earlier refactor in which that `=0` found a test that had meaningfully forgotten to handle the cancellation case, but I’m not finding it. If so, then I’m OK with this, as it doesn’t seem to be an issue in practice.
Related details
- Elly FJ
- Scott Violet
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |