CCA: Cloud save warning dialog [chromium/src : main]
0 views
Skip to first unread message
Orko Garai (Gerrit)
Dec 19, 2025, 8:25:48 PM12/19/25
to Pi-Hsun Shih, Aida Zolić, Shik Chen, chromium...@chromium.org, chromeos-ca...@google.com, oshima...@chromium.org
Attention needed from Aida Zolić, Pi-Hsun Shih and Shik Chen
New activity on the change![]( "Open in Gerrit")
Related details
Attention is currently required from:
- Aida Zolić
- Pi-Hsun Shih
- Shik Chen
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Aida Zolić (Gerrit)
Dec 22, 2025, 10:50:01 AM12/22/25
to Orko Garai, Pi-Hsun Shih, Shik Chen, chromium...@chromium.org, chromeos-ca...@google.com, oshima...@chromium.org
Attention needed from Orko Garai, Pi-Hsun Shih and Shik Chen
Aida Zolić voted Code-Review+1![]( "Open in Gerrit")
Attention is currently required from:
- Orko Garai
- Pi-Hsun Shih
- Shik Chen
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Orko Garai (Gerrit)
Jan 7, 2026, 9:49:43 AM (7 days ago) Jan 7
to Aida Zolić, Pi-Hsun Shih, Shik Chen, chromium...@chromium.org, chromeos-ca...@google.com, oshima...@chromium.org
Attention needed from Pi-Hsun Shih and Shik Chen
Orko Garai added 1 comment![]( "Open in Gerrit")
Attention is currently required from:
- Pi-Hsun Shih
- Shik Chen
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Pi-Hsun Shih (Gerrit)
Jan 8, 2026, 2:46:59 AM (6 days ago) Jan 8
to Orko Garai, Aida Zolić, Shik Chen, chromium...@chromium.org, chromeos-ca...@google.com, oshima...@chromium.org
Attention needed from Orko Garai and Shik Chen
Pi-Hsun Shih added 6 comments![]( "Open in Gerrit")
File ash/webui/camera_app_ui/resources/js/lit/components/cloud-save-warning-dialog.ts
Line 60, Patchset 15 (Latest):
return this.shadowRoot!Pi-Hsun Shih . unresolved
nit (same for other places that use querySelector):
```
return checkInstanceof(
this.renderRoot.querySelector('...'), CrCheckboxElement).checked;
```
(We'd like to minimize the use of non-null assertion but the lint was disabled when TypeScript was updated and haven't been added back)
Line 85, Patchset 15 (Latest):
return this.skipCloudSaveWarning ?Pi-Hsun Shih . unresolved
nit:
```
if (this.skipCloudSaveWarning) {
return nothing;
}
return html`...`
```
Line 88, Patchset 15 (Latest):
<cr-dialog id="cloudSaveWarningDialog" show-on-attach close-text="close">Pi-Hsun Shih . unresolved
HTML id should be dash-case instead of camelCase.
File ash/webui/camera_app_ui/resources/js/views/camera.ts
Line 191, Patchset 15 (Latest):
cloudSaveWarningDialog?.getAckButton()?.addEventListener('click', () => {Pi-Hsun Shih . unresolved
It'd be easier to just put this logic inside the warning dialog, since we don't need to customize this at other places.
File chrome/browser/ash/system_web_apps/apps/camera_app/chrome_camera_app_ui_delegate.cc
Line 540, Patchset 15 (Latest):
source->AddString("cloud_destination", cloud_destination);Pi-Hsun Shih . unresolved
Related details
Attention is currently required from:
- Orko Garai
- Shik Chen
Submit Requirements:
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. |
Orko Garai (Gerrit)
Jan 8, 2026, 4:05:11 PM (5 days ago) Jan 8
to Aida Zolić, Pi-Hsun Shih, Shik Chen, chromium...@chromium.org, chromeos-ca...@google.com, oshima...@chromium.org
Attention needed from Aida Zolić, Pi-Hsun Shih and Shik Chen
Orko Garai added 6 comments![]( "Open in Gerrit")
File ash/webui/camera_app_ui/resources/js/lit/components/cloud-save-warning-dialog.ts
nit (same for other places that use querySelector):
```
return checkInstanceof(
this.renderRoot.querySelector('...'), CrCheckboxElement).checked;
```(We'd like to minimize the use of non-null assertion but the lint was disabled when TypeScript was updated and haven't been added back)
Orko Garai
Done
nit:
```
if (this.skipCloudSaveWarning) {
return nothing;
}
return html`...`
```
Orko Garai
Done
Line 88, Patchset 15:
<cr-dialog id="cloudSaveWarningDialog" show-on-attach close-text="close">Pi-Hsun Shih . resolved
HTML id should be dash-case instead of camelCase.
Orko Garai
Done
File ash/webui/camera_app_ui/resources/js/views/camera.ts
Line 191, Patchset 15:
cloudSaveWarningDialog?.getAckButton()?.addEventListener('click', () => {Pi-Hsun Shih . resolved
It'd be easier to just put this logic inside the warning dialog, since we don't need to customize this at other places.
Orko Garai
Done
File chrome/browser/ash/system_web_apps/apps/camera_app/chrome_camera_app_ui_delegate.cc
Line 540, Patchset 15:
source->AddString("cloud_destination", cloud_destination);Pi-Hsun Shih . resolved
Attention is currently required from:
- Aida Zolić
- Pi-Hsun Shih
- Shik Chen
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Aida Zolić (Gerrit)
Jan 9, 2026, 5:51:02 AM (5 days ago) Jan 9
to Orko Garai, Pi-Hsun Shih, Shik Chen, chromium...@chromium.org, chromeos-ca...@google.com, oshima...@chromium.org
Attention needed from Orko Garai, Pi-Hsun Shih and Shik Chen
Aida Zolić added 1 comment![]( "Open in Gerrit")
Patchset-level comments
File-level comment, Patchset 16 (Latest):
Aida Zolić . resolved
Generally lgtm % screenshots are missing - let me know when they're ready for upload.
Related details
Attention is currently required from:
- Orko Garai
- Pi-Hsun Shih
- Shik Chen
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Orko Garai (Gerrit)
Jan 12, 2026, 1:19:18 PM (yesterday) Jan 12
to Aida Zolić, Pi-Hsun Shih, Shik Chen, chromium...@chromium.org, chromeos-ca...@google.com, oshima...@chromium.org
Attention needed from Pi-Hsun Shih and Shik Chen
Orko Garai added 1 comment![]( "Open in Gerrit")
Attention is currently required from:
- Pi-Hsun Shih
- Shik Chen
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Pi-Hsun Shih (Gerrit)
1:12 AM (22 hours ago) 1:12 AM
to Orko Garai, Aida Zolić, Shik Chen, chromium...@chromium.org, chromeos-ca...@google.com, oshima...@chromium.org
Attention needed from Orko Garai and Shik Chen
Pi-Hsun Shih added 3 comments![]( "Open in Gerrit")
File ash/webui/camera_app_ui/resources/js/lit/components/cloud-save-warning-dialog.ts
Line 61, Patchset 18 (Latest):
CrButtonElement)!;Pi-Hsun Shih . unresolved
Don't need this `!`. Use `assertInstanceof` if you want the return to be always non-null.
Line 92, Patchset 18 (Latest):
this.getAckButton()?.addEventListener('click', () => {
this.close();
});Pi-Hsun Shih . unresolved
Don't need to add the event listener manually. You can bind it inside the template (https://lit.dev/docs/components/events/#adding-event-listeners-in-the-element-template)
Related details
Attention is currently required from:
- Orko Garai
- Shik Chen
Submit Requirements:
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. |
Orko Garai (Gerrit)
11:00 AM (12 hours ago) 11:00 AM
to Aida Zolić, Pi-Hsun Shih, Shik Chen, chromium...@chromium.org, chromeos-ca...@google.com, oshima...@chromium.org
Attention needed from Pi-Hsun Shih and Shik Chen
Orko Garai added 3 comments![]( "Open in Gerrit")
File ash/webui/camera_app_ui/resources/js/lit/components/cloud-save-warning-dialog.ts
Don't need this `!`. Use `assertInstanceof` if you want the return to be always non-null.
Orko Garai
Done. Also, replaced with ref.
don't need this `!`.
Orko Garai
Replaced with ref.
Line 92, Patchset 18:
this.getAckButton()?.addEventListener('click', () => {
this.close();
});Pi-Hsun Shih . resolved
Don't need to add the event listener manually. You can bind it inside the template (https://lit.dev/docs/components/events/#adding-event-listeners-in-the-element-template)
Attention is currently required from:
- Pi-Hsun Shih
- Shik Chen
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Pi-Hsun Shih (Gerrit)
8:35 PM (3 hours ago) 8:35 PM
to Orko Garai, Aida Zolić, Shik Chen, chromium...@chromium.org, chromeos-ca...@google.com, oshima...@chromium.org
Attention needed from Orko Garai and Shik Chen
Pi-Hsun Shih voted and added 1 comment![]( "Open in Gerrit")
Votes added by Pi-Hsun Shih
| Code-Review | +1 |
1 comment
File ash/webui/camera_app_ui/resources/js/lit/components/cloud-save-warning-dialog.ts
Line 62, Patchset 19 (Latest):
assertInstanceof(this.dialog.value, CrDialogElement).close();Pi-Hsun Shih . unresolved
Can use `assertExists` here to omit the type since the type is already on the ref.
Related details
Attention is currently required from:
- Orko Garai
- Shik Chen
Submit Requirements:
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. |
Reply all
Reply to author
Forward
0 new messages