Commit c8026dea authored by Haiyang Pan's avatar Haiyang Pan Committed by Commit Bot

Revert "Make ContentCapture ready for experiment"

This reverts commit 564da6d9.

Reason for revert: Very likely the cause of crbug.com/1122472
See the analysis in crbug.com/1123677#c4 for detail

Original change's description:
> Make ContentCapture ready for experiment
> 
> Currently the triggering of ContentCapture is unpredictable, it is
> hard for us to get the unbiased result for the UserActivatedDelay
> experiment.
> 
> This patch adds an ExperimentContentCaptureConsumer which triggers
> the ContentCapture independently and is enabled by default.
> 
> The allow list check is moved to consumer if the experiment
> is enabled.
> 
> As ContentCapture is potentially used for all Android versions,
> we plan to run the experiment for all of them.
> 
> This mechanism will also be used for future experiments.
> 
> This patch hasn't supported multiple consumers yet.
> 
> Test: Added new tests and pass the existing tests
> 
> Bug: 1114819, 1119663
> Change-Id: I72e0b991767329caec37080caae2d5c2a9068eaf
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2368104
> Commit-Queue: Tao Bai <michaelbai@chromium.org>
> Reviewed-by: David Trainor <dtrainor@chromium.org>
> Reviewed-by: Changwan Ryu <changwan@chromium.org>
> Reviewed-by: Xianzhu Wang <wangxianzhu@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#801672}

