Revert^2 "Move allowlist check to native code"
This reverts commit c23c78ae3ddc7a32f0678374c99323fbfeade98b.
Reason for revert: fix forward
Original change's description:
> Revert "Move allowlist check to native code"
>
> This reverts commit 8cd912804e039159d4ac665597e899e959c5815d.
>
> Reason for revert:
> LUCI Bisection has identified this change as the culprit of a build failure. See the analysis: https://ci.chromium.org/ui/p/chromium/bisection/compile-analysis/b/8762987001342534337
>
> Sample failed build: https://ci.chromium.org/b/8762987001342534337
>
> If this is a false positive, please report it at https://bugs.chromium.org/p/chromium/issues/entry?comment=Analysis%3A+https%3A%2F%2Fci.chromium.org%2Fui%2Fp%2Fchromium%2Fbisection%2Fcompile-analysis%2Fb%2F8762987001342534337&components=Tools%3ETest%3EFindit&labels=LUCI-Bisection-Wrong%2CPri-3%2CType-Bug&status=Available&summary=Wrongly+blamed+https%3A%2F%2Fchromium-review.googlesource.com%2Fc%2Fchromium%2Fsrc%2F%2B%2F5069216
>
> Original change's description:
> > Move allowlist check to native code
> >
> >
> > Bug: 1502402
> > Change-Id: Ic7725de4ae895aed36842d2c900c61202709e74f
> > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5069216
> > Auto-Submit: Hanfeng Zhu <[email protected]>
> > Reviewed-by: Min Qin <[email protected]>
> > Commit-Queue: Theresa Sullivan <[email protected]>
> > Reviewed-by: Theresa Sullivan <[email protected]>
> > Code-Coverage: [email protected] <[email protected]>
> > Cr-Commit-Position: refs/heads/main@{#1231341}
> >
>
> Bug: 1502402
> Change-Id: Iff1afffb6f957ccfbfba4b7bf41545ac3ab217da
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5077609
> Commit-Queue: [email protected] <[email protected]>
> Bot-Commit: [email protected] <[email protected]>
> Owners-Override: [email protected] <[email protected]>
> Cr-Commit-Position: refs/heads/main@{#1231349}
Bug: 1502402
Change-Id: I938ff9066f31e6660d064012215e67a7d8296cf9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5082917
Bot-Commit: Rubber Stamper <[email protected]>
Reviewed-by: Theresa Sullivan <[email protected]>
Commit-Queue: Hanfeng Zhu <[email protected]>
Reviewed-by: Min Qin <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1233096}
diff --git a/chrome/browser/BUILD.gn b/chrome/browser/BUILD.gn
index aeb8b9b..6572022 100644
--- a/chrome/browser/BUILD.gn
+++ b/chrome/browser/BUILD.gn
@@ -3408,6 +3408,8 @@
"usb/android/web_usb_chooser_android.cc",
"usb/android/web_usb_chooser_android.h",
"wallet/android/boarding_pass_bridge.cc",
+ "wallet/android/boarding_pass_detector.cc",
+ "wallet/android/boarding_pass_detector.h",
"webapps/pwa_restore_promo_utils_android.cc",
"webapps/web_app_offline_android.cc",
"webauthn/android/cable_module_android.cc",
diff --git a/chrome/browser/wallet/android/BUILD.gn b/chrome/browser/wallet/android/BUILD.gn
index ff1cc6a6..40240dd8 100644
--- a/chrome/browser/wallet/android/BUILD.gn
+++ b/chrome/browser/wallet/android/BUILD.gn
@@ -31,11 +31,13 @@
sources = [ "javatests/src/org/chromium/chrome/browser/wallet/BoardingPassControllerTest.java" ]
deps = [
"//base:base_java",
+ "//base:base_java_test_support",
"//base:base_junit_test_support",
"//chrome/browser/flags:java",
"//chrome/browser/tab:java",
"//chrome/browser/wallet/android:java",
"//third_party/android_deps:guava_android_java",
+ "//third_party/jni_zero:jni_zero_java",
"//third_party/junit:junit",
"//third_party/mockito:mockito_java",
"//url:url_java",
diff --git a/chrome/browser/wallet/android/boarding_pass_bridge.cc b/chrome/browser/wallet/android/boarding_pass_bridge.cc
index 3d641fc7..be8cf35 100644
--- a/chrome/browser/wallet/android/boarding_pass_bridge.cc
+++ b/chrome/browser/wallet/android/boarding_pass_bridge.cc
@@ -2,13 +2,22 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
+#include "base/android/jni_string.h"
#include "base/android/scoped_java_ref.h"
+#include "chrome/browser/wallet/android/boarding_pass_detector.h"
#include "chrome/browser/wallet/android/jni_headers/BoardingPassBridge_jni.h"
+using base::android::ConvertJavaStringToUTF8;
using base::android::JavaParamRef;
namespace wallet {
+static jboolean JNI_BoardingPassBridge_ShouldDetect(
+ JNIEnv* env,
+ const JavaParamRef<jstring>& jurl) {
+ return BoardingPassDetector::ShouldDetect(ConvertJavaStringToUTF8(env, jurl));
+}
+
static void JNI_BoardingPassBridge_DetectBoardingPass(
JNIEnv* env,
const JavaParamRef<jobject>& jcallback) {
diff --git a/chrome/browser/wallet/android/boarding_pass_detector.cc b/chrome/browser/wallet/android/boarding_pass_detector.cc
new file mode 100644
index 0000000..c5efb18
--- /dev/null
+++ b/chrome/browser/wallet/android/boarding_pass_detector.cc
@@ -0,0 +1,29 @@
+// Copyright 2023 The Chromium Authors
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/wallet/android/boarding_pass_detector.h"
+
+#include "base/feature_list.h"
+#include "base/strings/string_split.h"
+#include "chrome/common/chrome_features.h"
+
+namespace wallet {
+
+bool BoardingPassDetector::ShouldDetect(const std::string& url) {
+ std::string param_val = base::GetFieldTrialParamValueByFeature(
+ features::kBoardingPassDetector,
+ features::kBoardingPassDetectorUrlParam.name);
+
+ std::vector<std::string> allowed_urls =
+ base::SplitString(std::move(param_val), ",", base::TRIM_WHITESPACE,
+ base::SPLIT_WANT_NONEMPTY);
+ for (const auto& allowed_url : allowed_urls) {
+ if (url.starts_with(allowed_url)) {
+ return true;
+ }
+ }
+ return false;
+}
+
+} // namespace wallet
diff --git a/chrome/browser/wallet/android/boarding_pass_detector.h b/chrome/browser/wallet/android/boarding_pass_detector.h
new file mode 100644
index 0000000..6cc22eb1
--- /dev/null
+++ b/chrome/browser/wallet/android/boarding_pass_detector.h
@@ -0,0 +1,20 @@
+// Copyright 2023 The Chromium Authors
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CHROME_BROWSER_WALLET_ANDROID_BOARDING_PASS_DETECTOR_H_
+#define CHROME_BROWSER_WALLET_ANDROID_BOARDING_PASS_DETECTOR_H_
+
+#include <string>
+
+namespace wallet {
+
+class BoardingPassDetector {
+ public:
+ // Decides whether to run boarding pass detection on given url.
+ static bool ShouldDetect(const std::string& url);
+};
+
+} // namespace wallet
+
+#endif // CHROME_BROWSER_WALLET_ANDROID_BOARDING_PASS_DETECTOR_H_
diff --git a/chrome/browser/wallet/android/boarding_pass_detector_unittest.cc b/chrome/browser/wallet/android/boarding_pass_detector_unittest.cc
new file mode 100644
index 0000000..8ed517e
--- /dev/null
+++ b/chrome/browser/wallet/android/boarding_pass_detector_unittest.cc
@@ -0,0 +1,39 @@
+// Copyright 2023 The Chromium Authors
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/wallet/android/boarding_pass_detector.h"
+
+#include "base/test/scoped_feature_list.h"
+#include "chrome/common/chrome_features.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace wallet {
+class BoardingPassDetectorTest : public ::testing::Test {
+ public:
+ void SetAllowList(const std::string& allowlist) {
+ feature_list_.InitAndEnableFeatureWithParameters(
+ features::kBoardingPassDetector,
+ {{features::kBoardingPassDetectorUrlParam.name, allowlist}});
+ }
+
+ private:
+ base::test::ScopedFeatureList feature_list_;
+};
+
+TEST_F(BoardingPassDetectorTest, ShouldDetect) {
+ SetAllowList("https://aa.com/adc, https://www.google.com/boarding");
+
+ EXPECT_TRUE(BoardingPassDetector::ShouldDetect("https://aa.com/adc"));
+ EXPECT_TRUE(BoardingPassDetector::ShouldDetect(
+ "https://www.google.com/boarding/abc"));
+}
+
+TEST_F(BoardingPassDetectorTest, ShouldNotDetect) {
+ SetAllowList("https://aa.com/adc, https://www.google.com/boarding");
+
+ EXPECT_FALSE(BoardingPassDetector::ShouldDetect("https://aa.com/"));
+ EXPECT_FALSE(
+ BoardingPassDetector::ShouldDetect("https://www.google.com/abc"));
+}
+} // namespace wallet
diff --git a/chrome/browser/wallet/android/java/src/org/chromium/chrome/browser/wallet/BoardingPassBridge.java b/chrome/browser/wallet/android/java/src/org/chromium/chrome/browser/wallet/BoardingPassBridge.java
index 2ffd0a3..6dc980a0 100644
--- a/chrome/browser/wallet/android/java/src/org/chromium/chrome/browser/wallet/BoardingPassBridge.java
+++ b/chrome/browser/wallet/android/java/src/org/chromium/chrome/browser/wallet/BoardingPassBridge.java
@@ -20,6 +20,16 @@
private BoardingPassBridge() {}
/**
+ * Decides whether to run boarding pass detection on given url.
+ *
+ * @param url URL of the current page.
+ * @return whether to run detection
+ */
+ public static boolean shouldDetect(String url) {
+ return BoardingPassBridgeJni.get().shouldDetect(url);
+ }
+
+ /**
* Detects boarding pass barcode raw string from web page.
*
* @return Detected boarding passes raw string.
@@ -38,6 +48,7 @@
@NativeMethods
public interface Natives {
+ boolean shouldDetect(String url);
void detectBoardingPass(Callback<String[]> callback);
}
diff --git a/chrome/browser/wallet/android/java/src/org/chromium/chrome/browser/wallet/BoardingPassController.java b/chrome/browser/wallet/android/java/src/org/chromium/chrome/browser/wallet/BoardingPassController.java
index c98442e..9f108e9a 100644
--- a/chrome/browser/wallet/android/java/src/org/chromium/chrome/browser/wallet/BoardingPassController.java
+++ b/chrome/browser/wallet/android/java/src/org/chromium/chrome/browser/wallet/BoardingPassController.java
@@ -6,15 +6,11 @@
import org.chromium.base.Log;
import org.chromium.base.supplier.ObservableSupplier;
-import org.chromium.chrome.browser.flags.ChromeFeatureList;
import org.chromium.chrome.browser.tab.CurrentTabObserver;
import org.chromium.chrome.browser.tab.EmptyTabObserver;
import org.chromium.chrome.browser.tab.Tab;
import org.chromium.url.GURL;
-import java.util.ArrayList;
-import java.util.List;
-
/** Controls the whole flow of boarding pass detection. */
public class BoardingPassController {
private static final String TAG = "BoardingPassCtrl";
@@ -31,7 +27,7 @@
return new EmptyTabObserver() {
@Override
public void onPageLoadFinished(Tab tab, GURL url) {
- if (shouldDetect(url.getSpec())) {
+ if (BoardingPassBridge.shouldDetect(url.getSpec())) {
Log.d(TAG, "Detect boarding pass on url: %s", url.getSpec());
}
}
@@ -41,26 +37,4 @@
public void destroy() {
mCurrentTabObserver.destroy();
}
-
- private static boolean shouldDetect(String url) {
- // TODO(crbug/1502330): Move shouldDetect logic to native code.
- List<String> allowedUrls = getAllowedUrls();
- Log.d(TAG, "allowed urls: %s, size: %d", allowedUrls, allowedUrls.size());
- return allowedUrls.stream().anyMatch(urlPrefix -> url.startsWith(urlPrefix));
- }
-
- private static List<String> getAllowedUrls() {
- String paramVal =
- ChromeFeatureList.getFieldTrialParamByFeature(
- ChromeFeatureList.BOARDING_PASS_DETECTOR, "boarding_pass_detector_urls");
- String[] urls = paramVal.trim().split(",");
- List<String> allowedUrls = new ArrayList<>();
- for (String url : urls) {
- String trimedUrl = url.trim();
- if (!trimedUrl.isEmpty()) {
- allowedUrls.add(trimedUrl);
- }
- }
- return allowedUrls;
- }
}
diff --git a/chrome/browser/wallet/android/javatests/src/org/chromium/chrome/browser/wallet/BoardingPassControllerTest.java b/chrome/browser/wallet/android/javatests/src/org/chromium/chrome/browser/wallet/BoardingPassControllerTest.java
index 7524f95..1a3c82c 100644
--- a/chrome/browser/wallet/android/javatests/src/org/chromium/chrome/browser/wallet/BoardingPassControllerTest.java
+++ b/chrome/browser/wallet/android/javatests/src/org/chromium/chrome/browser/wallet/BoardingPassControllerTest.java
@@ -6,10 +6,14 @@
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertTrue;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.eq;
import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
import org.junit.After;
import org.junit.Before;
+import org.junit.Rule;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.mockito.ArgumentCaptor;
@@ -20,10 +24,9 @@
import org.robolectric.shadows.ShadowLog;
import org.chromium.base.Callback;
-import org.chromium.base.FeatureList;
import org.chromium.base.supplier.ObservableSupplier;
import org.chromium.base.test.BaseRobolectricTestRunner;
-import org.chromium.chrome.browser.flags.ChromeFeatureList;
+import org.chromium.base.test.util.JniMocker;
import org.chromium.chrome.browser.tab.Tab;
import org.chromium.chrome.browser.tab.TabObserver;
import org.chromium.url.GURL;
@@ -35,10 +38,14 @@
@Config(manifest = Config.NONE)
public class BoardingPassControllerTest {
+ @Rule public JniMocker mJniMocker = new JniMocker();
+
@Mock private ObservableSupplier<Tab> mMockTabProvider;
@Mock private Tab mMockTab;
+ @Mock private BoardingPassBridge.Natives mMockBoardingPassBridgeJni;
+
@Captor private ArgumentCaptor<Callback<Tab>> mTabSupplierCallbackCaptor;
@Captor private ArgumentCaptor<TabObserver> mTabObserverCaptor;
@@ -48,6 +55,7 @@
@Before
public void setUp() {
MockitoAnnotations.initMocks(this);
+ mJniMocker.mock(BoardingPassBridgeJni.TEST_HOOKS, mMockBoardingPassBridgeJni);
createControllerAndVerify();
}
@@ -58,24 +66,24 @@
@Test
public void detectBoardingPass_urlAllowed() {
- setAllowedUrls("https://google.com, https://abc");
+ when(mMockBoardingPassBridgeJni.shouldDetect(any())).thenReturn(true);
mTabObserverCaptor
.getValue()
.onPageLoadFinished(mMockTab, new GURL("https://abc/boarding"));
List<ShadowLog.LogItem> logs = ShadowLog.getLogs();
assertTrue(logs.get(logs.size() - 1).msg.matches(".*Detect boarding pass on url:.*"));
+ verify(mMockBoardingPassBridgeJni).shouldDetect(eq("https://abc/boarding"));
}
@Test
public void detectBoardingPass_urlNotAllowed() {
- setAllowedUrls("");
- mTabObserverCaptor
- .getValue()
- .onPageLoadFinished(mMockTab, new GURL("https://abc/boarding"));
+ when(mMockBoardingPassBridgeJni.shouldDetect(any())).thenReturn(false);
+ mTabObserverCaptor.getValue().onPageLoadFinished(mMockTab, new GURL("https://abc/123"));
List<ShadowLog.LogItem> logs = ShadowLog.getLogs();
assertFalse(logs.get(logs.size() - 1).msg.matches(".*Detect boarding pass on url:.*"));
+ verify(mMockBoardingPassBridgeJni).shouldDetect(eq("https://abc/123"));
}
private void createControllerAndVerify() {
@@ -90,13 +98,4 @@
verify(mMockTab).removeObserver(mTabObserverCaptor.getValue());
verify(mMockTabProvider).removeObserver(mTabSupplierCallbackCaptor.getValue());
}
-
- private void setAllowedUrls(String allowedurls) {
- FeatureList.TestValues testValues = new FeatureList.TestValues();
- testValues.addFieldTrialParamOverride(
- ChromeFeatureList.BOARDING_PASS_DETECTOR,
- "boarding_pass_detector_urls",
- allowedurls);
- FeatureList.setTestValues(testValues);
- }
}
diff --git a/chrome/test/BUILD.gn b/chrome/test/BUILD.gn
index 1ef3a9d..f4cfa6b 100644
--- a/chrome/test/BUILD.gn
+++ b/chrome/test/BUILD.gn
@@ -7526,6 +7526,7 @@
"../browser/ui/autofill/payments/autofill_snackbar_controller_impl_unittest.cc",
"../browser/ui/autofill/payments/virtual_card_enroll_bubble_controller_impl_unittest.cc",
"../browser/ui/fast_checkout/fast_checkout_controller_impl_unittest.cc",
+ "../browser/wallet/android/boarding_pass_detector_unittest.cc",
"../browser/webauthn/android/cable_module_android_unittest.cc",
"../browser/webauthn/android/cable_registration_state_unittest.cc",
]
