Connect coral feature to backend [chromium/src : main]
0 views
Skip to first unread message
Howard Yang (Gerrit)
Sep 26, 2024, 5:10:00 AM9/26/24
to chromium...@chromium.org
Howard Yang has uploaded the change for review![]( "Open in Gerrit")
Commit message
Connect coral feature to backend
Implement CoralController as a mojo service proxy with some hardcoded
options settings. Make BirchCoralProvider parse the response from
controller.
Bug: b:358530885
Test: CQ
Change-Id: I70712f40f602929fe5b6cc312d0f269886e27101
Change diff
Change information
Files:
- M ash/birch/birch_coral_provider.cc
- M ash/wm/coral/coral_controller.cc
- M ash/wm/coral/coral_controller.h
Change size: M
Delta: 3 files changed, 126 insertions(+), 15 deletions(-)
Related details
Attention set is empty
Submit Requirements:
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Howard Yang (Gerrit)
Sep 26, 2024, 9:09:01 AM9/26/24
to Ahmed Fakhry, Yulun Wu, Chromium LUCI CQ, chromium...@chromium.org
Attention needed from Ahmed Fakhry and Yulun Wu
Howard Yang added 1 comment![]( "Open in Gerrit")
Patchset-level comments
File-level comment, Patchset 3 (Latest):
Howard Yang . resolved
Hi reviewers, please help me review this change, thanks.
Related details
Attention is currently required from:
- Ahmed Fakhry
- Yulun Wu
Submit Requirements:
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ahmed Fakhry (Gerrit)
Sep 26, 2024, 12:26:29 PM9/26/24
to Howard Yang, Yulun Wu, Chromium LUCI CQ, chromium...@chromium.org, oshima...@chromium.org
Attention needed from Howard Yang and Yulun Wu
Ahmed Fakhry added 1 comment![]( "Open in Gerrit")
Patchset-level comments
File-level comment, Patchset 5 (Latest):
Ahmed Fakhry . resolved
Can you please fix the failing tests?
Related details
Attention is currently required from:
- Howard Yang
- Yulun Wu
Submit Requirements:
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Howard Yang (Gerrit)
Sep 29, 2024, 7:16:10 AM9/29/24
to Yulun Wu, Xiaodan Zhu, Ahmed Fakhry, Chromium LUCI CQ, chromium...@chromium.org, oshima...@chromium.org
Attention needed from Xiaodan Zhu and Yulun Wu
Howard Yang voted Commit-Queue+0![]( "Open in Gerrit")
Attention is currently required from:
- Xiaodan Zhu
- Yulun Wu
Submit Requirements:
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Howard Yang (Gerrit)
Sep 29, 2024, 9:40:01 AM9/29/24
to Yulun Wu, Xiaodan Zhu, Ahmed Fakhry, Chromium LUCI CQ, chromium...@chromium.org, oshima...@chromium.org
Attention needed from Ahmed Fakhry, Xiaodan Zhu and Yulun Wu
Howard Yang added 1 comment![]( "Open in Gerrit")
Patchset-level comments
Ahmed Fakhry . resolved
Can you please fix the failing tests?
Attention is currently required from:
- Ahmed Fakhry
- Xiaodan Zhu
- Yulun Wu
Submit Requirements:
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Xiaodan Zhu (Gerrit)
Sep 30, 2024, 1:47:41 PM9/30/24
to Howard Yang, Yulun Wu, Ahmed Fakhry, Chromium LUCI CQ, chromium...@chromium.org, oshima...@chromium.org
Attention needed from Ahmed Fakhry, Howard Yang and Yulun Wu
Xiaodan Zhu voted and added 2 comments![]( "Open in Gerrit")
Votes added by Xiaodan Zhu
| Code-Review | +1 |
2 comments
Patchset-level comments
File ash/birch/birch_coral_provider.cc
Line 273, Patchset 9 (Latest):
Shell::Get()->birch_model()->SetCoralItems(items);Xiaodan Zhu . unresolved
To make the coral provider and birch model consistent, shall we clear the groups_ data as well or move L278 to L270? Also, do we need to empty the response_?
Related details
Attention is currently required from:
- Ahmed Fakhry
- Howard Yang
- Yulun Wu
Submit Requirements:
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ahmed Fakhry (Gerrit)
Sep 30, 2024, 6:36:44 PM9/30/24
to Howard Yang, Xiaodan Zhu, Yulun Wu, Chromium LUCI CQ, chromium...@chromium.org, oshima...@chromium.org
Attention needed from Howard Yang and Yulun Wu
Ahmed Fakhry added 5 comments![]( "Open in Gerrit")
File ash/wm/coral/coral_controller.h
Line 89, Patchset 9 (Latest):
coral::mojom::CacheEmbeddingsResultPtr result);Ahmed Fakhry . unresolved
Please for the above the functions, add proper docs.
Line 88, Patchset 9 (Latest):
base::OnceCallback<void(bool)> callback,Ahmed Fakhry . unresolved
Please document what is this callback and what is this bool.
Line 86, Patchset 9 (Latest):
coral::mojom::GroupResultPtr result);Ahmed Fakhry . unresolved
Nit: Blank line after this
File ash/wm/coral/coral_controller.cc
File chrome/browser/ui/ash/birch/BUILD.gn
Line 165, Patchset 9 (Latest):
"//chromeos/ash/components/mojo_service_manager",Ahmed Fakhry . unresolved
Is this needed here?
Related details
Attention is currently required from:
- Howard Yang
- Yulun Wu
Submit Requirements:
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Howard Yang (Gerrit)
Sep 30, 2024, 11:07:15 PM9/30/24
to Xiaodan Zhu, Yulun Wu, Ahmed Fakhry, Chromium LUCI CQ, chromium...@chromium.org, oshima...@chromium.org
Attention needed from Ahmed Fakhry, Xiaodan Zhu and Yulun Wu
Howard Yang voted and added 6 comments![]( "Open in Gerrit")
Votes added by Howard Yang
| Commit-Queue | +1 |
6 comments
File ash/birch/birch_coral_provider.cc
To make the coral provider and birch model consistent, shall we clear the groups_ data as well or move L278 to L270? Also, do we need to empty the response_?
Howard Yang
Done
File ash/wm/coral/coral_controller.h
Please for the above the functions, add proper docs.
Howard Yang
Done
Please document what is this callback and what is this bool.
Howard Yang
Done
Nit: Blank line after this
Howard Yang
Done
File ash/wm/coral/coral_controller.cc
File chrome/browser/ui/ash/birch/BUILD.gn
Line 165, Patchset 9:
"//chromeos/ash/components/mojo_service_manager",Ahmed Fakhry . resolved
Howard YangIs this needed here?
Removed
Related details
Attention is currently required from:
- Ahmed Fakhry
- Xiaodan Zhu
- Yulun Wu
Submit Requirements:
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Howard Yang (Gerrit)
Sep 30, 2024, 11:07:54 PM9/30/24
to Yulun Wu, Xiaodan Zhu, Ahmed Fakhry, Chromium LUCI CQ, chromium...@chromium.org, oshima...@chromium.org
Attention needed from Ahmed Fakhry, Xiaodan Zhu and Yulun Wu
Howard Yang voted Commit-Queue+0![]( "Open in Gerrit")
| Commit-Queue | +0 |
Related details
Attention is currently required from:
- Ahmed Fakhry
- Xiaodan Zhu
- Yulun Wu
Submit Requirements:
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Xiaodan Zhu (Gerrit)
Oct 1, 2024, 1:10:40 AM10/1/24
to Howard Yang, Yulun Wu, Ahmed Fakhry, Chromium LUCI CQ, chromium...@chromium.org, oshima...@chromium.org
Attention needed from Ahmed Fakhry, Howard Yang and Yulun Wu
Xiaodan Zhu voted and added 1 comment![]( "Open in Gerrit")
Votes added by Xiaodan Zhu
| Code-Review | +1 |
Attention is currently required from:
- Ahmed Fakhry
- Howard Yang
- Yulun Wu
Submit Requirements:
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ahmed Fakhry (Gerrit)
Oct 1, 2024, 12:15:45 PM10/1/24
to Howard Yang, Xiaodan Zhu, Yulun Wu, Chromium LUCI CQ, chromium...@chromium.org, oshima...@chromium.org
Attention needed from Howard Yang and Yulun Wu
Ahmed Fakhry voted and added 1 comment![]( "Open in Gerrit")
Votes added by Ahmed Fakhry
| Code-Review | +1 |
1 comment
Patchset-level comments
Attention is currently required from:
- Howard Yang
- Yulun Wu
Submit Requirements:
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Howard Yang (Gerrit)
Oct 1, 2024, 7:23:49 PM10/1/24
to Ahmed Fakhry, Xiaodan Zhu, Yulun Wu, Chromium LUCI CQ, chromium...@chromium.org, oshima...@chromium.org
Attention needed from Yulun Wu
Howard Yang voted Commit-Queue+2![]( "Open in Gerrit")
Attention is currently required from:
- Yulun Wu
Submit Requirements:
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Oct 1, 2024, 7:27:09 PM10/1/24
to Howard Yang, Ahmed Fakhry, Xiaodan Zhu, Yulun Wu, chromium...@chromium.org, oshima...@chromium.org
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Commit message:
Connect coral feature to backend
Implement CoralController as a mojo service proxy with some hardcoded
options settings. Make BirchCoralProvider parse the response from
controller.
Bug: b:358530885
Test: CQ
Change-Id: I70712f40f602929fe5b6cc312d0f269886e27101
Reviewed-by: Xiaodan Zhu <zx...@chromium.org>
Reviewed-by: Ahmed Fakhry <afa...@chromium.org>
Commit-Queue: Howard Yang <hcy...@google.com>
Cr-Commit-Position: refs/heads/main@{#1362751}
Files:
- M ash/BUILD.gn
- M ash/birch/birch_coral_provider.cc
- M ash/wm/coral/coral_controller.cc
- M ash/wm/coral/coral_controller.h
- M chrome/browser/ui/ash/birch/birch_coral_provider_browsertest.cc
Change size: M
Delta: 5 files changed, 141 insertions(+), 18 deletions(-)
Branch: refs/heads/main
Submit Requirements:
Code-Review: +1 by Xiaodan Zhu, +1 by Ahmed Fakhry
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Bao-Duy Tran (Gerrit)
Oct 2, 2024, 5:35:52 AM10/2/24
to Chromium LUCI CQ, Howard Yang, Ahmed Fakhry, Xiaodan Zhu, Yulun Wu, chromium...@chromium.org, oshima...@chromium.org
Bao-Duy Tran has created a revert of this change![]( "Open in Gerrit")
Related details
Attention set is empty
Submit Requirements:
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Bao-Duy Tran (Gerrit)
Oct 2, 2024, 5:41:21 AM10/2/24
to Chromium LUCI CQ, Howard Yang, Ahmed Fakhry, Xiaodan Zhu, Yulun Wu, chromium...@chromium.org, oshima...@chromium.org
Bao-Duy Tran added 1 comment![]( "Open in Gerrit")
Patchset-level comments
File-level comment, Patchset 13 (Latest):
Bao-Duy Tran . resolved
FYI. This caused CoralBrowserTest.PostLoginBrowser test to fail (b/370844780).
Related details
Attention set is empty
Submit Requirements:
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Howard Yang (Gerrit)
Oct 2, 2024, 5:42:23 AM10/2/24
to Chromium LUCI CQ, Bao-Duy Tran, Ahmed Fakhry, Xiaodan Zhu, Yulun Wu, chromium...@chromium.org, oshima...@chromium.org
Howard Yang added 1 comment![]( "Open in Gerrit")
Patchset-level comments
Bao-Duy Tran . resolved
FYI. This caused CoralBrowserTest.PostLoginBrowser test to fail (b/370844780).
Howard Yang
I'll look into this, but I wonder why it's not caught by CQ. Is the failure flaky?
Related details
Attention set is empty
Submit Requirements:
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Bao-Duy Tran (Gerrit)
Oct 2, 2024, 5:46:25 AM10/2/24
to Chromium LUCI CQ, Howard Yang, Ahmed Fakhry, Xiaodan Zhu, Yulun Wu, chromium...@chromium.org, oshima...@chromium.org
Bao-Duy Tran added 1 comment![]( "Open in Gerrit")
Patchset-level comments
Bao-Duy Tran . resolved
Howard YangFYI. This caused CoralBrowserTest.PostLoginBrowser test to fail (b/370844780).
I'll look into this, but I wonder why it's not caught by CQ. Is the failure flaky?
Bao-Duy Tran
I'm not sure. Failure is observed on linux-chromeos-chrome and it's deterministic, not flaky... Details @ b/370844780. This has now been reverted @ https://crrev.com/c/5904274
Related details
Attention set is empty
Submit Requirements:
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reply all
Reply to author
Forward
0 new messages