TBR=michaelbai@chromium.org,wangxianzhu@chromium.org,dtrainor@chromium.org,changwan@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 1114819
Bug: 1119663
Bug: 1122472
Change-Id: Ife98d73f4e77062606f18adb0ba12e9baaf4f6c3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2386578Reviewed-by: default avatarHaiyang Pan <hypan@google.com>
Commit-Queue: Haiyang Pan <hypan@google.com>
Cr-Commit-Position: refs/heads/master@{#803349}
parent bfd26dd3
......@@ -5,7 +5,6 @@
package org.chromium.android_webview.test;
import android.graphics.Rect;
import android.net.Uri;
import androidx.test.filters.LargeTest;
import androidx.test.filters.SmallTest;
......@@ -24,7 +23,6 @@ import org.chromium.base.test.util.Feature;
import org.chromium.components.content_capture.ContentCaptureConsumer;
import org.chromium.components.content_capture.ContentCaptureController;
import org.chromium.components.content_capture.ContentCaptureData;
import org.chromium.components.content_capture.ExperimentContentCaptureConsumer;
import org.chromium.components.content_capture.FrameSession;
import org.chromium.content_public.browser.WebContents;
import org.chromium.content_public.browser.test.util.TestThreadUtils;
......@@ -59,7 +57,6 @@ public class AwContentCaptureTest {
String[] allowlist = null;
boolean[] isRegEx = null;
if (mAllowlist != null && mIsRegEx != null) {
allowlist = new String[mAllowlist.size()];
mAllowlist.toArray(allowlist);
isRegEx = new boolean[mAllowlist.size()];
int i = 0;
......@@ -70,13 +67,6 @@ public class AwContentCaptureTest {
setAllowlist(allowlist, isRegEx);
}
public void setAllowURL(String host) {
mAllowlist = new ArrayList<String>();
mAllowlist.add(host);
mIsRegEx = new ArrayList<Boolean>();
mIsRegEx.add(Boolean.FALSE);
}
private ArrayList<String> mAllowlist;
private ArrayList<Boolean> mIsRegEx;
}
......@@ -94,10 +84,6 @@ public class AwContentCaptureTest {
mCapturedContentIds = new HashSet<Long>();
}
public void setContentCaptureController(ContentCaptureController controller) {
mController = controller;
}
@Override
public void onContentCaptured(
FrameSession parentFrame, ContentCaptureData contentCaptureData) {
......@@ -138,12 +124,6 @@ public class AwContentCaptureTest {
mCallbackHelper.notifyCalled();
}
@Override
public boolean shouldCapture(String[] urls) {
if (mController == null) return true;
return mController.shouldCapture(urls);
}
public FrameSession getParentFrame() {
return mParentFrame;
}
......@@ -207,7 +187,6 @@ public class AwContentCaptureTest {
// Use our own call count to avoid unexpected callback issue.
private int mCallCount;
// TODO: (crbug.com/1121827) Remove volatile if possible.
private volatile Set<Long> mCapturedContentIds;
private volatile FrameSession mParentFrame;
private volatile ContentCaptureData mCapturedContent;
......@@ -218,7 +197,6 @@ public class AwContentCaptureTest {
private volatile ArrayList<Integer> mCallbacks = new ArrayList<Integer>();
private CallbackHelper mCallbackHelper = new CallbackHelper();
private volatile ContentCaptureController mController;
}
private static final String MAIN_FRAME_FILE = "/main_frame.html";
......@@ -234,7 +212,6 @@ public class AwContentCaptureTest {
private AwTestContainerView mContainerView;
private TestAwContentCaptureConsumer mConsumer;
private TestAwContentCatpureController mController;
private TestAwContentCaptureConsumer mSecondConsumer;
private void loadUrlSync(String url) {
try {
......@@ -605,86 +582,4 @@ public class AwContentCaptureTest {
}, toIntArray(TestAwContentCaptureConsumer.SESSION_REMOVED));
verifyFrameSession(removedSession, mConsumer.getRemovedSession());
}
@Test
@SmallTest
@Feature({"AndroidWebView"})
public void testMultipleConsumers() throws Throwable {
TestThreadUtils.runOnUiThreadBlocking(() -> {
mSecondConsumer = new TestAwContentCaptureConsumer(mAwContents.getWebContents());
});
final String response = "<html><head></head><body>"
+ "<div id='place_holder'>"
+ "<p style=\"height: 100vh\">Hello</p>"
+ "<p>world</p>"
+ "</body></html>";
final String url = mWebServer.setResponse(MAIN_FRAME_FILE, response, null);
runAndVerifyCallbacks(() -> {
loadUrlSync(url);
}, toIntArray(TestAwContentCaptureConsumer.CONTENT_CAPTURED));
// Verify the other one also get the content.
verifyCallbacks(toIntArray(TestAwContentCaptureConsumer.CONTENT_CAPTURED),
mSecondConsumer.getCallbacks());
}
@Test
@SmallTest
@Feature({"AndroidWebView"})
@CommandLineFlags.Add({"enable-features=ContentCaptureTriggeringForExperiment"})
public void testHostNotAllowed() throws Throwable {
TestThreadUtils.runOnUiThreadBlocking(() -> {
mSecondConsumer = new TestAwContentCaptureConsumer(mAwContents.getWebContents());
});
final String response = "<html><head></head><body>"
+ "<div id='place_holder'>"
+ "<p style=\"height: 100vh\">Hello</p>"
+ "<p>world</p>"
+ "</body></html>";
final String url = mWebServer.setResponse(MAIN_FRAME_FILE, response, null);
mController.setAllowURL("www.chromium.org");
mSecondConsumer.setContentCaptureController(mController);
runAndVerifyCallbacks(() -> {
loadUrlSync(url);
}, toIntArray(TestAwContentCaptureConsumer.CONTENT_CAPTURED));
// Verify the other one didn't get the content.
Assert.assertEquals(0, mSecondConsumer.getCallbacks().length);
}
private void runHostAllowedTest() throws Throwable {
final String response = "<html><head></head><body>"
+ "<div id='place_holder'>"
+ "<p style=\"height: 100vh\">Hello</p>"
+ "<p>world</p>"
+ "</body></html>";
final String url = mWebServer.setResponse(MAIN_FRAME_FILE, response, null);
mController.setAllowURL(Uri.parse(url).getHost());
mConsumer.setContentCaptureController(mController);
runAndVerifyCallbacks(() -> {
loadUrlSync(url);
}, toIntArray(TestAwContentCaptureConsumer.CONTENT_CAPTURED));
}
@Test
@SmallTest
@Feature({"AndroidWebView"})
@CommandLineFlags.Add({"disable-features=ContentCaptureTriggeringForExperiment"})
public void testHostAllowed() throws Throwable {
runHostAllowedTest();
}
@Test
@SmallTest
@Feature({"AndroidWebView"})
@CommandLineFlags.Add({"enable-features=ContentCaptureTriggeringForExperiment"})
public void testHostAllowedForExperiment() throws Throwable {
runHostAllowedTest();
}
@Test
@SmallTest
@Feature({"AndroidWebView"})
@CommandLineFlags.Add({"disable-features=ContentCaptureTriggeringForExperiment"})
public void testCantCreateExperimentConsumer() throws Throwable {
Assert.assertNull(ExperimentContentCaptureConsumer.create(mAwContents.getWebContents()));
}
}
......@@ -71,7 +71,6 @@ import org.chromium.chrome.browser.util.ChromeAccessibilityUtil;
import org.chromium.components.browser_ui.widget.InsetObserverView;
import org.chromium.components.content_capture.ContentCaptureConsumer;
import org.chromium.components.content_capture.ContentCaptureConsumerImpl;
import org.chromium.components.content_capture.ExperimentContentCaptureConsumer;
import org.chromium.components.embedder_support.view.ContentView;
import org.chromium.content_public.browser.WebContents;
import org.chromium.ui.KeyboardVisibilityDelegate;
......@@ -190,9 +189,7 @@ public class CompositorViewHolder extends FrameLayout
// Indicates if ContentCaptureConsumer should be created, we only try to create it once.
private boolean mShouldCreateContentCaptureConsumer = true;
// TODO: (crbug.com/1119663) Move consumers out of this class while support multiple consumers.
private ArrayList<ContentCaptureConsumer> mContentCaptureConsumers =
new ArrayList<ContentCaptureConsumer>();
private ContentCaptureConsumer mContentCaptureConsumer;
private Set<Runnable> mOnCompositorLayoutCallbacks = new HashSet<>();
private Set<Runnable> mDidSwapFrameCallbacks = new HashSet<>();
......@@ -536,10 +533,10 @@ public class CompositorViewHolder extends FrameLayout
mInsetObserverView.removeObserver(this);
mInsetObserverView = null;
}
for (ContentCaptureConsumer consumer : mContentCaptureConsumers) {
consumer.onWebContentsChanged(null);
if (mContentCaptureConsumer != null) {
mContentCaptureConsumer.onWebContentsChanged(null);
mContentCaptureConsumer = null;
}
mContentCaptureConsumers.clear();
if (mContentView != null) {
mContentView.removeOnHierarchyChangeListener(this);
}
......@@ -1323,21 +1320,16 @@ public class CompositorViewHolder extends FrameLayout
if (mTabVisible != null) initializeTab(mTabVisible);
if (mShouldCreateContentCaptureConsumer) {
if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.Q) {
ContentCaptureConsumer consumer =
if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.Q) {
if (mShouldCreateContentCaptureConsumer) {
mContentCaptureConsumer =
ContentCaptureConsumerImpl.create(getContext(), this, getWebContents());
if (consumer != null) mContentCaptureConsumers.add(consumer);
}
ContentCaptureConsumer consumer =
ExperimentContentCaptureConsumer.create(getWebContents());
if (consumer != null) mContentCaptureConsumers.add(consumer);
mShouldCreateContentCaptureConsumer = false;
} else {
for (ContentCaptureConsumer consumer : mContentCaptureConsumers) {
consumer.onWebContentsChanged(getWebContents());
mShouldCreateContentCaptureConsumer = false;
}
}
if (mContentCaptureConsumer != null) {
mContentCaptureConsumer.onWebContentsChanged(getWebContents());
}
}
private void updateViewStateListener(ContentView newContentView) {
......
......@@ -38,7 +38,6 @@ android_library("java") {
"java/src/org/chromium/components/content_capture/ContentCapturedTask.java",
"java/src/org/chromium/components/content_capture/ContentRemovedTask.java",
"java/src/org/chromium/components/content_capture/ContentUpdateTask.java",
"java/src/org/chromium/components/content_capture/ExperimentContentCaptureConsumer.java",
"java/src/org/chromium/components/content_capture/FrameSession.java",
"java/src/org/chromium/components/content_capture/NotificationTask.java",
"java/src/org/chromium/components/content_capture/PlatformSession.java",
......
......@@ -93,17 +93,4 @@ bool ContentCaptureController::ShouldCapture(const GURL& url) {
return false;
}
bool ContentCaptureController::ShouldCapture(
JNIEnv* env,
const base::android::JavaParamRef<jobject>& jcaller,
const base::android::JavaParamRef<jobjectArray>& urls) {
std::vector<base::string16> url_list;
AppendJavaStringArrayToStringVector(env, urls, &url_list);
for (auto url : url_list) {
if (!ShouldCapture(GURL(url)))
return false;
}
return true;
}
} // namespace content_capture
......@@ -35,9 +35,6 @@ class ContentCaptureController {
const base::android::JavaParamRef<jbooleanArray>& jtype);
void SetJavaPeer(JNIEnv* env,
const base::android::JavaParamRef<jobject>& jcaller);
bool ShouldCapture(JNIEnv* env,
const base::android::JavaParamRef<jobject>& jcaller,
const base::android::JavaParamRef<jobjectArray>& urls);
private:
virtual ~ContentCaptureController();
......
......@@ -8,9 +8,3 @@
static jboolean JNI_ContentCaptureFeatures_IsEnabled(JNIEnv* env) {
return content_capture::features::IsContentCaptureEnabled();
}
static jboolean
JNI_ContentCaptureFeatures_ShouldTriggerContentCaptureForExperiment(
JNIEnv* env) {
return content_capture::features::ShouldTriggerContentCaptureForExperiment();
}
......@@ -12,7 +12,6 @@
#include "components/content_capture/android/content_capture_controller.h"
#include "components/content_capture/android/jni_headers/ContentCaptureData_jni.h"
#include "components/content_capture/android/jni_headers/ContentCaptureReceiverManager_jni.h"
#include "components/content_capture/common/content_capture_features.h"
#include "content/public/browser/web_contents.h"
using base::android::AttachCurrentThread;
......@@ -148,11 +147,6 @@ void ContentCaptureReceiverManagerAndroid::DidRemoveSession(
}
bool ContentCaptureReceiverManagerAndroid::ShouldCapture(const GURL& url) {
// Capture all urls for experiment, the url will be checked
// before the content is sent to the consumers.
if (features::ShouldTriggerContentCaptureForExperiment())
return true;
return ContentCaptureController::Get()->ShouldCapture(url);
}
......
......@@ -50,13 +50,9 @@ public abstract class ContentCaptureConsumer {
// of the removal of session.
if (current != null) {
mManager = ContentCaptureReceiverManager.createOrGet(current);
mManager.addContentCaptureConsumer(this);
mManager.setContentCaptureConsumer(this);
} else {
mManager = null;
}
}
protected boolean shouldCapture(String[] urls) {
return true;
}
}
......@@ -89,14 +89,4 @@ public class ContentCaptureConsumerImpl extends ContentCaptureConsumer {
new ContentRemovedTask(frame, removedIds, mPlatformSession)
.executeOnExecutor(AsyncTask.SERIAL_EXECUTOR);
}
@Override
protected boolean shouldCapture(String[] urls) {
// No need to check if the experiment is disabled, because it was done when the navigation
// committed, refer to ContentCaptureReceiverManager::ReadyToCommitNavigation().
if (!ContentCaptureFeatures.shouldTriggerContentCaptureForExperiment()) return true;
ContentCaptureController controller = ContentCaptureController.getInstance();
if (controller == null) return false;
return controller.shouldCapture(urls);
}
}
......@@ -44,15 +44,6 @@ public abstract class ContentCaptureController {
*/
public void clearContentCaptureDataForURLs(String[] urlsToDelete) {}
/**
* @param urls the urls need to check.
* @return if the content of all urls should be captured.
*/
public boolean shouldCapture(String[] urls) {
return ContentCaptureControllerJni.get().shouldCapture(
mNativeContentCaptureController, ContentCaptureController.this, urls);
}
/**
* Invoked by native side to pull the allowlist, the subclass should implement this and set
* the allowlist by call setAllowlist.
......@@ -78,7 +69,5 @@ public abstract class ContentCaptureController {
long init(Object contentCaptureController);
void setAllowlist(long nativeContentCaptureController, ContentCaptureController caller,
String[] allowlist, boolean[] isRegex);
boolean shouldCapture(long nativeContentCaptureController, ContentCaptureController caller,
String[] urls);
}
}
......@@ -20,13 +20,8 @@ public class ContentCaptureFeatures {
return CommandLine.getInstance().hasSwitch(FLAG);
}
public static boolean shouldTriggerContentCaptureForExperiment() {
return ContentCaptureFeaturesJni.get().shouldTriggerContentCaptureForExperiment();
}
@NativeMethods
interface Natives {
boolean isEnabled();
boolean shouldTriggerContentCaptureForExperiment();
}
}
......@@ -9,7 +9,6 @@ import org.chromium.base.annotations.CalledByNative;
import org.chromium.base.annotations.NativeMethods;
import org.chromium.content_public.browser.WebContents;
import java.util.ArrayList;
import java.util.Arrays;
/**
......@@ -19,8 +18,7 @@ public class ContentCaptureReceiverManager {
private static final String TAG = "ContentCapture";
private static Boolean sDump;
private ArrayList<ContentCaptureConsumer> mContentCaptureConsumers =
new ArrayList<ContentCaptureConsumer>();
private ContentCaptureConsumer mContentCaptureConsumer;
public static ContentCaptureReceiverManager createOrGet(WebContents webContents) {
return ContentCaptureReceiverManagerJni.get().createOrGet(webContents);
......@@ -31,30 +29,22 @@ public class ContentCaptureReceiverManager {
if (sDump == null) sDump = ContentCaptureFeatures.isDumpForTestingEnabled();
}
public void addContentCaptureConsumer(ContentCaptureConsumer consumer) {
mContentCaptureConsumers.add(consumer);
public void setContentCaptureConsumer(ContentCaptureConsumer consumer) {
mContentCaptureConsumer = consumer;
}
@CalledByNative
private void didCaptureContent(Object[] session, ContentCaptureData data) {
FrameSession frameSession = toFrameSession(session);
String[] urls = buildUrls(frameSession, data);
for (ContentCaptureConsumer consumer : mContentCaptureConsumers) {
if (consumer.shouldCapture(urls)) {
consumer.onContentCaptured(frameSession, data);
}
if (mContentCaptureConsumer != null) {
mContentCaptureConsumer.onContentCaptured(toFrameSession(session), data);
}
if (sDump.booleanValue()) Log.i(TAG, "Captured Content: %s", data);
}
@CalledByNative
private void didUpdateContent(Object[] session, ContentCaptureData data) {
FrameSession frameSession = toFrameSession(session);
String[] urls = buildUrls(frameSession, data);
for (ContentCaptureConsumer consumer : mContentCaptureConsumers) {
if (consumer.shouldCapture(urls)) {
consumer.onContentUpdated(frameSession, data);
}
if (mContentCaptureConsumer != null) {
mContentCaptureConsumer.onContentUpdated(toFrameSession(session), data);
}
if (sDump.booleanValue()) Log.i(TAG, "Updated Content: %s", data);
}
......@@ -62,11 +52,8 @@ public class ContentCaptureReceiverManager {
@CalledByNative
private void didRemoveContent(Object[] session, long[] data) {
FrameSession frameSession = toFrameSession(session);
String[] urls = buildUrls(frameSession, null);
for (ContentCaptureConsumer consumer : mContentCaptureConsumers) {
if (consumer.shouldCapture(urls)) {
consumer.onContentRemoved(frameSession, data);
}
if (mContentCaptureConsumer != null) {
mContentCaptureConsumer.onContentRemoved(frameSession, data);
}
if (sDump.booleanValue()) {
Log.i(TAG, "Removed Content: %s", frameSession.get(0) + " " + Arrays.toString(data));
......@@ -76,12 +63,7 @@ public class ContentCaptureReceiverManager {
@CalledByNative
private void didRemoveSession(Object[] session) {
FrameSession frameSession = toFrameSession(session);
String[] urls = buildUrls(frameSession, null);
for (ContentCaptureConsumer consumer : mContentCaptureConsumers) {
if (consumer.shouldCapture(urls)) {
consumer.onSessionRemoved(frameSession);
}
}
if (mContentCaptureConsumer != null) mContentCaptureConsumer.onSessionRemoved(frameSession);
if (sDump.booleanValue()) Log.i(TAG, "Removed Session: %s", frameSession.get(0));
}
......@@ -91,19 +73,6 @@ public class ContentCaptureReceiverManager {
return frameSession;
}
private String[] buildUrls(FrameSession session, ContentCaptureData data) {
ArrayList<String> urls = new ArrayList<String>();
if (session != null) {
for (ContentCaptureData d : session) {
urls.add(d.getValue());
}
}
if (data != null) urls.add(data.getValue());
String[] result = new String[urls.size()];
urls.toArray(result);
return result;
}
@NativeMethods
interface Natives {
ContentCaptureReceiverManager createOrGet(WebContents webContents);
......
// Copyright 2020 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
package org.chromium.components.content_capture;
import org.chromium.base.Log;
import org.chromium.content_public.browser.WebContents;
/**
* This class is used to trigger ContentCapture unconditionally for the experiment. It doesn't
* consume any content, but is necessary to keep capturing content.
*/
public class ExperimentContentCaptureConsumer extends ContentCaptureConsumer {
private static final String TAG = "ContentCapture";
private static boolean sDump;
public static ContentCaptureConsumer create(WebContents webContents) {
if (ContentCaptureFeatures.shouldTriggerContentCaptureForExperiment()) {
return new ExperimentContentCaptureConsumer(webContents);
}
return null;
}
private ExperimentContentCaptureConsumer(WebContents webContents) {
super(webContents);
}
@Override
public void onContentCaptured(FrameSession parentFrame, ContentCaptureData contentCaptureData) {
if (sDump) Log.d(TAG, "onContentCaptured " + contentCaptureData.toString());
}
@Override
public void onSessionRemoved(FrameSession session) {
if (sDump) Log.d(TAG, "onSessionRemoved");
}
@Override
public void onContentRemoved(FrameSession session, long[] removedIds) {
if (sDump) Log.d(TAG, "onContentRemoved");
}
@Override
public void onContentUpdated(FrameSession parentFrame, ContentCaptureData contentCaptureData) {
if (sDump) Log.d(TAG, "onContentUpdated");
}
}
......@@ -4,7 +4,6 @@
#include "components/content_capture/common/content_capture_features.h"
#include "base/feature_list.h"
#include "base/metrics/field_trial_params.h"
#include "build/build_config.h"
......@@ -14,25 +13,15 @@ namespace features {
#if defined(OS_ANDROID)
const base::Feature kContentCapture{"ContentCapture",
base::FEATURE_ENABLED_BY_DEFAULT};
const base::Feature kContentCaptureTriggeringForExperiment{
"ContentCaptureTriggeringForExperiment", base::FEATURE_ENABLED_BY_DEFAULT};
#else
const base::Feature kContentCapture{"ContentCapture",
base::FEATURE_DISABLED_BY_DEFAULT};
const base::Feature kContentCaptureTriggeringForExperiment{
"ContentCaptureTriggeringForExperiment", base::FEATURE_DISABLED_BY_DEFAULT};
#endif
bool IsContentCaptureEnabled() {
return base::FeatureList::IsEnabled(kContentCapture);
}
bool ShouldTriggerContentCaptureForExperiment() {
return base::FeatureList::IsEnabled(kContentCaptureTriggeringForExperiment);
}
int TaskLongDelayInMilliseconds() {
return base::GetFieldTrialParamByFeatureAsInt(
kContentCapture, "task_long_delay_in_milliseconds", 5000);
......
......@@ -13,13 +13,7 @@ namespace features {
extern const base::Feature kContentCaptureEnabled;
// ContentCapture is triggered in the unpredictable conditions which might be
// changed on different aiai release or configuration push, this feature allows
// us to trigger the ContentCapture independently to get the unbiased result.
extern const base::Feature kContentCaptureTriggeringForExperiment;
bool IsContentCaptureEnabled();
bool ShouldTriggerContentCaptureForExperiment();
int TaskLongDelayInMilliseconds();
int TaskShortDelayInMilliseconds();
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment