personalization: call setScreenSaverDuration from JS [chromium/src : main]
Jerry Liu (Gerrit)
Attention is currently required from: Jason Thai.
1 comment:
Patchset:
lgtm overall but can you add some tests?
Added a browser test.
To view, visit change 4420114. To unsubscribe, or for help writing mail filters, visit settings.
Jason Thai (Gerrit)
Attention is currently required from: Jerry Liu.
Patch set 5:Code-Review +1
3 comments:
File chrome/test/data/webui/chromeos/personalization_app/ambient_subpage_element_test.ts:
Patch Set #5, Line 414:
Duration setting not renderedThis should be: `Duration setting should be renderered`
Patch Set #5, Line 424:
starts selectednit: `is initially selected`
personalizationStore.expectAction(
AmbientActionName.SET_SCREEN_SAVER_DURATION);is this needed? We don't wait for this action anymore.
To view, visit change 4420114. To unsubscribe, or for help writing mail filters, visit settings.
Jerry Liu (Gerrit)
4 comments:
Patchset:
Thanks for reviewing, Jason!
File chrome/test/data/webui/chromeos/personalization_app/ambient_subpage_element_test.ts:
Patch Set #5, Line 414:
Duration setting not renderedThis should be: `Duration setting should be renderered`
Done
Patch Set #5, Line 424:
starts selectednit: `is initially selected`
Done
personalizationStore.expectAction(
AmbientActionName.SET_SCREEN_SAVER_DURATION);is this needed? We don't wait for this action anymore.
Removed
To view, visit change 4420114. To unsubscribe, or for help writing mail filters, visit settings.
Findit (Gerrit)
Attention is currently required from: Jerry Liu.
This change meets the code coverage requirements.
Patch set 7:Code-Coverage +1
Jerry Liu (Gerrit)
Patchset:
Hi Jason, I rewrote the parent CL to remove the `ScreenSaverDuration` enum. Could you please review this CL again?
To view, visit change 4420114. To unsubscribe, or for help writing mail filters, visit settings.
Jerry Liu (Gerrit)
Attention is currently required from: Chromium IPC Reviews, Jason Thai.
Jerry Liu would like Chromium IPC Reviews to review this change.
personalization: call setScreenSaverDuration from JS
When a new screen saver duration is selected from the frontend, we call
set screen saver duration to pass the value to the C++ side.
BUG=b:274175512
TEST=Manually
Change-Id: I39acc05cc7daa3be312592b38fb251aed7a56a37
---
M ash/webui/personalization_app/mojom/personalization_app.mojom
M ash/webui/personalization_app/resources/js/ambient/ambient_actions.ts
M ash/webui/personalization_app/resources/js/ambient/ambient_controller.ts
M ash/webui/personalization_app/resources/js/ambient/ambient_reducers.ts
M ash/webui/personalization_app/resources/js/ambient/ambient_state.ts
M ash/webui/personalization_app/resources/js/ambient/duration_list_element.html
M ash/webui/personalization_app/resources/js/ambient/duration_list_element.ts
M ash/webui/personalization_app/resources/js/personalization_app.ts
M chrome/test/data/webui/chromeos/personalization_app/ambient_subpage_element_test.ts
9 files changed, 115 insertions(+), 48 deletions(-)
gwsq (Gerrit)
Attention is currently required from: Chromium IPC Reviews, Jason Thai, Will Harris.
gwsq would like Will Harris to review this change authored by Jerry Liu.
gwsq (Gerrit)
Attention is currently required from: Jason Thai, Will Harris.
Jerry Liu has uploaded this change for review.
gwsq (Gerrit)
Attention is currently required from: Jason Thai, Will Harris.
From googleclient/chrome/chromium_gwsq/ipc/config.gwsq:
IPC: w...@chromium.org
📎 It looks like you’re making a possibly security-sensitive change! 📎 IPC security review isn’t a rubberstamp, so your friendly security reviewer will need a fair amount of context to review your CL effectively. Please review your CL description and code comments to make sure they provide context for someone unfamiliar with your project/area. Pay special attention to where data comes from and which processes it flows between (and their privilege levels). Feel free to point your security reviewer at design docs, bugs, or other links if you can’t reasonably make a self-contained CL description. (Also see https://cbea.ms/git-commit/).
IPC reviewer(s): w...@chromium.org
Reviewer source(s):
w...@chromium.org is from context(googleclient/chrome/chromium_gwsq/ipc/config.gwsq)
Will Harris (Gerrit)
Attention is currently required from: Jason Thai, Jerry Liu.
Patch set 13:Code-Review +1
Jason Thai (Gerrit)
Attention is currently required from: Jerry Liu.
1 comment:
File ash/webui/personalization_app/resources/js/ambient/duration_list_element.ts:
Patch Set #14, Line 69:
Objectdefine a proper object.
To view, visit change 4420114. To unsubscribe, or for help writing mail filters, visit settings.
Jerry Liu (Gerrit)
Attention is currently required from: Jason Thai.
1 comment:
File ash/webui/personalization_app/resources/js/ambient/duration_list_element.ts:
Patch Set #14, Line 69:
Objectdefine a proper object.
Done
To view, visit change 4420114. To unsubscribe, or for help writing mail filters, visit settings.
Jason Thai (Gerrit)
Attention is currently required from: Jerry Liu.
Patch set 15:Code-Review +1
1 comment:
File ash/webui/personalization_app/resources/js/ambient/ambient_state.ts:
Patch Set #15, Line 29:
duration: 10,Will this result in the selected state jumping around? The user might set a different duration but by default, we are still showing 10. When we finally observe the correct value, I think this will cause the a janky experience.
To view, visit change 4420114. To unsubscribe, or for help writing mail filters, visit settings.
Jerry Liu (Gerrit)
Attention is currently required from: Jason Thai.
1 comment:
File ash/webui/personalization_app/resources/js/ambient/ambient_state.ts:
Patch Set #15, Line 29:
duration: 10,
Will this result in the selected state jumping around? The user might set a different duration but b […]
For now, every time we open the hub/screensaver subpage, the value will be reset to 10 without calling `SetScreenSaverDuration(10)`. It is not a good user experience so far.
I plan to address this by adding a mojom call to notify JS side the current pref value.
To view, visit change 4420114. To unsubscribe, or for help writing mail filters, visit settings.
Jason Thai (Gerrit)
Attention is currently required from: Jerry Liu.
1 comment:
File ash/webui/personalization_app/resources/js/ambient/ambient_state.ts:
Patch Set #15, Line 29:
duration: 10,
For now, every time we open the hub/screensaver subpage, the value will be reset to 10 without calli […]
We should set this to null and only set the value after we finally observe duration, This is how we do it for other components.
To view, visit change 4420114. To unsubscribe, or for help writing mail filters, visit settings.
Jerry Liu (Gerrit)
Attention is currently required from: Jason Thai.
1 comment:
File ash/webui/personalization_app/resources/js/ambient/ambient_state.ts:
Patch Set #15, Line 29:
duration: 10,
We should set this to null and only set the value after we finally observe duration, This is how we […]
The default value in store is set to null. The UI doesn't have a pre-selected option temporarily. I will add the observer function in a separate CL.
To view, visit change 4420114. To unsubscribe, or for help writing mail filters, visit settings.
Jason Thai (Gerrit)
Attention is currently required from: Jerry Liu.
Patch set 16:Code-Review +1
1 comment:
Patchset:
lgtm
To view, visit change 4420114. To unsubscribe, or for help writing mail filters, visit settings.
Jerry Liu (Gerrit)
Attention is currently required from: Jerry Liu.
Patch set 16:Commit-Queue +2
Jerry Liu (Gerrit)
Jerry Liu (Gerrit)
Attention is currently required from: Jerry Liu.
Patch set 17:Commit-Queue +2
Jerry Liu (Gerrit)
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted this change.
16 is the latest approved patch-set.
No files were changed between the latest approved patch-set and the submitted one.
personalization: call setScreenSaverDuration from JS
When a new screen saver duration is selected from the frontend, we call
set screen saver duration to pass the value to the C++ side.
BUG=b:274175512
TEST=Manually
Change-Id: I39acc05cc7daa3be312592b38fb251aed7a56a37
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4420114
Reviewed-by: Jason Thai <jas...@chromium.org>
Reviewed-by: Will Harris <w...@chromium.org>
Commit-Queue: Jerry Liu <pz...@google.com>
Cr-Commit-Position: refs/heads/main@{#1131541}
---
M ash/webui/personalization_app/mojom/personalization_app.mojom
M ash/webui/personalization_app/resources/js/ambient/ambient_actions.ts
M ash/webui/personalization_app/resources/js/ambient/ambient_controller.ts
M ash/webui/personalization_app/resources/js/ambient/ambient_reducers.ts
M ash/webui/personalization_app/resources/js/ambient/ambient_state.ts
M ash/webui/personalization_app/resources/js/ambient/duration_list_element.html
M ash/webui/personalization_app/resources/js/ambient/duration_list_element.ts
M ash/webui/personalization_app/resources/js/personalization_app.ts
M chrome/test/data/webui/chromeos/personalization_app/ambient_subpage_element_test.ts
9 files changed, 117 insertions(+), 48 deletions(-)