Howard Yang (Gerrit)

unread,

Oct 16, 2024, 12:12:48 PM10/16/24

to chromium...@chromium.org, oshima...@chromium.org, ipc-securi...@chromium.org

Howard Yang has uploaded the change for review

Commit message

[CORAL] Sync title observer mojom change

Bug: b/370851826

Test: CQ

Change-Id: I662425a1b48b4b03135e47aada2d152cf90bc1b5

Change diff

Change information

Files:

M ash/birch/birch_coral_provider.cc
M ash/birch/birch_coral_provider.h
M ash/wm/coral/coral_controller.cc
M ash/wm/coral/coral_controller.h
M ash/wm/coral/fake_coral_service.cc
M ash/wm/coral/fake_coral_service.h
M chromeos/ash/services/coral/public/mojom/BUILD.gn
M chromeos/ash/services/coral/public/mojom/coral_service.mojom

Change size: M

Delta: 8 files changed, 110 insertions(+), 12 deletions(-)

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.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Howard Yang (Gerrit)

unread,

Oct 17, 2024, 4:27:44 AM10/17/24

to Ahmed Fakhry, Xiaodan Zhu, Dan Zhu, Chromium LUCI CQ, Tricium, chromium...@chromium.org, ipc-securi...@chromium.org, oshima...@chromium.org

Attention needed from Ahmed Fakhry and Xiaodan Zhu

Howard Yang added 2 comments

Patchset-level comments

File-level comment, Patchset 2 (Latest):

Howard Yang . resolved

Hi reviewers, please help me review this CL, thanks.

File ash/birch/birch_coral_provider.cc

Line 360, Patchset 2 (Latest): base::NullCallback());

Howard Yang . resolved

@zx...@google.com after you implemented the birch model notify for coral, simply change this null callback to binding OnGroupTitleUpdated and it should work.

Open in Gerrit

Related details

Attention is currently required from:

Ahmed Fakhry
Xiaodan Zhu

Submit Requirements:

Code-Coverage
Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Xiaodan Zhu (Gerrit)

unread,

Oct 17, 2024, 5:35:17 PM10/17/24

to Howard Yang, Ahmed Fakhry, Chromium LUCI CQ, Tricium, chromium...@chromium.org, ipc-securi...@chromium.org, oshima...@chromium.org

Attention needed from Ahmed Fakhry and Howard Yang

Xiaodan Zhu added 3 comments

Patchset-level comments

File-level comment, Patchset 5 (Latest):

Xiaodan Zhu . resolved

Thanks for the backend work!

File ash/wm/coral/coral_controller.h

Line 120, Patchset 5 (Latest): base::RepeatingCallback<void(const base::Token&, const std::string&)>

Xiaodan Zhu . unresolved

nit: this is used multiple times, shall we give it a short name with using, e.g. TitleUpdatedCallback?

File ash/wm/coral/coral_controller.cc

Line 96, Patchset 5 (Latest): // When receiving a new request, reset the previous title observer, if any.

Xiaodan Zhu . unresolved

q: will BirchCoralProvider::OnGroupTitleUpdated be used as the title changed callback? If so, why do we need to update the callback everytime? Is it because we may switch between sync and async title requests?

Open in Gerrit

Related details

Attention is currently required from:

Ahmed Fakhry
Howard Yang

Submit Requirements:

Code-Coverage
Code-Review

No-Unresolved-Comments

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Sammie Quon (Gerrit)

unread,

Oct 17, 2024, 7:04:49 PM10/17/24

to Howard Yang, Ahmed Fakhry, Xiaodan Zhu, Chromium LUCI CQ, Tricium, chromium...@chromium.org, ipc-securi...@chromium.org, oshima...@chromium.org

Attention needed from Howard Yang and Xiaodan Zhu

Sammie Quon added 3 comments

File ash/wm/coral/coral_controller.h

Line 75, Patchset 5 (Latest): // If |on_title_updated| is non-null, the backend will function in an async

Sammie Quon . unresolved

super nit: new way is to wrap using backticks `` instead of ||

File ash/wm/coral/coral_controller.cc

Line 96, Patchset 5 (Latest): // When receiving a new request, reset the previous title observer, if any.

Xiaodan Zhu . unresolved

q: will BirchCoralProvider::OnGroupTitleUpdated be used as the title changed callback? If so, why do we need to update the callback everytime? Is it because we may switch between sync and async title requests?

Sammie Quon

i'd make birch coral provider a override of coral::mojom::TitleObserver, or some hybrid an do

if (auto coral_provider = BirchCoralProvider::Get()) {
  coral_provider->UpdateTitle();
  }
  
  not sure if Howard you tried either and if its feasible. From this CL alone it looks like the callbacks are unnecessary and add more complexity

File chromeos/ash/services/coral/public/mojom/coral_service.mojom

Line 131, Patchset 5 (Latest):// The interface is used to provide updates of the |Group.title| field inside

Sammie Quon . unresolved

super nit: The -> This

Open in Gerrit

Related details

Attention is currently required from:

Howard Yang
Xiaodan Zhu

Submit Requirements:

Code-Coverage
Code-Review
No-Unresolved-Comments

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Howard Yang (Gerrit)

unread,

Oct 18, 2024, 12:30:54 AM10/18/24

to Sammie Quon, Ahmed Fakhry, Xiaodan Zhu, Chromium LUCI CQ, Tricium, chromium...@chromium.org, ipc-securi...@chromium.org, oshima...@chromium.org

Attention needed from Sammie Quon and Xiaodan Zhu

Howard Yang voted and added 4 comments

Votes added by Howard Yang

Commit-Queue

+1

4 comments

File ash/wm/coral/coral_controller.h

Line 120, Patchset 5: base::RepeatingCallback<void(const base::Token&, const std::string&)>

Xiaodan Zhu . resolved

nit: this is used multiple times, shall we give it a short name with using, e.g. TitleUpdatedCallback?

Howard Yang

The callback is no longer used.

Line 75, Patchset 5: // If |on_title_updated| is non-null, the backend will function in an async

Sammie Quon . resolved

super nit: new way is to wrap using backticks `` instead of ||

Howard Yang

Done

File ash/wm/coral/coral_controller.cc

Line 96, Patchset 5: // When receiving a new request, reset the previous title observer, if any.

Xiaodan Zhu . resolved

q: will BirchCoralProvider::OnGroupTitleUpdated be used as the title changed callback? If so, why do we need to update the callback everytime? Is it because we may switch between sync and async title requests?

Sammie Quon

i'd make birch coral provider a override of coral::mojom::TitleObserver, or some hybrid an do

if (auto coral_provider = BirchCoralProvider::Get()) {
  coral_provider->UpdateTitle();
  }
  
  not sure if Howard you tried either and if its feasible. From this CL alone it looks like the callbacks are unnecessary and add more complexity

Howard Yang

Changed to make birch coral provider a TitleObserver, and make CoralController take a TitleObserver remote as param directly.

File chromeos/ash/services/coral/public/mojom/coral_service.mojom

Line 131, Patchset 5:// The interface is used to provide updates of the |Group.title| field inside

Sammie Quon . resolved

super nit: The -> This

Howard Yang

Since the CrOS mojom change has already landed, will fix in the next modification of this file.

Open in Gerrit

Related details

Attention is currently required from:

Sammie Quon
Xiaodan Zhu

Submit Requirements:

Code-Coverage
Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Xiaodan Zhu (Gerrit)

unread,

Oct 18, 2024, 3:17:21 AM10/18/24

to Howard Yang, Sammie Quon, Ahmed Fakhry, Chromium LUCI CQ, Tricium, chromium...@chromium.org, ipc-securi...@chromium.org, oshima...@chromium.org

Attention needed from Howard Yang and Sammie Quon

Xiaodan Zhu voted and added 1 comment

Votes added by Xiaodan Zhu

Code-Review

+1

1 comment

Patchset-level comments

File-level comment, Patchset 6 (Latest):

Xiaodan Zhu . resolved

LGTM.

Open in Gerrit

Related details

Attention is currently required from:

Howard Yang
Sammie Quon

Submit Requirements:

Code-Coverage
Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

open

diffy

Howard Yang (Gerrit)

unread,

Oct 18, 2024, 3:20:30 AM10/18/24

to Takashi Toyoshima, Mitsuru Oshima, Xiaodan Zhu, Sammie Quon, Ahmed Fakhry, Chromium LUCI CQ, Tricium, chromium...@chromium.org, ipc-securi...@chromium.org, oshima...@chromium.org

Attention needed from Mitsuru Oshima, Sammie Quon and Takashi Toyoshima

Howard Yang added 1 comment

Patchset-level comments

File-level comment, Patchset 6 (Latest):

Howard Yang . resolved

+Takashi and Mitsuru for the mojom file/BUILD owner approval. Thanks!

Open in Gerrit

Related details

Attention is currently required from:

Mitsuru Oshima
Sammie Quon
Takashi Toyoshima

Submit Requirements:

Code-Coverage
Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

open

diffy

Takashi Toyoshima (Gerrit)

unread,

Oct 18, 2024, 7:22:01 AM10/18/24

to Howard Yang, Mitsuru Oshima, Xiaodan Zhu, Sammie Quon, Ahmed Fakhry, Chromium LUCI CQ, Tricium, chromium...@chromium.org, ipc-securi...@chromium.org, oshima...@chromium.org

Attention needed from Howard Yang, Mitsuru Oshima and Sammie Quon

Takashi Toyoshima voted Code-Review+1

Code-Review

+1

Open in Gerrit

Related details

Attention is currently required from:

Howard Yang
Mitsuru Oshima
Sammie Quon

Submit Requirements:

Code-Coverage
Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

open

diffy

Mitsuru Oshima (Gerrit)

unread,

Oct 18, 2024, 4:19:48 PM10/18/24

to Howard Yang, James Cook, Takashi Toyoshima, Xiaodan Zhu, Sammie Quon, Ahmed Fakhry, Chromium LUCI CQ, Tricium, chromium...@chromium.org, ipc-securi...@chromium.org, oshima...@chromium.org

Attention needed from Howard Yang, James Cook and Sammie Quon

Mitsuru Oshima added 1 comment

Patchset-level comments

File-level comment, Patchset 6 (Latest):

Mitsuru Oshima . resolved

I think jamescook@ is better reviewer.

Open in Gerrit

Related details

Attention is currently required from:

Howard Yang
James Cook
Sammie Quon

Submit Requirements:

Code-Coverage
Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

open

diffy

James Cook (Gerrit)

unread,

Oct 18, 2024, 4:31:54 PM10/18/24

to Howard Yang, Takashi Toyoshima, Xiaodan Zhu, Sammie Quon, Ahmed Fakhry, Chromium LUCI CQ, Tricium, chromium...@chromium.org, ipc-securi...@chromium.org, oshima...@chromium.org

Attention needed from Howard Yang and Sammie Quon

James Cook voted and added 3 comments

Votes added by James Cook

Code-Review

+1

3 comments

Patchset-level comments

File-level comment, Patchset 6 (Latest):

James Cook . resolved

LGTM with nits. You may not need Ahmed, btw, we have similar ownership.

File ash/birch/birch_coral_provider.cc

Line 444, Patchset 6 (Latest): for (coral::mojom::GroupPtr& group : groups) {

James Cook . unresolved

optional nit: I would inline `response_->groups()` so the reader doesn't have to think about the vector reference

File ash/wm/coral/coral_controller.h

Line 79, Patchset 6 (Latest): // the backend will return the titles together with the response.

James Cook . unresolved

nit: I would also describe *why* it's built this way. I had to read the mojom to understand that title generation is slow. I presume returning the grouping quickly allows for better UX than waiting a long time for titles?

Open in Gerrit

Related details

Attention is currently required from:

Howard Yang
Sammie Quon

Submit Requirements:

Code-Coverage
Code-Review

No-Unresolved-Comments

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Howard Yang (Gerrit)

unread,

Oct 20, 2024, 10:58:33 AM10/20/24

to James Cook, Takashi Toyoshima, Xiaodan Zhu, Sammie Quon, Ahmed Fakhry, Chromium LUCI CQ, Tricium, chromium...@chromium.org, ipc-securi...@chromium.org, oshima...@chromium.org

Attention needed from Sammie Quon

Howard Yang voted and added 2 comments

Votes added by Howard Yang

Commit-Queue

+2

2 comments

File ash/birch/birch_coral_provider.cc

Line 444, Patchset 6: for (coral::mojom::GroupPtr& group : groups) {

James Cook . resolved

optional nit: I would inline `response_->groups()` so the reader doesn't have to think about the vector reference

Howard Yang

Done

File ash/wm/coral/coral_controller.h

Line 79, Patchset 6: // the backend will return the titles together with the response.

James Cook . resolved

nit: I would also describe *why* it's built this way. I had to read the mojom to understand that title generation is slow. I presume returning the grouping quickly allows for better UX than waiting a long time for titles?

Howard Yang

Done, I'll submit first and if there are some more suggestions about the comments I'll update in next CL.

Open in Gerrit

Related details

Attention is currently required from:

Sammie Quon

Submit Requirements:

Code-Coverage
Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

open

diffy

Chromium LUCI CQ (Gerrit)

unread,

Oct 20, 2024, 3:50:04 PM10/20/24

to Howard Yang, James Cook, Takashi Toyoshima, Xiaodan Zhu, Sammie Quon, Ahmed Fakhry, Tricium, chromium...@chromium.org, ipc-securi...@chromium.org, oshima...@chromium.org

Chromium LUCI CQ submitted the change with unreviewed changes

Unreviewed changes

6 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:

```
The name of the file: ash/wm/coral/coral_controller.h
Insertions: 4, Deletions: 0.

@@ -77,6 +77,10 @@
   // grouping is done, but with empty titles. Then, `title_observer` will be
   // triggered once for each group when their title is generated. If it's null,


   // the backend will return the titles together with the response.

+  // This design is because title generation may take significantly longer
+  // compared to rest of the grouping process, so receiving the response before
+  // title is updated will allow UI to show the groupings with a loading title,
+  // enhancing the user experience.
   using CoralResponseCallback =
       base::OnceCallback<void(std::unique_ptr<CoralResponse>)>;
   void GenerateContentGroups(
```
```
The name of the file: ash/birch/birch_coral_provider.cc
Insertions: 1, Deletions: 2.

@@ -440,8 +440,7 @@
 
 void BirchCoralProvider::TitleUpdated(const base::Token& id,
                                       const std::string& title) {
-  std::vector<coral::mojom::GroupPtr>& groups = response_->groups();
-  for (coral::mojom::GroupPtr& group : groups) {
+  for (coral::mojom::GroupPtr& group : response_->groups()) {
     if (group->id == id) {
       group->title = title;
       return;
```

Change information

Commit message:

[CORAL] Sync title observer mojom change

Sync the title observer mojom change from https://crrev.com/c/5933812,
and modify the CoralController interface accordingly. Currently, the
coral provider doesn't support notifying birch model updates yet, so the
update callback isn't passed to CoralController in this CL yet
(otherwise the coral chips will have no titles and won't show).

Bug: b:370851826

Test: CQ

Change-Id: I662425a1b48b4b03135e47aada2d152cf90bc1b5

Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5939233

Reviewed-by: Takashi Toyoshima <toyo...@chromium.org>

Reviewed-by: James Cook <jame...@chromium.org>

Reviewed-by: Xiaodan Zhu <zx...@chromium.org>

Commit-Queue: Howard Yang <hcy...@google.com>

Cr-Commit-Position: refs/heads/main@{#1371143}

Files:

M ash/birch/birch_coral_provider.cc
M ash/birch/birch_coral_provider.h
M ash/wm/coral/coral_controller.cc
M ash/wm/coral/coral_controller.h
M ash/wm/coral/fake_coral_service.cc
M ash/wm/coral/fake_coral_service.h
M chromeos/ash/services/coral/public/mojom/BUILD.gn
M chromeos/ash/services/coral/public/mojom/coral_service.mojom

Change size: M

Delta: 8 files changed, 86 insertions(+), 13 deletions(-)

Branch: refs/heads/main

Submit Requirements:

Code-Review: +1 by Xiaodan Zhu, +1 by Takashi Toyoshima, +1 by James Cook

Open in Gerrit

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

open

diffy

satisfied_requirement

Reply all

Reply to author

Forward

[CORAL] Sync title observer mojom change [chromium/src : main]

Howard Yang (Gerrit)

Howard Yang has uploaded the change for review

Commit message

Change diff

Change information

Related details

Howard Yang (Gerrit)

Howard Yang added 2 comments

Related details

Xiaodan Zhu (Gerrit)

Xiaodan Zhu added 3 comments

Related details

Sammie Quon (Gerrit)

Sammie Quon added 3 comments

Related details

Howard Yang (Gerrit)

Howard Yang voted and added 4 comments

Votes added by Howard Yang

4 comments

Related details

Xiaodan Zhu (Gerrit)

Xiaodan Zhu voted and added 1 comment

Votes added by Xiaodan Zhu

1 comment

Related details

Howard Yang (Gerrit)

Howard Yang added 1 comment

Related details

Takashi Toyoshima (Gerrit)

Takashi Toyoshima voted Code-Review+1

Related details

Mitsuru Oshima (Gerrit)

Mitsuru Oshima added 1 comment

Related details

James Cook (Gerrit)

James Cook voted and added 3 comments

Votes added by James Cook

3 comments

Related details

Howard Yang (Gerrit)

Howard Yang voted and added 2 comments

Votes added by Howard Yang

2 comments

Related details

Chromium LUCI CQ (Gerrit)

Chromium LUCI CQ submitted the change with unreviewed changes

Unreviewed changes

Change information