[iOS] Create icb/passwords/bottom_sheet [chromium/src : main]
Rafał Godlewski (Gerrit)
Rafał Godlewski voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Tommy Martino
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tommy Martino (Gerrit)
Tommy Martino added 1 comment![]( "Open in Gerrit")
Thanks for picking this up!
As written, this largely replicates the same ui_bundled structure. My thinking was that the end state would instead be:
```
icb/passwords/bottom_sheet/
- coordinator/
- model/ (if necessary)
- public/
- ui/
- etc
```
so that `bottom_sheet` would be structured like top-level feature directories, just nested under `passwords`.
Was your plan to do that in a follow-up CL? Or was I perhaps not totally clear on what I had in mind? (Sorry, if the latter!)
Related details
- Rafał Godlewski
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. |
Rafał Godlewski (Gerrit)
Rafał Godlewski added 1 comment![]( "Open in Gerrit")
Thanks for picking this up!
As written, this largely replicates the same ui_bundled structure. My thinking was that the end state would instead be:
```
icb/passwords/bottom_sheet/
- coordinator/
- model/ (if necessary)
- public/
- ui/
- etc
```so that `bottom_sheet` would be structured like top-level feature directories, just nested under `passwords`.
Was your plan to do that in a follow-up CL? Or was I perhaps not totally clear on what I had in mind? (Sorry, if the latter!)
For some reason I thought the diffs would be cleaner if I split, but not sure what was the reason exactly 😄.
Added test/, coordinator/, ui/
Related details
- Tommy Martino
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tommy Martino (Gerrit)
Tommy Martino voted and added 3 comments![]( "Open in Gerrit")
Votes added by Tommy Martino
| Code-Review | +1 |
Rafał GodlewskiThanks for picking this up!
As written, this largely replicates the same ui_bundled structure. My thinking was that the end state would instead be:
```
icb/passwords/bottom_sheet/
- coordinator/
- model/ (if necessary)
- public/
- ui/
- etc
```so that `bottom_sheet` would be structured like top-level feature directories, just nested under `passwords`.
Was your plan to do that in a follow-up CL? Or was I perhaps not totally clear on what I had in mind? (Sorry, if the latter!)
For some reason I thought the diffs would be cleaner if I split, but not sure what was the reason exactly 😄.
Added test/, coordinator/, ui/
Thanks!
For future reference, typically what I do is:
1. Do a clean move of all files to their intended final destination, but don't update any includes, BUILD files, etc
2. (specific to this type of directory unbundling) Copy the original BUILD file into each of the new directories
3. `git cl upload`, ignoring any presubmits and warnings
4. Update everything so it actually builds, passes presubmits, etc
5. `git cl upload` again (on the same issue/branch)
That way the diff between patchset 1 and patchset 2 is exactly the set of non-move/branch changes you made.
source_set("common") {
sources = [
"scoped_credential_suggestion_bottom_sheet_reauth_module_override.h",
"scoped_credential_suggestion_bottom_sheet_reauth_module_override.mm",
]
deps = [
"//base",
"//ios/chrome/common/ui/reauthentication",
]
}Let's put this in `../public` instead, so that non-test things aren't depending on `test/`
"//ios/chrome/browser/favicon/model",Is this include needed? It's generally not allowed for anything `ui` to depend on anything `model`, and in this case I skimmed and didn't see any inclusions of this path.
Related details
- Rafał Godlewski
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. |