extensions: Tab group API visual data for desktop Android, part 3 [chromium/src : main]
James Cook (Gerrit)
James Cook added 1 comment![]( "Open in Gerrit")
Related details
- Zoraiz Naeem
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Zoraiz Naeem (Gerrit)
Zoraiz Naeem added 1 comment![]( "Open in Gerrit")
const tab_groups::TabGroupVisualData* visual_data = tab_group->visual_data();Before if a user wanted to just set the title, they would call this method, while keeping the other properties the same (color and is_collopased).
With the new methods, a user will override all the properties. It is the intended behavior?
Related details
- James Cook
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. |
James Cook (Gerrit)
James Cook added 2 comments![]( "Open in Gerrit")
Zoraiz, please take another look.
const tab_groups::TabGroupVisualData* visual_data = tab_group->visual_data();Before if a user wanted to just set the title, they would call this method, while keeping the other properties the same (color and is_collopased).
With the new methods, a user will override all the properties. It is the intended behavior?
Yes, this is the way in generally works on Win/Mac/Linux/ChromeOS. The caller is responsible for constructing a TabGroupVisualData with only the fields they want to change. For example,, see TabStripModel::ChangeTabGroupVisuals() which also takes a TabGroupVisualData parameter.
Related details
- Zoraiz Naeem
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Zoraiz Naeem (Gerrit)
Zoraiz Naeem voted and added 2 comments![]( "Open in Gerrit")
Votes added by Zoraiz Naeem
| Code-Review | +1 |
2 comments
const tab_groups::TabGroupVisualData* visual_data = tab_group->visual_data();James CookBefore if a user wanted to just set the title, they would call this method, while keeping the other properties the same (color and is_collopased).
With the new methods, a user will override all the properties. It is the intended behavior?
Yes, this is the way in generally works on Win/Mac/Linux/ChromeOS. The caller is responsible for constructing a TabGroupVisualData with only the fields they want to change. For example,, see TabStripModel::ChangeTabGroupVisuals() which also takes a TabGroupVisualData parameter.
Not a greatest fan, for delegating the responsibility to the user but it is what it is. Thanks for the explanation!
Related details
- James Cook
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
James Cook (Gerrit)
James Cook added 1 comment![]( "Open in Gerrit")
Thanks, Zoraiz! Owners, please take a look:
Calder: //chrome/android and //chrome/browser/ui/android
Mike: //chrome/browser/ui (non-android)
Related details
- Calder Kitagawa
- Mike Wasserman
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
James Cook (Gerrit)
James Cook added 1 comment![]( "Open in Gerrit")
@CalledByNativeThese methods already exist in TabCollectionTabModelImpl, the implementation of TabModelJniBridge. I'm just exposing them via JNI.
Related details
- Calder Kitagawa
- Mike Wasserman
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. |
Calder Kitagawa (Gerrit)
Calder Kitagawa voted and added 3 comments![]( "Open in Gerrit")
Votes added by Calder Kitagawa
| Code-Review | +1 |
3 comments
LGTM
@CalledByNativeThese methods already exist in TabCollectionTabModelImpl, the implementation of TabModelJniBridge. I'm just exposing them via JNI.
Ack. SGTM
Java_TabModelJniBridge_setTabGroupTitle(env, jobj, group_id.token(),
visual_data.title());
// The cast is safe because the enum values are synced across C++ and Java.
Java_TabModelJniBridge_setTabGroupColor(
env, jobj, group_id.token(), static_cast<jint>(visual_data.color()));
Java_TabModelJniBridge_setTabGroupCollapsed(env, jobj, group_id.token(),
visual_data.is_collapsed(),
/*animate=*/false);Optional: We could bundle these into one JNI method that takes title, color, and collapsed status/animate in one method and also does all the changes together on the TabCollectionTabModelImpl side. It would shave off up to 4 JNI calls which is a perf win and the API will be more aligned with desktop.
I can alternatively do this as a follow-up?
Related details
- James Cook
- Mike Wasserman
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. |
James Cook (Gerrit)
James Cook voted and added 2 comments![]( "Open in Gerrit")
Votes added by James Cook
| Auto-Submit | +1 |
2 comments
Mike, over to you.
Calder, thanks for the review!
Java_TabModelJniBridge_setTabGroupTitle(env, jobj, group_id.token(),
visual_data.title());
// The cast is safe because the enum values are synced across C++ and Java.
Java_TabModelJniBridge_setTabGroupColor(
env, jobj, group_id.token(), static_cast<jint>(visual_data.color()));
Java_TabModelJniBridge_setTabGroupCollapsed(env, jobj, group_id.token(),
visual_data.is_collapsed(),
/*animate=*/false);Optional: We could bundle these into one JNI method that takes title, color, and collapsed status/animate in one method and also does all the changes together on the TabCollectionTabModelImpl side. It would shave off up to 4 JNI calls which is a perf win and the API will be more aligned with desktop.
I can alternatively do this as a follow-up?
If you don't mind taking care of this in a follow-up, one method sounds great. (Frankly, I'm not very experienced with JNI/Java, and I've got a ton of other stuff to do to support this extension API.) Thanks for the offer, and let me know if I can help somehow.
Related details
- Calder Kitagawa
- Mike Wasserman
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. |
Calder Kitagawa (Gerrit)
Calder Kitagawa added 1 comment![]( "Open in Gerrit")
Java_TabModelJniBridge_setTabGroupTitle(env, jobj, group_id.token(),
visual_data.title());
// The cast is safe because the enum values are synced across C++ and Java.
Java_TabModelJniBridge_setTabGroupColor(
env, jobj, group_id.token(), static_cast<jint>(visual_data.color()));
Java_TabModelJniBridge_setTabGroupCollapsed(env, jobj, group_id.token(),
visual_data.is_collapsed(),
/*animate=*/false);James CookOptional: We could bundle these into one JNI method that takes title, color, and collapsed status/animate in one method and also does all the changes together on the TabCollectionTabModelImpl side. It would shave off up to 4 JNI calls which is a perf win and the API will be more aligned with desktop.
I can alternatively do this as a follow-up?
If you don't mind taking care of this in a follow-up, one method sounds great. (Frankly, I'm not very experienced with JNI/Java, and I've got a ton of other stuff to do to support this extension API.) Thanks for the offer, and let me know if I can help somehow.
@mf...@google.com you mentioned you have spare cycles could you do this?
Related details
- James Cook
- Mike Wasserman
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mike Wasserman (Gerrit)
Mike Wasserman voted and added 1 comment![]( "Open in Gerrit")
Votes added by Mike Wasserman
| Code-Review | +1 |
1 comment
c/b/ui lgtm
Related details
- James Cook
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
James Cook (Gerrit)
James Cook voted and added 1 comment![]( "Open in Gerrit")
Votes added by James Cook
| Auto-Submit | +0 |
| Commit-Queue | +2 |
1 comment
Thanks for the reviews, everyone!
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
extensions: Tab group API visual data for desktop Android, part 3
Prepare for supporting chrome.tabGroups.update() on desktop Android.
Add TabListInterface::SetTabGroupColor() and SetTabGroupCollapsed().
For Android, use the existing Java implementations of these methods in
TabCollectionTabModelImpl, just expose them via JNI. They are already
well tested in TabCollectionTabModelImplTest.
Extend the TabsGroupApiTest slightly for end-to-end coverage this
interface change.
- M chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabModelJniBridge.java
- M chrome/browser/extensions/api/tab_groups/tab_groups_api_apitest.cc
- M chrome/browser/extensions/extension_tab_util.h
- M chrome/browser/ui/android/tab_model/tab_model_jni_bridge.cc
- M chrome/browser/ui/android/tab_model/tab_model_jni_bridge.h
- M chrome/browser/ui/android/tab_model/tab_model_test_helper.cc
- M chrome/browser/ui/android/tab_model/tab_model_test_helper.h
- M chrome/browser/ui/tabs/tab_list_bridge.cc
- M chrome/browser/ui/tabs/tab_list_bridge.h
- M chrome/browser/ui/tabs/tab_list_interface.h
Code-Review: +1 by Mike Wasserman, +1 by Calder Kitagawa, +1 by Zoraiz Naeem
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |