Skip to content

Commit

Permalink
Strip not created actions from final array of actions passed to Java.
Browse files Browse the repository at this point in the history
This change guarantees actions not created do not take a slot
in the final array of actions that gets passed to Java.

The change utilizes `std::vector` to track the count of actions that
do get passed, and suppresses passing control over to Java when
- no actions are applicable to Android, or
- actions are not ready to be passed (e.g. the control comes from an
  intent handler).

The change replaces the former mechanism used to cache reference to
jclass with an updated mechanism that better aligns with Clank JNI
solutions.

(cherry picked from commit aefab5d)

Bug: 1450106
Change-Id: I958ccbc57dd727edc3f7fd5acb8578edafb38fde
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4575458
Reviewed-by: Fred Mello <fredmello@chromium.org>
Auto-Submit: Tomasz Wiszkowski <ender@google.com>
Code-Coverage: Findit <findit-for-me@appspot.gserviceaccount.com>
Commit-Queue: Tomasz Wiszkowski <ender@google.com>
Cr-Original-Commit-Position: refs/heads/main@{#1151320}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4585081
Commit-Queue: Krishna Govind <govind@chromium.org>
Reviewed-by: Krishna Govind <govind@chromium.org>
Owners-Override: Krishna Govind <govind@chromium.org>
Reviewed-by: Tomasz Wiszkowski <ender@google.com>
Cr-Commit-Position: refs/branch-heads/5790@{#284}
Cr-Branched-From: 1d71a33-refs/heads/main@{#1148114}
  • Loading branch information
tomasz-wiszkowski authored and Chromium LUCI CQ committed Jun 2, 2023
1 parent 446efdc commit b965bed
Show file tree
Hide file tree
Showing 2 changed files with 31 additions and 22 deletions.
Expand Up @@ -19,14 +19,9 @@ namespace {
// base class.
const char kOmniboxActionClass[] =
"org/chromium/components/omnibox/action/OmniboxAction";
JNI_REGISTRATION_EXPORT std::atomic<jclass>
g_org_chromium_components_omnibox_action_OmniboxAction_clazz(nullptr);
inline jclass org_chromium_components_omnibox_action_OmniboxAction_clazz(
JNIEnv* env) {
return base::android::LazyGetClass(
env, kOmniboxActionClass,
&g_org_chromium_components_omnibox_action_OmniboxAction_clazz);
}

base::LazyInstance<base::android::ScopedJavaGlobalRef<jclass>>::DestructorAtExit
g_java_omnibox_action = LAZY_INSTANCE_INITIALIZER;

base::LazyInstance<base::android::ScopedJavaGlobalRef<jobject>>::
DestructorAtExit g_java_factory = LAZY_INSTANCE_INITIALIZER;
Expand All @@ -35,7 +30,14 @@ base::LazyInstance<base::android::ScopedJavaGlobalRef<jobject>>::
/* static */ void JNI_OmniboxActionFactory_SetFactory(
JNIEnv* env,
const base::android::JavaParamRef<jobject>& factory) {
g_java_factory.Get().Reset(factory);
if (factory) {
g_java_omnibox_action.Get().Reset(
base::android::GetClass(env, kOmniboxActionClass));
g_java_factory.Get().Reset(factory);
} else {
g_java_omnibox_action.Get().Reset(nullptr);
g_java_factory.Get().Reset(nullptr);
}
}

base::android::ScopedJavaGlobalRef<jobject> BuildOmniboxPedal(
Expand Down Expand Up @@ -76,20 +78,27 @@ base::android::ScopedJavaGlobalRef<jobject> BuildOmniboxActionInSuggest(
base::android::ScopedJavaLocalRef<jobjectArray> ToJavaOmniboxActionsList(
JNIEnv* env,
const std::vector<scoped_refptr<OmniboxAction>>& actions) {
jclass clazz =
org_chromium_components_omnibox_action_OmniboxAction_clazz(env);
// Fires if OmniboxAction is not part of this build target.
DCHECK(clazz);
base::android::ScopedJavaLocalRef<jobjectArray> jactions(
env, env->NewObjectArray(actions.size(), clazz, nullptr));
base::android::CheckException(env);
// Early return for cases where Action creation is not yet possible, e.g.
// if the control is passed from the IntentHandler.
if (!g_java_omnibox_action.IsCreated() || !g_java_factory.IsCreated()) {
return {};
}

for (size_t index = 0; index < actions.size(); index++) {
auto jobj = actions[index]->GetOrCreateJavaObject(env);
std::vector<base::android::ScopedJavaLocalRef<jobject>> jactions_vec;
for (const auto& action : actions) {
auto jobj = action->GetOrCreateJavaObject(env);
if (jobj) {
env->SetObjectArrayElement(jactions.obj(), index, jobj.obj());
jactions_vec.emplace_back(std::move(jobj));
}
}

return jactions;
// Return only after all actions are created to capture cases where some
// actions were found, but none was applicable to Android.
if (!jactions_vec.size()) {
return {};
}

return base::android::ToTypedJavaArrayOfObjects(
env, jactions_vec,
base::android::ScopedJavaLocalRef<jclass>(g_java_omnibox_action.Get()));
}
Expand Up @@ -158,7 +158,7 @@ private static AutocompleteMatch build(long nativeObject, int nativeType, int[]
GURL url, GURL imageUrl, String imageDominantColor, boolean isDeletable,
String postContentType, byte[] postData, int groupId, List<QueryTile> tiles,
byte[] clipboardImageData, boolean hasTabMatch, String[] suggestTileTitles,
GURL[] suggestTileUrls, int[] suggestTileTypes, OmniboxAction[] actions) {
GURL[] suggestTileUrls, int[] suggestTileTypes, @Nullable OmniboxAction[] actions) {
assert contentClassificationOffsets.length == contentClassificationStyles.length;
List<MatchClassification> contentClassifications = new ArrayList<>();
for (int i = 0; i < contentClassificationOffsets.length; i++) {
Expand All @@ -183,7 +183,7 @@ private static AutocompleteMatch build(long nativeObject, int nativeType, int[]
relevance, transition, contents, contentClassifications, description,
new ArrayList<>(), answer, fillIntoEdit, url, imageUrl, imageDominantColor,
isDeletable, postContentType, postData, groupId, tiles, clipboardImageData,
hasTabMatch, suggestTiles, Arrays.asList(actions));
hasTabMatch, suggestTiles, actions == null ? null : Arrays.asList(actions));
match.updateNativeObjectRef(nativeObject);
match.setDescription(
description, descriptionClassificationOffsets, descriptionClassificationStyles);
Expand Down

0 comments on commit b965bed

Please sign in to comment.